Bug 30179

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?

(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 41250 [details] Patch. Clearing flags on attachment: 41250 Committed r49676: <http://trac.webkit.org/changeset/49676>

All reviewed patches have been landed. Closing bug.

PlatformDestroy() is called too late, after the plugin was destroyed. Need to move the signal to before the plugin is destroyed.

Created attachment 41293 [details] Patch Take care of the build break and also move the destroy signal to just before the plugin is destroyed. A modified Qtlauncher received the signals as expected.

Cleared the review flag, since I found out that we actually need a solution for all platforms, not only S60.

I'm afraid this is missing the Qt 4.6 train and will have to wait until the next release ;(

(In reply to comment #10) > I'm afraid this is missing the Qt 4.6 train and will have to wait until the > next release ;( What about the 4.7 train, I hear it's about to leave the station too.

We do not plan to do that anymore. Closing the bug.

A closed/wontfix bug should not block the qtwebkit-2.2 release... Removing dependency, please re-add it (and fix the bug status) if necessary.

I closed the bug too soon :(

(In reply to comment #14) > I closed the bug too soon :( OK, re-adding the release block then.

(In reply to comment #15) > (In reply to comment #14) > > I closed the bug too soon :( > > OK, re-adding the release block then. Updated patch will be attached sooooon

Created attachment 77334 [details] Updated patch proposal 2 questions: 1. Bring it to all Qt platforms? 2. Might be create public API?

Attachment 77334 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/plugins/PluginView.cpp', u'WebCore/plugins/symbian/PluginViewSymbian.cpp', u'WebKit/qt/Api/qwebpage.cpp', u'WebKit/qt/ChangeLog', u'WebKit/qt/symbian/eabi/QtWebKitu.def']" exit_code: 1 WebCore/plugins/PluginView.cpp:83: Extra space between extern and _qt_page_plugin_destroyed [whitespace/declaration] [3] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 77338 [details] Fixed style error

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?

Hmm, can you help me refresh my memory? :) I forgot: What kind of interaction do you need to do with the plugin in the browser again?

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.

Created attachment 78451 [details] Symbian only and create notification only version of patch.

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.

Created attachment 78454 [details] Fix style problems

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.

Created attachment 78721 [details] Moved exported static function to PluginViewSymbian.cpp

Comment on attachment 78721 [details] Moved exported static function to PluginViewSymbian.cpp Missed last Simon's comment about function rename.

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.

Created attachment 78744 [details] qt_setPluginCreatedCallback renamed to qtwebkit_setPluginCreatedCallback Hope I didn't miss anything this time.

Comment on attachment 78744 [details] qt_setPluginCreatedCallback renamed to qtwebkit_setPluginCreatedCallback Clearing flags on attachment: 78744 Committed r75713: <http://trac.webkit.org/changeset/75713>

All reviewed patches have been landed. Closing bug.

Created attachment 78862 [details] Fix symbian build broken by http://trac.webkit.org/changeset/75713 Cannot believe this :(

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.

Reopen it for last patch to be committed.

The commit-queue encountered the following flaky tests while processing attachment 78862 [details]: fast/css/font-face-download-error.html bug 51757 (author: yuzo@google.com) The commit-queue is continuing to process your patch.

Comment on attachment 78862 [details] Fix symbian build broken by http://trac.webkit.org/changeset/75713 Clearing flags on attachment: 78862 Committed r75801: <http://trac.webkit.org/changeset/75801>

All reviewed patches have been landed. Closing bug.

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?

My understanding is Qt Webkit 2.1 has not been released yet so maybe we could add this to both Qt Webkit 2.1 & 2.2 and therefore not have the change in API.

(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.

(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.

Revision r75713 cherry-picked into qtwebkit-2.2 with commit e94a2d3 <http://gitorious.org/webkit/qtwebkit/commit/e94a2d3> Revision r75801 cherry-picked into qtwebkit-2.2 with commit 415a230 <http://gitorious.org/webkit/qtwebkit/commit/415a230>