Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10872034:
Changing PluginPrefs to use PluginFinder's async interface. (Closed)
|
Created:
8 years, 4 months ago by ibraaaa Modified:
8 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, stuartmorgan+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@test_async Visibility:
Public. |
DescriptionChanging PluginPrefs to use PluginFinder's async interface. Also, making its methods doesn't rely on PluginGroup class instead use PluginList's method versions that directly use WebPluginInfo.
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=154995
Patch Set 1 : Converted PluginPref#CanEnablePlugin to use PluginFinder's async interface. #
Total comments: 4
Patch Set 2 : "Fixed review comments and added support for other methods in PluginPrefs to use async interface" #
Total comments: 15
Patch Set 3 : Fixing Pam's review comments #
Total comments: 2
Patch Set 4 : Fixing another comment from Pam #
Total comments: 1
Patch Set 5 : Fixed bauerb's review comments #
Total comments: 14
Patch Set 6 : Addressing more comments from bauerb #
Total comments: 2
Patch Set 7 : Addressed more more comments from bauerb #
Total comments: 4
Patch Set 8 : Fixed bauerb's More Nits #
Total comments: 5
Patch Set 9 : More Nits from bauerb #Patch Set 10 : Removed conditional includes on PluginFinder and PluginInstaller in pdf_unsupported_feature #Patch Set 11 : Remove exclusion of plugin_installer and plugin_finder from build when enable_plugin_installation i… #
Total comments: 8
Patch Set 12 : Addressed jhawkins' review comments #Patch Set 13 : Addressed 1 more comment from jhawkins #Patch Set 14 : Updated chrome_plugin_browsertest.cc to use new EnablePlugin method #Patch Set 15 : Fix chromeos compilation error #Patch Set 16 : Fixed bug in chromeos loading the plugins page #Messages
Total messages: 41 (0 generated)
https://chromiumcodereview.appspot.com/10872034/diff/2001/chrome/browser/auto... File chrome/browser/automation/testing_automation_provider.cc (right): https://chromiumcodereview.appspot.com/10872034/diff/2001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:218: if (automation) Nit: braces please. https://chromiumcodereview.appspot.com/10872034/diff/2001/chrome/browser/plug... File chrome/browser/plugin_prefs.h (right): https://chromiumcodereview.appspot.com/10872034/diff/2001/chrome/browser/plug... chrome/browser/plugin_prefs.h:71: void CanEnablePlugin(bool enable, const FilePath& file_path, I think we could combine these methods. There are no callers who only want to check whether a plug-in can be enabled/disabled; if it can be, then they always want to do it. So we could modify EnablePlugin below to take a void(bool) callback and pass in the result.
I fixed the comments and added more edits to this CL, basically changing other methods except IsPluginEnabled of this class to use the async interface. Also removed dependcies of the method in PlugPrefs on PluginGroup https://chromiumcodereview.appspot.com/10872034/diff/2001/chrome/browser/auto... File chrome/browser/automation/testing_automation_provider.cc (right): https://chromiumcodereview.appspot.com/10872034/diff/2001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:218: if (automation) On 2012/08/24 08:15:34, Bernhard Bauer wrote: > Nit: braces please. Done. https://chromiumcodereview.appspot.com/10872034/diff/2001/chrome/browser/plug... File chrome/browser/plugin_prefs.h (right): https://chromiumcodereview.appspot.com/10872034/diff/2001/chrome/browser/plug... chrome/browser/plugin_prefs.h:71: void CanEnablePlugin(bool enable, const FilePath& file_path, I wanted to leave this in another CL with modifying calls to no use PluginGroup. However, since we will hold to resolve the IsPluginEnabled CL then I fixed it and added support for not using PlugingGroup in this class and its dependencies. On 2012/08/24 08:15:34, Bernhard Bauer wrote: > I think we could combine these methods. There are no callers who only want to > check whether a plug-in can be enabled/disabled; if it can be, then they always > want to do it. So we could modify EnablePlugin below to take a void(bool) > callback and pass in the result.
https://chromiumcodereview.appspot.com/10872034/diff/5001/chrome/browser/plug... File chrome/browser/plugin_prefs.cc (right): https://chromiumcodereview.appspot.com/10872034/diff/5001/chrome/browser/plug... chrome/browser/plugin_prefs.cc:601: for (std::set<string16>::const_iterator it = group_names.begin(); I guess that here the order of having the plugins within a group inserted in the list followed by the group is not important, if it tended out that I missed something, then this should change to group plugins first.
I'm not the best reviewer for this code. I know Bernhard is on vacation for the week, but it would be good to find someone familiar with plugins. John, can you do it, or recommend someone? - Pam http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs.cc File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs... chrome/browser/plugin_prefs.cc:81: // A callback function that does nothing. This comment doesn't add any information. Please either expand it (when and why would this callback be used?) or remove it. http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs.h File chrome/browser/plugin_prefs.h (right): http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs... chrome/browser/plugin_prefs.h:71: // plug-in state cannot be changed because of a policy), it will silently nit: clarify "it": something like "this entire method will silently be ignored, and no callbacks will be called" http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs... chrome/browser/plugin_prefs.h:72: // be ignored. |canEnabledCallback| will be called after the plug-in typo: |canEnableCallback| http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs... chrome/browser/plugin_prefs.h:73: // change state check is called. I don't understand what a "plug-in change state check" is. Do you mean a "plug-in state-change check"? Or equivalently, "the method to check that the plug-in state can be changed"?
On 2012/08/27 13:18:05, Pam wrote: > I'm not the best reviewer for this code. I know Bernhard is on vacation for the > week, but it would be good to find someone familiar with plugins. John, can you > do it, or recommend someone? Bernhard is the person most familiar with the plugin group stuff. I'm not sure who else is. I'm too overloaded with reviews to understand the details of this code enough to review it.
Thanks John for getting back to me, do you know someone else other than Bernhard who know the plugin group details? I can also explain in details what this code is all about for anyone willing to help. Thanks. On Mon, Aug 27, 2012 at 7:05 PM, <jam@chromium.org> wrote: > > On 2012/08/27 13:18:05, Pam wrote: > >> I'm not the best reviewer for this code. I know Bernhard is on vacation >> for >> > the > >> week, but it would be good to find someone familiar with plugins. John, >> can >> > you > >> do it, or recommend someone? >> > > Bernhard is the person most familiar with the plugin group stuff. I'm not > sure > who else is. I'm too overloaded with reviews to understand the details of > this > code enough to review it. > > http://codereview.chromium.**org/10872034/<http://codereview.chromium.org/108... > -- ** Mohamed Hammad | Software Engineer Intern. | ibraaaa@google.com | +2 (0) 100 682 4347 Google Germany GmbH Alter Hof Lorenzistock Dienerstrasse 12 80331 Muenchen
http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs.cc File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs... chrome/browser/plugin_prefs.cc:81: // A callback function that does nothing. On 2012/08/27 13:18:05, Pam wrote: > This comment doesn't add any information. Please either expand it (when and why > would this callback be used?) or remove it. Done. http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs.h File chrome/browser/plugin_prefs.h (right): http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs... chrome/browser/plugin_prefs.h:71: // plug-in state cannot be changed because of a policy), it will silently On 2012/08/27 13:18:05, Pam wrote: > nit: clarify "it": something like "this entire method will silently be ignored, > and no callbacks will be called" Done. http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs... chrome/browser/plugin_prefs.h:72: // be ignored. |canEnabledCallback| will be called after the plug-in On 2012/08/27 13:18:05, Pam wrote: > typo: |canEnableCallback| Done. http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs... chrome/browser/plugin_prefs.h:73: // change state check is called. On 2012/08/27 13:18:05, Pam wrote: > I don't understand what a "plug-in change state check" is. Do you mean a > "plug-in state-change check"? Or equivalently, "the method to check that the > plug-in state can be changed"? Done.
http://codereview.chromium.org/10872034/diff/11001/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/11001/chrome/browser/plugin_pref... chrome/browser/plugin_prefs.cc:83: // is performed. Still pretty self-evident. Perhaps it would be better to remove the comment here, and instead explain down in EnablePluginGlobally why that particular call of EnablePlugin doesn't require any action in the callback when other calls do.
http://codereview.chromium.org/10872034/diff/11001/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/11001/chrome/browser/plugin_pref... chrome/browser/plugin_prefs.cc:83: // is performed. Check it now. On 2012/08/28 09:34:46, Pam wrote: > Still pretty self-evident. Perhaps it would be better to remove the comment > here, and instead explain down in EnablePluginGlobally why that particular call > of EnablePlugin doesn't require any action in the callback when other calls do.
I still can't review this effectively, but my nits have all been addressed. http://codereview.chromium.org/10872034/diff/17001/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/17001/chrome/browser/plugin_pref... chrome/browser/plugin_prefs.cc:251: // a fresh install. Thanks, that's a much more helpful comment.
http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs.cc File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs... chrome/browser/plugin_prefs.cc:82: void CanEnablePluginCallbackDoNothing(bool dummy) { This is available as base::DoNothing() in base/bind_helpers.h. http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs... chrome/browser/plugin_prefs.cc:148: void PluginPrefs::EnablePluginIfPossibleCallback( I still don't think we need two separate callbacks here. It should be fine to just call |canEnableCallback| (which should be |can_enable_callback|, BTW) with false if we can't enable the plug-in, or with true after enabling it if we can. http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs... chrome/browser/plugin_prefs.cc:217: // GetPluginInstaller will always return a non-empty name. If that is the case, then you should DCHECK, I guess.
http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs.cc File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs... chrome/browser/plugin_prefs.cc:82: void CanEnablePluginCallbackDoNothing(bool dummy) { It doesn't take bool parameter, does it? On 2012/08/29 12:56:36, Bernhard Bauer wrote: > This is available as base::DoNothing() in base/bind_helpers.h. http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs... chrome/browser/plugin_prefs.cc:148: void PluginPrefs::EnablePluginIfPossibleCallback( I wanted to keep the exact same interface but I guess what you thought is better. Done. On 2012/08/29 12:56:36, Bernhard Bauer wrote: > I still don't think we need two separate callbacks here. It should be fine to > just call |canEnableCallback| (which should be |can_enable_callback|, BTW) with > false if we can't enable the plug-in, or with true after enabling it if we can. http://codereview.chromium.org/10872034/diff/5001/chrome/browser/plugin_prefs... chrome/browser/plugin_prefs.cc:217: // GetPluginInstaller will always return a non-empty name. Added, however let me confirm if we can have a plugin with path set to an empty string, can we? On 2012/08/29 12:56:36, Bernhard Bauer wrote: > If that is the case, then you should DCHECK, I guess. http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs_unittest.cc (right): http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... chrome/browser/plugin_prefs_unittest.cc:27: MessageLoop::current()->QuitWhenIdle(); This is to maintain the exact same behavior, however, I would be happier if I knew how calling MessageLoop::QuitClosure at this point is useful to the test? :)
http://codereview.chromium.org/10872034/diff/22001/chrome/browser/automation/... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/10872034/diff/22001/chrome/browser/automation/... chrome/browser/automation/testing_automation_provider.cc:210: void CanEnablePluginCallback(base::WeakPtr<AutomationProvider> automation, We should name this method differently now. DidEnablePlugin()? http://codereview.chromium.org/10872034/diff/22001/chrome/browser/component_u... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): http://codereview.chromium.org/10872034/diff/22001/chrome/browser/component_u... chrome/browser/component_updater/pepper_flash_component_installer.cc:168: void CanEnablePluginCallbackDoNothing(bool dummy) { We could add a comment that we're fine with ignoring the result here. http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... chrome/browser/plugin_prefs.cc:69: void MaybeRunCallback(bool dummy) { Naming this parameter "dummy" is a bit confusing, because it's not actually a dummy parameter. It's also a bit weird to always return the last value. Maybe we should modify the behavior so that it returns true if all (or any?) of the plug-ins could be enabled? http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs_unittest.cc (right): http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... chrome/browser/plugin_prefs_unittest.cc:27: MessageLoop::current()->QuitWhenIdle(); On 2012/08/29 15:35:50, ibraaaa wrote: > This is to maintain the exact same behavior, however, I would be happier if I > knew how calling MessageLoop::QuitClosure at this point is useful to the test? > :) Yeah, something like base::IgnoreResult for parameters would be nice. However, in this case you might want to check the result anyway?
http://codereview.chromium.org/10872034/diff/22001/chrome/browser/automation/... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/10872034/diff/22001/chrome/browser/automation/... chrome/browser/automation/testing_automation_provider.cc:210: void CanEnablePluginCallback(base::WeakPtr<AutomationProvider> automation, On 2012/08/29 16:53:21, Bernhard Bauer wrote: > We should name this method differently now. DidEnablePlugin()? Done. http://codereview.chromium.org/10872034/diff/22001/chrome/browser/component_u... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): http://codereview.chromium.org/10872034/diff/22001/chrome/browser/component_u... chrome/browser/component_updater/pepper_flash_component_installer.cc:168: void CanEnablePluginCallbackDoNothing(bool dummy) { On 2012/08/29 16:53:21, Bernhard Bauer wrote: > We could add a comment that we're fine with ignoring the result here. Done. http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... chrome/browser/plugin_prefs.cc:69: void MaybeRunCallback(bool dummy) { If I didn't miss something: I checked the callers for EnablePluginGlobally, it is only called when Pepper flash plugin is freshly installed. It is also called without checking if the plugin state can be changed or not. So this gets us back a little bit, I assumed when you asked to merge CanEnablePlugin and EnablePlugin that checking if the plugin state can change (i.e. calling CanEnablePlugin) before calling EnablePlugin will not affect the behavior of the code in this case (I mean in EnablePluginGlobally). Now, I intentionally named the parameter dummy and didn't apply any logic because according to the old behavior of the code there is no check need to be performed on the result of 'CanEnablePlugin' simply because it was not called before :) I just don't see the point in Converting "dummy" to not "dummy"? Done. On 2012/08/29 16:53:21, Bernhard Bauer wrote: > Naming this parameter "dummy" is a bit confusing, because it's not actually a > dummy parameter. > > It's also a bit weird to always return the last value. Maybe we should modify > the behavior so that it returns true if all (or any?) of the plug-ins could be > enabled? http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs_unittest.cc (right): http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... chrome/browser/plugin_prefs_unittest.cc:27: MessageLoop::current()->QuitWhenIdle(); Sorry, I don't get what you mean? On 2012/08/29 16:53:21, Bernhard Bauer wrote: > On 2012/08/29 15:35:50, ibraaaa wrote: > > This is to maintain the exact same behavior, however, I would be happier if I > > knew how calling MessageLoop::QuitClosure at this point is useful to the test? > > :) > > Yeah, something like base::IgnoreResult for parameters would be nice. However, > in this case you might want to check the result anyway?
http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... chrome/browser/plugin_prefs.cc:69: void MaybeRunCallback(bool dummy) { On 2012/08/29 17:37:53, ibraaaa wrote: > If I didn't miss something: > > I checked the callers for EnablePluginGlobally, it is only called when Pepper > flash plugin is freshly installed. It is also called without checking if the > plugin state can be changed or not. So this gets us back a little bit, I assumed > when you asked to merge CanEnablePlugin and EnablePlugin that checking if the > plugin state can change (i.e. calling CanEnablePlugin) before calling > EnablePlugin will not affect the behavior of the code in this case (I mean in > EnablePluginGlobally). > > Now, I intentionally named the parameter dummy and didn't apply any logic > because according to the old behavior of the code there is no check need to be > performed on the result of 'CanEnablePlugin' simply because it was not called > before :) > > I just don't see the point in Converting "dummy" to not "dummy"? What I meant was that it's weird to return an arbitrary value, even if we know that it's going to be ignored. We could either not return any value at all, or have a defined behavior (which is what you implemented). I'm fine with any of those. > Done. > > On 2012/08/29 16:53:21, Bernhard Bauer wrote: > > Naming this parameter "dummy" is a bit confusing, because it's not actually a > > dummy parameter. > > > > It's also a bit weird to always return the last value. Maybe we should modify > > the behavior so that it returns true if all (or any?) of the plug-ins could be > > enabled? > http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs_unittest.cc (right): http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... chrome/browser/plugin_prefs_unittest.cc:27: MessageLoop::current()->QuitWhenIdle(); On 2012/08/29 17:37:53, ibraaaa wrote: > Sorry, I don't get what you mean? You ignore the result at the moment, but maybe you should ASSERT that it succeeded (which would obviate the need for a callback that ignores its value). > On 2012/08/29 16:53:21, Bernhard Bauer wrote: > > On 2012/08/29 15:35:50, ibraaaa wrote: > > > This is to maintain the exact same behavior, however, I would be happier if > I > > > knew how calling MessageLoop::QuitClosure at this point is useful to the > test? > > > :) > > > > Yeah, something like base::IgnoreResult for parameters would be nice. However, > > in this case you might want to check the result anyway? > http://codereview.chromium.org/10872034/diff/15006/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/15006/chrome/browser/plugin_pref... chrome/browser/plugin_prefs.cc:70: void MaybeRunCallback(bool can_enable) { Nit: |did_enable|?
http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... chrome/browser/plugin_prefs.cc:69: void MaybeRunCallback(bool dummy) { Ah, I see. My intention was not returning any value at all so I thought that fulfilling this can just be done by naming the value returned dummy, apparently not :) On 2012/08/29 18:10:17, Bernhard Bauer wrote: > On 2012/08/29 17:37:53, ibraaaa wrote: > > If I didn't miss something: > > > > I checked the callers for EnablePluginGlobally, it is only called when Pepper > > flash plugin is freshly installed. It is also called without checking if the > > plugin state can be changed or not. So this gets us back a little bit, I > assumed > > when you asked to merge CanEnablePlugin and EnablePlugin that checking if the > > plugin state can change (i.e. calling CanEnablePlugin) before calling > > EnablePlugin will not affect the behavior of the code in this case (I mean in > > EnablePluginGlobally). > > > > Now, I intentionally named the parameter dummy and didn't apply any logic > > because according to the old behavior of the code there is no check need to > be > > performed on the result of 'CanEnablePlugin' simply because it was not called > > before :) > > > > I just don't see the point in Converting "dummy" to not "dummy"? > > What I meant was that it's weird to return an arbitrary value, even if we know > that it's going to be ignored. We could either not return any value at all, or > have a defined behavior (which is what you implemented). I'm fine with any of > those. > > > Done. > > > > On 2012/08/29 16:53:21, Bernhard Bauer wrote: > > > Naming this parameter "dummy" is a bit confusing, because it's not actually > a > > > dummy parameter. > > > > > > It's also a bit weird to always return the last value. Maybe we should > modify > > > the behavior so that it returns true if all (or any?) of the plug-ins could > be > > > enabled? > > > http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs_unittest.cc (right): http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... chrome/browser/plugin_prefs_unittest.cc:27: MessageLoop::current()->QuitWhenIdle(); ok, I thought you were answering my first question "I would be happier if I knew how calling MessageLoop::QuitClosure at this point is useful to the test?", I mean originally the MessageLoop::QuitClosure was passed as a callback to EnablePlugin, I can't see the point in this. On 2012/08/29 18:10:17, Bernhard Bauer wrote: > On 2012/08/29 17:37:53, ibraaaa wrote: > > Sorry, I don't get what you mean? > > You ignore the result at the moment, but maybe you should ASSERT that it > succeeded (which would obviate the need for a callback that ignores its value). > > > On 2012/08/29 16:53:21, Bernhard Bauer wrote: > > > On 2012/08/29 15:35:50, ibraaaa wrote: > > > > This is to maintain the exact same behavior, however, I would be happier > if > > I > > > > knew how calling MessageLoop::QuitClosure at this point is useful to the > > test? > > > > :) > > > > > > Yeah, something like base::IgnoreResult for parameters would be nice. > However, > > > in this case you might want to check the result anyway? > > > http://codereview.chromium.org/10872034/diff/15006/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/15006/chrome/browser/plugin_pref... chrome/browser/plugin_prefs.cc:70: void MaybeRunCallback(bool can_enable) { Aha, check was already done and this is the result (past tense), I like that :) On 2012/08/29 18:10:17, Bernhard Bauer wrote: > Nit: |did_enable|?
LGTM with two nits below: http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs_unittest.cc (right): http://codereview.chromium.org/10872034/diff/22001/chrome/browser/plugin_pref... chrome/browser/plugin_prefs_unittest.cc:27: MessageLoop::current()->QuitWhenIdle(); On 2012/08/29 18:28:19, ibraaaa wrote: > ok, I thought you were answering my first question "I would be happier if I > knew how calling MessageLoop::QuitClosure at this point is useful to the test?", > I mean originally the MessageLoop::QuitClosure was passed as a callback to > EnablePlugin, I can't see the point in this. We are running a message loop in line 179, so we need to quit it when PluginPrefs is done doing its thing. If we don't do anything else (e.g. checking the success value like we do now), then passing in a callback that just calls MessageLoop::QuitWhenIdle instead of directly passing a QuitClosure would be unnecessary. > On 2012/08/29 18:10:17, Bernhard Bauer wrote: > > On 2012/08/29 17:37:53, ibraaaa wrote: > > > Sorry, I don't get what you mean? > > > > You ignore the result at the moment, but maybe you should ASSERT that it > > succeeded (which would obviate the need for a callback that ignores its > value). > > > > > On 2012/08/29 16:53:21, Bernhard Bauer wrote: > > > > On 2012/08/29 15:35:50, ibraaaa wrote: > > > > > This is to maintain the exact same behavior, however, I would be happier > > if > > > I > > > > > knew how calling MessageLoop::QuitClosure at this point is useful to the > > > test? > > > > > :) > > > > > > > > Yeah, something like base::IgnoreResult for parameters would be nice. > > However, > > > > in this case you might want to check the result anyway? > > > > > > http://codereview.chromium.org/10872034/diff/15010/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/15010/chrome/browser/plugin_pref... chrome/browser/plugin_prefs.cc:81: bool can_enable_; Nit: Call this one |did_enable_| as well? http://codereview.chromium.org/10872034/diff/15010/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs_unittest.cc (right): http://codereview.chromium.org/10872034/diff/15010/chrome/browser/plugin_pref... chrome/browser/plugin_prefs_unittest.cc:26: void CanEnablePluginCallback(bool dummy) { Nit: Aaaand now please don't call the parameter |dummy| anymore :-)
http://codereview.chromium.org/10872034/diff/15010/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs.cc (right): http://codereview.chromium.org/10872034/diff/15010/chrome/browser/plugin_pref... chrome/browser/plugin_prefs.cc:81: bool can_enable_; On 2012/08/31 12:54:11, Bernhard Bauer wrote: > Nit: Call this one |did_enable_| as well? Done. http://codereview.chromium.org/10872034/diff/15010/chrome/browser/plugin_pref... File chrome/browser/plugin_prefs_unittest.cc (right): http://codereview.chromium.org/10872034/diff/15010/chrome/browser/plugin_pref... chrome/browser/plugin_prefs_unittest.cc:26: void CanEnablePluginCallback(bool dummy) { On 2012/08/31 12:54:11, Bernhard Bauer wrote: > Nit: Aaaand now please don't call the parameter |dummy| anymore :-) Done.
Found some more nits, sorry ;-) http://codereview.chromium.org/10872034/diff/30001/chrome/browser/automation/... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/10872034/diff/30001/chrome/browser/automation/... chrome/browser/automation/testing_automation_provider.cc:214: bool can_enable) { Nit: |did_enable| :) http://codereview.chromium.org/10872034/diff/30001/chrome/browser/ui/webui/pl... File chrome/browser/ui/webui/plugins_ui.cc (right): http://codereview.chromium.org/10872034/diff/30001/chrome/browser/ui/webui/pl... chrome/browser/ui/webui/plugins_ui.cc:189: PluginGroups; Nit: Indent by four spaces.
I reverted some formatting changes that were accidentally caused by during during editing. PTAL. http://codereview.chromium.org/10872034/diff/30001/chrome/browser/automation/... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/10872034/diff/30001/chrome/browser/automation/... chrome/browser/automation/testing_automation_provider.cc:214: bool can_enable) { On 2012/09/03 08:57:12, Bernhard Bauer wrote: > Nit: |did_enable| :) Done. http://codereview.chromium.org/10872034/diff/30001/chrome/browser/ui/webui/pl... File chrome/browser/ui/webui/plugins_ui.cc (right): http://codereview.chromium.org/10872034/diff/30001/chrome/browser/ui/webui/pl... chrome/browser/ui/webui/plugins_ui.cc:189: PluginGroups; On 2012/09/03 08:57:12, Bernhard Bauer wrote: > Nit: Indent by four spaces. Done. http://codereview.chromium.org/10872034/diff/30001/chrome/browser/ui/webui/pl... chrome/browser/ui/webui/plugins_ui.cc:251: scoped_refptr<PluginPrefs> plugin_prefs(PluginPrefs::GetForProfile(profile)); there is no need to use scoped_refptr now, will revert it back.
still lgtm
Hi John & James, Can I have an OWNERS review for this CL?
Hi again, To be specific, James: can you take a look at stuff in chrome/browser/ui/webui/ ? John: the rest
http://codereview.chromium.org/10872034/diff/31008/chrome/browser/ui/webui/pl... File chrome/browser/ui/webui/plugins_ui.cc (right): http://codereview.chromium.org/10872034/diff/31008/chrome/browser/ui/webui/pl... chrome/browser/ui/webui/plugins_ui.cc:62: // Helper callback method to process result of CanEnablePlugin method. nit: No need to specify that this is a helper callback. For future reference this is a function not a method, the latter being a function on an object. http://codereview.chromium.org/10872034/diff/31008/chrome/browser/ui/webui/pl... chrome/browser/ui/webui/plugins_ui.cc:63: void CheckCanEnablePluginCallback(bool can_enable) { What is the purpose of this function? http://codereview.chromium.org/10872034/diff/31008/chrome/browser/ui/webui/pl... chrome/browser/ui/webui/plugins_ui.cc:181: typedef base::hash_map<std::string, std::vector<const WebPluginInfo*> > nit: Document the map.
Hi James, PTAL, Thanks. http://codereview.chromium.org/10872034/diff/31008/chrome/browser/ui/webui/pl... File chrome/browser/ui/webui/plugins_ui.cc (right): http://codereview.chromium.org/10872034/diff/31008/chrome/browser/ui/webui/pl... chrome/browser/ui/webui/plugins_ui.cc:62: // Helper callback method to process result of CanEnablePlugin method. On 2012/09/04 14:13:17, James Hawkins wrote: > nit: No need to specify that this is a helper callback. > > For future reference this is a function not a method, the latter being a > function on an object. Done. http://codereview.chromium.org/10872034/diff/31008/chrome/browser/ui/webui/pl... chrome/browser/ui/webui/plugins_ui.cc:63: void CheckCanEnablePluginCallback(bool can_enable) { We merged EnablePlugin and CanEnablePlugin into one method and accordingly the result of the state-change check done by CanEnablePlugin is now supplied as input to the Callback function (in this case CheckCanEnablePluginCallback). Since the old code used to DCHECK the result of CanEnablePlugin we maintained the same behavior by DCHECK-ing the result input to the callback. On 2012/09/04 14:13:17, James Hawkins wrote: > What is the purpose of this function? http://codereview.chromium.org/10872034/diff/31008/chrome/browser/ui/webui/pl... chrome/browser/ui/webui/plugins_ui.cc:181: typedef base::hash_map<std::string, std::vector<const WebPluginInfo*> > On 2012/09/04 14:13:17, James Hawkins wrote: > nit: Document the map. Done.
http://codereview.chromium.org/10872034/diff/31008/chrome/browser/ui/webui/pl... File chrome/browser/ui/webui/plugins_ui.cc (right): http://codereview.chromium.org/10872034/diff/31008/chrome/browser/ui/webui/pl... chrome/browser/ui/webui/plugins_ui.cc:63: void CheckCanEnablePluginCallback(bool can_enable) { On 2012/09/04 14:52:09, ibraaaa wrote: > We merged EnablePlugin and CanEnablePlugin into one method and accordingly the > result of the state-change check done by CanEnablePlugin is now supplied as > input to the Callback function (in this case CheckCanEnablePluginCallback). > > Since the old code used to DCHECK the result of CanEnablePlugin we maintained > the same behavior by DCHECK-ing the result input to the callback. > > On 2012/09/04 14:13:17, James Hawkins wrote: > > What is the purpose of this function? > OK. The function name seems a bit misleading as it's not checking anything (it is asserting, however). It's also misleading since a 'check' callback function (to me) implies there is a boolean return value that the caller will use.
PTAL, thanks. http://codereview.chromium.org/10872034/diff/31008/chrome/browser/ui/webui/pl... File chrome/browser/ui/webui/plugins_ui.cc (right): http://codereview.chromium.org/10872034/diff/31008/chrome/browser/ui/webui/pl... chrome/browser/ui/webui/plugins_ui.cc:63: void CheckCanEnablePluginCallback(bool can_enable) { Ahh, ok. Actually, the name is a left over from multiple review iterations. Just missed it out, also the documentation had CanEnablePlugin which does not exist now. I modified this and the parameter name to be consistent with other usages of EnablePlugin and also to be consistent with the fact that the check has already happened and this is the result of either the plugin was enabled or not. Thanks! On 2012/09/04 14:55:50, James Hawkins wrote: > On 2012/09/04 14:52:09, ibraaaa wrote: > > We merged EnablePlugin and CanEnablePlugin into one method and accordingly the > > result of the state-change check done by CanEnablePlugin is now supplied as > > input to the Callback function (in this case CheckCanEnablePluginCallback). > > > > Since the old code used to DCHECK the result of CanEnablePlugin we maintained > > the same behavior by DCHECK-ing the result input to the callback. > > > > On 2012/09/04 14:13:17, James Hawkins wrote: > > > What is the purpose of this function? > > > > OK. The function name seems a bit misleading as it's not checking anything (it > is asserting, however). It's also misleading since a 'check' callback function > (to me) implies there is a boolean return value that the caller will use.
webui LGTM
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ibraaaa@google.com/10872034/40001
Try job failure for 10872034-40001 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ibraaaa@google.com/10872034/31020
Bernhard, I made these modifications: - in plugins_ui.cc to fix a bug loading the plugins page. - in gview_request_interceptor_unittest.cc to use the new EnablePlugin method PTAL
still LGTM ;-)
Oh, one thing: You can probably remove the numbers in the CL description. You're not landing them in that order anyway, and it doesn't really add information.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ibraaaa@google.com/10872034/40005
Try job failure for 10872034-40005 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ibraaaa@google.com/10872034/40005
Change committed as 154995 |
