Introduce incognito preference settings.

Introduce incognito preference settings.

This CL introduces preference settings for incognito windows. The semantics are the following:

- An extension can set regular preferences as before. These affect regular and incognito windows.

- An extension can set regular preferences *and* incognito preferences. In this case, the incognito preferences affect only incognito windows.

- If extension A sets reg+incognito, extension B sets reg but no incognito, extension B has higher precedence than A --> B's preferences hold for all regular and incognito windows.

- Incognito preferences are never persisted to disk.


In order to realize this, the ExtensionPrefs class allows setting regular and incognito extension controlled preferences. It allows creating an incognito version of the PrefService with an independent PrefValueStore. This (incognito) PrefValueStore and the original PrefValueStore share several of their PrefStores (i.e. DefaultPrefStore, CommandLinePrefStore, Configuration PrefStores) but differ in two pref stores:

- We maintain two separate ExtensionPrefStores containing the effective preferences for regular and incognito windows.

- We maintain two separate user pref stores. The regular JsonPrefStore is expanded by an OverlayPersistentPrefStore that maintains all write-operations in an in-memory overlay. Therefore, incognito changes are not visible in the file-backed JsonPrefStore.

The two ExtensionPrefStores retrieve their effective values from a shared ExtensionPrefValueMap.

The OffTheRecordProfileImpl provides a request_context_ that uses the new PrefService already.

BUG=66027, 69057
TEST=unit tests, will be fully testable once the Proxy Settings API allows incognito settings.

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

[battre, 2010-12-16 16:14:32]

Sorry for the large CL. Replacing scoped_ptrs to scoped_refptrs turned out to be
necessary but a much bigger change than anticipated.

[Mattias Nissler (ping if slow), 2010-12-20 14:50:00]

Here's a first round of high level comments. I didn't do a thorough pass through
all of it yet, will do that after we have code that solves the
TestingPrefService construction issue (constructor vs. factory) in a cleaner
way.

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.h (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.h:45: static
scoped_refptr<ConfigurationPolicyPrefStore>
This is not needed. If the function name is Create...() then the caller should
be responsible for wrapping the pointer in a scoped_refptr (just like when using
an explicit new).

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/overla...
File chrome/browser/prefs/overlay_persistent_pref_store.h (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/overla...
chrome/browser/prefs/overlay_persistent_pref_store.h:17: class
OverlayPersistentPrefStore : public PersistentPrefStore,
Could use a class comment describing the behavior of this PersistentPrefStore
implementation.

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/overla...
chrome/browser/prefs/overlay_persistent_pref_store.h:21: // underlay as a weak
pointer.
A scoped_refptr is not weak.

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:109:
scoped_refptr<ConfigurationPolicyPrefStore> managed =
scoped_refptr construction not needed here either. Maybe it's time to just
inline the construction below in the constructor call.

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:121: scoped_refptr<PrefStore>
incognito_extension_prefs);
We should have a unit test for this, ensuring that values stored in the
incognito service don't end up in the original PrefService.

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:239: PrefService(scoped_refptr<PrefStore>
managed_platform_prefs,
The majority of the codebase passes reference counted objects as plain pointers.
The caller must guarantee that the object is alive in any case, so can you
please check and convert to plain pointers where possible?

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:247: // Constructor to be used only for
CreateIncognitoPrefService.
After pondering this for a while, I'm now leaning towards a different solution:
Can we just keep scoped_refptrs in PrefService for all our stores and use them
to construct a new PrefService using the above constructor from
CreateIncognitoPrefService? That would also obviate the need for the somewhat
fishy PrefValueStore::Derive().

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_mock_builder.h (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_mock_builder.h:31: scoped_refptr<PrefStore>
store);
Plain pointer parameters here.

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:178: scoped_ptr<PrefService>
incognito_prefs_;
Is this actually accessed? If not, can we postpone adding this code until it
actually gets used?

http://codereview.chromium.org/5915004/diff/41001/chrome/test/testing_pref_se...
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/test/testing_pref_se...
chrome/test/testing_pref_service.h:23: static TestingPrefService*
CreateTestingPrefService();
Introducing a factory function here makes it harder to work with
TestingPrefService, which somewhat defeats its purpose.

As discussed offline, how about something along these lines:

class TestingPrefServiceBase : public PrefService {
 public:
  // Existing TestingPrefService interface goes here.

 protected:
  TestingPrefServiceBase(PrefStore* managed, ...)
      : PrefService(managed, ...),
        managed_(managed) {
  }

 private:
  scoped_refptr<PrefStore> managed_;
};

class TestingPrefService : public TestingPrefServiceBase {
 public:
  TestingPrefService()
    : TestingPrefServiceBase(new TestingPrefStore(), ...) {
  }
};

http://codereview.chromium.org/5915004/diff/41001/chrome/test/testing_profile.cc
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/test/testing_profile...
chrome/test/testing_profile.cc:336: if (testing_prefs_) {
Why this conditional? GetPrefs() should suffice.

http://codereview.chromium.org/5915004/diff/41001/chrome/test/testing_profile.h
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/test/testing_profile...
chrome/test/testing_profile.h:325: scoped_ptr<PrefService> incognito_prefs_;
Used?

[battre, 2010-12-21 18:51:59]

I have addressed the comments and the CL is significantly smaller.

Currently the unit tests fail with a SIGSEGV that I still need to investigate:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7331623 in
policy::ConfigurationPolicyPrefStore::CreateDeviceManagementPolicyPrefStore
(profile=0x7fffe10f6580) at
chrome/browser/policy/configuration_policy_pref_store.cc:394
394         provider =
profile->GetPolicyContext()->GetDeviceManagementPolicyProvider();
(gdb) bt
#0  0x00007ffff7331623 in
policy::ConfigurationPolicyPrefStore::CreateDeviceManagementPolicyPrefStore
(profile=0x7fffe10f6580) at
chrome/browser/policy/configuration_policy_pref_store.cc:394
#1  0x00007ffff73702c4 in PrefValueStore::RefreshPolicyPrefs
(this=0x7fffe11afc60) at chrome/browser/prefs/pref_value_store.cc:245
#2  0x00007ffff7370aee in PrefValueStore::Observe (this=0x7fffe11afc60,
type=..., source=..., details=...) at
chrome/browser/prefs/pref_value_store.cc:343
#3  0x00007ffff5f2b0c1 in NotificationService::Notify (this=0x7fffe26db020,
type=..., source=..., details=...) at chrome/common/notification_service.cc:113
#4  0x00007ffff7334215 in
policy::ConfigurationPolicyProvider::NotifyStoreOfPolicyChange
(this=0x7fffe128e000) at
chrome/browser/policy/configuration_policy_provider.cc:23
#5  0x00007ffff733d7fe in
policy::DeviceManagementPolicyProvider::HandlePolicyResponse
(this=0x7fffe128e000, response=...) at
chrome/browser/policy/device_management_policy_provider.cc:123

Problem: GetPolicyContext of TestingProfile returns NULL

I don't understand this, yet. As TestingProfile always returns NULL and the code
has not changed. Anyway, I will investigate but I think I have reached a state
that is sufficient for further review.

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.h (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.h:45: static
scoped_refptr<ConfigurationPolicyPrefStore>
On 2010/12/20 14:50:02, Mattias Nissler wrote:
> This is not needed. If the function name is Create...() then the caller should
> be responsible for wrapping the pointer in a scoped_refptr (just like when
using
> an explicit new).

Done.

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/overla...
File chrome/browser/prefs/overlay_persistent_pref_store.h (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/overla...
chrome/browser/prefs/overlay_persistent_pref_store.h:17: class
OverlayPersistentPrefStore : public PersistentPrefStore,
On 2010/12/20 14:50:02, Mattias Nissler wrote:
> Could use a class comment describing the behavior of this PersistentPrefStore
> implementation.

Done.

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/overla...
chrome/browser/prefs/overlay_persistent_pref_store.h:21: // underlay as a weak
pointer.
On 2010/12/20 14:50:02, Mattias Nissler wrote:
> A scoped_refptr is not weak.

Done.

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:109:
scoped_refptr<ConfigurationPolicyPrefStore> managed =
On 2010/12/20 14:50:02, Mattias Nissler wrote:
> scoped_refptr construction not needed here either. Maybe it's time to just
> inline the construction below in the constructor call.

Done.

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:121: scoped_refptr<PrefStore>
incognito_extension_prefs);
On 2010/12/20 14:50:02, Mattias Nissler wrote:
> We should have a unit test for this, ensuring that values stored in the
> incognito service don't end up in the original PrefService.

Done.

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:247: // Constructor to be used only for
CreateIncognitoPrefService.
On 2010/12/20 14:50:02, Mattias Nissler wrote:
> After pondering this for a while, I'm now leaning towards a different
solution:
> Can we just keep scoped_refptrs in PrefService for all our stores and use them
> to construct a new PrefService using the above constructor from
> CreateIncognitoPrefService? That would also obviate the need for the somewhat
> fishy PrefValueStore::Derive().

Done.

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_mock_builder.h (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_mock_builder.h:31: scoped_refptr<PrefStore>
store);
On 2010/12/20 14:50:02, Mattias Nissler wrote:
> Plain pointer parameters here.

Done.

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:178: scoped_ptr<PrefService>
incognito_prefs_;
On 2010/12/20 14:50:02, Mattias Nissler wrote:
> Is this actually accessed? If not, can we postpone adding this code until it
> actually gets used?

Done.

http://codereview.chromium.org/5915004/diff/41001/chrome/test/testing_pref_se...
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/test/testing_pref_se...
chrome/test/testing_pref_service.h:23: static TestingPrefService*
CreateTestingPrefService();
On 2010/12/20 14:50:02, Mattias Nissler wrote:
> Introducing a factory function here makes it harder to work with
> TestingPrefService, which somewhat defeats its purpose.
> 
> As discussed offline, how about something along these lines:
> 
> class TestingPrefServiceBase : public PrefService {
>  public:
>   // Existing TestingPrefService interface goes here.
> 
>  protected:
>   TestingPrefServiceBase(PrefStore* managed, ...)
>       : PrefService(managed, ...),
>         managed_(managed) {
>   }
> 
>  private:
>   scoped_refptr<PrefStore> managed_;
> };
> 
> class TestingPrefService : public TestingPrefServiceBase {
>  public:
>   TestingPrefService()
>     : TestingPrefServiceBase(new TestingPrefStore(), ...) {
>   }
> };

Done.

http://codereview.chromium.org/5915004/diff/41001/chrome/test/testing_profile.cc
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/test/testing_profile...
chrome/test/testing_profile.cc:336: if (testing_prefs_) {
On 2010/12/20 14:50:02, Mattias Nissler wrote:
> Why this conditional? GetPrefs() should suffice.

Some mock classes derived from TestingProfile do not guarantee that
testing_prefs_ are defined. They only set prefs_ of type PrefService.
PrefService did not contain a pointer to the extension prefs until recently. Do
we want to expose this pointer in PrefService?

http://codereview.chromium.org/5915004/diff/41001/chrome/test/testing_profile.h
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/5915004/diff/41001/chrome/test/testing_profile...
chrome/test/testing_profile.h:325: scoped_ptr<PrefService> incognito_prefs_;
On 2010/12/20 14:50:02, Mattias Nissler wrote:
> Used?

Done.

[danno, 2010-12-22 10:48:21]

a first round of comments, more to come

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

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:831: for (int incognito = 0;
incognito <= 1; ++incognito) {
Ouch. This fast-and-loose use of boolean/int conversion just poked me in the
eye. How about creating a private helper function that you call once with
incognito_prefs_ and once with prefs_?

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1246: DictionaryValue* preferences
= GetExtensionControlledPrefs(*ext_id, !!i);
whoa. !!!(likes(danno, this)). Is this really more readable that unrolling the
loop?

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1325: !!incognito);
!!!(still_likes(danno, this))

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:63: // Does not assume owner ship of
|prefs| and |incognito_prefs|.
nit: s/owner ship/ownership/

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:682: int iteration;
should be iteration_

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:893: int iteration;
should be iteration_

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_api.cc:164:
->SetExtensionControlledPref(extension_id(), pref_path, incognito,
Why not pass false directly at the call site?

[Mattias Nissler (ping if slow), 2010-12-22 12:09:03]

some high-level comments to consider.

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:63: // Does not assume owner ship of
|prefs| and |incognito_prefs|.
s/owner ship/ownership/

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:68: ExtensionPrefStore*
incognito_pref_store);
It worries me that we need the incognito_prefs pointer here as well. The pref
service should only be used for keeping stuff that needs to be persisted.

How about the following design change: We change the ExtensionPrefStore
interface to keep not only the effective extension controlled prefs, but push
all of them there.  Figuring out what is the effective one could then be
implemented in the ExtensionPrefStore. That would mean that we don't need to go
to the PrefService to find out about all currently configured extension
controlled prefs, and the incognito PrefService pointer here could go away.

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:743: { 1, 2, 3, 4,  3, 4}
How about using a struct with descriptive variable names instead of the inner
int[6] array?

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/prefs/defaul...
File chrome/browser/prefs/default_pref_store.h (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/prefs/defaul...
chrome/browser/prefs/default_pref_store.h:33: : Value::TYPE_NULL;
Time for creating a default_pref_store.cc and putting this stuff there?

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:46: Value::ValueType type);
Can't we just ask the PrefService of the type?

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/extension_popup_controller_unittest.mm
(right):

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_popup_controller_unittest.mm:33:
NULL,
Is this safe?

http://codereview.chromium.org/5915004/diff/45002/chrome/common/pref_store.h
File chrome/common/pref_store.h (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/common/pref_store.h#...
chrome/common/pref_store.h:61: friend class base::RefCounted<PrefStore>;
Why not move these to the private section?

http://codereview.chromium.org/5915004/diff/45002/chrome/test/testing_pref_se...
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/test/testing_pref_se...
chrome/test/testing_pref_service.h:31: scoped_refptr<TestingPrefStore>
user_prefs_;
Didn't we agree to just move all the interface into this class and have the
actual TestingPrefService only have a constructor for initialization? Keeping
the pointers here, but the interface in the subclass seems odd to me.

http://codereview.chromium.org/5915004/diff/45002/chrome/test/testing_pref_se...
chrome/test/testing_pref_service.h:61: ExtensionPrefStore* GetExtensionPrefs();
No! This class is meant for the case when you need a PrefService in which you
can control all layers without the normally used dependencies. So we should
really use a plain PrefStore pointer for the extension store here and construct
a TestingPrefStore internally.

http://codereview.chromium.org/5915004/diff/45002/chrome/test/testing_profile.cc
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/test/testing_profile...
chrome/test/testing_profile.cc:337: if (testing_prefs_) {
GetPrefs() should be fine, no need for the conditional.

[battre, 2010-12-22 18:34:52]

I have addressed the first batch of comments. No need to review, yet, as the big
one (remove incognito_prefs_ from ExtensionPrefs) will require some significant
change and I wanted to upload a checkpoint in case this does not work out as
hoped.

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

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:831: for (int incognito = 0;
incognito <= 1; ++incognito) {
On 2010/12/22 10:48:21, danno wrote:
> Ouch. This fast-and-loose use of boolean/int conversion just poked me in the
> eye. How about creating a private helper function that you call once with
> incognito_prefs_ and once with prefs_?

Done. - Though with "boolean incognito" instead of the PrefService, will
probably change when I address Mattias' request that ExtensionPrefs does not
know incognito_prefs_.

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1246: DictionaryValue* preferences
= GetExtensionControlledPrefs(*ext_id, !!i);
On 2010/12/22 10:48:21, danno wrote:
> whoa. !!!(likes(danno, this)). Is this really more readable that unrolling the
> loop?

Philistine! This is art. ;-)

Done.

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1325: !!incognito);
On 2010/12/22 10:48:21, danno wrote:
> !!!(still_likes(danno, this))

Done.

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:63: // Does not assume owner ship of
|prefs| and |incognito_prefs|.
On 2010/12/22 10:48:21, danno wrote:
> nit: s/owner ship/ownership/

Done.

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:63: // Does not assume owner ship of
|prefs| and |incognito_prefs|.
On 2010/12/22 12:09:03, Mattias Nissler wrote:
> s/owner ship/ownership/

Done.

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:68: ExtensionPrefStore*
incognito_pref_store);
On 2010/12/22 12:09:03, Mattias Nissler wrote:
> It worries me that we need the incognito_prefs pointer here as well. The pref
> service should only be used for keeping stuff that needs to be persisted.
> 
> How about the following design change: We change the ExtensionPrefStore
> interface to keep not only the effective extension controlled prefs, but push
> all of them there.  Figuring out what is the effective one could then be
> implemented in the ExtensionPrefStore. That would mean that we don't need to
go
> to the PrefService to find out about all currently configured extension
> controlled prefs, and the incognito PrefService pointer here could go away.

Will do this in the next patch set of this CL.

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:682: int iteration;
On 2010/12/22 10:48:21, danno wrote:
> should be iteration_

Done.

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:743: { 1, 2, 3, 4,  3, 4}
On 2010/12/22 12:09:03, Mattias Nissler wrote:
> How about using a struct with descriptive variable names instead of the inner
> int[6] array?

Done.

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:893: int iteration;
On 2010/12/22 10:48:21, danno wrote:
> should be iteration_

Done.

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_api.cc:164:
->SetExtensionControlledPref(extension_id(), pref_path, incognito,
On 2010/12/22 10:48:21, danno wrote:
> Why not pass false directly at the call site?

If one of four parameters is a plain "false", I consider this bad style because
you cannot just grasp the parameter list without looking up the function
declaration.

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/prefs/defaul...
File chrome/browser/prefs/default_pref_store.h (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/prefs/defaul...
chrome/browser/prefs/default_pref_store.h:33: : Value::TYPE_NULL;
On 2010/12/22 12:09:03, Mattias Nissler wrote:
> Time for creating a default_pref_store.cc and putting this stuff there?

Done.

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:46: Value::ValueType type);
On 2010/12/22 12:09:03, Mattias Nissler wrote:
> Can't we just ask the PrefService of the type?

Will leave this in the Preference as discussed offline (Preference is a cache
anyway).

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/extension_popup_controller_unittest.mm
(right):

http://codereview.chromium.org/5915004/diff/45002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_popup_controller_unittest.mm:33:
NULL,
On 2010/12/22 12:09:03, Mattias Nissler wrote:
> Is this safe?

Yes. The Popup Controller unittest does not require incognito preferences and
all access to the incognito pref store/service is properly guarded.

http://codereview.chromium.org/5915004/diff/45002/chrome/common/pref_store.h
File chrome/common/pref_store.h (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/common/pref_store.h#...
chrome/common/pref_store.h:61: friend class base::RefCounted<PrefStore>;
On 2010/12/22 12:09:03, Mattias Nissler wrote:
> Why not move these to the private section?

PrefStore is a base class. Derived classes need to be destructible and therefore
access to the destructor.

http://codereview.chromium.org/5915004/diff/45002/chrome/test/testing_pref_se...
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/test/testing_pref_se...
chrome/test/testing_pref_service.h:31: scoped_refptr<TestingPrefStore>
user_prefs_;
On 2010/12/22 12:09:03, Mattias Nissler wrote:
> Didn't we agree to just move all the interface into this class and have the
> actual TestingPrefService only have a constructor for initialization? Keeping
> the pointers here, but the interface in the subclass seems odd to me.

Done.

http://codereview.chromium.org/5915004/diff/45002/chrome/test/testing_pref_se...
chrome/test/testing_pref_service.h:61: ExtensionPrefStore* GetExtensionPrefs();
On 2010/12/22 12:09:03, Mattias Nissler wrote:
> No! This class is meant for the case when you need a PrefService in which you
> can control all layers without the normally used dependencies. So we should
> really use a plain PrefStore pointer for the extension store here and
construct
> a TestingPrefStore internally.

This method was introduced because the ExtensionPrefs constructor called in
TestingProfile::CreateExtensionService needs an ExtensionPrefStore. The
TestingPrefService owns and creates the pref stores. If we want to work around
this, we probably need to touch all occurrences where TestingPrefService is used
and inject an ExtensionPrefStore. This will be messy.

http://codereview.chromium.org/5915004/diff/45002/chrome/test/testing_profile.cc
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/5915004/diff/45002/chrome/test/testing_profile...
chrome/test/testing_profile.cc:337: if (testing_prefs_) {
On 2010/12/22 12:09:03, Mattias Nissler wrote:
> GetPrefs() should be fine, no need for the conditional.

There are classes derived from TestingProfile that override GetPrefs() and do
not initialize testing_prefs_. They don't use and create it but inject a
different PrefService that is not derived from TestingPrefService.

[battre, 2011-01-05 09:59:40]

I think this CL is now complete. I appreciate your reviews.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_pref_value_map.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.h:18: class
ExtensionPrefValueMap {
If you find a better name for this class, I'd be happy to change it. Otherwise
we have
- ExtensionPrefValueMap
- ExtensionPrefStore
- ExtensionPrefs

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:659: };
In the following code there is quite some overlap with the new
extension_pref_value_map_unittest.cc. Yet I see the need for this kind of
integration testing. What do you think?

[Mattias Nissler (ping if slow), 2011-01-05 12:08:07]

Design-wise I like this revision much better!

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_pref_store.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_store.h:46: // ExtensionPrefValueMap.
Maybe it's useful to put the semantics comment above to the
ExtensionPrefValueMap description and just say here that the ExtensionPrefStore
fetches the effective values from the ExtensionPrefValueMap?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_store.h:56: ExtensionPrefValueMap*
extension_pref_value_map);
Why can't we pass this to the constructor?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_pref_value_map.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:17:
STLDeleteValues(&entries_);
Hm, let's put an entries_.clear() here just to be on the safe side :)

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:31: if (!modified)
no need for the modified local, just inline the condition.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:34: prefs->SetValue(key,
scoped_value.release());
You can just do the following:

if (prefs->SetValue(key, value))
  NotifyPrefValueChanged(key)

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:42:
prefs->RemoveValue(key);
RemoveValue returns a bool indicating whether there actually was a change. So
make the NotifyPrefValueChanged(key) call only when RemoveValue returns true?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:47:
DCHECK(!initialization_complete_);
It seems overkill to have that flag only for the sake of being able to DCHECK.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:49:
NotifyInitializationCompleted();
Why not just inline the NotifyInitializationCompleted code here?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:56: LOG(WARNING) <<
"Extension " << ext_id << " registered multiple times.";
Make this a NOTREACHED()?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:67: << " being
registered.";
NOTREACHED() again?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:71:
GetExtensionControlledKeys(ext_id, &keys);
It seems this function should really operate on ExtensionEntry and not make
another lookup...

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:73: delete
entries_[ext_id];
If you do the find() call above, can we use that iterator for the delete and
erase() calls instead of looking it up again?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:95: :
&(entries_[ext_id]->reg_preferences);
Again, three lookups where one would suffice. Why not do it along the lines of
the const variant below?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:109: 
excess newline

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:112: const std::string&
ext_id, std::set<std::string>* out) const {
I think you should break the line after the comma.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:112: const std::string&
ext_id, std::set<std::string>* out) const {
Do you actually require ordering here? If not, I'd swap the std::set for a
std::vector, which is more efficient.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:128: base::Time
winners_install_time = base::Time();
no need for the assignment here, the default constructor will be called anyway
:)

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:136: if (!enabled)
inline i->second->enabled?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_pref_value_map.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.h:53: bool is_enabled);
Would SetExtensionState be a more precise name for this?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.h:61: void
RemoveObserver(PrefStore::Observer* observer);
I can see that it's convenient to reuse the PrefStore::Observer class here since
it happens to provide the right callbacks. However, I'd vote for making a local
Observer class definition here, you can still cheat by inheriting from
PrefStore::Observer. But changing the definition to something explicit when the
requirements diverge is easier.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.h:65: private:
newline before private: declaration.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.h:73: // Incognito
preferences, empty for regular ExtensionPerfStore.
s/Perf/Pref/

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_pref_value_map_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:29: class
ExtensionPrefStoreTestBase : public BASECLASS {
I think the classes here should be named ExtensionPrefValueMapTest...

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:40: static
base::Time createTime(long t) {
can you sync createVal and createTime to the effect that both are either member
functions or not?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:53: class
ExtensionPrefStoreTestParameterized
Seems this class definition is not really helpful, just make
ExtensionPrefStoreTestIncognitoTest derive directly from
ExtensionPrefStoreTestBase.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:69:
TEST_F(ExtensionPrefStoreTest, Overwrite) {
nit: name this Override

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:118: class
ExtensionPrefStoreTestIncognitoTests
Can you move this test fixture down below all the other tests in the regular
fixture? I find it easier to make sense of the file if each fixture is in its
own block.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:177:
TEST_F(ExtensionPrefStoreTest, OverrideInterleaved) {
Does this really give us additional coverage over Override? Can we perhaps test
all of it in one test case?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:284: // Write the
same value again
Comment missing period (also below).

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:289: // Overwrite
incognito value
nit: I think you should decide on either overwrite or override (I prefer the
latter) and be consistent.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:316: // Write new
value --> no notification ofter removing observer
s/ofter/after/

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

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:844: if (!prefs_)  // May be null
for unit tests.
Is that actually the case? Why not just pass a TestingPrefService for these
tests?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1227: // Store winning preference
for each extension controlled preference.
Comment is outdated.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:68: ExtensionPrefStore*
incognito_pref_store);
Why does this accept the pref stores? It should only get a single pointer to the
ExtensionPrefValueMap as far as I can see.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:273: PrefService*
CreateIncognitoPrefService() const;
This shouldn't be here.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:597: const bool incognito
= false;
that's a very elaborate way of saying false below :)

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:602: const std::string&
key,
indentation

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:605: const bool incognito
= true;
same here, just inline.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:633: void
EnsureExtensionInstalled(Extension *ext) {
Why not just put this code into InstallExtension?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:659: };
On 2011/01/05 09:59:40, battre wrote:
> In the following code there is quite some overlap with the new
> extension_pref_value_map_unittest.cc. Yet I see the need for this kind of
> integration testing. What do you think?

Yes, we need the integration tests, but they don't need to check for each corner
case, but only if the operations on ExtensionPrefs do show up in the
ExtensionPrefValueMap (which is easy to check if we make that a dependency we
pass to ExtensionPrefs at construction time btw).

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_api.cc:173: const bool incognito =
false;
Again your elaborate version of saying false?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_apitest.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_apitest.cc:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
Don't update the header if you don't touch the file.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_updater.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_updater.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
Same here.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_updater_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_updater_unittest.cc:95: // no two extensions
share the same name.
Out of curiosity: Why is this now required?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/overla...
File chrome/browser/prefs/overlay_persistent_pref_store.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/overla...
chrome/browser/prefs/overlay_persistent_pref_store.h:20: // the look-up passed
on to an underlying PersistentPrefStore |underlay_|.
This comment is outdated to the effect that the in-memory thing is not a
PrefStore.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/overla...
File chrome/browser/prefs/overlay_persistent_pref_store_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/overla...
chrome/browser/prefs/overlay_persistent_pref_store_unittest.cc:27: class
PrefStoreObserverMock : public PrefStore::Observer {
Use pref_store_observer_mock.h?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/overla...
chrome/browser/prefs/overlay_persistent_pref_store_unittest.cc:37: // check that
underlay first value is reported
Make it a proper sentence.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:165: if (init_from_storage)
I believe I have commented on this before but don't remember the conclusion:
Can't we just drop the init_from_storage parameter and call InitFromStorage()
unconditionally?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:442: prefs_.insert(new Preference(this,
path, orig_type));
If new do this, we don't need the lazy creation code at line 365 above, no?

http://codereview.chromium.org/5915004/diff/60002/chrome/common/pref_store.h
File chrome/common/pref_store.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/common/pref_store.h#...
chrome/common/pref_store.h:61: friend class base::RefCounted<PrefStore>;
Maybe add a comment saying that we do this to ensure PrefStore is deleted by
RefCounted only and drop the newline below, so readers know immediately what's
going on.

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_pref_se...
File chrome/test/testing_pref_service.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_pref_se...
chrome/test/testing_pref_service.cc:16: // which they want, and expand usage of
this class to more unit tests.
This comment is outdated, just remove it. The solution to the problem described
is PrefServiceMockBuilder.

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_pref_se...
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_pref_se...
chrome/test/testing_pref_service.h:39: ExtensionPrefStore* GetExtensionPrefs();
This is bad, we should have TestingPrefStores exclusively. The rationale is that
TestingPrefService is designed s.t. tests can setup up values in the stores at
their own discretion easily.

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_pref_se...
chrome/test/testing_pref_service.h:73: private:
new line before private:

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_profile.cc
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_profile...
chrome/test/testing_profile.cc:336: if (testing_prefs_) {
If have commented this before, just use GetPrefs() and drop the conditional.

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_profile.h
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_profile...
chrome/test/testing_profile.h:118: ExtensionService*
CreateExtensionService(const CommandLine* command_line,
What's the reason from making this refcounted?

[battre, 2011-01-05 20:23:08]

Next iteration. :-)

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_pref_store.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_store.h:46: // ExtensionPrefValueMap.
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Maybe it's useful to put the semantics comment above to the
> ExtensionPrefValueMap description and just say here that the
ExtensionPrefStore
> fetches the effective values from the ExtensionPrefValueMap?

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_store.h:56: ExtensionPrefValueMap*
extension_pref_value_map);
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Why can't we pass this to the constructor?

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_pref_value_map.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:17:
STLDeleteValues(&entries_);
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Hm, let's put an entries_.clear() here just to be on the safe side :)

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:31: if (!modified)
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> no need for the modified local, just inline the condition.

Gone. (due to following comment)

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:34: prefs->SetValue(key,
scoped_value.release());
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> You can just do the following:
> 
> if (prefs->SetValue(key, value))
>   NotifyPrefValueChanged(key)

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:42:
prefs->RemoveValue(key);
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> RemoveValue returns a bool indicating whether there actually was a change. So
> make the NotifyPrefValueChanged(key) call only when RemoveValue returns true?

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:47:
DCHECK(!initialization_complete_);
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> It seems overkill to have that flag only for the sake of being able to DCHECK.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:49:
NotifyInitializationCompleted();
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Why not just inline the NotifyInitializationCompleted code here?

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:56: LOG(WARNING) <<
"Extension " << ext_id << " registered multiple times.";
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Make this a NOTREACHED()?

Cannot be NOTREACHED. An extension can be reinstalled in which case it should
have empty preferences.

I have removed the warning.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:67: << " being
registered.";
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> NOTREACHED() again?

Everytime the extension black list is updated, all their configuration values
are deleted. I have removed the warning as unregistering an extension twice is
not really critical in any way.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:71:
GetExtensionControlledKeys(ext_id, &keys);
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> It seems this function should really operate on ExtensionEntry and not make
> another lookup...

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:73: delete
entries_[ext_id];
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> If you do the find() call above, can we use that iterator for the delete and
> erase() calls instead of looking it up again?

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:95: :
&(entries_[ext_id]->reg_preferences);
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Again, three lookups where one would suffice. Why not do it along the lines of
> the const variant below?

I prefer the visual clarity of a [] lookup over some obscure "i->second", but I
don't care too much.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:109: 
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> excess newline

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:112: const std::string&
ext_id, std::set<std::string>* out) const {
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> I think you should break the line after the comma.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:112: const std::string&
ext_id, std::set<std::string>* out) const {
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Do you actually require ordering here? If not, I'd swap the std::set for a
> std::vector, which is more efficient.

I don't require ordering but uniqueness. An extension might set pref1 and pref2
in reg_prefs and pref2 and pref3 in inc_prefs. We want to retrieve {pref1,
pref2, pref3}.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:128: base::Time
winners_install_time = base::Time();
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> no need for the assignment here, the default constructor will be called anyway
> :)

right... one a Java programmer by heart you never change your habits. :-)

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.cc:136: if (!enabled)
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> inline i->second->enabled?

I prefer my variant: The first three lines extract the relevant information from
some obscure iterator syntax. It should not make any difference at runtime.

If you disagree, I could do a
const ExtensionEntry& entry = i->second;
instead of
const base::Time& install_time = i->second->install_time;
const bool enabled = i->second->enabled;

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_pref_value_map.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.h:53: bool is_enabled);
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Would SetExtensionState be a more precise name for this?

The SetExtensionState name collides with SetExtensionState in PrefService which
has a slightly different meaning there. The state is not just "enabled" and
"disabled" but also "killbit". OTOH changing SetExtensionState to
UpdateExtensionsState is not much better. Therefore, I have changed it to
SetExtensionState.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.h:61: void
RemoveObserver(PrefStore::Observer* observer);
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> I can see that it's convenient to reuse the PrefStore::Observer class here
since
> it happens to provide the right callbacks. However, I'd vote for making a
local
> Observer class definition here, you can still cheat by inheriting from
> PrefStore::Observer. But changing the definition to something explicit when
the
> requirements diverge is easier.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.h:65: private:
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> newline before private: declaration.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map.h:73: // Incognito
preferences, empty for regular ExtensionPerfStore.
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> s/Perf/Pref/

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_pref_value_map_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:29: class
ExtensionPrefStoreTestBase : public BASECLASS {
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> I think the classes here should be named ExtensionPrefValueMapTest...

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:40: static
base::Time createTime(long t) {
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> can you sync createVal and createTime to the effect that both are either
member
> functions or not?

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:53: class
ExtensionPrefStoreTestParameterized
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Seems this class definition is not really helpful, just make
> ExtensionPrefStoreTestIncognitoTest derive directly from
> ExtensionPrefStoreTestBase.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:69:
TEST_F(ExtensionPrefStoreTest, Overwrite) {
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> nit: name this Override

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:118: class
ExtensionPrefStoreTestIncognitoTests
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Can you move this test fixture down below all the other tests in the regular
> fixture? I find it easier to make sense of the file if each fixture is in its
> own block.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:177:
TEST_F(ExtensionPrefStoreTest, OverrideInterleaved) {
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Does this really give us additional coverage over Override? Can we perhaps
test
> all of it in one test case?

This is mostly legacy from the old unit tests. It does not increase coverage.

Deleted.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:284: // Write the
same value again
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Comment missing period (also below).

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:289: // Overwrite
incognito value
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> nit: I think you should decide on either overwrite or override (I prefer the
> latter) and be consistent.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:316: // Write new
value --> no notification ofter removing observer
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> s/ofter/after/

Done.

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

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:844: if (!prefs_)  // May be null
for unit tests.
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Is that actually the case? Why not just pass a TestingPrefService for these
> tests?

Is not NULL any more.

Removed.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1227: // Store winning preference
for each extension controlled preference.
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Comment is outdated.

Replaced "Collect the uniqe..." with this.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:68: ExtensionPrefStore*
incognito_pref_store);
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Why does this accept the pref stores? It should only get a single pointer to
the
> ExtensionPrefValueMap as far as I can see.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:273: PrefService*
CreateIncognitoPrefService() const;
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> This shouldn't be here.

Gone.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:597: const bool incognito
= false;
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> that's a very elaborate way of saying false below :)

I got the same comment from Danno, so I will change it. I don't agree though. I
don't like passing boolean values to a function as they are somewhat untyped.
You cannot parse the meaning of false in "SetExtensionControlledPref(ext->id(),
key, false, val)" without lookup up the definition of
SetExtensionControlledPref.

In Java it is good practice to cast null to the correct type (even though it
does not make any difference at all) to improve readability.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:602: const std::string&
key,
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:605: const bool incognito
= true;
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> same here, just inline.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:633: void
EnsureExtensionInstalled(Extension *ext) {
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Why not just put this code into InstallExtension?

Because InstallExtensionControlled{Incognito}Pref need to ensure that the
extension is installed. They shall not install one each time a preference value
is installed for an extension. At the same time we sometimes need to just
install an extension w/o setting any pref values, i.e. we need an
InstallExtension function.

Moving this code into InstallExtension would not reflect what it does.

For symmetry reasons I have moved the code of UninstallExtension into
EnsureExtensionUinstalled.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs_unittest.cc:659: };
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> On 2011/01/05 09:59:40, battre wrote:
> > In the following code there is quite some overlap with the new
> > extension_pref_value_map_unittest.cc. Yet I see the need for this kind of
> > integration testing. What do you think?
> 
> Yes, we need the integration tests, but they don't need to check for each
corner
> case, but only if the operations on ExtensionPrefs do show up in the
> ExtensionPrefValueMap (which is easy to check if we make that a dependency we
> pass to ExtensionPrefs at construction time btw).

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_api.cc:173: const bool incognito =
false;
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Again your elaborate version of saying false?

Done.

Prepare to be asked about the meaning of "false" in this line in about two
weeks. ;-)

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_apitest.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_apitest.cc:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Don't update the header if you don't touch the file.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_updater.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_updater.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Same here.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_updater_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_updater_unittest.cc:95: // no two extensions
share the same name.
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Out of curiosity: Why is this now required?

Otherwise the two extensions receive the same hashed name and are
indistinguishable in the ExtensionPrefValueMap. I.e. the same key is installed
twice and removed twice.

The test cases do not break any more because I removed the DCHECK for double
registration/unregistration. Shall I revert this?

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/overla...
File chrome/browser/prefs/overlay_persistent_pref_store.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/overla...
chrome/browser/prefs/overlay_persistent_pref_store.h:20: // the look-up passed
on to an underlying PersistentPrefStore |underlay_|.
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> This comment is outdated to the effect that the in-memory thing is not a
> PrefStore.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/overla...
File chrome/browser/prefs/overlay_persistent_pref_store_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/overla...
chrome/browser/prefs/overlay_persistent_pref_store_unittest.cc:27: class
PrefStoreObserverMock : public PrefStore::Observer {
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Use pref_store_observer_mock.h?

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/overla...
chrome/browser/prefs/overlay_persistent_pref_store_unittest.cc:37: // check that
underlay first value is reported
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Make it a proper sentence.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:165: if (init_from_storage)
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> I believe I have commented on this before but don't remember the conclusion:
> Can't we just drop the init_from_storage parameter and call InitFromStorage()
> unconditionally?

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:442: prefs_.insert(new Preference(this,
path, orig_type));
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> If new do this, we don't need the lazy creation code at line 365 above, no?

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/common/pref_store.h
File chrome/common/pref_store.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/common/pref_store.h#...
chrome/common/pref_store.h:61: friend class base::RefCounted<PrefStore>;
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> Maybe add a comment saying that we do this to ensure PrefStore is deleted by
> RefCounted only and drop the newline below, so readers know immediately what's
> going on.

This is part of the RefCounted contract and documented in the RefCounted header.
I did a quick sample and did not find any "friend class base::RefCounted<...>"
declaration that had such a comment.

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_pref_se...
File chrome/test/testing_pref_service.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_pref_se...
chrome/test/testing_pref_service.cc:16: // which they want, and expand usage of
this class to more unit tests.
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> This comment is outdated, just remove it. The solution to the problem
described
> is PrefServiceMockBuilder.

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_pref_se...
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_pref_se...
chrome/test/testing_pref_service.h:39: ExtensionPrefStore* GetExtensionPrefs();
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> This is bad, we should have TestingPrefStores exclusively. The rationale is
that
> TestingPrefService is designed s.t. tests can setup up values in the stores at
> their own discretion easily.

Gone. TestingPrefService cannot use ExtensionPrefStores, however.

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_pref_se...
chrome/test/testing_pref_service.h:73: private:
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> new line before private:

Done.

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_profile.cc
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_profile...
chrome/test/testing_profile.cc:336: if (testing_prefs_) {
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> If have commented this before, just use GetPrefs() and drop the conditional.

I have commented this as well. ;-)
See
http://codereview.chromium.org/5915004/diff/45002/chrome/test/testing_profile.cc
for the justification why we needed it.

This is gone now.

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_profile.h
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/5915004/diff/60002/chrome/test/testing_profile...
chrome/test/testing_profile.h:118: ExtensionService*
CreateExtensionService(const CommandLine* command_line,
On 2011/01/05 12:08:07, Mattias Nissler wrote:
> What's the reason from making this refcounted?

n/a

[Mattias Nissler (ping if slow), 2011-01-07 10:12:57]

I very much like the current version of this! Do you think it is worthwhile to
also adjust GetPrefs() of the incognito profile in this CL? That would make the
thing complete :)

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_store.cc:15: if
(extension_pref_value_map_)
Why the conditional? This parameter should always be non-null, no?

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_value_map.cc (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.cc:25: scoped_ptr<Value>
scoped_value(value);
You can just drop the scoped_value declaration and use value directly.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_value_map.h (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.h:59: // unsubscription.
Requires unsubscription is a bit vague, can you just say that clients must
unsubscribe?

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.h:102: void
NotifyOfDestruction();
Why is this public?

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.h:127: // Returns all keys of
pref values that are set by the extension of entry,
|entry|

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.h:137: // are stored in the
datastructures of the base class.
What base class?

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:720: // at the
extension_pref_value_map_.
But then you should probably update the function name to reflect that it now
does more than just deleting prefs.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:14: #include "base/ref_counted.h"
No need for this header.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:16: #include
"chrome/browser/extensions/extension_pref_value_map.h"
I think a forward declaration would suffice.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:21: class ExtensionPrefStore;
No need for this.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:726:
scoped_ptr<NotificationObserverMock> observer(
Any reason for not allocating the observers on the stack?

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:686: 
Why this newline?

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.h:185: scoped_ptr<ExtensionPrefValueMap>
extension_pref_value_map_;
Don't you need to put this before the |prefs_| declaration to guarantee
destruction order?

http://codereview.chromium.org/5915004/diff/101001/chrome/test/testing_profil...
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/test/testing_profil...
chrome/test/testing_profile.cc:337: // inject a new
ExtensionPrefStore(extension_pref_value_map_.get(), false);.
I think you can drop the semicolon :-D

[battre, 2011-01-10 16:55:47]

Addressed Matthias comments.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_store.cc:15: if
(extension_pref_value_map_)
On 2011/01/07 10:12:58, Mattias Nissler wrote:
> Why the conditional? This parameter should always be non-null, no?

Done.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_value_map.cc (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.cc:25: scoped_ptr<Value>
scoped_value(value);
On 2011/01/07 10:12:58, Mattias Nissler wrote:
> You can just drop the scoped_value declaration and use value directly.

Done.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_value_map.h (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.h:59: // unsubscription.
On 2011/01/07 10:12:58, Mattias Nissler wrote:
> Requires unsubscription is a bit vague, can you just say that clients must
> unsubscribe?

Done.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.h:102: void
NotifyOfDestruction();
On 2011/01/07 10:12:58, Mattias Nissler wrote:
> Why is this public?

Done.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.h:127: // Returns all keys of
pref values that are set by the extension of entry,
On 2011/01/07 10:12:58, Mattias Nissler wrote:
> |entry|

Done.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.h:137: // are stored in the
datastructures of the base class.
On 2011/01/07 10:12:58, Mattias Nissler wrote:
> What base class?

This referred to the old code. I have updated the comment.

Done.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:720: // at the
extension_pref_value_map_.
On 2011/01/07 10:12:58, Mattias Nissler wrote:
> But then you should probably update the function name to reflect that it now
> does more than just deleting prefs.

Actually, I think I can delete the comment. "Unregistering the extension at the
extension_pref_value_map_" means "Deleting all extension prefs in the
extension_pref_value_map_".

--> I have deleted this.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:14: #include "base/ref_counted.h"
On 2011/01/07 10:12:58, Mattias Nissler wrote:
> No need for this header.

Done.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:16: #include
"chrome/browser/extensions/extension_pref_value_map.h"
On 2011/01/07 10:12:58, Mattias Nissler wrote:
> I think a forward declaration would suffice.

Done.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:21: class ExtensionPrefStore;
On 2011/01/07 10:12:58, Mattias Nissler wrote:
> No need for this.

Done.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:726:
scoped_ptr<NotificationObserverMock> observer(
On 2011/01/07 10:12:58, Mattias Nissler wrote:
> Any reason for not allocating the observers on the stack?

Done.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:686: 
On 2011/01/07 10:12:58, Mattias Nissler wrote:
> Why this newline?

Done.

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.h:185: scoped_ptr<ExtensionPrefValueMap>
extension_pref_value_map_;
On 2011/01/07 10:12:58, Mattias Nissler wrote:
> Don't you need to put this before the |prefs_| declaration to guarantee
> destruction order?

Done.

http://codereview.chromium.org/5915004/diff/101001/chrome/test/testing_profil...
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/5915004/diff/101001/chrome/test/testing_profil...
chrome/test/testing_profile.cc:337: // inject a new
ExtensionPrefStore(extension_pref_value_map_.get(), false);.
On 2011/01/07 10:12:58, Mattias Nissler wrote:
> I think you can drop the semicolon :-D

Done.

[battre, 2011-01-10 16:58:41]

@aa: Please let me know whether you want to review this.
@danno: Please review this :-)

The current state is that Mattias was happy with the state of the implementation
on Friday. He has not seen the most recent changes, yet.

In order to simplify the review of this huge CL, here is some guideline:

The goal of this change list was that the regular profile and the incognito
profile share several PrefStores (e.g. Policy related, CommandLinePrefStore,
Default values, etc.) but differ in their ExtensionPrefStores.
- This required that the PrefStores became Ref Counted.
- That default values are managed by the DefaultPrefStore (shared) and not by
the PrefValueStore (one per profile). The type is derived from the stored
default value. This has the consequence that a default "empty dictionary" is now
represented by an empty ValueDictionary* instead of a NULL pointer.
- Once the PrefStores became sharable, I could derive an incognito profile.
- In order to test this in practice (and because Matthias requested this ;-)), I
use the incognito profile for different proxy settings. It works :-)

This is a grouping and explanation of the changes in order to make them easier
reviewable.

New implementation of ExtensionPrefStore. It is now a simple frontend that
subscribes to ExtensionPrefValue map and fetches the effective preference values
upon changes:
chrome/browser/extensions/extension_pref_store.h
chrome/browser/extensions/extension_pref_store.cc

A pref value map is shared between the incognito profile and the regular
profile. It stores for all extensions, what preference values they have set, and
can tell the effective preference values.
chrome/browser/extensions/extension_pref_value_map.h
chrome/browser/extensions/extension_pref_value_map.cc
chrome/browser/extensions/extension_pref_value_map_unittest.cc

This is moslty deletions of code that was moved into ExtensionPrefValueMap.
chrome/browser/extensions/extension_prefs.h
chrome/browser/extensions/extension_prefs.cc
chrome/browser/extensions/extension_prefs_unittest.cc

Add support for incognito preference store to OffTheRecordProfile
chrome/browser/profiles/profile.h
chrome/browser/profiles/profile.cc
chrome/browser/profiles/profile_impl.h
chrome/browser/profiles/profile_impl.cc
chrome/test/testing_profile.h
chrome/test/testing_profile.cc

Make the OffTheRecordProfile respect incognito proxy settings
chrome/browser/net/chrome_url_request_context.cc

DefaultPrefStore became the primary source of registered pref types (i.e. key
XYZ is associated with a ValueList)
chrome/browser/prefs/default_pref_store.h
chrome/browser/prefs/default_pref_store.cc

Implemented an overplay pref store such that write operations from the incognito
profile cannot be propagated into the JSON preferences file.
chrome/browser/prefs/overlay_persistent_pref_store.h
chrome/browser/prefs/overlay_persistent_pref_store.cc
chrome/browser/prefs/overlay_persistent_pref_store_unittest.cc

The incognito profile is destroyed when the last incognito window is closed.
OTOH the last Window contains PrefMembers that depend on the PrefStore owned by
the incognito profile. This dependency needs to be resolved at destruction time.
chrome/browser/prefs/pref_member.h
chrome/browser/prefs/pref_member.cc
chrome/browser/ui/browser.cc

The DefaultPrefStore became the master copy for types of registered preferences
because it can be shared between a regular profile and an incognito profile (or
rather a regular PrefValueStore and the one incognito version).
Therefore, the "preference->type" mapping could be removed from the
PrefValueStore.
chrome/browser/prefs/pref_service.h
chrome/browser/prefs/pref_service.cc
chrome/browser/prefs/pref_service_unittest.cc
chrome/browser/prefs/pref_value_store.h
chrome/browser/prefs/pref_value_store.cc
chrome/browser/prefs/pref_value_store_unittest.cc

Asked eroman@ to verify this. Thread-destructor needs to be executed on I/O
thread.
net/proxy/multi_threaded_proxy_resolver.cc

Added temporary base::ThreadRestrictions::ScopedAllowIO for bug 69057 (GConf
wrapper KDE does I/O on UI thread)
net/proxy/proxy_config_service_linux.cc

The translate manager is a singleton and therefore, can subscribe to only one
Profile. We make sure this is the original profile.
chrome/browser/translate/translate_manager.cc

Tiny and boring stuff:
======================
SetExtensionControlledPref got a new parameter that indicates whether we want to
store a preference for incognito mode or for regular mode.
chrome/browser/extensions/extension_proxy_api.cc

Fix whitespaces / typos
chrome/browser/extensions/extension_service.cc
chrome/common/persistent_pref_store.h

Make sure that extensions have unique identifiers (before, two extensions with
the same ID were inserted and then the same ID was deleted twice).
chrome/browser/extensions/extension_updater_unittest.cc

Migrate to ExtensionPrefValueMap:
chrome/browser/extensions/test_extension_prefs.h
chrome/browser/extensions/test_extension_prefs.cc

Empty Dictionaries and Lists are now stored as those, not as nulls. This is
necessary because the DefaultPrefStore became the master copy for preference
types.
chrome/browser/host_zoom_map_unittest.cc
chrome/browser/sync/glue/preference_model_associator_unittest.cc
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm
chrome/browser/ui/cocoa/window_size_autosaver_unittest.mm

All PrefStores became refcounted and need to be allocated on the heap:
chrome/common/pref_store.h
chrome/browser/policy/configuration_policy_pref_store_unittest.cc
chrome/browser/prefs/command_line_pref_store_unittest.cc
chrome/browser/prefs/pref_service_mock_builder.h
chrome/browser/prefs/pref_service_mock_builder.cc
chrome/common/json_pref_store_unittest.cc
chrome/test/testing_pref_service.h
chrome/test/testing_pref_service.cc

Added iterators
chrome/browser/prefs/pref_value_map.h
chrome/browser/prefs/pref_value_map.cc

Support new ExtensionPrefStore
chrome/browser/ui/cocoa/extensions/extension_popup_controller_unittest.mm

Other stuff:
chrome/chrome_browser.gypi
chrome/chrome_tests.gypi

[danno, 2011-01-14 13:05:09]

LGTM with few comments, mostly nits.

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_value_map.cc (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.cc:49:
ExtensionEntryMap::iterator i = entries_.find(ext_id);
const

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.cc:63:
ExtensionEntryMap::iterator i = entries_.find(ext_id);
const, also elsewhere in this file.

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_value_map.h (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.h:98: void
AddObserver(Observer* observer);
Perhaps the observer interface should be protected if there is only one client
(class) that should be allowed to use it.

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_value_map_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:26: return
Value::CreateStringValue(str);
nit: probably should be in an anon namespace, too?

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:29: static
base::Time createTime(int64 t) {
same here? Also capitalization.

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:40: static void
AddPattern(ExtensionExtent* extent, const std::string& pattern) {
maybe an anon namespace because of the generic name?

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service.cc:134: default_store_);
In previous versions, there was an effort to keep the knowledge of stores other
than the default and user store out of the PrefService. This change seems to
revisit this strategy. Why can the PrefValueStore not have a CloneAndSpecialize
method or equivalent that obviates the need to explicitly track the pref stores
in the PrefService?

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_unittest.cc:160: Value::CreateStringValue("not
an integer"));
nit: indent

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_value_store.h:68: // calling this method directly.
Please add comment about treatment of type (i.e. that keys with mismatched types
are ignored).

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile.cc:627: ExtensionPrefValueMap*
GetExtensionPrefValueMap() {
should it be virtual, maybe private :-)

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.h:149: ExtensionPrefValueMap*
GetExtensionPrefValueMap();
virtual, maybe private, again your call.

http://codereview.chromium.org/5915004/diff/139001/chrome/test/testing_pref_s...
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/test/testing_pref_s...
chrome/test/testing_pref_service.h:47: scoped_refptr<TestingPrefStore>
user_prefs_;
make these private?

http://codereview.chromium.org/5915004/diff/139001/chrome/test/testing_profile.h
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/test/testing_profil...
chrome/test/testing_profile.h:325: }
private maybe :-)

[battre, 2011-01-20 17:59:28]

Addressed the nits and fixed a nasty crasher on MacOS. Please review the changes
since Patch Set 12.

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_value_map.cc (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.cc:49:
ExtensionEntryMap::iterator i = entries_.find(ext_id);
On 2011/01/14 13:05:09, danno wrote:
> const

Gives a compiler error.

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.cc:63:
ExtensionEntryMap::iterator i = entries_.find(ext_id);
On 2011/01/14 13:05:09, danno wrote:
> const, also elsewhere in this file.

Done.

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_value_map.h (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map.h:98: void
AddObserver(Observer* observer);
On 2011/01/14 13:05:09, danno wrote:
> Perhaps the observer interface should be protected if there is only one client
> (class) that should be allowed to use it.

Discussed this with mnissler and decided not to do this. It creates a tighter
coupling and is syntactically not very nice.

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_pref_value_map_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:26: return
Value::CreateStringValue(str);
On 2011/01/14 13:05:09, danno wrote:
> nit: probably should be in an anon namespace, too?

Style guide allows both versions (note that the function is declared static) and
several unittests use static version.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Nonmember,_Sta...

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:29: static
base::Time createTime(int64 t) {
On 2011/01/14 13:05:09, danno wrote:
> same here? Also capitalization.

Done.

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:40: static void
AddPattern(ExtensionExtent* extent, const std::string& pattern) {
On 2011/01/14 13:05:09, danno wrote:
> maybe an anon namespace because of the generic name? 

Is already static.

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service.cc:134: default_store_);
On 2011/01/14 13:05:09, danno wrote:
> In previous versions, there was an effort to keep the knowledge of stores
other
> than the default and user store out of the PrefService. This change seems to
> revisit this strategy. Why can the PrefValueStore not have a
CloneAndSpecialize
> method or equivalent that obviates the need to explicitly track the pref
stores
> in the PrefService?

Done.

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_unittest.cc:160: Value::CreateStringValue("not
an integer"));
On 2011/01/14 13:05:09, danno wrote:
> nit: indent

Done.

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_value_store.h:68: // calling this method directly.
On 2011/01/14 13:05:09, danno wrote:
> Please add comment about treatment of type (i.e. that keys with mismatched
types
> are ignored).

Done.

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile.cc:627: ExtensionPrefValueMap*
GetExtensionPrefValueMap() {
On 2011/01/14 13:05:09, danno wrote:
> should it be virtual, maybe private :-)

Made virtual, not private.

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.h:149: ExtensionPrefValueMap*
GetExtensionPrefValueMap();
On 2011/01/14 13:05:09, danno wrote:
> virtual, maybe private, again your call.

Made virtual, not private.

http://codereview.chromium.org/5915004/diff/139001/chrome/test/testing_pref_s...
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/test/testing_pref_s...
chrome/test/testing_pref_service.h:47: scoped_refptr<TestingPrefStore>
user_prefs_;
On 2011/01/14 13:05:09, danno wrote:
> make these private?

Done.

http://codereview.chromium.org/5915004/diff/139001/chrome/test/testing_profile.h
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/5915004/diff/139001/chrome/test/testing_profil...
chrome/test/testing_profile.h:325: }
On 2011/01/14 13:05:09, danno wrote:
> private maybe :-)

decided not to change this

[battre, 2011-01-20 18:04:37]

+eroman --> Could you please review the changes in net/

+Nico --> FYI:
chrome/browser/ui/cocoa/extensions/extension_action_context_menu.* is how I
addressed the destruction order issue. All tests pass now. I will delay the CL
to after the M10 branch so that we can see whether any crashers remain.

[eroman, 2011-01-20 23:17:25]

> +eroman --> Could you please review the changes in net/

LGTM for the changes under net/

http://codereview.chromium.org/5915004/diff/142002/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/5915004/diff/142002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_linux.cc:439: // This has to be called on the UI
thread (bug 69057).
nit: I recommend providing URLs to bugs, as in "http://crbug.com/69057", makes
it a little more discoverable for newcomers. (Also we used to have an
internal+external bug tracker, so specifying the namespace of the bug ID is
useful).

[Mattias Nissler (ping if slow), 2011-01-21 08:53:05]

I felt like nitting a bit in the morning :-P

No, just had a quick glance to catch up after vacation and what I saw looks
pretty good.

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1188: // Store winning preference
for each extension controlled preference.
Are you really storing the winning preference? AFAICS, you're just uploading all
extension prefs to extension_pref_value_map_, so you might want to update the
comment to that effect.

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service.h:248: PrefStore* incognito_extension_prefs);
can this be private?

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_value_store.cc:81: if (!recommended_prefs)
recommended_prefs = GetPrefStore(RECOMMENDED_STORE);
The style guide says "Short conditional statements may be written on one line if
this enhances readability. You may use this only when the line is brief and the
statement does not use the else clause."

The line in question is not brief but about as long as it can get and having a
common style for all of the declarations here would make it more readable, no?

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile.cc:145: new
ExtensionPrefStore(profile_->GetExtensionPrefValueMap(), true)));
Hm, what's the contract of GetExtensionPrefValueMap()? As far as I can see it
may return NULL if you don't call real_profile->GetPrefs() first, but you're not
prepared to handle that here. Maybe you want to move the lazy initialization
into GetExtensionPrefValueMap so it always returns something?

[battre, 2011-01-24 17:47:18]

Addressed nits.

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1188: // Store winning preference
for each extension controlled preference.
On 2011/01/21 08:53:08, Mattias Nissler wrote:
> Are you really storing the winning preference? AFAICS, you're just uploading
all
> extension prefs to extension_pref_value_map_, so you might want to update the
> comment to that effect.

Done.

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service.h:248: PrefStore* incognito_extension_prefs);
On 2011/01/21 08:53:08, Mattias Nissler wrote:
> can this be private?

Done.

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_value_store.cc:81: if (!recommended_prefs)
recommended_prefs = GetPrefStore(RECOMMENDED_STORE);
On 2011/01/21 08:53:08, Mattias Nissler wrote:
> The style guide says "Short conditional statements may be written on one line
if
> this enhances readability. You may use this only when the line is brief and
the
> statement does not use the else clause."
> 
> The line in question is not brief but about as long as it can get and having a
> common style for all of the declarations here would make it more readable, no?

Done.

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/5915004/diff/142002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile.cc:145: new
ExtensionPrefStore(profile_->GetExtensionPrefValueMap(), true)));
On 2011/01/21 08:53:08, Mattias Nissler wrote:
> Hm, what's the contract of GetExtensionPrefValueMap()? As far as I can see it
> may return NULL if you don't call real_profile->GetPrefs() first, but you're
not
> prepared to handle that here. Maybe you want to move the lazy initialization
> into GetExtensionPrefValueMap so it always returns something?

I cannot move the entire lazy initialization of |extension_pref_value_map_| into
GetExtensionPrefValueMap because |pref_| requires it at construction time. I
call GetPrefs() now in ProfileImpl::GetExtensionPrefValueMap(). That does the
trick.

Done.

http://codereview.chromium.org/5915004/diff/142002/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/5915004/diff/142002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_linux.cc:439: // This has to be called on the UI
thread (bug 69057).
On 2011/01/20 23:17:25, eroman wrote:
> nit: I recommend providing URLs to bugs, as in "http://crbug.com/69057", makes
> it a little more discoverable for newcomers. (Also we used to have an
> internal+external bug tracker, so specifying the namespace of the bug ID is
> useful).

Done.

[Dan Beam, 2016-06-17 21:56:16]

https://codereview.chromium.org/5915004/diff/145019/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_member.cc (right):

https://codereview.chromium.org/5915004/diff/145019/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_member.cc:44: }
out of curiosity, could pref_name_.clear() be called here so that PrefMembers
could be later Init()ialized?

[battre, 2016-06-20 07:53:47]

https://codereview.chromium.org/5915004/diff/145019/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_member.cc (right):

https://codereview.chromium.org/5915004/diff/145019/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_member.cc:44: }
On 2016/06/17 21:56:15, Dan Beam wrote:
> out of curiosity, could pref_name_.clear() be called here so that PrefMembers
> could be later Init()ialized?

I did not find anything subtle that would break.

[Dan Beam, 2016-06-21 22:50:39]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-06-21 22:50:41]

https://codereview.chromium.org/5915004/diff/145019/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_member.cc (right):

https://codereview.chromium.org/5915004/diff/145019/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_member.cc:44: }
On 2016/06/20 07:53:46, battre wrote:
> On 2016/06/17 21:56:15, Dan Beam wrote:
> > out of curiosity, could pref_name_.clear() be called here so that
PrefMembers
> > could be later Init()ialized?
> 
> I did not find anything subtle that would break.

I think making Destroy() private and encouraging folks to use
unique_ptr<PrefMember> + reset() might be better.

[battre, 2016-06-22 05:03:04]

On 2016/06/21 22:50:41, Dan Beam wrote:
>
https://codereview.chromium.org/5915004/diff/145019/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_member.cc (right):
> 
>
https://codereview.chromium.org/5915004/diff/145019/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_member.cc:44: }
> On 2016/06/20 07:53:46, battre wrote:
> > On 2016/06/17 21:56:15, Dan Beam wrote:
> > > out of curiosity, could pref_name_.clear() be called here so that
> PrefMembers
> > > could be later Init()ialized?
> > 
> > I did not find anything subtle that would break.
> 
> I think making Destroy() private and encouraging folks to use
> unique_ptr<PrefMember> + reset() might be better.

I think that the downside is that you need to call reset() on the correct
thread.