Bug 30179 - [Qt] Need a way to inform the application when a Netscape plugin is created or deleted.
: [Qt] Need a way to inform the application when a Netscape plugin is created o...
Status: RESOLVED FIXED
: WebKit
WebKit Qt
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: Qt
:
: 35962
  Show dependency treegraph
 
Reported: 2009-10-07 12:00 PST by
Modified: 2011-01-24 07:19 PST (History)


Attachments
Patch. (3.16 KB, patch)
2009-10-15 16:16 PST, Yael
no flags Review Patch | Details | Formatted Diff | Diff
Patch (3.99 KB, patch)
2009-10-16 10:02 PST, Yael
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch proposal (4.89 KB, patch)
2010-12-23 08:16 PST, Viatcheslav Ostapenko
no flags Review Patch | Details | Formatted Diff | Diff
Fixed style error (4.89 KB, patch)
2010-12-23 08:34 PST, Viatcheslav Ostapenko
hausmann: review-
Review Patch | Details | Formatted Diff | Diff
Symbian only and create notification only version of patch. (4.48 KB, patch)
2011-01-10 14:37 PST, Viatcheslav Ostapenko
no flags Review Patch | Details | Formatted Diff | Diff
Fix style problems (4.46 KB, patch)
2011-01-10 14:59 PST, Viatcheslav Ostapenko
hausmann: review-
Review Patch | Details | Formatted Diff | Diff
Moved exported static function to PluginViewSymbian.cpp (4.39 KB, patch)
2011-01-12 12:23 PST, Viatcheslav Ostapenko
no flags Review Patch | Details | Formatted Diff | Diff
qt_setPluginCreatedCallback renamed to qtwebkit_setPluginCreatedCallback (4.11 KB, patch)
2011-01-12 15:17 PST, Viatcheslav Ostapenko
no flags Review Patch | Details | Formatted Diff | Diff
Fix symbian build broken by http://trac.webkit.org/changeset/75713 (1.24 KB, patch)
2011-01-13 15:16 PST, Viatcheslav Ostapenko
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-10-07 12:00:52 PST
S60 has some quirks that require the application to communicate with the plugin directly. In order to do that, the application needs to know that the plugin exists.
------- Comment #1 From 2009-10-08 01:33:28 PST -------
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?
------- Comment #2 From 2009-10-08 05:18:58 PST -------
(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.
------- Comment #3 From 2009-10-09 07:41:41 PST -------
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? :-)
------- Comment #4 From 2009-10-15 16:16:17 PST -------
Created an attachment (id=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 #5 From 2009-10-16 06:18:45 PST -------
(From update of attachment 41250 [details])
Clearing flags on attachment: 41250

Committed r49676: <http://trac.webkit.org/changeset/49676>
------- Comment #6 From 2009-10-16 06:18:48 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #7 From 2009-10-16 07:54:38 PST -------
PlatformDestroy() is called too late, after the plugin was destroyed. 
Need to move the signal to before the plugin is destroyed.
------- Comment #8 From 2009-10-16 10:02:14 PST -------
Created an attachment (id=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.
------- Comment #9 From 2009-10-21 17:07:37 PST -------
Cleared the review flag, since I found out that we actually need a solution for all platforms, not only S60.
------- Comment #10 From 2009-11-11 07:21:41 PST -------
I'm afraid this is missing the Qt 4.6 train and will have to wait until the next release ;(
------- Comment #11 From 2010-03-12 05:43:13 PST -------
(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.
------- Comment #12 From 2010-11-11 11:50:06 PST -------
We do not plan to do that anymore. Closing the bug.
------- Comment #13 From 2010-12-21 11:36:31 PST -------
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.
------- Comment #14 From 2010-12-21 11:38:02 PST -------
I closed the bug too soon :(
------- Comment #15 From 2010-12-21 12:05:52 PST -------
(In reply to comment #14)
> I closed the bug too soon :(

OK, re-adding the release block then.
------- Comment #16 From 2010-12-21 12:24:55 PST -------
(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
------- Comment #17 From 2010-12-23 08:16:09 PST -------
Created an attachment (id=77334) [details]
Updated patch proposal

2 questions:

1. Bring it to all Qt platforms?
2. Might be create public API?
------- Comment #18 From 2010-12-23 08:18:41 PST -------
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.
------- Comment #19 From 2010-12-23 08:34:11 PST -------
Created an attachment (id=77338) [details]
Fixed style error
------- Comment #20 From 2010-12-23 12:41:48 PST -------
(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, and might possibly not work for webkit2.

Can not you do this from FrameLoaderClientQt::createFrame()?
------- Comment #21 From 2010-12-23 13:14:00 PST -------
(In reply to comment #20)
> (From update of attachment 77338 [details] [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
------- Comment #22 From 2010-12-28 11:29:18 PST -------
> > 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?
------- Comment #23 From 2010-12-29 00:38:05 PST -------
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?
------- Comment #24 From 2011-01-10 07:07:42 PST -------
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 #25 From 2011-01-10 07:10:32 PST -------
(From update of attachment 77338 [details])
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.
------- Comment #26 From 2011-01-10 14:37:15 PST -------
Created an attachment (id=78451) [details]
Symbian only and create notification only version of patch.
------- Comment #27 From 2011-01-10 14:39:19 PST -------
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 #28 From 2011-01-10 14:59:06 PST -------
Created an attachment (id=78454) [details]
Fix style problems
------- Comment #29 From 2011-01-12 05:50:37 PST -------
(From update of attachment 78454 [details])
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 #30 From 2011-01-12 12:23:22 PST -------
Created an attachment (id=78721) [details]
Moved exported static function to PluginViewSymbian.cpp
------- Comment #31 From 2011-01-12 12:33:04 PST -------
(From update of attachment 78721 [details])
Missed last Simon's comment about function rename.
------- Comment #32 From 2011-01-12 13:56:02 PST -------
(From update of attachment 78721 [details])
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.
------- Comment #33 From 2011-01-12 15:17:56 PST -------
Created an attachment (id=78744) [details]
qt_setPluginCreatedCallback renamed to qtwebkit_setPluginCreatedCallback

Hope I didn't miss anything this time.
------- Comment #34 From 2011-01-13 07:45:57 PST -------
(From update of attachment 78744 [details])
Clearing flags on attachment: 78744

Committed r75713: <http://trac.webkit.org/changeset/75713>
------- Comment #35 From 2011-01-13 07:46:05 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #36 From 2011-01-13 15:16:43 PST -------
Created an attachment (id=78862) [details]
Fix symbian build broken by http://trac.webkit.org/changeset/75713

Cannot believe this :(
------- Comment #37 From 2011-01-13 15:18:28 PST -------
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.
------- Comment #38 From 2011-01-14 09:51:48 PST -------
Reopen it for last patch to be committed.
------- Comment #39 From 2011-01-14 10:28:58 PST -------
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 #40 From 2011-01-14 10:30:29 PST -------
(From update of attachment 78862 [details])
Clearing flags on attachment: 78862

Committed r75801: <http://trac.webkit.org/changeset/75801>
------- Comment #41 From 2011-01-14 10:30:39 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #42 From 2011-01-17 05:20:49 PST -------
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?
------- Comment #43 From 2011-01-17 06:11:14 PST -------
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.
------- Comment #44 From 2011-01-17 06:14:43 PST -------
(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.
------- Comment #45 From 2011-01-17 06:23:41 PST -------
(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.
------- Comment #46 From 2011-01-17 11:51:19 PST -------
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>