Bug 45857

Summary: PluginStrategy should satisfy the needs of Qt
Product: WebKit Reporter: Balazs Kelemen <kbalazs>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, abecsi, ademar, darin, eric, kenneth, kling, mjs, ossy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 45881, 46078, 46181    
Attachments:
Description Flags
proposed patch
none
proposed patch v2 kenneth: review+, kbalazs: commit-queue-

Balazs Kelemen
Reported 2010-09-15 17:58:26 PDT
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.
Attachments
proposed patch (7.12 KB, patch)
2010-09-15 18:12 PDT, Balazs Kelemen 		no flags
DetailsFormatted DiffDiff
proposed patch v2 (7.16 KB, patch)
2010-09-16 04:47 PDT, Balazs Kelemen 		kenneth: review+
kbalazs: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Balazs Kelemen
Comment 1 2010-09-15 18:12:36 PDT
Created attachment 67752 [details] proposed patch
Kenneth Rohde Christiansen
Comment 2 2010-09-16 00:27:16 PDT
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
Comment 3 2010-09-16 01:06:27 PDT
Attachment 67752 [details] did not build on win: Build output: http://queues.webkit.org/results/4005036
Balazs Kelemen
Comment 4 2010-09-16 02:14:25 PDT
Comment on attachment 67752 [details] proposed patch Windows needs investigation.
Balazs Kelemen
Comment 5 2010-09-16 02:17:26 PDT
(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
Comment 6 2010-09-16 04:47:28 PDT
Created attachment 67780 [details] proposed patch v2
Andreas Kling
Comment 7 2010-09-19 15:06:59 PDT
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
Comment 8 2010-09-20 01:44:22 PDT
(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 9 2010-09-20 01:47:51 PDT
Comment on attachment 67780 [details] proposed patch v2 Review cq? since the Changelog should be typo fixed.
Balazs Kelemen
Comment 10 2010-09-20 01:48:06 PDT
(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 11 2010-09-22 06:12:55 PDT
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
Comment 12 2010-09-22 06:15:23 PDT
(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
Comment 13 2010-09-22 07:23:07 PDT
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
Comment 14 2010-09-22 07:24:30 PDT
Committed r68038: <http://trac.webkit.org/changeset/68038>
Note You need to log in before you can comment on or make changes to this bug.