Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(112)

Issue 3530017: This changes GetPluginInfo so that we can request a list of plugins (Closed)

Created:
10 years, 2 months ago by Greg Spencer (Chromium)
Modified:
9 years, 6 months ago
Reviewers:
jam, piman
CC:
chromium-reviews, brettw-cc_chromium.org, darin-cc_chromium.org, stuartmorgan+watch_chromium.org, ben+cc_chromium.org, pam+watch_chromium.org, jam
Visibility:
Public.

Description

This changes GetPluginInfo so that we can request a list of plugins that match the criterion (mime type/url) instead of just the first one that matches. This is in preparation for implementing a way for us to switch which flash plugin we select based on a whitelist of domains. BUG=http://crosbug.com/7403 TEST=ran chrome unit_tests, ran chromeos version of chrome, passed trybots. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=62261

Patch Set 1 #

Patch Set 2 : fixed comment #

Patch Set 3 : fixing test compile #

Patch Set 4 : Fixing more tests #

Total comments: 3

Patch Set 5 : review changes; fixed subtle bug with default plugin #

Patch Set 6 : Revamping info logic to return a complete list of plugins in order of desirability #

Patch Set 7 : simplified an if #

Total comments: 6

Patch Set 8 : fixing default case #

Patch Set 9 : Adding non-list API to plugin_list #

Total comments: 21

Patch Set 10 : added DCHECK #

Patch Set 11 : review changes #

Patch Set 12 : Renaming to GetPluginInfoArray #

Patch Set 13 : Fixed compile #

Patch Set 14 : merge with TOT #

Patch Set 15 : Fixing indents #

Unified diffs Side-by-side diffs Delta from patch set Stats (+216 lines, -191 lines) Patch
M chrome/browser/plugin_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/renderer_host/resource_message_filter.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +9 lines, -9 lines 0 comments Download
M chrome/browser/renderer_host/resource_message_filter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +29 lines, -25 lines 0 comments Download
M chrome/common/render_messages_internal.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +23 lines, -18 lines 0 comments Download
M chrome/renderer/render_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +37 lines, -40 lines 0 comments Download
M webkit/glue/plugins/plugin_list.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +18 lines, -24 lines 0 comments Download
M webkit/glue/plugins/plugin_list.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +96 lines, -66 lines 0 comments Download
M webkit/support/webkit_support.cc View 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +0 lines, -3 lines 0 comments Download
M webkit/tools/test_shell/test_webview_delegate.cc View 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -5 lines 0 comments Download

Messages

Total messages: 27 (0 generated)
Greg Spencer (Chromium)
10 years, 2 months ago (2010-10-07 21:45:09 UTC) #1
jam
(didn't look at the whole code) also, since most (5 out of 7) of the ...
10 years, 2 months ago (2010-10-07 21:59:12 UTC) #2
Greg Spencer (Chromium)
On 2010/10/07 21:59:12, John Abd-El-Malek wrote: > (didn't look at the whole code) > > ...
10 years, 2 months ago (2010-10-07 22:14:03 UTC) #3
Greg Spencer (Chromium)
http://codereview.chromium.org/3530017/diff/7001/8003 File chrome/browser/renderer_host/resource_message_filter.cc (right): http://codereview.chromium.org/3530017/diff/7001/8003#newcode791 chrome/browser/renderer_host/resource_message_filter.cc:791: actual_mime_types, policy_url, &info, reply_msg)); On 2010/10/07 21:59:13, John Abd-El-Malek ...
10 years, 2 months ago (2010-10-07 22:14:24 UTC) #4
jam
On 2010/10/07 22:14:03, Greg Spencer (Chromium) wrote: > On 2010/10/07 21:59:12, John Abd-El-Malek wrote: > ...
10 years, 2 months ago (2010-10-07 22:23:24 UTC) #5
piman
http://codereview.chromium.org/3530017/diff/7001/8007 File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/3530017/diff/7001/8007#newcode460 webkit/glue/plugins/plugin_list.cc:460: } I have to say, the logic is really ...
10 years, 2 months ago (2010-10-07 22:28:10 UTC) #6
Greg Spencer (Chromium)
On 2010/10/07 22:28:10, piman wrote: > http://codereview.chromium.org/3530017/diff/7001/8007 > File webkit/glue/plugins/plugin_list.cc (right): > > http://codereview.chromium.org/3530017/diff/7001/8007#newcode460 > ...
10 years, 2 months ago (2010-10-08 19:40:35 UTC) #7
jam
On 2010/10/07 22:23:24, John Abd-El-Malek wrote: > On 2010/10/07 22:14:03, Greg Spencer (Chromium) wrote: > ...
10 years, 2 months ago (2010-10-08 19:53:46 UTC) #8
Greg Spencer (Chromium)
On 2010/10/08 19:53:46, John Abd-El-Malek wrote: > ping. I still have concerns about this new ...
10 years, 2 months ago (2010-10-08 20:16:03 UTC) #9
piman
http://codereview.chromium.org/3530017/diff/21001/22007 File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/3530017/diff/21001/22007#newcode411 webkit/glue/plugins/plugin_list.cc:411: visited_plugins.find(path) == visited_plugins.end()) { one way to avoid doing ...
10 years, 2 months ago (2010-10-08 20:20:02 UTC) #10
Greg Spencer (Chromium)
http://codereview.chromium.org/3530017/diff/21001/22007 File webkit/glue/plugins/plugin_list.cc (right): http://codereview.chromium.org/3530017/diff/21001/22007#newcode411 webkit/glue/plugins/plugin_list.cc:411: visited_plugins.find(path) == visited_plugins.end()) { On 2010/10/08 20:20:02, piman wrote: ...
10 years, 2 months ago (2010-10-08 20:39:19 UTC) #11
piman
LGTM, but check with John on his request.
10 years, 2 months ago (2010-10-08 20:41:34 UTC) #12
jam
On Fri, Oct 8, 2010 at 1:16 PM, <gspencer@chromium.org> wrote: > On 2010/10/08 19:53:46, John ...
10 years, 2 months ago (2010-10-08 20:51:31 UTC) #13
Greg Spencer
On Fri, Oct 8, 2010 at 1:43 PM, John Abd-El-Malek <jam@chromium.org> wrote: > I'm not ...
10 years, 2 months ago (2010-10-08 20:52:56 UTC) #14
jam
On Fri, Oct 8, 2010 at 1:51 PM, Greg Spencer <gspencer@google.com> wrote: > On Fri, ...
10 years, 2 months ago (2010-10-08 20:54:14 UTC) #15
Greg Spencer (Chromium)
OK, I've added the non-list API. Please take another look.
10 years, 2 months ago (2010-10-08 21:15:26 UTC) #16
jam
http://codereview.chromium.org/3530017/diff/30001/20011 File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/3530017/diff/30001/20011#newcode252 chrome/browser/plugin_service.cc:252: url, mime_type, allow_wildcard, &info, NULL) && info.enabled) why take ...
10 years, 2 months ago (2010-10-08 21:36:41 UTC) #17
jam
http://codereview.chromium.org/3530017/diff/30001/20011 File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/3530017/diff/30001/20011#newcode252 chrome/browser/plugin_service.cc:252: url, mime_type, allow_wildcard, &info, NULL) && info.enabled) why take ...
10 years, 2 months ago (2010-10-08 21:36:42 UTC) #18
Greg Spencer (Chromium)
http://codereview.chromium.org/3530017/diff/30001/20011 File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/3530017/diff/30001/20011#newcode252 chrome/browser/plugin_service.cc:252: url, mime_type, allow_wildcard, &info, NULL) && info.enabled) On 2010/10/08 ...
10 years, 2 months ago (2010-10-08 22:38:46 UTC) #19
jam
http://codereview.chromium.org/3530017/diff/30001/20011 File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/3530017/diff/30001/20011#newcode252 chrome/browser/plugin_service.cc:252: url, mime_type, allow_wildcard, &info, NULL) && info.enabled) On 2010/10/08 ...
10 years, 2 months ago (2010-10-08 22:50:27 UTC) #20
Greg Spencer (Chromium)
http://codereview.chromium.org/3530017/diff/30001/20016 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3530017/diff/30001/20016#newcode573 chrome/renderer/render_view.cc:573: if (info.empty() || !info[0].enabled) On 2010/10/08 22:50:27, John Abd-El-Malek ...
10 years, 2 months ago (2010-10-08 23:18:32 UTC) #21
jam
http://codereview.chromium.org/3530017/diff/30001/20016 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3530017/diff/30001/20016#newcode573 chrome/renderer/render_view.cc:573: if (info.empty() || !info[0].enabled) On 2010/10/08 23:18:32, Greg Spencer ...
10 years, 2 months ago (2010-10-08 23:33:11 UTC) #22
piman
On Fri, Oct 8, 2010 at 4:33 PM, <jam@chromium.org> wrote: > > http://codereview.chromium.org/3530017/diff/30001/20016 > File ...
10 years, 2 months ago (2010-10-08 23:37:02 UTC) #23
jam
On Fri, Oct 8, 2010 at 4:36 PM, Antoine Labour <piman@chromium.org> wrote: > > > ...
10 years, 2 months ago (2010-10-09 00:09:43 UTC) #24
piman
On Fri, Oct 8, 2010 at 5:09 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > ...
10 years, 2 months ago (2010-10-09 00:16:55 UTC) #25
Greg Spencer (Chromium)
OK, I've renamed things. John, are you OK with this change now? http://codereview.chromium.org/3530017/diff/30001/20018 File webkit/glue/plugins/plugin_list.h ...
10 years, 2 months ago (2010-10-11 16:17:11 UTC) #26
jam
10 years, 2 months ago (2010-10-11 20:34:17 UTC) #27
lgtm

On Mon, Oct 11, 2010 at 9:17 AM, <gspencer@chromium.org> wrote:

> OK, I've renamed things.  John, are you OK with this change now?
>
>
>
> http://codereview.chromium.org/3530017/diff/30001/20018
> File webkit/glue/plugins/plugin_list.h (right):
>
> http://codereview.chromium.org/3530017/diff/30001/20018#newcode154
> webkit/glue/plugins/plugin_list.h:154: bool GetPluginInfoBestMatch(const
> GURL& url,
> On 2010/10/08 23:33:11, John Abd-El-Malek wrote:
>
>> I'll just note that I would prefer GetPluginInfoArray/GetPluginInfo
>>
> and leave it
>
>> up to you.
>>
>
> Oooh.  GetPluginInfoArray.  I like it.  Sold.
>
>
> http://codereview.chromium.org/3530017/show
>

Powered by Google App Engine
This is Rietveld 408576698