(In reply to comment #1)
> Given that for the Symbian port we're going to use QPluginLoader, would it be
> sufficient for you to be able to get a pointer to the QObject instance of the
> plugin?
I was wondering myself what parameters to pass in the signals :-)
Getting a QObject and then using qobject_cast might actually be enough.

I agree about the use-case, but for the moment it would be best if we could defer this addition in the public API and instead add private API for this.
Yael, could you try to solve this using one of those evil c-style functions? :-)

Created attachment 41250[details]
Patch.
Add 2 static methods for the appliation to register for notifications when a plugin is created or destroyed.
This is a temporary private API, and for now, the notifications will be sent only for Symbian platform.

Comment on attachment 77338[details]
Fixed style error
View in context: https://bugs.webkit.org/attachment.cgi?id=77338&action=review> WebCore/plugins/symbian/PluginViewSymbian.cpp:425
> + if (qt_page_plugin_created)
> + qt_page_plugin_created(QWebFramePrivate::kit(m_parentFrame.get()), m_instance, (void*)(m_plugin->pluginFuncs()));
This looks like a strong layer violation, and might possibly not work for webkit2.
Can not you do this from FrameLoaderClientQt::createFrame()?

(In reply to comment #20)
> (From update of attachment 77338[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77338&action=review
>
> > WebCore/plugins/symbian/PluginViewSymbian.cpp:425
> > + if (qt_page_plugin_created)
> > + qt_page_plugin_created(QWebFramePrivate::kit(m_parentFrame.get()), m_instance, (void*)(m_plugin->pluginFuncs()));
>
> This looks like a strong layer violation,
Sorry, what do you mean?
> and might possibly not work for webkit2.
Yes, likely it will not.
> Can not you do this from FrameLoaderClientQt::createFrame()?
Do you mean FrameLoaderClientQt::createPlugin ? ;)
I'll try to extract plugin instance and plugin functions, but do you know better place for "plugin destroyed" hook? Cannot figure out, where it could be place in FrameLoaderClientQt .
Thanks,
Sl

> > This looks like a strong layer violation,
>
> Sorry, what do you mean?
I mean that code within WebCore/platform/ can not depend on code from outside it. Nor in WebCore/* and specially in WebKit/*.
> > Can not you do this from FrameLoaderClientQt::createFrame()?
>
> Do you mean FrameLoaderClientQt::createPlugin ? ;)
Exactly :)
> I'll try to extract plugin instance and plugin functions, but do you know better place for "plugin destroyed" hook? Cannot figure out, where it could be place in FrameLoaderClientQt .
Maybe we can add one?

For reference, here's the justification:
It's a awful hack that is currently only needed on Symbian. It allows the Browser and the Plugin to establish their own private interface to circumvent event handling. For example it allows the browser to send special gesture events to the plugin. Or it would allow the browser to paint the plugin whenever/however it wants. Essentially it allows the Browser to implement plugin support, instead of WebKit. The latter is only used for the scripting bridge.
I think it's okay to let this one slip through, under the condition that it's only available for Symbian and totally private API. I really want to see PluginViewSymbian.cpp go away with WebKit2 anyway and return to a sane rendering model with proper AC integration.

Comment on attachment 77338[details]
Fixed style error
As far as I can see only a callback for the _creation_ is needed, so I think it the deletion callback should be removed (QObject's destroyed signal can be used instead).
Also I think the changes can be limited to PluginViewSymbian.cpp then. The code from qwebpage.cpp can move there, too. That way we don't need
#ifdefs for this Symbian specific feature.

Attachment 78451[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/plugins/symbian/PluginViewSymbian.cpp', u'WebKit/qt/Api/qwebpage.cpp', u'WebKit/qt/ChangeLog', u'WebKit/qt/symbian/bwins/QtWebKitu.def', u'WebKit/qt/symbian/eabi/QtWebKitu.def']" exit_code: 1
Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:68: The parameter name "frame" adds no information, so it should be removed. [readability/parameter_name] [5]
WebKit/qt/Api/qwebpage.cpp:142: The parameter name "frame" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 78454[details]
Fix style problems
View in context: https://bugs.webkit.org/attachment.cgi?id=78454&action=review
Looks okay to me, I think this will be the last round of review. However please move the exported function into PluginViewSymbian.cpp, as this is a feature currently only implemented and needed in this one particular use-case on Symbian.
> WebKit/qt/Api/qwebpage.cpp:146
> +typedef void (*_qt_page_plugin_created)(QWebFrame*, void*, void*); // frame, plugin instance, plugin functions
> +_qt_page_plugin_created qt_page_plugin_created = 0;
> +QWEBKIT_EXPORT void qt_setPluginCreatedCallback(_qt_page_plugin_created cb)
> +{
> + qt_page_plugin_created = cb;
> +}
This should go into PluginViewSymbian.cpp. The variable then also becomes either a class variable or local to the compilation unit (static).
Also, please rename qt_setPluginCreatedCallback to qtwebkit_setPluginCreatedCallback.

Comment on attachment 78721[details]
Moved exported static function to PluginViewSymbian.cpp
View in context: https://bugs.webkit.org/attachment.cgi?id=78721&action=review> Source/WebCore/ChangeLog:11
> + No new tests. (OOPS!)
This line should also be removed :)
> WebKit/qt/Api/qwebpage.cpp:-141
> -
And qwebpage.cpp shouldn't appear in the diff.
> WebKit/qt/ChangeLog:11
> + * Api/qwebpage.cpp:
This entry would also disappear from the ChangeLog.

Attachment 78862[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:69: qtwebkit_page_plugin_created is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.

This patch changes the Qt API in QWebPage. Looks like it's plugin related, but I'm afraid it'll break the ABI and since we're planning to release QtWebKit-2.2 as an update for 2.1, we can't do that.
Any comments?

(In reply to comment #44)
> (In reply to comment #42)
> > This patch changes the Qt API in QWebPage.
>
> Do I miss something? Final version doesn't change anything in qwebpage class/files.
Simon clarified it to me in the channel. The API change was reverted in r49681 (not referenced in this bug).
I'm cherry-picking/backporting r75713 and r75801.