Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7980011:
Convert the PluginService interface to be an async wrapper around PluginList. (Closed)
|
Created:
9 years, 3 months ago by Robert Sesek Modified:
9 years, 3 months ago CC:
chromium-reviews, kkania, dpranke+watch-content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionConvert the PluginService interface to be an async wrapper around PluginList.
This adds additional methods to PluginService so that most callers can be moved
off PluginList and use the new asynchronous interface. This is in preparation
for moving plugin probing out-of-process on Mac and Linux.
BUG=17863, 95114
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102421
Patch Set 1 #Patch Set 2 : Ready for review #
Total comments: 33
Patch Set 3 : Address comments #
Total comments: 4
Patch Set 4 : '' #
Total comments: 2
Patch Set 5 : New interface as discussed #
Total comments: 7
Patch Set 6 : Revert to PS3 and rebase #
Total comments: 9
Patch Set 7 : Address comments #Messages
Total messages: 30 (0 generated)
jar: metrics service jam: overall bauerb: plugins
very exciting :) http://codereview.chromium.org/7980011/diff/6001/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.h (right): http://codereview.chromium.org/7980011/diff/6001/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.h:497: void GetPluginsInfoSendReply(Browser* browser, info: GetPluginsInfoReply or GetPluginsInfoCallback? it seems we don't need to indicate what the impl of the callback does in its name http://codereview.chromium.org/7980011/diff/6001/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.h:500: std::vector<webkit::WebPluginInfo> plugins); nit: this should be passed as a const ref per the style guide http://codereview.chromium.org/7980011/diff/6001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_content_settings_api.h (right): http://codereview.chromium.org/7980011/diff/6001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_content_settings_api.h:10: #include "webkit/plugins/npapi/plugin_group.h" nit: can you just forward declare instead? http://codereview.chromium.org/7980011/diff/6001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_content_settings_api.h:12: class PluginService; nit: doesn't look like this is needed http://codereview.chromium.org/7980011/diff/6001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_content_settings_api.h:45: void OnGotPluginGroups(std::vector<webkit::npapi::PluginGroup> groups); nit: const ref http://codereview.chromium.org/7980011/diff/6001/chrome/browser/metrics/metri... File chrome/browser/metrics/metrics_service.h (right): http://codereview.chromium.org/7980011/diff/6001/chrome/browser/metrics/metri... chrome/browser/metrics/metrics_service.h:158: // When in the INIT_TASK_SCHEDULED state, multiple tasks are performed. In the are all these changes really necessary? can't we keep metrics service the same, and just do the plugins/hwclass tasks serially to minimize the changes from this cl? http://codereview.chromium.org/7980011/diff/6001/chrome/browser/plugin_except... File chrome/browser/plugin_exceptions_table_model.h (right): http://codereview.chromium.org/7980011/diff/6001/chrome/browser/plugin_except... chrome/browser/plugin_exceptions_table_model.h:15: #include "content/browser/plugin_service.h" nit: why? we should try to forward declare as much as possible. http://codereview.chromium.org/7980011/diff/6001/chrome/browser/ui/webui/plug... File chrome/browser/ui/webui/plugins_ui.cc (right): http://codereview.chromium.org/7980011/diff/6001/chrome/browser/ui/webui/plug... chrome/browser/ui/webui/plugins_ui.cc:241: void PluginsDOMHandler::PluginsLoaded(const std::vector<PluginGroup> groups) { reference http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... File content/browser/plugin_service.cc (right): http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.cc:57: std::vector<webkit::WebPluginInfo> /* unused */) { nit: const ref. also, "unused" isn't a convention in any of the plugin classes for unused parameters. i think it's clear to the reader that it's unused, so you can just put infos actually, why pass in webplugininfo if it's not used at all? and even, why not just give it the PluginGroup objects as a parameter instead of calling PS? http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.cc:452: base::Unretained(&context), how do you know that the ResourceContext will still be around when we go to the file thread? it seems that this can cause crashes when profiles go away http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.cc:476: const PluginService::GetPluginsCallback& callback) { probably want to copy the DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); from line 425 http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... File content/browser/plugin_service.h (right): http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.h:29: #include "webkit/plugins/npapi/plugin_group.h" nit: can you get away with forward declaring? http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.h:63: // content layer and above. above->below? (see http://dev.chromium.org/developers/content-module) http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.h:75: typedef base::Callback<void(std::vector<webkit::WebPluginInfo>)> const ref (and below) http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.h:142: // Asynchronously loads plugins on the FILE thread and then calls back to the nit: i think it's better not to mention file thread here, since on mac/win we'll be going to a different process http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.h:168: void SetPluginListForTesting(webkit::npapi::MockPluginList* plugin_list); nit: can you make this private and friend the test class, so we can be assured that other code doesn't call it? actually, since this is only used by the extension code, why did you move this stuff here instead of keeping it where it was? http://codereview.chromium.org/7980011/diff/6001/content/browser/renderer_hos... File content/browser/renderer_host/buffered_resource_handler.h (right): http://codereview.chromium.org/7980011/diff/6001/content/browser/renderer_hos... content/browser/renderer_host/buffered_resource_handler.h:12: #include "webkit/plugins/webplugininfo.h" nit: forward declare http://codereview.chromium.org/7980011/diff/6001/content/browser/renderer_hos... content/browser/renderer_host/buffered_resource_handler.h:74: void OnPluginsLoaded(std::vector<webkit::WebPluginInfo> plugins); nit: const reference http://codereview.chromium.org/7980011/diff/6001/content/browser/renderer_hos... File content/browser/renderer_host/render_message_filter.h (right): http://codereview.chromium.org/7980011/diff/6001/content/browser/renderer_hos... content/browser/renderer_host/render_message_filter.h:161: std::vector<webkit::WebPluginInfo> plugins); nit: const ref http://codereview.chromium.org/7980011/diff/6001/webkit/plugins/npapi/plugin_... File webkit/plugins/npapi/plugin_list.h (right): http://codereview.chromium.org/7980011/diff/6001/webkit/plugins/npapi/plugin_... webkit/plugins/npapi/plugin_list.h:40: // querying plugins is synchronous and it is preferable to use the PluginService nit: we shouldn't talk about PS in webkit layer, since that's a layering violation in comments
http://codereview.chromium.org/7980011/diff/6001/content/browser/renderer_hos... File content/browser/renderer_host/render_message_filter.cc (right): http://codereview.chromium.org/7980011/diff/6001/content/browser/renderer_hos... content/browser/renderer_host/render_message_filter.cc:298: case ViewHostMsg_GetPlugins::ID: nit: not needed anymore
Switched to const ref and forward declared. http://codereview.chromium.org/7980011/diff/6001/chrome/browser/metrics/metri... File chrome/browser/metrics/metrics_service.h (right): http://codereview.chromium.org/7980011/diff/6001/chrome/browser/metrics/metri... chrome/browser/metrics/metrics_service.h:158: // When in the INIT_TASK_SCHEDULED state, multiple tasks are performed. In the On 2011/09/21 00:04:51, John Abd-El-Malek wrote: > are all these changes really necessary? can't we keep metrics service the same, > and just do the plugins/hwclass tasks serially to minimize the changes from this > cl? Ok. Made the init tasks serial. http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... File content/browser/plugin_service.cc (right): http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.cc:57: std::vector<webkit::WebPluginInfo> /* unused */) { On 2011/09/21 00:04:51, John Abd-El-Malek wrote: > actually, why pass in webplugininfo if it's not used at all? and even, why not > just give it the PluginGroup objects as a parameter instead of calling PS? As stated in the comments, this is the callback for the call to GetPlugins(). This is done so that the plugin cache is fresh and reloaded if necessary, after which the plugin groups can be fetched directly from the cache. http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.cc:452: base::Unretained(&context), On 2011/09/21 00:04:51, John Abd-El-Malek wrote: > how do you know that the ResourceContext will still be around when we go to the > file thread? it seems that this can cause crashes when profiles go away I was concerned about this too, but was wondering the best approach. Should I make the ResourceContext extend from SupportsWeakPtr<T>? http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.cc:476: const PluginService::GetPluginsCallback& callback) { On 2011/09/21 00:04:51, John Abd-El-Malek wrote: > probably want to copy the > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); from line 425 No, this will be changing shortly anyways. http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... File content/browser/plugin_service.h (right): http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.h:63: // content layer and above. On 2011/09/21 00:04:51, John Abd-El-Malek wrote: > above->below? (see http://dev.chromium.org/developers/content-module) I guess it depends if you're looking up or down ;). Got suggestions for better wording? http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.h:168: void SetPluginListForTesting(webkit::npapi::MockPluginList* plugin_list); On 2011/09/21 00:04:51, John Abd-El-Malek wrote: > nit: can you make this private and friend the test class, so we can be assured > that other code doesn't call it? I personally hate friending tests in shared components. Since it takes a MockPluginList, I think this is pretty safe. There's also a PRESUBMIT when you check in code that calls a "ForTesting" method. > actually, since this is only used by the extension code, why did you move this > stuff here instead of keeping it where it was? The extension code needs to "load" fake plugins, which is part of the PluginList interface. Since the PluginService just uses the Singleton(), there needs to be a way to inject. They previously did this just through the injector in their code, but now that the extension code uses PluginService, the injector needed to move here.
http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... File content/browser/plugin_service.cc (right): http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.cc:452: base::Unretained(&context), On 2011/09/21 01:13:44, rsesek wrote: > On 2011/09/21 00:04:51, John Abd-El-Malek wrote: > > how do you know that the ResourceContext will still be around when we go to > the > > file thread? it seems that this can cause crashes when profiles go away > > I was concerned about this too, but was wondering the best approach. Should I > make the ResourceContext extend from SupportsWeakPtr<T>? You'd have to check with Will Chan about this. It seems like a workaround because one is unsure about lifetime. also, weakptr is not threadsafe to use on threads other than the one it was created on. it looks like there's only one caller of this function (RMF), so perhaps the best approach for now is to have only one GetPlugins(callback) call, and have RMF do its filtering when it gets the result. http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.cc:476: const PluginService::GetPluginsCallback& callback) { On 2011/09/21 01:13:44, rsesek wrote: > On 2011/09/21 00:04:51, John Abd-El-Malek wrote: > > probably want to copy the > > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); from line 425 > > No, this will be changing shortly anyways. On Windows this will still be in-process, so we still want the assert there to ensure people don't do this heavy disk operation on UI/IO http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... File content/browser/plugin_service.h (right): http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.h:168: void SetPluginListForTesting(webkit::npapi::MockPluginList* plugin_list); On 2011/09/21 01:13:44, rsesek wrote: > On 2011/09/21 00:04:51, John Abd-El-Malek wrote: > > nit: can you make this private and friend the test class, so we can be assured > > that other code doesn't call it? > > I personally hate friending tests in shared components. Since it takes a > MockPluginList, I think this is pretty safe. There's also a PRESUBMIT when you > check in code that calls a "ForTesting" method. oh I didn't know about that presubmit check. I'm curious, why do you hate friending tests? > > > actually, since this is only used by the extension code, why did you move this > > stuff here instead of keeping it where it was? > > The extension code needs to "load" fake plugins, which is part of the PluginList > interface. Since the PluginService just uses the Singleton(), there needs to be > a way to inject. They previously did this just through the injector in their > code, but now that the extension code uses PluginService, the injector needed to > move here. so in the test, the extensions code calls PS. currently you call into PS from the test to override the returned list. why not have the test call into the extensions code to override the list, and if that's set, it doesn't even call PS? it seems nicer if testing code is localized as much as possible to the code that needs that override.
http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... File content/browser/plugin_service.cc (right): http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.cc:32: #include "webkit/plugins/npapi/mock_plugin_list.h" Nit: Should PluginService really know about testing-specific code? You could just as well pass in a regular PluginList to SetPluginListForTesting. http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.cc:452: base::Unretained(&context), On 2011/09/21 06:31:46, John Abd-El-Malek wrote: > On 2011/09/21 01:13:44, rsesek wrote: > > On 2011/09/21 00:04:51, John Abd-El-Malek wrote: > > > how do you know that the ResourceContext will still be around when we go to > > the > > > file thread? it seems that this can cause crashes when profiles go away > > > > I was concerned about this too, but was wondering the best approach. Should I > > make the ResourceContext extend from SupportsWeakPtr<T>? > > You'd have to check with Will Chan about this. It seems like a workaround > because one is unsure about lifetime. also, weakptr is not threadsafe to use on > threads other than the one it was created on. > > it looks like there's only one caller of this function (RMF), so perhaps the > best approach for now is to have only one GetPlugins(callback) call, and have > RMF do its filtering when it gets the result. Yes please! Then we can do the filtering on the IO thread and get rid of the resource context map in ChromePluginServiceFilter. http://codereview.chromium.org/7980011/diff/4022/content/browser/plugin_servi... File content/browser/plugin_service.cc (right): http://codereview.chromium.org/7980011/diff/4022/content/browser/plugin_servi... content/browser/plugin_service.cc:50: std::vector<webkit::WebPluginInfo> result) { Nit: This should probably be const ref as well? http://codereview.chromium.org/7980011/diff/4022/content/browser/renderer_hos... File content/browser/renderer_host/buffered_resource_handler.cc (right): http://codereview.chromium.org/7980011/diff/4022/content/browser/renderer_hos... content/browser/renderer_host/buffered_resource_handler.cc:347: PluginService::GetInstance()->GetPlugins( If you just pass in |*info->context()| here, you could get by without an additional method on PluginService.
http://codereview.chromium.org/7980011/diff/4022/content/browser/renderer_hos... File content/browser/renderer_host/buffered_resource_handler.cc (right): http://codereview.chromium.org/7980011/diff/4022/content/browser/renderer_hos... content/browser/renderer_host/buffered_resource_handler.cc:347: PluginService::GetInstance()->GetPlugins( On 2011/09/21 14:31:26, Bernhard Bauer wrote: > If you just pass in |*info->context()| here, you could get by without an > additional method on PluginService. given that we probably want to take out the filtering from PS and only have the one place that does it (RMF) do it directly, i dont think we want to do this
On 2011/09/21 16:22:54, John Abd-El-Malek wrote: > http://codereview.chromium.org/7980011/diff/4022/content/browser/renderer_hos... > File content/browser/renderer_host/buffered_resource_handler.cc (right): > > http://codereview.chromium.org/7980011/diff/4022/content/browser/renderer_hos... > content/browser/renderer_host/buffered_resource_handler.cc:347: > PluginService::GetInstance()->GetPlugins( > On 2011/09/21 14:31:26, Bernhard Bauer wrote: > > If you just pass in |*info->context()| here, you could get by without an > > additional method on PluginService. > > given that we probably want to take out the filtering from PS and only have the > one place that does it (RMF) do it directly, i dont think we want to do this Yes, we chatted and I'm removing the GetPlugins() call that takes a ResourceContext and will do the filtering exclusively in RMF.
On 2011/09/21 06:31:46, John Abd-El-Malek wrote: > it looks like there's only one caller of this function (RMF), so perhaps the > best approach for now is to have only one GetPlugins(callback) call, and have > RMF do its filtering when it gets the result. Done. Filtering now only happens in the RMF. > oh I didn't know about that presubmit check. I'm curious, why do you hate > friending tests? When you're trying to understand code, you read the header as it's the de facto documentation. I don't mind friend-ing the class' single test, but when I see a list of other tests friended, I think two things 1) this is cluttered 2) this is probably hard to test around. With regard to (2), I think that it usually indicates that some work should be done to make it more easily testable/mockable either through refactoring out to a pure virtual interface or providing a subclassed testing implementation. It also just encourages more clutter when other people try to test with it. > so in the test, the extensions code calls PS. currently you call into PS from > the test to override the returned list. why not have the test call into the > extensions code to override the list, and if that's set, it doesn't even call > PS? it seems nicer if testing code is localized as much as possible to the code > that needs that override. Done. http://codereview.chromium.org/7980011/diff/4022/content/browser/plugin_servi... File content/browser/plugin_service.cc (right): http://codereview.chromium.org/7980011/diff/4022/content/browser/plugin_servi... content/browser/plugin_service.cc:50: std::vector<webkit::WebPluginInfo> result) { On 2011/09/21 14:31:26, Bernhard Bauer wrote: > Nit: This should probably be const ref as well? Done.
lgtm! for the future, please avoid syncing before uploading patchsets that address comment. that makes it a little harder to see what changed to address the comments. in this case, it was pretty easy, but sometimes it makes things very hard! http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... File content/browser/plugin_service.h (right): http://codereview.chromium.org/7980011/diff/6001/content/browser/plugin_servi... content/browser/plugin_service.h:63: // content layer and above. On 2011/09/21 01:13:44, rsesek wrote: > On 2011/09/21 00:04:51, John Abd-El-Malek wrote: > > above->below? (see http://dev.chromium.org/developers/content-module) > > I guess it depends if you're looking up or down ;). Got suggestions for better > wording? (big nit!) It's been my experience that we use down, for example that's why i made the diagram in the wiki above that way. A rough check with a few others here confirmed that that's our convention. http://codereview.chromium.org/7980011/diff/1041/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_content_settings_api.cc (right): http://codereview.chromium.org/7980011/diff/1041/chrome/browser/extensions/ex... chrome/browser/extensions/extension_content_settings_api.cc:32: const std::vector<webkit::npapi::PluginGroup>* g_testing_plugin_groups_ = NULL; nit: "= NULL" is not necessary http://codereview.chromium.org/7980011/diff/1041/content/browser/plugin_servi... File content/browser/plugin_service.cc (right): http://codereview.chromium.org/7980011/diff/1041/content/browser/plugin_servi... content/browser/plugin_service.cc:447: webkit::npapi::PluginList::Singleton()->RefreshPlugins(); do we really need this wrapper function that just calls another method?
Another go, please. I've changed the interface to be as discussed over email. I think it's more consistent now.
http://codereview.chromium.org/7980011/diff/4034/chrome/browser/ui/webui/plug... File chrome/browser/ui/webui/plugins_ui.cc (right): http://codereview.chromium.org/7980011/diff/4034/chrome/browser/ui/webui/plug... chrome/browser/ui/webui/plugins_ui.cc:237: PluginService::GetInstance()->LoadPluginList(true, We don't need to force a reload here (the flag for |GetPluginGroups| was "reload if necessary", not "force reload"). Sorry it's confusing :-) http://codereview.chromium.org/7980011/diff/4034/content/browser/plugin_servi... File content/browser/plugin_service.cc (right): http://codereview.chromium.org/7980011/diff/4034/content/browser/plugin_servi... content/browser/plugin_service.cc:450: webkit::npapi::PluginList::Singleton()->GetPlugins(&plugins); Would it be bad to make PluginList::LoadPlugins() public and use that here? If you're not supposed to use PluginList in higher-level code *at all*, it shouldn't matter :-) http://codereview.chromium.org/7980011/diff/4034/content/browser/plugin_servi... File content/browser/plugin_service.h (right): http://codereview.chromium.org/7980011/diff/4034/content/browser/plugin_servi... content/browser/plugin_service.h:136: // thread, but the data can be stale. Nit: I think the more common case will be that the plug-in list has not yet been loaded at all. Could you change the comment that way? http://codereview.chromium.org/7980011/diff/4034/content/browser/renderer_hos... File content/browser/renderer_host/render_message_filter.h (right): http://codereview.chromium.org/7980011/diff/4034/content/browser/renderer_hos... content/browser/renderer_host/render_message_filter.h:160: void OnGetPluginsCallback(IPC::Message* reply_msg); Nit: |OnGetPluginsCallback| reads a bit like it's a message handler. Maybe call it |GetPluginsCallback| or |GotPlugins|?
And thanks for doing this, BTW! This interface is very nice.
metrics_service changes LGTM
I think having two ways of getting the plugins, either calling GetCachedPlugins or calling LoadPlugins(refresh=false) is confusing to callers. how do they figure out which version to call? if someone wants a cached version, then it seems that LoadPlugins(refresh=false) is only there for the first-use scenario. however, with all the threads we have, no code can know for sure that they might not be the first caller, in which case it'll never be safe for them to call GetCachedPlugins. http://codereview.chromium.org/7980011/diff/4034/content/browser/plugin_servi... File content/browser/plugin_service.h (right): http://codereview.chromium.org/7980011/diff/4034/content/browser/plugin_servi... content/browser/plugin_service.h:133: void LoadPluginList(bool refresh, const base::Closure& completion_callback); i think having only one GetPlugins function is better, with a boolean parameter. no need to have two functions when one will do.
http://codereview.chromium.org/7980011/diff/4034/content/browser/plugin_servi... File content/browser/plugin_service.h (right): http://codereview.chromium.org/7980011/diff/4034/content/browser/plugin_servi... content/browser/plugin_service.h:133: void LoadPluginList(bool refresh, const base::Closure& completion_callback); On 2011/09/21 23:45:13, John Abd-El-Malek wrote: > i think having only one GetPlugins function is better, with a boolean parameter. > no need to have two functions when one will do. please ignore this comment, i put before i wrote the other bigger reply
On 2011/09/21 23:45:13, John Abd-El-Malek wrote: > I think having two ways of getting the plugins, either calling GetCachedPlugins > or calling LoadPlugins(refresh=false) is confusing to callers. how do they > figure out which version to call? if someone wants a cached version, then it > seems that LoadPlugins(refresh=false) is only there for the first-use scenario. > however, with all the threads we have, no code can know for sure that they might > not be the first caller, in which case it'll never be safe for them to call > GetCachedPlugins. (thinking more about this) btw in case I wasn't clear, by safe I meant that now it's possible for callers to get no plugins at all, if they were the first. that seems error-prone. before your change, they might have caused the plugins to be loaded from disk, which is also bad, but at least it wouldn't affect user-visible features. what we really need is to remove _all_ sync calls to get the plugins\groups using PluginService. that way there'll be no API that can be misused in the future. of the remaining sync calls: -plugin_exceptions_table_model.h is actually not used anymore, so we should remove that code altogether (bauerb can confirm) -pdf_unsupported_feature.cc is trivial to make async -print_preview_tab_controller.cc: this, like pdf_unsupported_feature.cc, used the overriden plugin stuff when it was originally in content. now that that concept moved to chrome, we can make ChromePluginServiceFilter just know to not use Adobe Reader for a given tab. that would simplify things, and this would also fix pdf_unsupported_feature.cc I'd be happy to help do these tasks if you want.
On 2011/09/22 01:15:32, John Abd-El-Malek wrote: > On 2011/09/21 23:45:13, John Abd-El-Malek wrote: > > I think having two ways of getting the plugins, either calling > GetCachedPlugins > > or calling LoadPlugins(refresh=false) is confusing to callers. how do they > > figure out which version to call? if someone wants a cached version, then it > > seems that LoadPlugins(refresh=false) is only there for the first-use > scenario. > > however, with all the threads we have, no code can know for sure that they > might > > not be the first caller, in which case it'll never be safe for them to call > > GetCachedPlugins. > > (thinking more about this) > > btw in case I wasn't clear, by safe I meant that now it's possible for callers > to get no plugins at all, if they were the first. that seems error-prone. before > your change, they might have caused the plugins to be loaded from disk, which is > also bad, but at least it wouldn't affect user-visible features. > > what we really need is to remove _all_ sync calls to get the plugins\groups > using PluginService. that way there'll be no API that can be misused in the > future. > > of the remaining sync calls: > -plugin_exceptions_table_model.h is actually not used anymore, so we should > remove that code altogether (bauerb can confirm) > -pdf_unsupported_feature.cc is trivial to make async > -print_preview_tab_controller.cc: this, like pdf_unsupported_feature.cc, used > the overriden plugin stuff when it was originally in content. now that that > concept moved to chrome, we can make ChromePluginServiceFilter just know to not > use Adobe Reader for a given tab. that would simplify things, and this would > also fix pdf_unsupported_feature.cc > > I'd be happy to help do these tasks if you want. ok, I decided to just do this to help save you time in all your cleanup: http://codereview.chromium.org/7977042/
On 2011/09/22 01:15:32, John Abd-El-Malek wrote: > On 2011/09/21 23:45:13, John Abd-El-Malek wrote: > > I think having two ways of getting the plugins, either calling > GetCachedPlugins > > or calling LoadPlugins(refresh=false) is confusing to callers. how do they > > figure out which version to call? if someone wants a cached version, then it > > seems that LoadPlugins(refresh=false) is only there for the first-use > scenario. > > however, with all the threads we have, no code can know for sure that they > might > > not be the first caller, in which case it'll never be safe for them to call > > GetCachedPlugins. > > (thinking more about this) > > btw in case I wasn't clear, by safe I meant that now it's possible for callers > to get no plugins at all, if they were the first. that seems error-prone. before > your change, they might have caused the plugins to be loaded from disk, which is > also bad, but at least it wouldn't affect user-visible features. > > what we really need is to remove _all_ sync calls to get the plugins\groups > using PluginService. that way there'll be no API that can be misused in the > future. Ok, but what about calls to GetPluginInfo/GetPluginInfoByPath? I seriously doubt that we can make all callers of those methods asynchronous or prove that they will never be called before the PluginList has been loaded. > of the remaining sync calls: > -plugin_exceptions_table_model.h is actually not used anymore, so we should > remove that code altogether (bauerb can confirm) > -pdf_unsupported_feature.cc is trivial to make async > -print_preview_tab_controller.cc: this, like pdf_unsupported_feature.cc, used > the overriden plugin stuff when it was originally in content. now that that > concept moved to chrome, we can make ChromePluginServiceFilter just know to not > use Adobe Reader for a given tab. that would simplify things, and this would > also fix pdf_unsupported_feature.cc > > I'd be happy to help do these tasks if you want.
On 2011/09/22 09:13:53, Bernhard Bauer wrote: > On 2011/09/22 01:15:32, John Abd-El-Malek wrote: > > On 2011/09/21 23:45:13, John Abd-El-Malek wrote: > > > I think having two ways of getting the plugins, either calling > > GetCachedPlugins > > > or calling LoadPlugins(refresh=false) is confusing to callers. how do they > > > figure out which version to call? if someone wants a cached version, then it > > > seems that LoadPlugins(refresh=false) is only there for the first-use > > scenario. > > > however, with all the threads we have, no code can know for sure that they > > might > > > not be the first caller, in which case it'll never be safe for them to call > > > GetCachedPlugins. > > > > (thinking more about this) > > > > btw in case I wasn't clear, by safe I meant that now it's possible for callers > > to get no plugins at all, if they were the first. that seems error-prone. > before > > your change, they might have caused the plugins to be loaded from disk, which > is > > also bad, but at least it wouldn't affect user-visible features. > > > > what we really need is to remove _all_ sync calls to get the plugins\groups > > using PluginService. that way there'll be no API that can be misused in the > > future. > > Ok, but what about calls to GetPluginInfo/GetPluginInfoByPath? I seriously doubt > that we can make all callers of those methods asynchronous or prove that they > will never be called before the PluginList has been loaded. I just looked through all of them. I think we'll be fine to have these methods which always use stale data, since in almost all cases, they must have been called after the plugin list was generated (i.e. a lot of them start with a path already, so they must have got it from the list). The only cases I wasn't sure about were: -PluginDataRemover -drag_util.mm The former goes to file thread in PS to get the plugin data. So that's easy. The later has to be synchronous, from the UI thread. it does use stale data at the moment. We just have to make PluginService::GetPluginInfo() always use stale data (i.e. not to be an option). GetPluginInfoByPath is on PluginList, and when that's called from PS, it already has a path, so it must have loaded the plugins already. > > > of the remaining sync calls: > > -plugin_exceptions_table_model.h is actually not used anymore, so we should > > remove that code altogether (bauerb can confirm) > > -pdf_unsupported_feature.cc is trivial to make async > > -print_preview_tab_controller.cc: this, like pdf_unsupported_feature.cc, used > > the overriden plugin stuff when it was originally in content. now that that > > concept moved to chrome, we can make ChromePluginServiceFilter just know to > not > > use Adobe Reader for a given tab. that would simplify things, and this would > > also fix pdf_unsupported_feature.cc > > > > I'd be happy to help do these tasks if you want. just committed at r102283 btw
On Thu, Sep 22, 2011 at 18:18, <jam@chromium.org> wrote: > On 2011/09/22 09:13:53, Bernhard Bauer wrote: >> >> On 2011/09/22 01:15:32, John Abd-El-Malek wrote: >> > On 2011/09/21 23:45:13, John Abd-El-Malek wrote: >> > > I think having two ways of getting the plugins, either calling >> > GetCachedPlugins >> > > or calling LoadPlugins(refresh=false) is confusing to callers. how do >> > > they >> > > figure out which version to call? if someone wants a cached version, >> > > then > > it >> >> > > seems that LoadPlugins(refresh=false) is only there for the first-use >> > scenario. >> > > however, with all the threads we have, no code can know for sure that >> > > they >> > might >> > > not be the first caller, in which case it'll never be safe for them to > > call >> >> > > GetCachedPlugins. >> > >> > (thinking more about this) >> > >> > btw in case I wasn't clear, by safe I meant that now it's possible for > > callers >> >> > to get no plugins at all, if they were the first. that seems >> > error-prone. >> before >> > your change, they might have caused the plugins to be loaded from disk, > > which >> >> is >> > also bad, but at least it wouldn't affect user-visible features. >> > >> > what we really need is to remove _all_ sync calls to get the >> > plugins\groups >> > using PluginService. that way there'll be no API that can be misused in >> > the >> > future. > >> Ok, but what about calls to GetPluginInfo/GetPluginInfoByPath? I seriously > > doubt >> >> that we can make all callers of those methods asynchronous or prove that >> they >> will never be called before the PluginList has been loaded. > > I just looked through all of them. I think we'll be fine to have these > methods > which always use stale data, since in almost all cases, they must have been > called after the plugin list was generated (i.e. a lot of them start with a > path > already, so they must have got it from the list). > > The only cases I wasn't sure about were: > -PluginDataRemover > -drag_util.mm > > The former goes to file thread in PS to get the plugin data. So that's easy. > The later has to be synchronous, from the UI thread. it does use stale data > at > the moment. > > We just have to make PluginService::GetPluginInfo() always use stale data > (i.e. > not to be an option). GetPluginInfoByPath is on PluginList, and when that's > called from PS, it already has a path, so it must have loaded the plugins > already. But the path could also come from PathService (for internal plug-ins), and I think those don't get added to the PluginList either until it is loaded. >> > of the remaining sync calls: >> > -plugin_exceptions_table_model.h is actually not used anymore, so we >> > should >> > remove that code altogether (bauerb can confirm) >> > -pdf_unsupported_feature.cc is trivial to make async >> > -print_preview_tab_controller.cc: this, like pdf_unsupported_feature.cc, > > used >> >> > the overriden plugin stuff when it was originally in content. now that >> > that >> > concept moved to chrome, we can make ChromePluginServiceFilter just know >> > to >> not >> > use Adobe Reader for a given tab. that would simplify things, and this >> > would >> > also fix pdf_unsupported_feature.cc >> > >> > I'd be happy to help do these tasks if you want. > > just committed at r102283 btw > > http://codereview.chromium.org/7980011/ >
On Thu, Sep 22, 2011 at 9:26 AM, Bernhard Bauer <bauerb@chromium.org> wrote: > On Thu, Sep 22, 2011 at 18:18, <jam@chromium.org> wrote: > > On 2011/09/22 09:13:53, Bernhard Bauer wrote: > >> > >> On 2011/09/22 01:15:32, John Abd-El-Malek wrote: > >> > On 2011/09/21 23:45:13, John Abd-El-Malek wrote: > >> > > I think having two ways of getting the plugins, either calling > >> > GetCachedPlugins > >> > > or calling LoadPlugins(refresh=false) is confusing to callers. how > do > >> > > they > >> > > figure out which version to call? if someone wants a cached version, > >> > > then > > > > it > >> > >> > > seems that LoadPlugins(refresh=false) is only there for the > first-use > >> > scenario. > >> > > however, with all the threads we have, no code can know for sure > that > >> > > they > >> > might > >> > > not be the first caller, in which case it'll never be safe for them > to > > > > call > >> > >> > > GetCachedPlugins. > >> > > >> > (thinking more about this) > >> > > >> > btw in case I wasn't clear, by safe I meant that now it's possible for > > > > callers > >> > >> > to get no plugins at all, if they were the first. that seems > >> > error-prone. > >> before > >> > your change, they might have caused the plugins to be loaded from > disk, > > > > which > >> > >> is > >> > also bad, but at least it wouldn't affect user-visible features. > >> > > >> > what we really need is to remove _all_ sync calls to get the > >> > plugins\groups > >> > using PluginService. that way there'll be no API that can be misused > in > >> > the > >> > future. > > > >> Ok, but what about calls to GetPluginInfo/GetPluginInfoByPath? I > seriously > > > > doubt > >> > >> that we can make all callers of those methods asynchronous or prove that > >> they > >> will never be called before the PluginList has been loaded. > > > > I just looked through all of them. I think we'll be fine to have these > > methods > > which always use stale data, since in almost all cases, they must have > been > > called after the plugin list was generated (i.e. a lot of them start with > a > > path > > already, so they must have got it from the list). > > > > The only cases I wasn't sure about were: > > -PluginDataRemover > > -drag_util.mm > > > > The former goes to file thread in PS to get the plugin data. So that's > easy. > > The later has to be synchronous, from the UI thread. it does use stale > data > > at > > the moment. > > > > We just have to make PluginService::GetPluginInfo() always use stale data > > (i.e. > > not to be an option). GetPluginInfoByPath is on PluginList, and when > that's > > called from PS, it already has a path, so it must have loaded the plugins > > already. > > But the path could also come from PathService (for internal plug-ins), > and I think those don't get added to the PluginList either until it is > loaded. > Can you give me a specific example of when this would fail? -Browser::CrashedPluginHelper is obviously fine -PluginService::FindOrStartNpapiPluginProcess as well (it already went through an asynchronous hop) -PluginService::RegisterPepperPlugins is not related to npapi, no file reading will happen since they're registered beforehand -GViewRequestInterceptor::ShouldUsePdfPlugin calls it for the pdf plugin, but that's added at startup through the pepper registry > > >> > of the remaining sync calls: > >> > -plugin_exceptions_table_model.h is actually not used anymore, so we > >> > should > >> > remove that code altogether (bauerb can confirm) > >> > -pdf_unsupported_feature.cc is trivial to make async > >> > -print_preview_tab_controller.cc: this, like > pdf_unsupported_feature.cc, > > > > used > >> > >> > the overriden plugin stuff when it was originally in content. now that > >> > that > >> > concept moved to chrome, we can make ChromePluginServiceFilter just > know > >> > to > >> not > >> > use Adobe Reader for a given tab. that would simplify things, and this > >> > would > >> > also fix pdf_unsupported_feature.cc > >> > > >> > I'd be happy to help do these tasks if you want. > > > > just committed at r102283 btw > > > > http://codereview.chromium.org/7980011/ > > >
If I reverted to Path Set 3, rebased John's patch (thanks for that cleanup!), and made the unsupported PDF instance async, would that be the right direction? I think we should tackle GetPluginInfo calls in a separate CL.
yes, sounds great, thanks On Thu, Sep 22, 2011 at 10:03 AM, <rsesek@chromium.org> wrote: > If I reverted to Path Set 3, rebased John's patch (thanks for that > cleanup!), > and made the unsupported PDF instance async, would that be the right > direction? > I think we should tackle GetPluginInfo calls in a separate CL. > > > http://codereview.chromium.**org/7980011/<http://codereview.chromium.org/7980... >
On 2011/09/22 17:12:24, John Abd-El-Malek wrote: > yes, sounds great, thanks Done.
http://codereview.chromium.org/7980011/diff/4034/chrome/browser/metrics/metri... File chrome/browser/metrics/metrics_service.cc (right): http://codereview.chromium.org/7980011/diff/4034/chrome/browser/metrics/metri... chrome/browser/metrics/metrics_service.cc:791: PluginService::GetInstance()->GetPlugins(&plugins); nit: can just get rid of the plugins local variable http://codereview.chromium.org/7980011/diff/9002/chrome/browser/pdf_unsupport... File chrome/browser/pdf_unsupported_feature.cc (right): http://codereview.chromium.org/7980011/diff/9002/chrome/browser/pdf_unsupport... chrome/browser/pdf_unsupported_feature.cc:396: base::Bind(&GotPluginGroupsCallback, tab)); the TabContentsWrapper might not be around when the callback is called. so you'll want to send the tab->render_view_host()->process()->id() and tab->render_view_host()->routing_id(). then in the callback you can use tab_util::GetTabContentsByID() to get the TC, and from there use TabContentsWrapper::GetCurrentWrapperForContents http://codereview.chromium.org/7980011/diff/9002/chrome/browser/printing/prin... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/7980011/diff/9002/chrome/browser/printing/prin... chrome/browser/printing/print_preview_tab_controller.cc:25: #include "content/browser/plugin_service.h" nit: is this change still needed? http://codereview.chromium.org/7980011/diff/9002/content/browser/plugin_servi... File content/browser/plugin_service.h (right): http://codereview.chromium.org/7980011/diff/9002/content/browser/plugin_servi... content/browser/plugin_service.h:60: // querying plugin information, and this should instead of that class wherever nit: we should make this more forceful, i.e. "This must be used instead of that class to get the plugin list, otherwise expensive disk operation could happen on the UI/IO threads." http://codereview.chromium.org/7980011/diff/9002/content/browser/plugin_servi... content/browser/plugin_service.h:140: // Asynchronously loads plugins and then calls back to the provided function nit: this doesn't always load the plugins, so how about Returns the plugin list to the provided function asynchronously, loading the plugins if necessary. http://codereview.chromium.org/7980011/diff/9002/content/browser/plugin_servi... content/browser/plugin_service.h:144: // Gets the list of plugin groups, reloading them from disk if marked as dirty nit, we should make this the same comment above. better to be vague and say "if necessary" since RefreshPluginList is not the only thing that causes loading, i.e. initial load.
http://codereview.chromium.org/7980011/diff/9002/chrome/browser/pdf_unsupport... File chrome/browser/pdf_unsupported_feature.cc (right): http://codereview.chromium.org/7980011/diff/9002/chrome/browser/pdf_unsupport... chrome/browser/pdf_unsupported_feature.cc:396: base::Bind(&GotPluginGroupsCallback, tab)); On 2011/09/22 21:10:59, John Abd-El-Malek wrote: > the TabContentsWrapper might not be around when the callback is called. so > you'll want to send the tab->render_view_host()->process()->id() and > tab->render_view_host()->routing_id(). then in the callback you can use > tab_util::GetTabContentsByID() to get the TC, and from there use > TabContentsWrapper::GetCurrentWrapperForContents Done. Thanks for the detailed how-to :-). http://codereview.chromium.org/7980011/diff/9002/chrome/browser/printing/prin... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/7980011/diff/9002/chrome/browser/printing/prin... chrome/browser/printing/print_preview_tab_controller.cc:25: #include "content/browser/plugin_service.h" On 2011/09/22 21:10:59, John Abd-El-Malek wrote: > nit: is this change still needed? Nope. http://codereview.chromium.org/7980011/diff/9002/content/browser/plugin_servi... File content/browser/plugin_service.h (right): http://codereview.chromium.org/7980011/diff/9002/content/browser/plugin_servi... content/browser/plugin_service.h:60: // querying plugin information, and this should instead of that class wherever On 2011/09/22 21:10:59, John Abd-El-Malek wrote: > nit: we should make this more forceful, i.e. "This must be used instead of that > class to get the plugin list, otherwise expensive disk operation could happen on > the UI/IO threads." Done. Happily. http://codereview.chromium.org/7980011/diff/9002/content/browser/plugin_servi... content/browser/plugin_service.h:140: // Asynchronously loads plugins and then calls back to the provided function On 2011/09/22 21:10:59, John Abd-El-Malek wrote: > nit: this doesn't always load the plugins, so how about > > Returns the plugin list to the provided function asynchronously, loading the > plugins if necessary. Done and below.
lgtm, thanks
CQ is trying tha patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/7980011/17001
Change committed as 102421 |
