|
|
Created:
10 years, 2 months ago by Greg Spencer (Chromium) Modified:
9 years, 6 months ago 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. |
DescriptionThis 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 #Messages
Total messages: 27 (0 generated)
(didn't look at the whole code) also, since most (5 out of 7) of the callers of this function only want one item, I think it would make the calling code easier to read if you had two versions of this function, one that took in a vector and one like before. 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)); you're passing a pointer of a local variable to a different thread
On 2010/10/07 21:59:12, John Abd-El-Malek wrote: > (didn't look at the whole code) > > also, since most (5 out of 7) of the callers of this function only want one > item, I think it would make the calling code easier to read if you had two > versions of this function, one that took in a vector and one like before. Well, except that I'm about to change that in a later CL -- in the production code (for ChromeOS only) we're going to have some code that goes through the returned list instead of just taking the first one. Two of the callers are also tests, which I'm less inclined to make separate APIs for.
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 wrote: > you're passing a pointer of a local variable to a different thread Whoops. Thanks.
On 2010/10/07 22:14:03, Greg Spencer (Chromium) wrote: > On 2010/10/07 21:59:12, John Abd-El-Malek wrote: > > (didn't look at the whole code) > > > > also, since most (5 out of 7) of the callers of this function only want one > > item, I think it would make the calling code easier to read if you had two > > versions of this function, one that took in a vector and one like before. > > Well, except that I'm about to change that in a later CL -- in the production > code (for ChromeOS only) we're going to have some code that goes through the > returned list instead of just taking the first one. Two of the callers are also > tests, which I'm less inclined to make separate APIs for. This is such a simple addition though, that moves 5 duplicate blocks that deal with the vector and make the client code easier to read. I think in this case it's well worth having two versions of GetPluginInfo. Basically, I don't think the fact that two or three callers need a vector should necessitate changing all the other places.
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 confusing. Not your fault, and I understand that you are trying to match the previous behavior which was already confusing. Couple of ways to help: - remove the overload on FindPlugins (e.g. FindPluginsByExtension) - simply return after FindPluginsByExtension if it returns non-empty. That way you can avoid the logic around fill_mimetypes Also, I think if FindDisabledPlugins fails, we want to return the default plugin if it matches (that's what would have happened previously). TBH though, I think with this rework we may want to return a list of: - all the enabled plugins that match the requested mime type - all the enabled plugins that match the extension - all the disabled plugins that match the requested mime type - the default plugin (if it matches/allow_wildcard is on). And then we can push the decision logic to the callers.
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 > webkit/glue/plugins/plugin_list.cc:460: } > I have to say, the logic is really confusing. Not your fault, and I understand > that you are trying to match the previous behavior which was already confusing. > > Couple of ways to help: > - remove the overload on FindPlugins (e.g. FindPluginsByExtension) > - simply return after FindPluginsByExtension if it returns non-empty. That way > you can avoid the logic around fill_mimetypes > > Also, I think if FindDisabledPlugins fails, we want to return the default plugin > if it matches (that's what would have happened previously). > > > TBH though, I think with this rework we may want to return a list of: > - all the enabled plugins that match the requested mime type > - all the enabled plugins that match the extension > - all the disabled plugins that match the requested mime type > - the default plugin (if it matches/allow_wildcard is on). > > And then we can push the decision logic to the callers. OK, I implemented what we talked about. The changes are all in plugin_list.{cc,h}
On 2010/10/07 22:23:24, John Abd-El-Malek wrote: > On 2010/10/07 22:14:03, Greg Spencer (Chromium) wrote: > > On 2010/10/07 21:59:12, John Abd-El-Malek wrote: > > > (didn't look at the whole code) > > > > > > also, since most (5 out of 7) of the callers of this function only want one > > > item, I think it would make the calling code easier to read if you had two > > > versions of this function, one that took in a vector and one like before. > > > > Well, except that I'm about to change that in a later CL -- in the production > > code (for ChromeOS only) we're going to have some code that goes through the > > returned list instead of just taking the first one. Two of the callers are > also > > tests, which I'm less inclined to make separate APIs for. > > This is such a simple addition though, that moves 5 duplicate blocks that deal > with the vector and make the client code easier to read. I think in this case > it's well worth having two versions of GetPluginInfo. Basically, I don't think > the fact that two or three callers need a vector should necessitate changing all > the other places. ping. I still have concerns about this new API.
On 2010/10/08 19:53:46, John Abd-El-Malek wrote: > ping. I still have concerns about this new API. Yes, I know... One step at a time. :) So, of the total of six call sites in the code, two of them are only for tests, one is a pass-through for RPC, and two of them are going to change in the next CL to not simply select the first item. There will only be one caller left (BufferedResourceHandler::ShouldDownload) of the non-list API. Is it still worth it? -Greg.
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 both find and insert into the set is by just calling insert, that will return a bool to know if it's inserted or not (i.e. wasn't there or not). http://codereview.chromium.org/3530017/diff/21001/22007#newcode436 webkit/glue/plugins/plugin_list.cc:436: // Always add the default plugin at the end. we should only do that if SupportsType(plugin, mime_type, allow_wildcard) is true. It may be equivalent to allow_wildcard == true, but I'm not positive. http://codereview.chromium.org/3530017/diff/21001/22007#newcode438 webkit/glue/plugins/plugin_list.cc:438: info->push_back(plugins_[plugins_.size() - 1]); could you add a DCHECK that it is the default plugin (path == kDefaultPluginLibraryName) ?
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: > one way to avoid doing both find and insert into the set is by just calling > insert, that will return a bool to know if it's inserted or not (i.e. wasn't > there or not). Right. I forgot about that. Done. http://codereview.chromium.org/3530017/diff/21001/22007#newcode436 webkit/glue/plugins/plugin_list.cc:436: // Always add the default plugin at the end. On 2010/10/08 20:20:02, piman wrote: > we should only do that if SupportsType(plugin, mime_type, allow_wildcard) is > true. It may be equivalent to allow_wildcard == true, but I'm not positive. Done. http://codereview.chromium.org/3530017/diff/21001/22007#newcode438 webkit/glue/plugins/plugin_list.cc:438: info->push_back(plugins_[plugins_.size() - 1]); On 2010/10/08 20:20:02, piman wrote: > could you add a DCHECK that it is the default plugin (path == > kDefaultPluginLibraryName) ? Done.
LGTM, but check with John on his request.
On Fri, Oct 8, 2010 at 1:16 PM, <gspencer@chromium.org> wrote: > On 2010/10/08 19:53:46, John Abd-El-Malek wrote: > >> ping. I still have concerns about this new API. >> > > Yes, I know... One step at a time. :) > > So, of the total of six call sites in the code, two of them are only for > tests, > one is a pass-through for RPC, and two of them are going to change in the > next > CL to not simply select the first item. > > There will only be one caller left > (BufferedResourceHandler::ShouldDownload) of > the non-list API. > I'm not sure why you think tests don't count. If we make writing testing more verbose, there will be less of them written. Given that 3 places don't need a vector API, and the fact that jut adding another function that calls it and returns the first is easy, I don't think we should make these three original callers more verbose when they don't need to. Also, does the cros bug really need to be private? I don't have access to it so I can't read it. > Is it still worth it? > > -Greg. > > > http://codereview.chromium.org/3530017/show >
On Fri, Oct 8, 2010 at 1:43 PM, John Abd-El-Malek <jam@chromium.org> wrote: > I'm not sure why you think tests don't count. If we make writing testing > more verbose, there will be less of them written. Given that 3 places don't > need a vector API, and the fact that jut adding another function that calls > it and returns the first is easy, I don't think we should make these three > original callers more verbose when they don't need to. > I do think tests count. But if there's a list API and a non-list API, they should call both of them (to test them both). If there's only one API, obviously they should test only that API. I'll add the non-list API. Also, does the cros bug really need to be private? I don't have access to > it so I can't read it. > Yes, it does need to be private. -Greg.
On Fri, Oct 8, 2010 at 1:51 PM, Greg Spencer <gspencer@google.com> wrote: > On Fri, Oct 8, 2010 at 1:43 PM, John Abd-El-Malek <jam@chromium.org> wrote > : > >> I'm not sure why you think tests don't count. If we make writing testing >> more verbose, there will be less of them written. Given that 3 places don't >> need a vector API, and the fact that jut adding another function that calls >> it and returns the first is easy, I don't think we should make these three >> original callers more verbose when they don't need to. >> > > I do think tests count. But if there's a list API and a non-list API, they > should call both of them (to test them both). If there's only one API, > obviously they should test only that API. > If one of the functions just passes through to the other, then we don't need to test the vector version. The tests aren't to check that we can pull one item out of a vector correctly, it's for the meaty plugin part.. > > I'll add the non-list API. > > Also, does the cros bug really need to be private? I don't have access to >> it so I can't read it. >> > > Yes, it does need to be private. > > -Greg. >
OK, I've added the non-list API. Please take another look.
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 out the brace brackets? they're needed per the style guide (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...) http://codereview.chromium.org/3530017/diff/30001/20013 File chrome/browser/renderer_host/resource_message_filter.cc (right): http://codereview.chromium.org/3530017/diff/30001/20013#newcode799 chrome/browser/renderer_host/resource_message_filter.cc:799: std::vector<WebPluginInfo> local_info(info); nit: you can just change the parameter to not have const reference, then you can edit it directly. 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) why send the vector if only the first one is taken? what'sthe plan for picking the plugin this in the future, will it be render or browser side? Can you add me to the bug? I'd like to better understand what the criteria is for choosing a plugin since there are security implications. It seems that we want this decision to be made in the browser process, to avoid an exploited renderer choosing to get non-sandboxed flash to run. 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, Isn't BestMatch redundant, i.e. the caller wants only one WebPluginInfo returned. http://codereview.chromium.org/3530017/diff/30001/20019 File webkit/support/webkit_support.cc (right): http://codereview.chromium.org/3530017/diff/30001/20019#newcode253 webkit/support/webkit_support.cc:253: NPAPI::PluginList::Singleton()->GetPluginInfo( this should just call the single plugin version of GetPluginInfo http://codereview.chromium.org/3530017/diff/30001/20020 File webkit/tools/test_shell/test_webview_delegate.cc (left): http://codereview.chromium.org/3530017/diff/30001/20020#oldcode710 webkit/tools/test_shell/test_webview_delegate.cc:710: actual_mime_type = params.mimeType.utf8(); why take this out?
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 out the brace brackets? they're needed per the style guide (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...) http://codereview.chromium.org/3530017/diff/30001/20013 File chrome/browser/renderer_host/resource_message_filter.cc (right): http://codereview.chromium.org/3530017/diff/30001/20013#newcode799 chrome/browser/renderer_host/resource_message_filter.cc:799: std::vector<WebPluginInfo> local_info(info); nit: you can just change the parameter to not have const reference, then you can edit it directly. 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) why send the vector if only the first one is taken? what'sthe plan for picking the plugin this in the future, will it be render or browser side? Can you add me to the bug? I'd like to better understand what the criteria is for choosing a plugin since there are security implications. It seems that we want this decision to be made in the browser process, to avoid an exploited renderer choosing to get non-sandboxed flash to run. 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, Isn't BestMatch redundant, i.e. the caller wants only one WebPluginInfo returned. http://codereview.chromium.org/3530017/diff/30001/20019 File webkit/support/webkit_support.cc (right): http://codereview.chromium.org/3530017/diff/30001/20019#newcode253 webkit/support/webkit_support.cc:253: NPAPI::PluginList::Singleton()->GetPluginInfo( this should just call the single plugin version of GetPluginInfo http://codereview.chromium.org/3530017/diff/30001/20020 File webkit/tools/test_shell/test_webview_delegate.cc (left): http://codereview.chromium.org/3530017/diff/30001/20020#oldcode710 webkit/tools/test_shell/test_webview_delegate.cc:710: actual_mime_type = params.mimeType.utf8(); why take this out?
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 21:36:42, John Abd-El-Malek wrote: > why take out the brace brackets? they're needed per the style guide. Actually, it says you don't need them, but can add them if you like them. And in other parts of the code it's been convention to not have braces on single line statements, so now I'm in the habit of removing them, sorry. That said, I actually prefer to have braces so I'll add 'em back in. http://codereview.chromium.org/3530017/diff/30001/20013 File chrome/browser/renderer_host/resource_message_filter.cc (right): http://codereview.chromium.org/3530017/diff/30001/20013#newcode799 chrome/browser/renderer_host/resource_message_filter.cc:799: std::vector<WebPluginInfo> local_info(info); On 2010/10/08 21:36:42, John Abd-El-Malek wrote: > nit: you can just change the parameter to not have const reference, then you can > edit it directly. Done. This does make it an implicit copy rather than explicit copy, which I'm not fond of, but does read a little better. 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 21:36:42, John Abd-El-Malek wrote: > why send the vector if only the first one is taken? It looks a little strange here because I'm not including the plugin selection logic: I'm trying to keep this CL small and simple. When the logic is added, it will use more of the list than the first item. Also, I really don't want to have two separate RPC APIs (one with a list, and one without. The point of this CL is to add a list based API so that we can make more involved decisions (replace info[0] below with some logic) about which plugin to load at a higher level. We can discuss where and how the decision should be made, but either way we'll need a list to make the decision from, because PluginList is far too low level to make the decision. I added you to the bug. 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 21:36:42, John Abd-El-Malek wrote: > Isn't BestMatch redundant, i.e. the caller wants only one WebPluginInfo > returned. Well, but there's no plural for "info", so then it would have the same name as the list-based one. Maybe GetFirstPluginInfo? But the first one is first because it's the best match, so I ended up with what you see. GetOnePluginInfo? http://codereview.chromium.org/3530017/diff/30001/20019 File webkit/support/webkit_support.cc (right): http://codereview.chromium.org/3530017/diff/30001/20019#newcode253 webkit/support/webkit_support.cc:253: NPAPI::PluginList::Singleton()->GetPluginInfo( On 2010/10/08 21:36:42, John Abd-El-Malek wrote: > this should just call the single plugin version of GetPluginInfo Yeah... Thought I did that. Looks like I forgot to save it. http://codereview.chromium.org/3530017/diff/30001/20020 File webkit/tools/test_shell/test_webview_delegate.cc (left): http://codereview.chromium.org/3530017/diff/30001/20020#oldcode710 webkit/tools/test_shell/test_webview_delegate.cc:710: actual_mime_type = params.mimeType.utf8(); On 2010/10/08 21:36:42, John Abd-El-Malek wrote: > why take this out? Because this was working around a shortcoming in the old PluginList code where it wouldn't set the mime type unless it happened to try and resolve it via the URL. That's fixed in the new version (it always gets set if you ask for it), so I removed this bandaid.
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 22:38:46, Greg Spencer (Chromium) wrote: > On 2010/10/08 21:36:42, John Abd-El-Malek wrote: > > why take out the brace brackets? they're needed per the style guide. > > Actually, it says you don't need them, but can add them if you like them. > > And in other parts of the code it's been convention to not have braces on single > line statements, so now I'm in the habit of removing them, sorry. > > That said, I actually prefer to have braces so I'll add 'em back in. oh, I actually always thought that the rule was to put them if the if statements take more than 2 lines (including the condition). Looks like I was incorrect, so ignore me :) 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:38:46, Greg Spencer (Chromium) wrote: > On 2010/10/08 21:36:42, John Abd-El-Malek wrote: > > why send the vector if only the first one is taken? > > It looks a little strange here because I'm not including the plugin selection > logic: I'm trying to keep this CL small and simple. When the logic is added, it > will use more of the list than the first item. > > Also, I really don't want to have two separate RPC APIs (one with a list, and > one without. The point of this CL is to add a list based API so that we can > make more involved decisions (replace info[0] below with some logic) about which > plugin to load at a higher level. We can discuss where and how the decision > should be made, but either way we'll need a list to make the decision from, > because PluginList is far too low level to make the decision. > > I added you to the bug. If the decision is made in the browser process, which I'm arguing it should be, then the IPC message won't take a vector. The vector doesn't need to be passed to the renderer process. 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 22:38:46, Greg Spencer (Chromium) wrote: > On 2010/10/08 21:36:42, John Abd-El-Malek wrote: > > Isn't BestMatch redundant, i.e. the caller wants only one WebPluginInfo > > returned. > > Well, but there's no plural for "info", so then it would have the same name as > the list-based one. I think calling it the same is preferable (that's what I was getting at earlier, if not clearly enough). i.e. just because new code wants to get an array doesn't mean that the old code which doesn't care about an array needs to change > Maybe GetFirstPluginInfo? But the first one is first > because it's the best match, so I ended up with what you see. GetOnePluginInfo? http://codereview.chromium.org/3530017/diff/30001/20020 File webkit/tools/test_shell/test_webview_delegate.cc (left): http://codereview.chromium.org/3530017/diff/30001/20020#oldcode710 webkit/tools/test_shell/test_webview_delegate.cc:710: actual_mime_type = params.mimeType.utf8(); On 2010/10/08 22:38:46, Greg Spencer (Chromium) wrote: > On 2010/10/08 21:36:42, John Abd-El-Malek wrote: > > why take this out? > > Because this was working around a shortcoming in the old PluginList code where > it wouldn't set the mime type unless it happened to try and resolve it via the > URL. That's fixed in the new version (it always gets set if you ask for it), so > I removed this bandaid. I see, ok.
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 wrote: > If the decision is made in the browser process, which I'm arguing it should be, > then the IPC message won't take a vector. The vector doesn't need to be passed > to the renderer process. So, it sounds like with what you're arguing that we should also move all plugin loading into a separate process (not the browser and not the renderer -- which I'm pretty sure couldn't even work)? Because as long as it's Webkit loading the plugins in the end, a compromised renderer could rewrite which one to use after the decision has been made just as easily as it could make the decision come out in its favor, so we can't just make the decision in the browser and send it back. I guess I don't quite understand the details of what you are proposing. Can you outline it a little better? And maybe I didn't mention this before, but this selection scheme for plugins is something that we're only doing for ChromeOS Beta, and will eventually not be needed anymore. That's not to say we're OK with it being less secure, of course. 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 22:50:27, John Abd-El-Malek wrote: > > Well, but there's no plural for "info", so then it would have the same name as > > the list-based one. > > I think calling it the same is preferable (that's what I was getting at earlier, > if not clearly enough). i.e. just because new code wants to get an array > doesn't mean that the old code which doesn't care about an array needs to change Well, while the style guide doesn't forbid it, it certainly strongly suggests that instead of overloading a function it is better to name it something similar. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... "Decision: If you want to overload a function, consider qualifying the name with some information about the arguments, e.g., AppendString(), AppendInt() rather than just Append()."
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 (Chromium) wrote: > On 2010/10/08 22:50:27, John Abd-El-Malek wrote: > > If the decision is made in the browser process, which I'm arguing it should > be, > > then the IPC message won't take a vector. The vector doesn't need to be > passed > > to the renderer process. > > So, it sounds like with what you're arguing that we should also move all plugin > loading into a separate process (not the browser and not the renderer -- which > I'm pretty sure couldn't even work)? Because as long as it's Webkit loading the > plugins in the end, a compromised renderer could rewrite which one to use after > the decision has been made just as easily as it could make the decision come out > in its favor, so we can't just make the decision in the browser and send it > back. I overlooked the fact that the renderer tells the browser right afterwards which one to use (but note, if we really wanted to, we can have the browser cache this just like it caches which list of files the renderer has access to from the open-file dialog). Ignoring the security issue for now, my question is where is the decision to pick which plugin going to be made? If it's made in the browser process, then the IPC doesn't have to change. > > I guess I don't quite understand the details of what you are proposing. Can you > outline it a little better? > > And maybe I didn't mention this before, but this selection scheme for plugins is > something that we're only doing for ChromeOS Beta, and will eventually not be > needed anymore. That's not to say we're OK with it being less secure, of > course. 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:18:32, Greg Spencer (Chromium) wrote: > On 2010/10/08 22:50:27, John Abd-El-Malek wrote: > > > Well, but there's no plural for "info", so then it would have the same name > as > > > the list-based one. > > > > I think calling it the same is preferable (that's what I was getting at > earlier, > > if not clearly enough). i.e. just because new code wants to get an array > > doesn't mean that the old code which doesn't care about an array needs to > change > > Well, while the style guide doesn't forbid it, it certainly strongly suggests > that instead of overloading a function it is better to name it something > similar. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > > "Decision: If you want to overload a function, consider qualifying the name with > some information about the arguments, e.g., AppendString(), AppendInt() rather > than just Append()." > I think in this case, there's not much confusion and the reader doesn't have to be familiar with C++'s matching rules.. This is getting into style nits, so I'll just note that I would prefer GetPluginInfoArray/GetPluginInfo and leave it up to you.
On Fri, Oct 8, 2010 at 4:33 PM, <jam@chromium.org> wrote: > > 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 (Chromium) wrote: > >> On 2010/10/08 22:50:27, John Abd-El-Malek wrote: >> > If the decision is made in the browser process, which I'm arguing it >> > should > >> be, >> > then the IPC message won't take a vector. The vector doesn't need >> > to be > >> passed >> > to the renderer process. >> > > So, it sounds like with what you're arguing that we should also move >> > all plugin > >> loading into a separate process (not the browser and not the renderer >> > -- which > >> I'm pretty sure couldn't even work)? Because as long as it's Webkit >> > loading the > >> plugins in the end, a compromised renderer could rewrite which one to >> > use after > >> the decision has been made just as easily as it could make the >> > decision come out > >> in its favor, so we can't just make the decision in the browser and >> > send it > >> back. >> > > I overlooked the fact that the renderer tells the browser right > afterwards which one to use (but note, if we really wanted to, we can > have the browser cache this just like it caches which list of files the > renderer has access to from the open-file dialog). > > Ignoring the security issue for now, my question is where is the > decision to pick which plugin going to be made? If it's made in the > browser process, then the IPC doesn't have to change. It's not for this change, but I want the renderer to be able to make the choice. The reason is that we may try to instantiate the pepper plugin, which might fail (e.g. ppapi version mismatch), in which case we can fallback to a regular NPAPI plugin. The browser can't instantiate the pepper plugin so it can't make that choice. Antoine > > > > I guess I don't quite understand the details of what you are >> > proposing. Can you > >> outline it a little better? >> > > And maybe I didn't mention this before, but this selection scheme for >> > plugins is > >> something that we're only doing for ChromeOS Beta, and will eventually >> > not be > >> needed anymore. That's not to say we're OK with it being less secure, >> > of > >> course. >> > > 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:18:32, Greg Spencer (Chromium) wrote: > >> On 2010/10/08 22:50:27, John Abd-El-Malek wrote: >> > > Well, but there's no plural for "info", so then it would have the >> > same name > >> as >> > > the list-based one. >> > >> > I think calling it the same is preferable (that's what I was getting >> > at > >> earlier, >> > if not clearly enough). i.e. just because new code wants to get an >> > array > >> > doesn't mean that the old code which doesn't care about an array >> > needs to > >> change >> > > Well, while the style guide doesn't forbid it, it certainly strongly >> > suggests > >> that instead of overloading a function it is better to name it >> > something > >> similar. >> > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > > "Decision: If you want to overload a function, consider qualifying the >> > name with > >> some information about the arguments, e.g., AppendString(), >> > AppendInt() rather > >> than just Append()." >> > > > I think in this case, there's not much confusion and the reader doesn't > have to be familiar with C++'s matching rules.. This is getting into > style nits, so I'll just note that I would prefer > GetPluginInfoArray/GetPluginInfo and leave it up to you. > > > http://codereview.chromium.org/3530017/show >
On Fri, Oct 8, 2010 at 4:36 PM, Antoine Labour <piman@chromium.org> wrote: > > > On Fri, Oct 8, 2010 at 4:33 PM, <jam@chromium.org> wrote: > >> >> 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 (Chromium) wrote: >> >>> On 2010/10/08 22:50:27, John Abd-El-Malek wrote: >>> > If the decision is made in the browser process, which I'm arguing it >>> >> should >> >>> be, >>> > then the IPC message won't take a vector. The vector doesn't need >>> >> to be >> >>> passed >>> > to the renderer process. >>> >> >> So, it sounds like with what you're arguing that we should also move >>> >> all plugin >> >>> loading into a separate process (not the browser and not the renderer >>> >> -- which >> >>> I'm pretty sure couldn't even work)? Because as long as it's Webkit >>> >> loading the >> >>> plugins in the end, a compromised renderer could rewrite which one to >>> >> use after >> >>> the decision has been made just as easily as it could make the >>> >> decision come out >> >>> in its favor, so we can't just make the decision in the browser and >>> >> send it >> >>> back. >>> >> >> I overlooked the fact that the renderer tells the browser right >> afterwards which one to use (but note, if we really wanted to, we can >> have the browser cache this just like it caches which list of files the >> renderer has access to from the open-file dialog). >> >> Ignoring the security issue for now, my question is where is the >> decision to pick which plugin going to be made? If it's made in the >> browser process, then the IPC doesn't have to change. > > > It's not for this change, but I want the renderer to be able to make the > choice. The reason is that we may try to instantiate the pepper plugin, > which might fail (e.g. ppapi version mismatch), in which case we can > fallback to a regular NPAPI plugin. The browser can't instantiate the pepper > plugin so it can't make that choice. > If we're shipping incompatible version of pepper flash, don't we have bigger issues to worry about? but if you really think this is ok, and bandaid-ing it by falling back in the renderer process is ok, then ok. > > Antoine > > >> >> >> >> I guess I don't quite understand the details of what you are >>> >> proposing. Can you >> >>> outline it a little better? >>> >> >> And maybe I didn't mention this before, but this selection scheme for >>> >> plugins is >> >>> something that we're only doing for ChromeOS Beta, and will eventually >>> >> not be >> >>> needed anymore. That's not to say we're OK with it being less secure, >>> >> of >> >>> course. >>> >> >> 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:18:32, Greg Spencer (Chromium) wrote: >> >>> On 2010/10/08 22:50:27, John Abd-El-Malek wrote: >>> > > Well, but there's no plural for "info", so then it would have the >>> >> same name >> >>> as >>> > > the list-based one. >>> > >>> > I think calling it the same is preferable (that's what I was getting >>> >> at >> >>> earlier, >>> > if not clearly enough). i.e. just because new code wants to get an >>> >> array >> >>> > doesn't mean that the old code which doesn't care about an array >>> >> needs to >> >>> change >>> >> >> Well, while the style guide doesn't forbid it, it certainly strongly >>> >> suggests >> >>> that instead of overloading a function it is better to name it >>> >> something >> >>> similar. >>> >> >> >> >> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... >> >> "Decision: If you want to overload a function, consider qualifying the >>> >> name with >> >>> some information about the arguments, e.g., AppendString(), >>> >> AppendInt() rather >> >>> than just Append()." >>> >> >> >> I think in this case, there's not much confusion and the reader doesn't >> have to be familiar with C++'s matching rules.. This is getting into >> style nits, so I'll just note that I would prefer >> GetPluginInfoArray/GetPluginInfo and leave it up to you. >> >> >> http://codereview.chromium.org/3530017/show >> > >
On Fri, Oct 8, 2010 at 5:09 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Fri, Oct 8, 2010 at 4:36 PM, Antoine Labour <piman@chromium.org> wrote: > >> >> >> On Fri, Oct 8, 2010 at 4:33 PM, <jam@chromium.org> wrote: >> >>> >>> 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 (Chromium) wrote: >>> >>>> On 2010/10/08 22:50:27, John Abd-El-Malek wrote: >>>> > If the decision is made in the browser process, which I'm arguing it >>>> >>> should >>> >>>> be, >>>> > then the IPC message won't take a vector. The vector doesn't need >>>> >>> to be >>> >>>> passed >>>> > to the renderer process. >>>> >>> >>> So, it sounds like with what you're arguing that we should also move >>>> >>> all plugin >>> >>>> loading into a separate process (not the browser and not the renderer >>>> >>> -- which >>> >>>> I'm pretty sure couldn't even work)? Because as long as it's Webkit >>>> >>> loading the >>> >>>> plugins in the end, a compromised renderer could rewrite which one to >>>> >>> use after >>> >>>> the decision has been made just as easily as it could make the >>>> >>> decision come out >>> >>>> in its favor, so we can't just make the decision in the browser and >>>> >>> send it >>> >>>> back. >>>> >>> >>> I overlooked the fact that the renderer tells the browser right >>> afterwards which one to use (but note, if we really wanted to, we can >>> have the browser cache this just like it caches which list of files the >>> renderer has access to from the open-file dialog). >>> >>> Ignoring the security issue for now, my question is where is the >>> decision to pick which plugin going to be made? If it's made in the >>> browser process, then the IPC doesn't have to change. >> >> >> It's not for this change, but I want the renderer to be able to make the >> choice. The reason is that we may try to instantiate the pepper plugin, >> which might fail (e.g. ppapi version mismatch), in which case we can >> fallback to a regular NPAPI plugin. The browser can't instantiate the pepper >> plugin so it can't make that choice. >> > > If we're shipping incompatible version of pepper flash, don't we have > bigger issues to worry about? > There's shipping and there's developer workflow. Flash is broken 95% of the time on trunk because of churn in ppapi. Right now it crashes the renderer, very soon it will display a "plugin not available" (even though there is an available, working plugin), which is better but it still prevent people from using the system. > but if you really think this is ok, and bandaid-ing it by falling back in > the renderer process is ok, then ok. > It's not restricted to the couple of plugins we ship. In the future with arbitrary nacl plugins we'll have the same issue. Antoine > >> Antoine >> >> >>> >>> >>> >>> I guess I don't quite understand the details of what you are >>>> >>> proposing. Can you >>> >>>> outline it a little better? >>>> >>> >>> And maybe I didn't mention this before, but this selection scheme for >>>> >>> plugins is >>> >>>> something that we're only doing for ChromeOS Beta, and will eventually >>>> >>> not be >>> >>>> needed anymore. That's not to say we're OK with it being less secure, >>>> >>> of >>> >>>> course. >>>> >>> >>> 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:18:32, Greg Spencer (Chromium) wrote: >>> >>>> On 2010/10/08 22:50:27, John Abd-El-Malek wrote: >>>> > > Well, but there's no plural for "info", so then it would have the >>>> >>> same name >>> >>>> as >>>> > > the list-based one. >>>> > >>>> > I think calling it the same is preferable (that's what I was getting >>>> >>> at >>> >>>> earlier, >>>> > if not clearly enough). i.e. just because new code wants to get an >>>> >>> array >>> >>>> > doesn't mean that the old code which doesn't care about an array >>>> >>> needs to >>> >>>> change >>>> >>> >>> Well, while the style guide doesn't forbid it, it certainly strongly >>>> >>> suggests >>> >>>> that instead of overloading a function it is better to name it >>>> >>> something >>> >>>> similar. >>>> >>> >>> >>> >>> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... >>> >>> "Decision: If you want to overload a function, consider qualifying the >>>> >>> name with >>> >>>> some information about the arguments, e.g., AppendString(), >>>> >>> AppendInt() rather >>> >>>> than just Append()." >>>> >>> >>> >>> I think in this case, there's not much confusion and the reader doesn't >>> have to be familiar with C++'s matching rules.. This is getting into >>> style nits, so I'll just note that I would prefer >>> GetPluginInfoArray/GetPluginInfo and leave it up to you. >>> >>> >>> http://codereview.chromium.org/3530017/show >>> >> >> >
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.
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 > |