Bug 96507 – [Chromium] Move getPluginsList out of PlatformSupport

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Mark Pilgrim (Google)]

[Chromium] Move getPluginsList out of PlatformSupport

[Mark Pilgrim (Google)]

Created attachment 163616 [details]
WIP patch

[Adam Barth]

Comment on attachment 163616 [details]
WIP patch

The extra whitespace changes make the diff harder to read.  Typically we don't remove trailing whitespace unless we're editing a line nearby.

[Mark Pilgrim (Google)]

No idea how that happened. My editor doesn't usually do this. Re-diffing...

[Mark Pilgrim (Google)]

Created attachment 163668 [details]
WIP Patch #2

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 163668 [details]
WIP Patch #2

Attachment 163668 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13832471

[Build Bot]

Comment on attachment 163668 [details]
WIP Patch #2

Attachment 163668 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13828607

[Early Warning System Bot]

Comment on attachment 163668 [details]
WIP Patch #2

Attachment 163668 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13820923

[Early Warning System Bot]

Comment on attachment 163668 [details]
WIP Patch #2

Attachment 163668 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13841069

[Peter Beverloo (cr-android ews)]

Comment on attachment 163668 [details]
WIP Patch #2

Attachment 163668 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13841068

[Gyuyoung Kim]

Comment on attachment 163668 [details]
WIP Patch #2

Attachment 163668 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13824808

[Build Bot]

Comment on attachment 163668 [details]
WIP Patch #2

Attachment 163668 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13839258

[Mark Pilgrim (Google)]

Created attachment 178144 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 178144 [details]
Patch

Latest patch implements idea from fishd. Notes from conversation:

Goals:
1. remove plugins() interface from PlatformSupport.h
2. remove plugins() implementation from PlatformSupport.cpp
3. move getPluginListBuilder() interface from WebKitPlatformSupport.h
to Platform.h

Steps:
1. PlatformSupport::plugins() is called from one place,
WebCore/plugins/chromium/PluginDataChromium.cpp. Modify
PluginDataChromium.cpp to inline the code that is currently in
PlatformSupport.cpp.
2. #1 won't compile, because the code relies on
WebKit::WebPluginListBuilderImpl, which we don't have access to from
PluginDataChromium. Solution: move WebPluginListBuilderImpl.cpp/.h
into WebCore/plugins/chromium/ .
3. #2 won't compile because WebPluginListBuilderImpl is a subclass of
WebPluginListBuilder, which lives in WebKit/chromium/public/.
Solution: move WebPluginListBuilder.h to Platform/chromium/public/
(should be safe, no non-Platform dependencies).
4. Remove plugins() interface from PlatformSupport.h
5. Remove plugins() implementation from PlatformSupport.cpp
6. Move getPluginListBuilder interface to Platform.h
7. Profit.

[Darin Fisher (:fishd, Google)]

Comment on attachment 178144 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178144&action=review

> Source/WebCore/plugins/chromium/WebPluginListBuilderImpl.h:42
> +    WebPluginListBuilderImpl(Vector<WebCore::PluginInfo>* results) : m_results(results) { }

nit: since the job of this class is to produce a list of PluginInfo objects, you
can use a name for the class like PluginListBuilder, which is nice b/c it is
shorter.  You could also use a name like PluginInfoListBuilder, but it seems OK
to leave out the "Info" term.

[Mark Pilgrim (Google)]

Created attachment 178267 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 178267 [details]
Patch

Interface renamed to PluginListBuilder

[Darin Fisher (:fishd, Google)]

Comment on attachment 178267 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178267&action=review

> Source/WebCore/plugins/chromium/PluginListBuilder.h:38
> +namespace WebKit {

nit: should be in the WebCore namespace

[Mark Pilgrim (Google)]

Created attachment 178271 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 178271 [details]
Patch

moved PluginListBuilder to WebCore namespace

[WebKit Review Bot]

Comment on attachment 178271 [details]
Patch

Clearing flags on attachment: 178271

Committed r137177: <http://trac.webkit.org/changeset/137177>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.