Ensure that --disable-extensions disables extension prefs from being enacted

Ensure that --disable-extensions disables extension prefs from being enacted

The ExtensionPrefs injects extension controlled preferences into the general PrefService. This was only prevented if individual extensions were disabled but not if all extensions were disabled by the --disable-extensions command-line flag. This CL fixes this.

BUG=96766
TEST=no


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103477

[battre, 2011-09-27 18:27:18]

Could you please review this? Thanks.

[Mattias Nissler (ping if slow), 2011-09-28 09:24:51]

I put a comment for your consideration. If you decide to not make the change,
LGTM, otherwise I think we need another cycle.

http://codereview.chromium.org/8060017/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/8060017/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:242:
CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableExtensions);
I must say I personally don't like depending on static stuff such as
CommandLine::ForCurrentProcess() and in most cases it's much better to just pass
the parameters to the ctor, s.t. writing tests for the class becomes easier.

Admittedly, for the test cases that use TestExtensionPrefs, there's not so much
of a difference, but then it may be a good idea to at least keep the
dependencies of the non-testing code as lightweight as possible.

Bottom line: I'd prefer passing |extensions_disabled| to the ctor, but I leave
the decision to you.

[battre, 2011-09-29 11:33:22]

On 2011/09/28 09:24:51, Mattias Nissler wrote:
> I put a comment for your consideration. If you decide to not make the change,
> LGTM, otherwise I think we need another cycle.
> 
>
http://codereview.chromium.org/8060017/diff/2001/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_prefs.cc (right):
> 
>
http://codereview.chromium.org/8060017/diff/2001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_prefs.cc:242:
> CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableExtensions);
> I must say I personally don't like depending on static stuff such as
> CommandLine::ForCurrentProcess() and in most cases it's much better to just
pass
> the parameters to the ctor, s.t. writing tests for the class becomes easier.
> 
> Admittedly, for the test cases that use TestExtensionPrefs, there's not so
much
> of a difference, but then it may be a good idea to at least keep the
> dependencies of the non-testing code as lightweight as possible.
> 
> Bottom line: I'd prefer passing |extensions_disabled| to the ctor, but I leave
> the decision to you.

I don't have a strong opinion either way, so I have changed this as requested.

Done.

[Mattias Nissler (ping if slow), 2011-09-29 12:20:07]

LGTM, even if I have put another cleanup suggestion ;) Feel free to decide
whether you want to handle it, and if so, maybe in a separate CL.

Also, you seem to have not fixed all unit tests yet, there's a compile failure
on the try jobs.

http://codereview.chromium.org/8060017/diff/5/chrome/browser/extensions/test_...
File chrome/browser/extensions/test_extension_prefs.h (right):

http://codereview.chromium.org/8060017/diff/5/chrome/browser/extensions/test_...
chrome/browser/extensions/test_extension_prefs.h:65: // active after calling
RecreateExtensionPrefs(). Defaults to false;
nit: s/;/./

http://codereview.chromium.org/8060017/diff/5/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8060017/diff/5/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:938:
CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableExtensions);
Hm, maybe handle the switch in CommandLinePrefStore so client code doesn't have
to check both the pref and the switch?

[battre, 2011-09-29 14:50:18]

http://codereview.chromium.org/8060017/diff/5/chrome/browser/extensions/test_...
File chrome/browser/extensions/test_extension_prefs.h (right):

http://codereview.chromium.org/8060017/diff/5/chrome/browser/extensions/test_...
chrome/browser/extensions/test_extension_prefs.h:65: // active after calling
RecreateExtensionPrefs(). Defaults to false;
On 2011/09/29 12:20:07, Mattias Nissler wrote:
> nit: s/;/./

Done.

http://codereview.chromium.org/8060017/diff/5/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8060017/diff/5/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:938:
CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableExtensions);
On 2011/09/29 12:20:07, Mattias Nissler wrote:
> Hm, maybe handle the switch in CommandLinePrefStore so client code doesn't
have
> to check both the pref and the switch?

I looked into this and decided against it: It would be nice in principle but it
would create inconsistencies with other switches that are not handled via the
command line pref store (switches::kExtensionsUpdateFrequency,
switches::kDisableExtensionsFileAccessCheck).

[battre, 2011-09-29 16:29:50]

+asargent for OWNERS approval.

Antony, could you please have a look at this?

Thanks.
Dominic

[asargent_no_longer_on_chrome, 2011-09-29 18:06:59]

http://codereview.chromium.org/8060017/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/8060017/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:238:
InitPrefStore(extensions_disabled);
It is sort of against the style guide to be doing non-trivial work in the
constructor like this. 

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Doing_Work_in_...

I wonder if a better approach would be to move the call to InitPrefStore to
after the object is created. Then in profile_impl.cc, you can not call it if
extensions are disabled.

Does that seem workable?

[battre, 2011-09-29 20:37:47]

http://codereview.chromium.org/8060017/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/8060017/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:238:
InitPrefStore(extensions_disabled);
On 2011/09/29 18:06:59, Antony Sargent wrote:
> It is sort of against the style guide to be doing non-trivial work in the
> constructor like this. 
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Doing_Work_in_...
> 
> I wonder if a better approach would be to move the call to InitPrefStore to
> after the object is created. Then in profile_impl.cc, you can not call it if
> extensions are disabled.
> 
> Does that seem workable?
> 

I would still want to call InitPrefStore(false) to call
extension_pref_value_map_->NotifyInitializationCompleted();

It's a trivial change. I can do that tomorrow.

[asargent_no_longer_on_chrome, 2011-09-29 22:00:14]

Ok, pre-emptive LGTM so you don't get tied up waiting for me.

[commit-bot: I haz the power, 2011-09-30 12:53:01]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/8060017/13002

[commit-bot: I haz the power, 2011-09-30 12:53:06]

Presubmit check for 8060017-13002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/profiles/profile_impl.cc

Presubmit checks took 1.6s to calculate.

[battre, 2011-09-30 13:01:06]

+mirandac for OWNERS approval for chrome/browser/profiles/

[Miranda Callahan, 2011-09-30 13:03:27]

On 2011/09/30 13:01:06, battre wrote:
> +mirandac for OWNERS approval for chrome/browser/profiles/

Profiles LGTM.

[commit-bot: I haz the power, 2011-09-30 13:04:21]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/8060017/13002

[commit-bot: I haz the power, 2011-09-30 13:58:28]

Try job failure for 8060017-13002 on win_rel for steps "update_scripts, update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2011-09-30 15:21:00]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/8060017/13002

[commit-bot: I haz the power, 2011-09-30 16:43:55]

Change committed as 103477