Bug 45857 – PluginStrategy should satisfy the needs of Qt

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Balazs Kelemen]

The WebKit2 port makes the need of using the PlatformStrategies in Qt but currently PluginStrategy does not fit our needs. Qt has API to set custom plugins for a specific page.  See http://doc.qt.nokia.com/4.6/qwebpage.html#setPluginFactory for more information.
To be able to implement this API through PluginStrategy PluginStrategy::getPluginInfo should take a Page* argument that would be unused by other ports.

[Balazs Kelemen]

Created attachment 67752 [details]
proposed patch

[Kenneth Rohde Christiansen]

Comment on attachment 67752 [details]
proposed patch

The patch doesn't contain the Qt part, not say why the page is needed.

[WebKit Review Bot]

Attachment 67752 [details] did not build on win:
Build output: http://queues.webkit.org/results/4005036

[Balazs Kelemen]

Comment on attachment 67752 [details]
proposed patch

Windows needs investigation.

[Balazs Kelemen]

(In reply to comment #2)
> (From update of attachment 67752 [details])
> The patch doesn't contain the Qt part, not say why the page is needed.

The qt part will coming soon in a separate patch but first I need to fix the windows build.

[Balazs Kelemen]

Created attachment 67780 [details]
proposed patch v2

[Andreas Kling]

Comment on attachment 67780 [details]
proposed patch v2

> +        * plugins/PluginStrategy.h: Added a |const Page*| argument to initPlugins.
Typo: you're adding the argument to getPluginInfo, not initPlugins.

This approach looks good to me, it fixes the current crashiness and API regression.

Why does the Qt part needs to be in a separate patch?

[Balazs Kelemen]

(In reply to comment #7)
> (From update of attachment 67780 [details])
> > +        * plugins/PluginStrategy.h: Added a |const Page*| argument to initPlugins.
> Typo: you're adding the argument to getPluginInfo, not initPlugins.
> 
> This approach looks good to me, it fixes the current crashiness and API regression.
> 
> Why does the Qt part needs to be in a separate patch?

For easier review :). My imagination was that this will be reviewed by a core developer from Apple and the qt part will be reviewed by a Qt folk. Furthermore,
as I know the policy is to keep patches as small as we can.
Actually, the Qt part is out-of-date by means of http://trac.webkit.org/changeset/67787. I think we should land this first and then fix our PlatformStrategies.

[Balazs Kelemen]

Comment on attachment 67780 [details]
proposed patch v2

Review cq? since the Changelog should be typo fixed.

[Balazs Kelemen]

(In reply to comment #9)
> (From update of attachment 67780 [details])
> Review cq? since the Changelog should be typo fixed.

Remove

[Kenneth Rohde Christiansen]

Comment on attachment 67780 [details]
proposed patch v2

PlatformStrategies is supposed to be a global instance for global platform things. Getting additional [Qt] plugins associated with a page isn't really a platform things and might have to be done separately.

[Kenneth Rohde Christiansen]

(In reply to comment #11)
> (From update of attachment 67780 [details])
> PlatformStrategies is supposed to be a global instance for global platform things. Getting additional [Qt] plugins associated with a page isn't really a platform things and might have to be done separately.

Maybe it is not such a bad idea anyway, as it would allow us to restrict certain plugins for certain pages.

Another solution would be to create a PluginDatabaseClient for accessing the Qt page related plugins, though that seems like a bit of overkill.

[WebKit Review Bot]

http://trac.webkit.org/changeset/68038 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/68037
http://trac.webkit.org/changeset/68038

[Andreas Kling]

Committed r68038: <http://trac.webkit.org/changeset/68038>