Bug 30179 – [Qt] Need a way to inform the application when a Netscape plugin is created or deleted.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Yael]

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.

[Simon Hausmann]

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?

[Yael]

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

[Simon Hausmann]

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? :-)

[Yael]

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.

[WebKit Commit Bot]

Comment on attachment 41250 [details]
Patch.

Clearing flags on attachment: 41250

Committed r49676: <http://trac.webkit.org/changeset/49676>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yael]

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

[Yael]

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.

[Yael]

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

[Simon Hausmann]

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

[Kent Hansen]

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

[Yael]

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

[Ademar Reis]

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.

[Yael]

I closed the bug too soon :(

[Ademar Reis]

(In reply to comment #14)
> I closed the bug too soon :(

OK, re-adding the release block then.

[Viatcheslav Ostapenko]

(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

[Viatcheslav Ostapenko]

Created attachment 77334 [details]
Updated patch proposal

2 questions:

1. Bring it to all Qt platforms?
2. Might be create public API?

[WebKit Review Bot]

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.

[Viatcheslav Ostapenko]

Created attachment 77338 [details]
Fixed style error

[Antonio Gomes]

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()?

[Viatcheslav Ostapenko]

(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

[Antonio Gomes]

> > 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?

[Simon Hausmann]

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?

[Simon Hausmann]

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.

[Simon Hausmann]

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.

[Viatcheslav Ostapenko]

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

[WebKit Review Bot]

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.

[Viatcheslav Ostapenko]

Created attachment 78454 [details]
Fix style problems

[Simon Hausmann]

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.

[Viatcheslav Ostapenko]

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

[Viatcheslav Ostapenko]

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

Missed last Simon's comment about function rename.

[Simon Hausmann]

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.

[Viatcheslav Ostapenko]

Created attachment 78744 [details]
qt_setPluginCreatedCallback renamed to qtwebkit_setPluginCreatedCallback

Hope I didn't miss anything this time.

[WebKit Commit Bot]

Comment on attachment 78744 [details]
qt_setPluginCreatedCallback renamed to qtwebkit_setPluginCreatedCallback

Clearing flags on attachment: 78744

Committed r75713: <http://trac.webkit.org/changeset/75713>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Viatcheslav Ostapenko]

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

Cannot believe this :(

[WebKit Review Bot]

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.

[Viatcheslav Ostapenko]

Reopen it for last patch to be committed.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 78862 [details]:

fast/css/font-face-download-error.html bug 51757 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

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?

[Nancy Piedra]

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.

[Viatcheslav Ostapenko]

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

[Ademar Reis]

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

[Ademar Reis]

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>