Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10876083:
[4] Changing content_settings_api to use PluginFinder's async interface (Closed)
|
Created:
8 years, 3 months ago by ibraaaa Modified:
8 years, 3 months ago CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, darin-cc_chromium.org, jam Base URL:
http://git.chromium.org/chromium/src.git@async_start Visibility:
Public. |
DescriptionGetting content_settings_api to use PluginFinder's async interface and remove dependency on PluginGroup
4th CL in the series to delete PluginGroup and move hardcoded plugin group definitions to metadata files.
BUG=124396
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155309
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed review comments by Mattias #
Total comments: 2
Patch Set 3 : Addressed one of bauerb's review comments #Patch Set 4 : Adding CallbackBarrier but wait! #
Total comments: 10
Patch Set 5 : Using new Callback barrier convenience method #Patch Set 6 : Substitute include with forward declaration in .h file #
Messages
Total messages: 20 (0 generated)
I don't know anything about plugins in Chrome and webkit, but here's my review :) https://chromiumcodereview.appspot.com/10876083/diff/1/chrome/browser/extensi... File chrome/browser/extensions/api/content_settings/content_settings_api.cc (right): https://chromiumcodereview.appspot.com/10876083/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/content_settings/content_settings_api.cc:265: void GetResourceIdentifiersFunction::GetPluginFinderForRunImpl( The name of this function is unfortunate. It actually doesn't "get" anything. https://chromiumcodereview.appspot.com/10876083/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/content_settings/content_settings_api.cc:273: finder)); Aha, so we make a call to PluginFinder::Get that invokes a callback in which we run PluginService::GetPlugins, which again results in an asynchronous callback? Any chance to get the thing you need in a single async operation, e.g. making PluginService::GetPlugins() get the finder behind the scenes? https://chromiumcodereview.appspot.com/10876083/diff/1/chrome/browser/extensi... File chrome/browser/extensions/api/content_settings/content_settings_api.h (right): https://chromiumcodereview.appspot.com/10876083/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/content_settings/content_settings_api.h:66: void OnGotPlugins(PluginFinder* finder, This could also use some documentation on when and from where it gets called. https://chromiumcodereview.appspot.com/10876083/diff/1/webkit/plugins/npapi/p... File webkit/plugins/npapi/plugin_list.h (right): https://chromiumcodereview.appspot.com/10876083/diff/1/webkit/plugins/npapi/p... webkit/plugins/npapi/plugin_list.h:201: PluginList(int dummy); Just use the constructor below instead, passing NULL and 0 for the parameters?
Thanks Mattias, that was helpful. http://codereview.chromium.org/10876083/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/content_settings/content_settings_api.cc (right): http://codereview.chromium.org/10876083/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/content_settings/content_settings_api.cc:273: finder)); Unfortunately, we can't make PluginService::GetPlugins get the finder behind the scenes because not all callers of PluginService::GetPlugins will need to operate on the list of plugins using PluginFinder instance. So it will be useful for this call (and some others) but not for the rest. On 2012/08/27 15:42:09, Mattias Nissler wrote: > Aha, so we make a call to PluginFinder::Get that invokes a callback in which we > run PluginService::GetPlugins, which again results in an asynchronous callback? > Any chance to get the thing you need in a single async operation, e.g. making > PluginService::GetPlugins() get the finder behind the scenes? http://codereview.chromium.org/10876083/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/content_settings/content_settings_api.h (right): http://codereview.chromium.org/10876083/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/content_settings/content_settings_api.h:66: void OnGotPlugins(PluginFinder* finder, On 2012/08/27 15:42:09, Mattias Nissler wrote: > This could also use some documentation on when and from where it gets called. Done. http://codereview.chromium.org/10876083/diff/1/webkit/plugins/npapi/plugin_li... File webkit/plugins/npapi/plugin_list.h (right): http://codereview.chromium.org/10876083/diff/1/webkit/plugins/npapi/plugin_li... webkit/plugins/npapi/plugin_list.h:201: PluginList(int dummy); The Ctor below is going to be deleted anyways and this one will be needed instead but I guess you are right we don't need to introduce a new one in this CL. On 2012/08/27 15:42:09, Mattias Nissler wrote: > Just use the constructor below instead, passing NULL and 0 for the parameters?
http://codereview.chromium.org/10876083/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/content_settings/content_settings_api.cc (right): http://codereview.chromium.org/10876083/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/content_settings/content_settings_api.cc:273: finder)); On 2012/08/27 17:34:05, ibraaaa wrote: > Unfortunately, we can't make PluginService::GetPlugins get the finder behind the > scenes because not all callers of PluginService::GetPlugins will need to operate > on the list of plugins using PluginFinder instance. So it will be useful for > this call (and some others) but not for the rest. Also, PluginService lives in content/, so it can't know about PluginFinder. Another thing we could try is to issue both asynchronous calls in parallel and only continue when both have returned their value. That would require more bookkeeping, but we might be able to extract it to a generic method. > On 2012/08/27 15:42:09, Mattias Nissler wrote: > > Aha, so we make a call to PluginFinder::Get that invokes a callback in which > we > > run PluginService::GetPlugins, which again results in an asynchronous > callback? > > Any chance to get the thing you need in a single async operation, e.g. making > > PluginService::GetPlugins() get the finder behind the scenes? > http://codereview.chromium.org/10876083/diff/6002/chrome/browser/extensions/a... File chrome/browser/extensions/api/content_settings/content_settings_api.cc (right): http://codereview.chromium.org/10876083/diff/6002/chrome/browser/extensions/a... chrome/browser/extensions/api/content_settings/content_settings_api.cc:279: base::hash_set<std::string> group_identifiers; I think using a std::set for this is more idiomatic in Chrome code.
http://codereview.chromium.org/10876083/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/content_settings/content_settings_api.cc (right): http://codereview.chromium.org/10876083/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/content_settings/content_settings_api.cc:273: finder)); mmm, let me clarify how can we end up with PluginFinder and vector<WebPluginInfo> (where each of them is returned in a different callback method) in the same method so we can operate on the plugins using the finder? On 2012/08/29 12:56:54, Bernhard Bauer wrote: > On 2012/08/27 17:34:05, ibraaaa wrote: > > Unfortunately, we can't make PluginService::GetPlugins get the finder behind > the > > scenes because not all callers of PluginService::GetPlugins will need to > operate > > on the list of plugins using PluginFinder instance. So it will be useful for > > this call (and some others) but not for the rest. > > Also, PluginService lives in content/, so it can't know about PluginFinder. > > Another thing we could try is to issue both asynchronous calls in parallel and > only continue when both have returned their value. That would require more > bookkeeping, but we might be able to extract it to a generic method. > > > On 2012/08/27 15:42:09, Mattias Nissler wrote: > > > Aha, so we make a call to PluginFinder::Get that invokes a callback in which > > we > > > run PluginService::GetPlugins, which again results in an asynchronous > > callback? > > > Any chance to get the thing you need in a single async operation, e.g. > making > > > PluginService::GetPlugins() get the finder behind the scenes? > > > http://codereview.chromium.org/10876083/diff/6002/chrome/browser/extensions/a... File chrome/browser/extensions/api/content_settings/content_settings_api.cc (right): http://codereview.chromium.org/10876083/diff/6002/chrome/browser/extensions/a... chrome/browser/extensions/api/content_settings/content_settings_api.cc:279: base::hash_set<std::string> group_identifiers; On 2012/08/29 12:56:54, Bernhard Bauer wrote: > I think using a std::set for this is more idiomatic in Chrome code. Done.
http://codereview.chromium.org/10876083/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/content_settings/content_settings_api.cc (right): http://codereview.chromium.org/10876083/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/content_settings/content_settings_api.cc:273: finder)); On 2012/08/29 16:39:37, ibraaaa wrote: > mmm, let me clarify how can we end up with PluginFinder and > vector<WebPluginInfo> (where each of them is returned in a different callback > method) in the same method so we can operate on the plugins using the finder? It's similar to the CallbackBarrier we use in PluginPrefs: We construct an object that takes a Callback<void(PluginFinder*, vector<WebPluginInfo>)> and hands out a Callback<void(PluginFinder*)> and a Callback<void(vector<WebPluginInfo>)>. In these callbacks the objects stores the passed in values and calls the final callback as soon as both callbacks have been called. > On 2012/08/29 12:56:54, Bernhard Bauer wrote: > > On 2012/08/27 17:34:05, ibraaaa wrote: > > > Unfortunately, we can't make PluginService::GetPlugins get the finder behind > > the > > > scenes because not all callers of PluginService::GetPlugins will need to > > operate > > > on the list of plugins using PluginFinder instance. So it will be useful for > > > this call (and some others) but not for the rest. > > > > Also, PluginService lives in content/, so it can't know about PluginFinder. > > > > Another thing we could try is to issue both asynchronous calls in parallel and > > only continue when both have returned their value. That would require more > > bookkeeping, but we might be able to extract it to a generic method. > > > > > On 2012/08/27 15:42:09, Mattias Nissler wrote: > > > > Aha, so we make a call to PluginFinder::Get that invokes a callback in > which > > > we > > > > run PluginService::GetPlugins, which again results in an asynchronous > > > callback? > > > > Any chance to get the thing you need in a single async operation, e.g. > > making > > > > PluginService::GetPlugins() get the finder behind the scenes? > > > > > >
http://codereview.chromium.org/10876083/diff/17001/chrome/browser/extensions/... File chrome/browser/extensions/api/content_settings/content_settings_api.cc (right): http://codereview.chromium.org/10876083/diff/17001/chrome/browser/extensions/... chrome/browser/extensions/api/content_settings/content_settings_api.cc:56: class CallbackBarrier : public base::RefCountedThreadSafe<CallbackBarrier> { How about moving this to chrome/browser/plugins_pluginfinder_callback_barrier.(h/cc) (in a separate CL to be finalized first) and reusing it here as well: http://codereview.chromium.org/10872034/diff/15010/chrome/browser/plugin_pref... for EnablePlugin and EnablePluginGroup?
http://codereview.chromium.org/10876083/diff/17001/chrome/browser/extensions/... File chrome/browser/extensions/api/content_settings/content_settings_api.cc (right): http://codereview.chromium.org/10876083/diff/17001/chrome/browser/extensions/... chrome/browser/extensions/api/content_settings/content_settings_api.cc:56: class CallbackBarrier : public base::RefCountedThreadSafe<CallbackBarrier> { we can also update code here to make use of it: http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/renderer_ho... since it used to do the same thing On 2012/08/30 08:51:52, ibraaaa wrote: > How about moving this to > chrome/browser/plugins_pluginfinder_callback_barrier.(h/cc) (in a separate CL to > be finalized first) and reusing it here as well: > http://codereview.chromium.org/10872034/diff/15010/chrome/browser/plugin_pref... > for EnablePlugin and EnablePluginGroup?
http://codereview.chromium.org/10876083/diff/17001/chrome/browser/extensions/... File chrome/browser/extensions/api/content_settings/content_settings_api.cc (right): http://codereview.chromium.org/10876083/diff/17001/chrome/browser/extensions/... chrome/browser/extensions/api/content_settings/content_settings_api.cc:56: class CallbackBarrier : public base::RefCountedThreadSafe<CallbackBarrier> { Apparently the callback required to get plugins (I mentioned above in plugin_info_message_filter.cc) is useless because the fetched plugins are never used after that, isn't it? On 2012/08/30 11:58:51, ibraaaa wrote: > we can also update code here to make use of it: > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/renderer_ho... > since it used to do the same thing > > On 2012/08/30 08:51:52, ibraaaa wrote: > > How about moving this to > > chrome/browser/plugins_pluginfinder_callback_barrier.(h/cc) (in a separate CL > to > > be finalized first) and reusing it here as well: > > > http://codereview.chromium.org/10872034/diff/15010/chrome/browser/plugin_pref... > > for EnablePlugin and EnablePluginGroup? >
http://codereview.chromium.org/10876083/diff/17001/chrome/browser/extensions/... File chrome/browser/extensions/api/content_settings/content_settings_api.cc (right): http://codereview.chromium.org/10876083/diff/17001/chrome/browser/extensions/... chrome/browser/extensions/api/content_settings/content_settings_api.cc:56: class CallbackBarrier : public base::RefCountedThreadSafe<CallbackBarrier> { On 2012/08/30 08:51:52, ibraaaa wrote: > How about moving this to > chrome/browser/plugins_pluginfinder_callback_barrier.(h/cc) (in a separate CL to > be finalized first) and reusing it here as well: > http://codereview.chromium.org/10872034/diff/15010/chrome/browser/plugin_pref... > for EnablePlugin and EnablePluginGroup? SGTM! (Both here and in PluginInfoMessageFilter) We could also add a convenience method somewhere that uses this and calls the individual methods on PluginFinder and PluginService. http://codereview.chromium.org/10876083/diff/17001/chrome/browser/extensions/... chrome/browser/extensions/api/content_settings/content_settings_api.cc:56: class CallbackBarrier : public base::RefCountedThreadSafe<CallbackBarrier> { On 2012/08/31 12:48:49, ibraaaa wrote: > Apparently the callback required to get plugins (I mentioned above in > plugin_info_message_filter.cc) is useless because the fetched plugins are never > used after that, isn't it? The plugins are used in PluginInfoMessageFilter::PluginsLoaded() to find the first enabled plug-in for the MIME type. > On 2012/08/30 11:58:51, ibraaaa wrote: > > we can also update code here to make use of it: > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/renderer_ho... > > since it used to do the same thing > > > > On 2012/08/30 08:51:52, ibraaaa wrote: > > > How about moving this to > > > chrome/browser/plugins_pluginfinder_callback_barrier.(h/cc) (in a separate > CL > > to > > > be finalized first) and reusing it here as well: > > > > > > http://codereview.chromium.org/10872034/diff/15010/chrome/browser/plugin_pref... > > > for EnablePlugin and EnablePluginGroup? > > > http://codereview.chromium.org/10876083/diff/17001/chrome/browser/extensions/... chrome/browser/extensions/api/content_settings/content_settings_api.cc:65: plugins_(NULL) { The default constructor for a scoped_ptr already initializes it to NULL. http://codereview.chromium.org/10876083/diff/17001/chrome/browser/extensions/... chrome/browser/extensions/api/content_settings/content_settings_api.cc:90: void SavePluginFinderAndMaybeRunCallback(PluginFinder* finder) { This method name is a bit verbose. Maybe just GotPluginFinder() (and below GotPlugins())? http://codereview.chromium.org/10876083/diff/17001/chrome/browser/extensions/... chrome/browser/extensions/api/content_settings/content_settings_api.cc:96: plugins_.reset(new PluginVector(plugins.begin(), plugins.end())); Could you simply use the copy constructor on vector? http://codereview.chromium.org/10876083/diff/17001/chrome/browser/extensions/... chrome/browser/extensions/api/content_settings/content_settings_api.cc:109: scoped_ptr<std::vector<webkit::WebPluginInfo> > plugins_; You typedef'd this as PluginVector above, so you could the typedef here. http://codereview.chromium.org/10876083/diff/17001/chrome/browser/extensions/... chrome/browser/extensions/api/content_settings/content_settings_api.cc:317: PluginService::GetInstance()->GetPlugins( You could switch the order of the parameters in OnGotPlugin(). Then, if |g_testing_plugins_| is not NULL, you could pass in the callback to OnGotPlugins() with the first parameter bound to |g_testing_plugins_|. That way you don't need the extra logic in CallbackBarrier to deal with |g_testing_plugins_|.
PTAL
LGTM
Hi John, Can I have an OWNERS review for this CL? Thanks.
chrome/browser/extensions has different OWNERS.
chrome/browser/extensions has different OWNERS.
Hi Matt, Can I have an OWNERS review for this CL? Thanks.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ibraaaa@google.com/10876083/26006
Change committed as 155309 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
