Bug 96507

Summary: [Chromium] Move getPluginsList out of PlatformSupport
Product: WebKit Reporter: Mark Pilgrim (Google) <pilgrim>
Component: WebKit Misc.Assignee: Mark Pilgrim (Google) <pilgrim>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fishd, gustavo, jamesr, japhet, peter+ews, philn, tkent+wkapi, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 82948    
Attachments:
Description Flags
WIP patch
none
WIP Patch #2
none
Patch
none
Patch
none
Patch none

Mark Pilgrim (Google)
Reported 2012-09-12 06:44:33 PDT
[Chromium] Move getPluginsList out of PlatformSupport
Attachments
WIP patch (28.32 KB, patch)
2012-09-12 06:45 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
WIP Patch #2 (15.43 KB, patch)
2012-09-12 11:59 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
Patch (23.77 KB, patch)
2012-12-06 20:20 PST, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
Patch (23.81 KB, patch)
2012-12-07 13:57 PST, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
Patch (23.87 KB, patch)
2012-12-07 14:06 PST, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Mark Pilgrim (Google)
Comment 1 2012-09-12 06:45:00 PDT
Created attachment 163616 [details] WIP patch
Adam Barth
Comment 2 2012-09-12 09:55:56 PDT
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)
Comment 3 2012-09-12 11:08:39 PDT
No idea how that happened. My editor doesn't usually do this. Re-diffing...
Mark Pilgrim (Google)
Comment 4 2012-09-12 11:59:41 PDT
Created attachment 163668 [details] WIP Patch #2
WebKit Review Bot
Comment 5 2012-09-12 12:03:13 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
WebKit Review Bot
Comment 6 2012-09-12 12:10:40 PDT
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 7 2012-09-12 12:20:31 PDT
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 8 2012-09-12 12:24:07 PDT
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 9 2012-09-12 12:34:29 PDT
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 10 2012-09-12 12:41:31 PDT
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 11 2012-09-12 12:51:21 PDT
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 12 2012-09-12 15:40:08 PDT
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)
Comment 13 2012-12-06 20:20:11 PST
Created attachment 178144 [details] Patch
Mark Pilgrim (Google)
Comment 14 2012-12-06 20:21:02 PST
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 15 2012-12-07 13:27:52 PST
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)
Comment 16 2012-12-07 13:57:07 PST
Created attachment 178267 [details] Patch
Mark Pilgrim (Google)
Comment 17 2012-12-07 13:57:33 PST
Comment on attachment 178267 [details] Patch Interface renamed to PluginListBuilder
Darin Fisher (:fishd, Google)
Comment 18 2012-12-07 14:02:09 PST
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)
Comment 19 2012-12-07 14:06:46 PST
Created attachment 178271 [details] Patch
Mark Pilgrim (Google)
Comment 20 2012-12-07 14:07:09 PST
Comment on attachment 178271 [details] Patch moved PluginListBuilder to WebCore namespace
WebKit Review Bot
Comment 21 2012-12-10 10:23:29 PST
Comment on attachment 178271 [details] Patch Clearing flags on attachment: 178271 Committed r137177: <http://trac.webkit.org/changeset/137177>
WebKit Review Bot
Comment 22 2012-12-10 10:23:35 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.