Bug 30179 - [Qt] Need a way to inform the application when a Netscape plugin is created or deleted.
Summary: [Qt] Need a way to inform the application when a Netscape plugin is created o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Viatcheslav Ostapenko
URL:
Keywords: Qt
Depends on:
Blocks: 35962
  Show dependency treegraph
 
Reported: 2009-10-07 12:00 PDT by Yael
Modified: 2011-01-24 07:19 PST (History)
10 users (show)

See Also:


Attachments
Patch. (3.16 KB, patch)
2009-10-15 16:16 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (3.99 KB, patch)
2009-10-16 10:02 PDT, Yael
no flags Details | Formatted Diff | Diff
Updated patch proposal (4.89 KB, patch)
2010-12-23 08:16 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Fixed style error (4.89 KB, patch)
2010-12-23 08:34 PST, Viatcheslav Ostapenko
hausmann: review-
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 Details | Formatted Diff | Diff
Fix style problems (4.46 KB, patch)
2011-01-10 14:59 PST, Viatcheslav Ostapenko
hausmann: review-
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 Details | Formatted Diff | Diff
qt_setPluginCreatedCallback renamed to qtwebkit_setPluginCreatedCallback (4.11 KB, patch)
2011-01-12 15:17 PST, Viatcheslav Ostapenko
no flags 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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2009-10-07 12:00:52 PDT
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 Simon Hausmann 2009-10-08 01:33:28 PDT
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 Yael 2009-10-08 05:18:58 PDT
(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 Simon Hausmann 2009-10-09 07:41:41 PDT
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 Yael 2009-10-15 16:16:17 PDT
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 5 WebKit Commit Bot 2009-10-16 06:18:45 PDT
Comment on attachment 41250 [details]
Patch.

Clearing flags on attachment: 41250

Committed r49676: <http://trac.webkit.org/changeset/49676>
Comment 6 WebKit Commit Bot 2009-10-16 06:18:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Yael 2009-10-16 07:54:38 PDT
PlatformDestroy() is called too late, after the plugin was destroyed. 
Need to move the signal to before the plugin is destroyed.
Comment 8 Yael 2009-10-16 10:02:14 PDT
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.
Comment 9 Yael 2009-10-21 17:07:37 PDT
Cleared the review flag, since I found out that we actually need a solution for all platforms, not only S60.
Comment 10 Simon Hausmann 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 Kent Hansen 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 Yael 2010-11-11 11:50:06 PST
We do not plan to do that anymore. Closing the bug.
Comment 13 Ademar Reis 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 Yael 2010-12-21 11:38:02 PST
I closed the bug too soon :(
Comment 15 Ademar Reis 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 Viatcheslav Ostapenko 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 Viatcheslav Ostapenko 2010-12-23 08:16:09 PST
Created attachment 77334 [details]
Updated patch proposal

2 questions:

1. Bring it to all Qt platforms?
2. Might be create public API?
Comment 18 WebKit Review Bot 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 Viatcheslav Ostapenko 2010-12-23 08:34:11 PST
Created attachment 77338 [details]
Fixed style error
Comment 20 Antonio Gomes 2010-12-23 12:41:48 PST
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()?
Comment 21 Viatcheslav Ostapenko 2010-12-23 13:14:00 PST
(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
Comment 22 Antonio Gomes 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 Simon Hausmann 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 Simon Hausmann 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 Simon Hausmann 2011-01-10 07:10:32 PST
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.
Comment 26 Viatcheslav Ostapenko 2011-01-10 14:37:15 PST
Created attachment 78451 [details]
Symbian only and create notification only version of patch.
Comment 27 WebKit Review Bot 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 Viatcheslav Ostapenko 2011-01-10 14:59:06 PST
Created attachment 78454 [details]
Fix style problems
Comment 29 Simon Hausmann 2011-01-12 05:50:37 PST
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 30 Viatcheslav Ostapenko 2011-01-12 12:23:22 PST
Created attachment 78721 [details]
Moved exported static function to PluginViewSymbian.cpp
Comment 31 Viatcheslav Ostapenko 2011-01-12 12:33:04 PST
Comment on attachment 78721 [details]
Moved exported static function to PluginViewSymbian.cpp

Missed last Simon's comment about function rename.
Comment 32 Simon Hausmann 2011-01-12 13:56:02 PST
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.
Comment 33 Viatcheslav Ostapenko 2011-01-12 15:17:56 PST
Created attachment 78744 [details]
qt_setPluginCreatedCallback renamed to qtwebkit_setPluginCreatedCallback

Hope I didn't miss anything this time.
Comment 34 WebKit Commit Bot 2011-01-13 07:45:57 PST
Comment on attachment 78744 [details]
qt_setPluginCreatedCallback renamed to qtwebkit_setPluginCreatedCallback

Clearing flags on attachment: 78744

Committed r75713: <http://trac.webkit.org/changeset/75713>
Comment 35 WebKit Commit Bot 2011-01-13 07:46:05 PST
All reviewed patches have been landed.  Closing bug.
Comment 36 Viatcheslav Ostapenko 2011-01-13 15:16:43 PST
Created attachment 78862 [details]
Fix symbian build broken by http://trac.webkit.org/changeset/75713

Cannot believe this :(
Comment 37 WebKit Review Bot 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 Viatcheslav Ostapenko 2011-01-14 09:51:48 PST
Reopen it for last patch to be committed.
Comment 39 WebKit Commit Bot 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 WebKit Commit Bot 2011-01-14 10:30:29 PST
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>
Comment 41 WebKit Commit Bot 2011-01-14 10:30:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 42 Ademar Reis 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 Nancy Piedra 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 Viatcheslav Ostapenko 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 Ademar Reis 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 Ademar Reis 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>