Disable ExtensionPrefStore for local-state pref store

Disable ExtensionPrefStore for local-state pref store

The local-state pref store shares preferences between all profiles on a computer. This CL prevents extensions to use the ExtensionPrefStore in the local-sate. 

Reasoning:
The ExtensionPrefStore cannot be used for storing extension preferences in the local-state (shared among all profiles on a machine). In this context, there is no meaningful definition of a precedence order as the same extensions can be installed in different profiles. One extension instance could override the preferences of another instance. If the "winning preferences" (the ones with highest preference) are persisted and carried over between browser restarts, this could also make debugging very difficult: The browser behaves strangely even though no extensions are installed. Therefore, one needs to start the right profile in order to uninstall an extension and fix problems of in other profiles. For these reasons we (pamg, battre) decided to not support the ExtensionPrefStore for local-state.

Unit tests can only verify that the DCHECKs are triggered. I have implemented one such test but do not include it in this CL because DCHECK triggers a termination.

This is an extension of http://codereview.chromium.org/4438001/show

Contributed by battre@google.com

BUG=50726
TEST=none

[battre (please use the other), 2010-11-05 00:43:55]

Hi.

Is this the right way to disable the ExtensionPrefStore?

I basically just prevent the registration to the Message Service.
ExtensionPrefStore::Observe contains just some safe-guards in case somebody
enables the registration to the Message Service again.

Best regards,
Dominic

[Pam (message me for reviews), 2010-11-05 12:07:15]

http://codereview.chromium.org/4551001/diff/1/2
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/4551001/diff/1/2#newcode26
chrome/browser/extensions/extension_pref_store.cc:26: // If profile_==NULL, this
ExtensionPrefStore is for local-state.
This last sentence is enough. The first one doesn't add any information that
isn't pretty obvious from the code.

http://codereview.chromium.org/4551001/diff/1/2#newcode52
chrome/browser/extensions/extension_pref_store.cc:52: // is consistent among all
local-state and user ExtensionPrefStores.
Comment and code are both outdated now. With no local-state store, I can't think
why we need to keep the ordering of extensions consistent among different
profiles anymore, so we don't need to store every extension anymore either.

http://codereview.chromium.org/4551001/diff/1/2#newcode109
chrome/browser/extensions/extension_pref_store.cc:109: // it and let the other
one handle it.
This piece of code is no longer needed. If we're getting a pref-change
notification for our profile, the pref being changed must have been registered
with our PrefService.

http://codereview.chromium.org/4551001/diff/1/2#newcode165
chrome/browser/extensions/extension_pref_store.cc:165: return
g_browser_process->local_state();
This method isn't needed anymore. We only ever have a non-NULL profile when it's
used now, so profile_->GetPrefs() can simply be inlined.

http://codereview.chromium.org/4551001/diff/1/2#newcode175
chrome/browser/extensions/extension_pref_store.cc:175:
NotificationService::AllSources());
Since we no longer want to observe everything in any ExtensionPrefStore, these
can both be changed to only register for the right profile as a source. In that
case I'd suggest moving the profile_ check

if (!profile_)
  return;

from the constructor down to the top of this method, to keep all the
profile-related observer stuff in one place.

http://codereview.chromium.org/4551001/diff/1/2#newcode182
chrome/browser/extensions/extension_pref_store.cc:182: // extensions (see class
header for reasoning)
This comment isn't needed. It's already mentioned several other places,
including in the DCHECK message just below.

http://codereview.chromium.org/4551001/diff/1/2#newcode186
chrome/browser/extensions/extension_pref_store.cc:186: DCHECK(profile_) << "The
local-state does not support ExtensionPrefStore";
How about "...does not support setting prefs from extensions".

Also, NOTREACHED() would be appropriate here, since it really shouldn't ever
happen. And it can happen once, at the top of the method, instead of once per
case.

http://codereview.chromium.org/4551001/diff/1/2#newcode188
chrome/browser/extensions/extension_pref_store.cc:188: if (profile_ ==
extension_profile) {
Once you're only registering for the applicable profile as a source, it's safe
to assume that you're only getting those notifications. Maybe a DCHECK just to
be sure, but it doesn't need to be verified in "real" code.

http://codereview.chromium.org/4551001/diff/1/2#newcode197
chrome/browser/extensions/extension_pref_store.cc:197: DCHECK(profile_) << "The
local-state does not support ExtensionPrefStore";
Same comments here.

http://codereview.chromium.org/4551001/diff/1/3
File chrome/browser/extensions/extension_pref_store.h (right):

http://codereview.chromium.org/4551001/diff/1/3#newcode34
chrome/browser/extensions/extension_pref_store.h:34: // The ExtensionPrefStore
cannot be useed for storing extension preferences in
nit: useed

http://codereview.chromium.org/4551001/diff/1/3#newcode45
chrome/browser/extensions/extension_pref_store.h:45: // ExtensionPrefStore for
local-state.
I think it would be better to put most of this justification into the patch
description, and keep the source file briefer. Also, this comment is pretty much
a duplicate of the one below. What about deleting this one and moving what's
included here down there?

http://codereview.chromium.org/4551001/diff/1/3#newcode52
chrome/browser/extensions/extension_pref_store.h:52: // empty dictionary.
nit: The last sentence here is unnecessary. If it can't store anything, its
dictionary must be empty. Likewise, if people really need to know that there's a
DCHECK enforcing the prohibition, they can go look at the .cc file. Also, this
comment is pretty much a duplicate of the one above. What about just

The local-state ExtensionPrefStore (shared among all profiles on a machine),
indicated by a NULL |profile|, is prohibited from storing preferences. Since
extensions are installed per profile, allowing them to control machine-wide
settings could lead to unsolvable conflicts, and should not be necessary anyway.

[battre (please use the other), 2010-11-05 22:05:31]

Addressed issues.

http://codereview.chromium.org/4551001/diff/1/2
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/4551001/diff/1/2#newcode26
chrome/browser/extensions/extension_pref_store.cc:26: // If profile_==NULL, this
ExtensionPrefStore is for local-state.
On 2010/11/05 12:07:15, Pam wrote:
> This last sentence is enough. The first one doesn't add any information that
> isn't pretty obvious from the code.

Done.

http://codereview.chromium.org/4551001/diff/1/2#newcode52
chrome/browser/extensions/extension_pref_store.cc:52: // is consistent among all
local-state and user ExtensionPrefStores.
On 2010/11/05 12:07:15, Pam wrote:
> Comment and code are both outdated now. With no local-state store, I can't
think
> why we need to keep the ordering of extensions consistent among different
> profiles anymore, so we don't need to store every extension anymore either.

Done.

http://codereview.chromium.org/4551001/diff/1/2#newcode109
chrome/browser/extensions/extension_pref_store.cc:109: // it and let the other
one handle it.
On 2010/11/05 12:07:15, Pam wrote:
> This piece of code is no longer needed. If we're getting a pref-change
> notification for our profile, the pref being changed must have been registered
> with our PrefService.

Done.

http://codereview.chromium.org/4551001/diff/1/2#newcode165
chrome/browser/extensions/extension_pref_store.cc:165: return
g_browser_process->local_state();
On 2010/11/05 12:07:15, Pam wrote:
> This method isn't needed anymore. We only ever have a non-NULL profile when
it's
> used now, so profile_->GetPrefs() can simply be inlined.

Done. Not inlined though for PrefService injection in unit tests.

http://codereview.chromium.org/4551001/diff/1/2#newcode175
chrome/browser/extensions/extension_pref_store.cc:175:
NotificationService::AllSources());
On 2010/11/05 12:07:15, Pam wrote:
> Since we no longer want to observe everything in any ExtensionPrefStore, these
> can both be changed to only register for the right profile as a source. In
that
> case I'd suggest moving the profile_ check
> 
> if (!profile_)
>   return;
> 
> from the constructor down to the top of this method, to keep all the
> profile-related observer stuff in one place.

Done.

http://codereview.chromium.org/4551001/diff/1/2#newcode182
chrome/browser/extensions/extension_pref_store.cc:182: // extensions (see class
header for reasoning)
On 2010/11/05 12:07:15, Pam wrote:
> This comment isn't needed. It's already mentioned several other places,
> including in the DCHECK message just below.

Done.

http://codereview.chromium.org/4551001/diff/1/2#newcode186
chrome/browser/extensions/extension_pref_store.cc:186: DCHECK(profile_) << "The
local-state does not support ExtensionPrefStore";
On 2010/11/05 12:07:15, Pam wrote:
> How about "...does not support setting prefs from extensions".
> 
> Also, NOTREACHED() would be appropriate here, since it really shouldn't ever
> happen. And it can happen once, at the top of the method, instead of once per
> case.

Done. Removed this completely, due to explicit registration to
Source<Profile>(profile_)

http://codereview.chromium.org/4551001/diff/1/2#newcode188
chrome/browser/extensions/extension_pref_store.cc:188: if (profile_ ==
extension_profile) {
On 2010/11/05 12:07:15, Pam wrote:
> Once you're only registering for the applicable profile as a source, it's safe
> to assume that you're only getting those notifications. Maybe a DCHECK just to
> be sure, but it doesn't need to be verified in "real" code.

Done. Actually, I went a step further and registered only to
Source<Profile>(profile_).

http://codereview.chromium.org/4551001/diff/1/2#newcode197
chrome/browser/extensions/extension_pref_store.cc:197: DCHECK(profile_) << "The
local-state does not support ExtensionPrefStore";
On 2010/11/05 12:07:15, Pam wrote:
> Same comments here.

Done.

http://codereview.chromium.org/4551001/diff/1/3
File chrome/browser/extensions/extension_pref_store.h (right):

http://codereview.chromium.org/4551001/diff/1/3#newcode34
chrome/browser/extensions/extension_pref_store.h:34: // The ExtensionPrefStore
cannot be useed for storing extension preferences in
On 2010/11/05 12:07:15, Pam wrote:
> nit: useed

Done.

http://codereview.chromium.org/4551001/diff/1/3#newcode45
chrome/browser/extensions/extension_pref_store.h:45: // ExtensionPrefStore for
local-state.
On 2010/11/05 12:07:15, Pam wrote:
> I think it would be better to put most of this justification into the patch
> description, and keep the source file briefer. Also, this comment is pretty
much
> a duplicate of the one below. What about deleting this one and moving what's
> included here down there?

Done.

http://codereview.chromium.org/4551001/diff/1/3#newcode52
chrome/browser/extensions/extension_pref_store.h:52: // empty dictionary.
On 2010/11/05 12:07:15, Pam wrote:
> nit: The last sentence here is unnecessary. If it can't store anything, its
> dictionary must be empty. Likewise, if people really need to know that there's
a
> DCHECK enforcing the prohibition, they can go look at the .cc file. Also, this
> comment is pretty much a duplicate of the one above. What about just
> 
> The local-state ExtensionPrefStore (shared among all profiles on a machine),
> indicated by a NULL |profile|, is prohibited from storing preferences. Since
> extensions are installed per profile, allowing them to control machine-wide
> settings could lead to unsolvable conflicts, and should not be necessary
anyway.
> 

Done.

[Pam (message me for reviews), 2010-11-06 16:26:22]

LGTM with minor nits below addressed.

- Pam

http://codereview.chromium.org/4551001/diff/15001/16001
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/4551001/diff/15001/16001#newcode17
chrome/browser/extensions/extension_pref_store.cc:17: #include
"chrome/common/notification_service.h"
Is this header still needed, now that we're not using
NotificationService::AllSources anymore?

http://codereview.chromium.org/4551001/diff/15001/16001#newcode115
chrome/browser/extensions/extension_pref_store.cc:115: // may be null in unit
tests
Please start with a capital letter and end with a period, even when the comment
is a sentence fragment. The first is just for visual consistency and ease of
reading, the second to make it completely clear that you didn't accidentally end
the comment in the middle.

http://codereview.chromium.org/4551001/diff/15001/16001#newcode143
chrome/browser/extensions/extension_pref_store.cc:143: // may be null in unit
tests
Same here (cap and period). Also, the profile_ will be null for the local-store
instance of the ExtensionPrefStore, not just in unit tests.

[battre (please use the other), 2010-11-09 14:47:41]

Done

http://codereview.chromium.org/4551001/diff/15001/16001
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/4551001/diff/15001/16001#newcode17
chrome/browser/extensions/extension_pref_store.cc:17: #include
"chrome/common/notification_service.h"
On 2010/11/06 16:26:22, Pam wrote:
> Is this header still needed, now that we're not using
> NotificationService::AllSources anymore?

Done. Replaced it with necessary headers.

http://codereview.chromium.org/4551001/diff/15001/16001#newcode115
chrome/browser/extensions/extension_pref_store.cc:115: // may be null in unit
tests
On 2010/11/06 16:26:22, Pam wrote:
> Please start with a capital letter and end with a period, even when the
comment
> is a sentence fragment. The first is just for visual consistency and ease of
> reading, the second to make it completely clear that you didn't accidentally
end
> the comment in the middle.

Done.

http://codereview.chromium.org/4551001/diff/15001/16001#newcode143
chrome/browser/extensions/extension_pref_store.cc:143: // may be null in unit
tests
On 2010/11/06 16:26:22, Pam wrote:
> Same here (cap and period). Also, the profile_ will be null for the
local-store
> instance of the ExtensionPrefStore, not just in unit tests.

Done.