Remove PrefServiceSimple, replacing it with PrefService and PrefRegistrySimple.

Remove PrefServiceSimple, replacing it with PrefService and PrefRegistrySimple.

This is a step towards removing registration methods from PrefService and simplifying its class hierarchy.

TBR=ben@chromium.org
BUG=155525
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180873

[Jói, 2013-01-02 17:19:51]

Hi Mattias,

What do you think?  This is a bit preliminary (compiles locally but hasn't
passed trybots) but should mostly work out I think.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2013-01-03 13:11:16]

https://codereview.chromium.org/11741003/diff/1/chrome/browser/prefs/pref_reg...
File chrome/browser/prefs/pref_registrar_simple.h (right):

https://codereview.chromium.org/11741003/diff/1/chrome/browser/prefs/pref_reg...
chrome/browser/prefs/pref_registrar_simple.h:40: PrefService* pref_service_;
Ah, so I had a different thing in mind: Make PrefRegistrar an interface like so:

class PrefRegistrar {
 public:
  virtual ~PrefRegistrar();

  virtual ValueType GetType(const std::string& pref_name) = 0;
  virtual const base::Value* GetDefaultValue(const std::string& pref_name) = 0;
};

This is the type we'd pass to PrefService in a constructor parameter and it
would keep a pointer. PrefService would also wrap it in a PrefStore adapter for
use as |default_store|.

Then, derived classes could add the registration functions with or without the
sync parameter as appropriate.

[Jói, 2013-01-03 14:57:59]

Thanks, and thanks for the discussion by IM on these ideas.

Unfortunately, my main dev box is having some problems, so there will
be some delay in a new version of the patch, but I have looked at the
code and come to a few conclusions:

a) We can go with the idea of a PrefRegistry base class through which
PrefService gets its information on registration, and we can have
separate subclasses PrefRegistrySimple and PrefRegistrySyncable, that
have the non-sync vs. with-sync-parameter versions of the registration
functions.

b) Because of the mechanics of DefaultPrefStore participating as a
PrefStore in the "layers of prefs" in PrefValueStore, we need the act
of registration/unregistration to actually modify the values stored in
DefaultPrefStore, and to do so immediately.

c) To hide more details and thus make things safer (especially
important in the PrefRegistrySyncable case, where registration _must_
go through the registration functions and not e.g. bypass through the
SetDefaultValue method of DefaultPrefStore or SetValue method of
PrefStore), I think we should leave the PrefRegistry classes separate
from DefaultPrefStore, but they should take a DefaultPrefStore as a
construction parameter.  This will be an implementation detail; what
happens is that in PrefServiceBuilder, we create a DefaultPrefStore as
today, use that to initialize the PrefRegistry, then use the
PrefRegistry to initialize the PrefService, and the DefaultPrefStore
to initialize the PrefValueStore.  From outside PrefServiceBuilder,
what you get is a PrefService* and a PrefRegistry* that you use to do
your registration.

d) Later, once we've removed all registration outside of
chrome::Register(User)Prefs, and thought about whether to remove the
ability to unregister, we could consider what we discussed about
removing all access to the PrefRegistry object (and maybe changing its
internal implementation so that it builds up a list of registrations
that are copied to the DefaultPrefStore, and then discarded, rather
than as it must do today, affecting the DefaultPrefStore immediately
on every call to a registration and unregistration function).

Does this make sense to you?  Once I have my development box back up
and running I will continue to work towards this.

BTW, with this change (or more correctly a follow-up to this change),
I think some of the interfacing-with-sync stuff that is currently in
PrefServiceSyncable can move to PrefRegistrySyncable.  Not sure yet
whether that will mean PrefServiceSyncable can go away entirely,
although that would be nice.

Cheers,
Jói



On Thu, Jan 3, 2013 at 1:11 PM,  <mnissler@chromium.org> wrote:
>
>
https://codereview.chromium.org/11741003/diff/1/chrome/browser/prefs/pref_reg...
> File chrome/browser/prefs/pref_registrar_simple.h (right):
>
>
https://codereview.chromium.org/11741003/diff/1/chrome/browser/prefs/pref_reg...
> chrome/browser/prefs/pref_registrar_simple.h:40: PrefService*
> pref_service_;
> Ah, so I had a different thing in mind: Make PrefRegistrar an interface
> like so:
>
> class PrefRegistrar {
>  public:
>   virtual ~PrefRegistrar();
>
>   virtual ValueType GetType(const std::string& pref_name) = 0;
>   virtual const base::Value* GetDefaultValue(const std::string&
> pref_name) = 0;
> };
>
> This is the type we'd pass to PrefService in a constructor parameter and
> it would keep a pointer. PrefService would also wrap it in a PrefStore
> adapter for use as |default_store|.
>
> Then, derived classes could add the registration functions with or
> without the sync parameter as appropriate.
>
> https://codereview.chromium.org/11741003/

[Mattias Nissler (ping if slow), 2013-01-03 15:46:19]

On 2013/01/03 14:57:59, Jói wrote:
> Thanks, and thanks for the discussion by IM on these ideas.
> 
> Unfortunately, my main dev box is having some problems, so there will
> be some delay in a new version of the patch, but I have looked at the
> code and come to a few conclusions:
> 
> a) We can go with the idea of a PrefRegistry base class through which
> PrefService gets its information on registration, and we can have
> separate subclasses PrefRegistrySimple and PrefRegistrySyncable, that
> have the non-sync vs. with-sync-parameter versions of the registration
> functions.

Sounds good.

> 
> b) Because of the mechanics of DefaultPrefStore participating as a
> PrefStore in the "layers of prefs" in PrefValueStore, we need the act
> of registration/unregistration to actually modify the values stored in
> DefaultPrefStore, and to do so immediately.

Note that this is only true as long as we still have consumers that do ad-hoc
pref registrations. If we didn't have them, we'd only have to look at
PrefRegistry state at PrefService construction time, the DefaultPrefStore
contents won't change afterwards. Alas, we do have ad-hoc pref registrations, so
this is all moot :)

> 
> c) To hide more details and thus make things safer (especially
> important in the PrefRegistrySyncable case, where registration _must_
> go through the registration functions and not e.g. bypass through the
> SetDefaultValue method of DefaultPrefStore or SetValue method of
> PrefStore), I think we should leave the PrefRegistry classes separate
> from DefaultPrefStore, but they should take a DefaultPrefStore as a
> construction parameter.  This will be an implementation detail; what
> happens is that in PrefServiceBuilder, we create a DefaultPrefStore as
> today, use that to initialize the PrefRegistry, then use the
> PrefRegistry to initialize the PrefService, and the DefaultPrefStore
> to initialize the PrefValueStore.  From outside PrefServiceBuilder,
> what you get is a PrefService* and a PrefRegistry* that you use to do
> your registration.

I'd do it the other way round: We can change DefaultPrefStore to keep a
PrefRegistry pointer and implement PrefStore::GetValue (which is the only
DefaultPrefStore API that PrefValueStore calls) via PrefRegistry's public
interface. PrefService can retain a pointer to PrefRegistry (passed at
constructions) which is the used for all the operations in PrefService that
access |PrefService::default_store|.

> 
> d) Later, once we've removed all registration outside of
> chrome::Register(User)Prefs, and thought about whether to remove the
> ability to unregister, we could consider what we discussed about
> removing all access to the PrefRegistry object (and maybe changing its
> internal implementation so that it builds up a list of registrations
> that are copied to the DefaultPrefStore, and then discarded, rather
> than as it must do today, affecting the DefaultPrefStore immediately
> on every call to a registration and unregistration function).

Yes, that's exactly what I meant in my response to (b) above.

> 
> Does this make sense to you?  Once I have my development box back up
> and running I will continue to work towards this.

It does indeed. The area where we're not quite aligned yet seems to be the
relationship between DefaultPrefStore and PrefRegistry. Feel free to play around
with it, I'm happy to be convinced for the way that turns out to be cleaner.

> 
> BTW, with this change (or more correctly a follow-up to this change),
> I think some of the interfacing-with-sync stuff that is currently in
> PrefServiceSyncable can move to PrefRegistrySyncable.  Not sure yet
> whether that will mean PrefServiceSyncable can go away entirely,
> although that would be nice.

Yes, that would indeed be very nice. The other half of the sync code is
concerned with handling pref changes and writing sync changes back to prefs.
That should be perfectly doable with pref observers though.

> 
> Cheers,
> Jói
> 
> 
> 
> On Thu, Jan 3, 2013 at 1:11 PM,  <mailto:mnissler@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/11741003/diff/1/chrome/browser/prefs/pref_reg...
> > File chrome/browser/prefs/pref_registrar_simple.h (right):
> >
> >
>
https://codereview.chromium.org/11741003/diff/1/chrome/browser/prefs/pref_reg...
> > chrome/browser/prefs/pref_registrar_simple.h:40: PrefService*
> > pref_service_;
> > Ah, so I had a different thing in mind: Make PrefRegistrar an interface
> > like so:
> >
> > class PrefRegistrar {
> >  public:
> >   virtual ~PrefRegistrar();
> >
> >   virtual ValueType GetType(const std::string& pref_name) = 0;
> >   virtual const base::Value* GetDefaultValue(const std::string&
> > pref_name) = 0;
> > };
> >
> > This is the type we'd pass to PrefService in a constructor parameter and
> > it would keep a pointer. PrefService would also wrap it in a PrefStore
> > adapter for use as |default_store|.
> >
> > Then, derived classes could add the registration functions with or
> > without the sync parameter as appropriate.
> >
> > <crxref style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">https://codereview.chromium.org/11741003/</crxref>

[tim (not reviewing), 2013-01-03 16:57:58]

Just driving by! I didn't look too closely, but will your work here let us close
crbug.com/85906 ??

[Mattias Nissler (ping if slow), 2013-01-03 17:08:52]

Yes!


On Thu, Jan 3, 2013 at 5:57 PM, <tim@chromium.org> wrote:

> Just driving by! I didn't look too closely, but will your work here let us
> close
> crbug.com/85906 ??
>
>
https://codereview.chromium.**org/11741003/<https://codereview.chromium.org/1...
>

[Jói, 2013-01-03 17:10:22]

Yes, in fact they are already separate subclasses since
http://crrev.com/174531 that kaiwang@ and I collaborated on and
mnissler@ reviewed; this is further cleanup.  Will tag this and other
related changes with the bug now that I know of it :)

Cheers,
Jói


On Thu, Jan 3, 2013 at 4:57 PM,  <tim@chromium.org> wrote:
> Just driving by! I didn't look too closely, but will your work here let us
> close
> crbug.com/85906 ??
>
> https://codereview.chromium.org/11741003/

[tim (not reviewing), 2013-01-03 17:18:14]

Awesome!


On Thu, Jan 3, 2013 at 9:09 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> Yes, in fact they are already separate subclasses since
> http://crrev.com/174531 that kaiwang@ and I collaborated on and
> mnissler@ reviewed; this is further cleanup.  Will tag this and other
> related changes with the bug now that I know of it :)
>
> Cheers,
> Jói
>
>
> On Thu, Jan 3, 2013 at 4:57 PM,  <tim@chromium.org> wrote:
> > Just driving by! I didn't look too closely, but will your work here let
> us
> > close
> > crbug.com/85906 ??
> >
> > https://codereview.chromium.org/11741003/
>

[Jói, 2013-01-23 15:36:54]

Hi Mattias,

This went to sleep for a long time while I was visiting MTV and getting super
busy with planning and other stuff.

Here at last is what I think we can do at the moment.  You'll see (PrefService*
and PrefRegistry*) that the interface is what I think we discussed, but the
implementation can't be quite that clean until we get rid of the various places
where registration is being done after the fact.

This still needs some cleanup, but I wanted to get your opinion on the
interfaces etc. before I go further.  What do you think?

Cheers,
Jói

[Jói, 2013-01-23 15:37:17]

TestingPrefService* is also worth looking at.

[Jói, 2013-01-25 13:19:59]

Mattias, since I know you're reviewing: I think it may be safest for now to make
PrefRegistry a refcounted object, rather than using the pass semantics that I
currently have in there; have been encountering memory management bugs in unit
tests because of the current setup.

Once we clean up registration post PrefService construction, we could change
this so that the PrefRegistry only needs to be available during construction of
PrefService, and so could just be a short-lived automatic.

WDYT?

Cheers,
Jói

[Mattias Nissler (ping if slow), 2013-01-25 13:40:16]

On Fri, Jan 25, 2013 at 2:20 PM, <joi@chromium.org> wrote:

> Mattias, since I know you're reviewing: I think it may be safest for now
> to make
> PrefRegistry a refcounted object, rather than using the pass semantics
> that I
> currently have in there; have been encountering memory management bugs in
> unit
> tests because of the current setup.
>

Umm, I'd like to understand the issues before we do that, can you
elaborate? Is this because of derived PrefServices, such as for incognito?


>
> Once we clean up registration post PrefService construction, we could
> change
> this so that the PrefRegistry only needs to be available during
> construction of
> PrefService, and so could just be a short-lived automatic.
>
> WDYT?
>
> Cheers,
> Jói
>
>
>
https://codereview.chromium.**org/11741003/<https://codereview.chromium.org/1...
>

[Jói, 2013-01-25 13:44:25]

I'm debugging through what the problem is right now.

I don't think there's anything fundamentally wrong with transferring
the ownership of PrefRegistry objects to PrefService as done in the
current patch, but it is a bit confusing when code keeps a raw pointer
to the PrefRegistry object for use after it has passed the ownership
to PrefService, and such.  It was mainly for this latter reason I
started to think maybe it should be refcounted for now, since anyway
its ownership semantics can be made simpler once it is only needed at
construction time.

Cheers,
Jói


On Fri, Jan 25, 2013 at 1:39 PM, Mattias Nissler <mnissler@chromium.org> wrote:
>
>
> On Fri, Jan 25, 2013 at 2:20 PM, <joi@chromium.org> wrote:
>>
>> Mattias, since I know you're reviewing: I think it may be safest for now
>> to make
>> PrefRegistry a refcounted object, rather than using the pass semantics
>> that I
>> currently have in there; have been encountering memory management bugs in
>> unit
>> tests because of the current setup.
>
>
> Umm, I'd like to understand the issues before we do that, can you elaborate?
> Is this because of derived PrefServices, such as for incognito?
>
>>
>>
>> Once we clean up registration post PrefService construction, we could
>> change
>> this so that the PrefRegistry only needs to be available during
>> construction of
>> PrefService, and so could just be a short-lived automatic.
>>
>> WDYT?
>>
>> Cheers,
>> Jói
>>
>>
>> https://codereview.chromium.org/11741003/
>
>

[Mattias Nissler (ping if slow), 2013-01-25 13:49:11]

On Fri, Jan 25, 2013 at 2:43 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> I'm debugging through what the problem is right now.
>
> I don't think there's anything fundamentally wrong with transferring
> the ownership of PrefRegistry objects to PrefService as done in the
> current patch, but it is a bit confusing when code keeps a raw pointer
> to the PrefRegistry object for use after it has passed the ownership
> to PrefService, and such.  It was mainly for this latter reason I
> started to think maybe it should be refcounted for now, since anyway
> its ownership semantics can be made simpler once it is only needed at
> construction time.
>

Yeah, that seems like the right solution: Convert the tests to pass the
registry at construction time.


>
> Cheers,
> Jói
>
>
> On Fri, Jan 25, 2013 at 1:39 PM, Mattias Nissler <mnissler@chromium.org>
> wrote:
> >
> >
> > On Fri, Jan 25, 2013 at 2:20 PM, <joi@chromium.org> wrote:
> >>
> >> Mattias, since I know you're reviewing: I think it may be safest for now
> >> to make
> >> PrefRegistry a refcounted object, rather than using the pass semantics
> >> that I
> >> currently have in there; have been encountering memory management bugs
> in
> >> unit
> >> tests because of the current setup.
> >
> >
> > Umm, I'd like to understand the issues before we do that, can you
> elaborate?
> > Is this because of derived PrefServices, such as for incognito?
> >
> >>
> >>
> >> Once we clean up registration post PrefService construction, we could
> >> change
> >> this so that the PrefRegistry only needs to be available during
> >> construction of
> >> PrefService, and so could just be a short-lived automatic.
> >>
> >> WDYT?
> >>
> >> Cheers,
> >> Jói
> >>
> >>
> >> https://codereview.chromium.org/11741003/
> >
> >
>

[Mattias Nissler (ping if slow), 2013-01-25 14:57:11]

I only looked at the PrefService bits, not the consumers for now.

https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
File base/prefs/public/pref_member_unittest.cc (right):

https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
base/prefs/public/pref_member_unittest.cc:105:
RegisterTestPrefs(prefs.registry());
I think it would be clearer if we changed TestingPrefServiceSimple to take a
scoped_ptr<PrefRegistry> in its ctor. And then we can have a
CreatePrefRegistry() helper function that does the job of RegisterTestPrefs()
and returns the registry.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:188:
NotificationPrefsManager::RegisterPrefs(local_state, registry);
Ah, this is the reason. That set-to-default piece of code in the function looks
weird and shouldn't be needed. Can you ask the author why this is there?

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:337:
registry->RegisterListPref(prefs::kDnsStartupPrefetchList);
It seems like we could register these prefs through the regular paths just as
well instead of doing it conditionally here.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.h (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.h:16: void
RegisterLocalState(PrefRegistrySimple* registry, PrefService* local_state);
Why does this take the PrefService pointer?

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.cc:108: bool async) {
Maybe we should move the browser_prefs::RegisterLocalState call here?

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
File chrome/browser/prefs/chrome_pref_service_factory.h (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.h:36: // CreateProfilePrefs
yet).
Once we use it for profile prefs, there's a slight complication: The incognito
PrefService we derive from the regular PrefService also needs a registry, but we
can't have both PrefService instances own the pointer. At that point we either
have to copy, or make PrefRegistry RefCounted (which would be in sync with the
approach we took for PrefStore, which is in the same situation).

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_notifier_impl_unittest.cc:100: true);
again, it would be nice if we could just use a CreateRegistry() helper that
creates and returns a registry so we can pass it into the
TestingPrefServiceSimple ctor.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_registry.cc (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.cc:17:
pref_service_->UnregisterPreference(path);
I'd rather not call into pref_service and back. Given that
PrefService::UnregisterPreference() is meant to go away eventually, how about
letting consumers call that function instead and have it call
PrefRegistry::UnregisterPreference()?

That way, we should be able to remove the |pref_service_| pointer here entirely
(which would make the design much cleaner in that PrefRegistry is now a true
dependency of PrefService and we don't have inverse arrows).

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.cc:21: result->swap(default_prefs_);  // This
releases the member reference.
Why would we want to release the pointer here?

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.cc:30: }
It seems like instead of sometimes delegating to PrefService, we should rather
move the entire logic over to PrefRegistry (including the RegisterXYZPref calls
eventually).

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_registry.h (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.h:9: #include "base/memory/ref_counted.h"
once is enough ;)

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.h:38: void
RetrieveDefaults(scoped_refptr<DefaultPrefStore>* result);
I'd make this public with the eventual goal of removing the friend class
PrefService declaration.

Also, why isn't this just a public accessor like so:

scoped_refptr<PrefStore> pref_store() { return default_prefs_; }

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.h:53: friend class PrefValueStore;
I don't understand this comment, why can a PrefRegistry be used in place of a
PrefStore? Why would PrefValueStore be concerned with either PrefStore or
PrefRegistry?

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service.cc (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.cc:332: // pref_map_ member for
UnregisterPreference.
The default_store_ it controls already! The user_pref_store_ piece is
unfortunate and the whole MarkNeedsEmptyValue thing seems like a big hack to me.
I just looked at the CL that added it, and I think the proper way of handling
this would have been to fix JsonPrefStore to never discard prefs that are set,
even if empty. I'll think a bit more about this and see whether there's a good
way to get rid of PersistentPrefStore::MarkNeedsEmptyValue().

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service.h (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.h:172: PrefRegistry*
DeprecatedGetPrefRegistry() const;
Does this have to be const? The caller can change state by manipulating
PrefRegistry, so the const is inaccurate.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.h:198: virtual bool AllowPrefRegistrarSimple()
const;
Why would we need that? Subclasses that need more should take a pointer of the
appropriate type in their ctor.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_builder.h (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_builder.h:53: virtual PrefService*
Create(scoped_ptr<PrefRegistry> registry);
Hm, it's a bit inconsistent to single out the registry parameter and pass it in
Create() instead of having a WithRegistry() function. However, that's probably
justified given PrefRegistry is essential - every consumer will want to pass it.
That's just my thoughts for your consideration, I leave the decision to you.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:97: dummy_registry->default_prefs_
= default_store_;
shouldn't the two stores share the same registry?

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_value_store.h (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_value_store.h:59: PrefRegistry* pref_registry,
PrefValueStore shouldn't have to know about PrefRegistry, PrefService should
just get the default pref store pointer from PrefRegistry and pass it on here.

[Jói, 2013-01-25 16:41:03]

Hi Mattias,

Thanks.  I'll need to take a better look at some of the stuff before
responding.

On delegating to PrefService or not:  Note that PrefRegistry only
pre-populates the DefaultPrefStore and then relinquishes control of it
at PrefService construction time.  PrefRegistry doesn't really need to
use a DefaultPrefStore to store its registration details; it could
have used something else, e.g. a regular STL map from string to value.
 It just seemed wasteful to write into one map and then copy to
another, so I did it this way, but perhaps it would be cleaner to
fully separate them so it doesn't even use a DefaultPrefStore, and
just fulfills an abstract interface as you suggested initially (the
interface would just need some additional bits for enumerating all
preferences - it only had GetType and GetValue before).

If we want to fully decouple the registration mechanics from
PrefService, then IMHO we would absolutely need to find a way to get
rid of needing a pref_map_ and the MarkNeedsEmptyValue thing.  I'm not
sure that it really matters though; there doesn't seem to me to be any
issue with PrefService keeping its own list of registered prefs things
(in a way that makes sense to PrefService, e.g. updating the pref_map_
which is much more a PrefService detail than something that
PrefRegistry should know about) based on information given to it at
construction time by PrefRegistry.  The post-construction-time bits
can go away once we make a few incremental changes to get rid of the
non-conforming registrations etc.; they are an unfortunate relic at
the moment.

> I think it would be clearer if we changed TestingPrefServiceSimple to
> take a scoped_ptr<PrefRegistry> in its ctor. And then we can have a
> CreatePrefRegistry() helper function that does the job of
> RegisterTestPrefs() and returns the registry.

This will complicate a lot of tests I think, by requiring them to
handle both a service and a registry, whereas today they only need to
keep the service, which owns the registry.  On the other hand if we
want to clean up the tests to only do registration before construction
(do you think that will be possible?) then we'll need to get rid of
the .registry() thing anyway.  Need to look at this some more.

> Once we use it for profile prefs, there's a slight complication: The
> incognito PrefService we derive from the regular PrefService also needs
> a registry, but we can't have both PrefService instances own the
> pointer. At that point we either have to copy, or make PrefRegistry
> RefCounted (which would be in sync with the approach we took for
> PrefStore, which is in the same situation).

Ah, yes.  So even long-term the PrefRegistry instance cannot actually
be a construction-time thing only (that lives only briefly while
PrefService is constructed and initializes itself), it needs to be
owned by the PrefService.  Unless PrefService can create a new
PrefRegistry from its internal state (i.e. assuming it keeps its
internal registration logic) to initialize the incognito PrefService.

> It seems like instead of sometimes delegating to PrefService, we should
> rather move the entire logic over to PrefRegistry (including the
> RegisterXYZPref calls eventually).

I think this is the main bone of contention. I'm not convinced there
is any value in moving the actual registration mechanism (and the
associated optimizations e.g. pref_map_) out of PrefService. The value
lies in removing knowledge of what the registration interface we
require users to use, and possible side aspects of that (e.g. sync in
the case of PrefRegistrySyncable).  If we move the registration logic
out of PrefService, it's not like we'll have separate implementations
of it in PrefRegistrySimple and PrefRegistrySyncable; we would just
have the same (or very similar) implementation in the PrefRegistry
base class.

The RegisterXYZPref calls are already moved to PrefRegistrySimple for
the local state case, and would move to PrefRegistrySyncable for the
profile case.

> AllowPrefRegistrarSimple() const;
> Why would we need that? Subclasses that need more should take a pointer
> of the appropriate type in their ctor.

I simply forgot to remove this since the earlier patch version.

Cheers,
Jói


On Fri, Jan 25, 2013 at 2:57 PM,  <mnissler@chromium.org> wrote:
> I only looked at the PrefService bits, not the consumers for now.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
> File base/prefs/public/pref_member_unittest.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
> base/prefs/public/pref_member_unittest.cc:105:
> RegisterTestPrefs(prefs.registry());
> I think it would be clearer if we changed TestingPrefServiceSimple to
> take a scoped_ptr<PrefRegistry> in its ctor. And then we can have a
> CreatePrefRegistry() helper function that does the job of
> RegisterTestPrefs() and returns the registry.
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> File chrome/browser/prefs/browser_prefs.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> chrome/browser/prefs/browser_prefs.cc:188:
> NotificationPrefsManager::RegisterPrefs(local_state, registry);
> Ah, this is the reason. That set-to-default piece of code in the
> function looks weird and shouldn't be needed. Can you ask the author why
> this is there?
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> chrome/browser/prefs/browser_prefs.cc:337:
> registry->RegisterListPref(prefs::kDnsStartupPrefetchList);
> It seems like we could register these prefs through the regular paths
> just as well instead of doing it conditionally here.
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> File chrome/browser/prefs/browser_prefs.h (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> chrome/browser/prefs/browser_prefs.h:16: void
> RegisterLocalState(PrefRegistrySimple* registry, PrefService*
> local_state);
> Why does this take the PrefService pointer?
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> File chrome/browser/prefs/chrome_pref_service_factory.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> chrome/browser/prefs/chrome_pref_service_factory.cc:108: bool async) {
> Maybe we should move the browser_prefs::RegisterLocalState call here?
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> File chrome/browser/prefs/chrome_pref_service_factory.h (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> chrome/browser/prefs/chrome_pref_service_factory.h:36: //
> CreateProfilePrefs yet).
> Once we use it for profile prefs, there's a slight complication: The
> incognito PrefService we derive from the regular PrefService also needs
> a registry, but we can't have both PrefService instances own the
> pointer. At that point we either have to copy, or make PrefRegistry
> RefCounted (which would be in sync with the approach we took for
> PrefStore, which is in the same situation).
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_notifier_impl_unittest.cc:100: true);
> again, it would be nice if we could just use a CreateRegistry() helper
> that creates and returns a registry so we can pass it into the
> TestingPrefServiceSimple ctor.
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_registry.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry.cc:17:
> pref_service_->UnregisterPreference(path);
> I'd rather not call into pref_service and back. Given that
> PrefService::UnregisterPreference() is meant to go away eventually, how
> about letting consumers call that function instead and have it call
> PrefRegistry::UnregisterPreference()?
>
> That way, we should be able to remove the |pref_service_| pointer here
> entirely (which would make the design much cleaner in that PrefRegistry
> is now a true dependency of PrefService and we don't have inverse
> arrows).
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry.cc:21: result->swap(default_prefs_);
> // This releases the member reference.
> Why would we want to release the pointer here?
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry.cc:30: }
> It seems like instead of sometimes delegating to PrefService, we should
> rather move the entire logic over to PrefRegistry (including the
> RegisterXYZPref calls eventually).
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_registry.h (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry.h:9: #include
> "base/memory/ref_counted.h"
> once is enough ;)
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry.h:38: void
> RetrieveDefaults(scoped_refptr<DefaultPrefStore>* result);
> I'd make this public with the eventual goal of removing the friend class
> PrefService declaration.
>
> Also, why isn't this just a public accessor like so:
>
> scoped_refptr<PrefStore> pref_store() { return default_prefs_; }
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry.h:53: friend class PrefValueStore;
> I don't understand this comment, why can a PrefRegistry be used in place
> of a PrefStore? Why would PrefValueStore be concerned with either
> PrefStore or PrefRegistry?
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_service.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service.cc:332: // pref_map_ member for
> UnregisterPreference.
> The default_store_ it controls already! The user_pref_store_ piece is
> unfortunate and the whole MarkNeedsEmptyValue thing seems like a big
> hack to me. I just looked at the CL that added it, and I think the
> proper way of handling this would have been to fix JsonPrefStore to
> never discard prefs that are set, even if empty. I'll think a bit more
> about this and see whether there's a good way to get rid of
> PersistentPrefStore::MarkNeedsEmptyValue().
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_service.h (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service.h:172: PrefRegistry*
> DeprecatedGetPrefRegistry() const;
> Does this have to be const? The caller can change state by manipulating
> PrefRegistry, so the const is inaccurate.
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service.h:198: virtual bool
> AllowPrefRegistrarSimple() const;
> Why would we need that? Subclasses that need more should take a pointer
> of the appropriate type in their ctor.
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_service_builder.h (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service_builder.h:53: virtual PrefService*
> Create(scoped_ptr<PrefRegistry> registry);
> Hm, it's a bit inconsistent to single out the registry parameter and
> pass it in Create() instead of having a WithRegistry() function.
> However, that's probably justified given PrefRegistry is essential -
> every consumer will want to pass it. That's just my thoughts for your
> consideration, I leave the decision to you.
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_service_syncable.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service_syncable.cc:97:
> dummy_registry->default_prefs_ = default_store_;
> shouldn't the two stores share the same registry?
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_value_store.h (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_value_store.h:59: PrefRegistry* pref_registry,
> PrefValueStore shouldn't have to know about PrefRegistry, PrefService
> should just get the default pref store pointer from PrefRegistry and
> pass it on here.
>
> https://codereview.chromium.org/11741003/

[Jói, 2013-01-28 10:41:00]

Hi Mattias,

Thinking about this some more, I think there are two main things to discuss:

a) Whether to extract the registration logic out of PrefService
altogether.  I think this can be done, but I personally don't see a
large amount of value in it, maybe slightly better separation of
concerns.  It's certainly not necessary for switching to the
PrefRegistry type of interface, to enforce usage of
PrefRegistrySyncable for profile Prefs, and PrefRegistrySimple for
local state prefs.

b) How to ensure that people do registration up front, or at least
encourage it strongly.  Some of your feedback suggested that
PrefRegistry should expose the DefaultPrefStore as a public member,
and not release it once it has been given to the PrefService.
Exposing it publicly would mean users could register preferences
without any necessary secondary things happening (e.g. if we have a
pref_map_ kind of optimization, this may need to be kept in sync).  In
the current approach, not giving the DefaultPrefStore up when the
PrefService has taken it over means we don't know whether registration
functions should now fail or not; this is something that would change
in a different implementation though.

Perhaps we should try to chat about this today so we can move forward
on the change.

Cheers,
Jói


On Fri, Jan 25, 2013 at 4:40 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Hi Mattias,
>
> Thanks.  I'll need to take a better look at some of the stuff before
responding.
>
> On delegating to PrefService or not:  Note that PrefRegistry only
> pre-populates the DefaultPrefStore and then relinquishes control of it
> at PrefService construction time.  PrefRegistry doesn't really need to
> use a DefaultPrefStore to store its registration details; it could
> have used something else, e.g. a regular STL map from string to value.
>  It just seemed wasteful to write into one map and then copy to
> another, so I did it this way, but perhaps it would be cleaner to
> fully separate them so it doesn't even use a DefaultPrefStore, and
> just fulfills an abstract interface as you suggested initially (the
> interface would just need some additional bits for enumerating all
> preferences - it only had GetType and GetValue before).
>
> If we want to fully decouple the registration mechanics from
> PrefService, then IMHO we would absolutely need to find a way to get
> rid of needing a pref_map_ and the MarkNeedsEmptyValue thing.  I'm not
> sure that it really matters though; there doesn't seem to me to be any
> issue with PrefService keeping its own list of registered prefs things
> (in a way that makes sense to PrefService, e.g. updating the pref_map_
> which is much more a PrefService detail than something that
> PrefRegistry should know about) based on information given to it at
> construction time by PrefRegistry.  The post-construction-time bits
> can go away once we make a few incremental changes to get rid of the
> non-conforming registrations etc.; they are an unfortunate relic at
> the moment.
>
>> I think it would be clearer if we changed TestingPrefServiceSimple to
>> take a scoped_ptr<PrefRegistry> in its ctor. And then we can have a
>> CreatePrefRegistry() helper function that does the job of
>> RegisterTestPrefs() and returns the registry.
>
> This will complicate a lot of tests I think, by requiring them to
> handle both a service and a registry, whereas today they only need to
> keep the service, which owns the registry.  On the other hand if we
> want to clean up the tests to only do registration before construction
> (do you think that will be possible?) then we'll need to get rid of
> the .registry() thing anyway.  Need to look at this some more.
>
>> Once we use it for profile prefs, there's a slight complication: The
>> incognito PrefService we derive from the regular PrefService also needs
>> a registry, but we can't have both PrefService instances own the
>> pointer. At that point we either have to copy, or make PrefRegistry
>> RefCounted (which would be in sync with the approach we took for
>> PrefStore, which is in the same situation).
>
> Ah, yes.  So even long-term the PrefRegistry instance cannot actually
> be a construction-time thing only (that lives only briefly while
> PrefService is constructed and initializes itself), it needs to be
> owned by the PrefService.  Unless PrefService can create a new
> PrefRegistry from its internal state (i.e. assuming it keeps its
> internal registration logic) to initialize the incognito PrefService.
>
>> It seems like instead of sometimes delegating to PrefService, we should
>> rather move the entire logic over to PrefRegistry (including the
>> RegisterXYZPref calls eventually).
>
> I think this is the main bone of contention. I'm not convinced there
> is any value in moving the actual registration mechanism (and the
> associated optimizations e.g. pref_map_) out of PrefService. The value
> lies in removing knowledge of what the registration interface we
> require users to use, and possible side aspects of that (e.g. sync in
> the case of PrefRegistrySyncable).  If we move the registration logic
> out of PrefService, it's not like we'll have separate implementations
> of it in PrefRegistrySimple and PrefRegistrySyncable; we would just
> have the same (or very similar) implementation in the PrefRegistry
> base class.
>
> The RegisterXYZPref calls are already moved to PrefRegistrySimple for
> the local state case, and would move to PrefRegistrySyncable for the
> profile case.
>
>> AllowPrefRegistrarSimple() const;
>> Why would we need that? Subclasses that need more should take a pointer
>> of the appropriate type in their ctor.
>
> I simply forgot to remove this since the earlier patch version.
>
> Cheers,
> Jói
>
>
> On Fri, Jan 25, 2013 at 2:57 PM,  <mnissler@chromium.org> wrote:
>> I only looked at the PrefService bits, not the consumers for now.
>>
>>
>>
https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
>> File base/prefs/public/pref_member_unittest.cc (right):
>>
>>
https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
>> base/prefs/public/pref_member_unittest.cc:105:
>> RegisterTestPrefs(prefs.registry());
>> I think it would be clearer if we changed TestingPrefServiceSimple to
>> take a scoped_ptr<PrefRegistry> in its ctor. And then we can have a
>> CreatePrefRegistry() helper function that does the job of
>> RegisterTestPrefs() and returns the registry.
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>> File chrome/browser/prefs/browser_prefs.cc (right):
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>> chrome/browser/prefs/browser_prefs.cc:188:
>> NotificationPrefsManager::RegisterPrefs(local_state, registry);
>> Ah, this is the reason. That set-to-default piece of code in the
>> function looks weird and shouldn't be needed. Can you ask the author why
>> this is there?
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>> chrome/browser/prefs/browser_prefs.cc:337:
>> registry->RegisterListPref(prefs::kDnsStartupPrefetchList);
>> It seems like we could register these prefs through the regular paths
>> just as well instead of doing it conditionally here.
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>> File chrome/browser/prefs/browser_prefs.h (right):
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>> chrome/browser/prefs/browser_prefs.h:16: void
>> RegisterLocalState(PrefRegistrySimple* registry, PrefService*
>> local_state);
>> Why does this take the PrefService pointer?
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
>> File chrome/browser/prefs/chrome_pref_service_factory.cc (right):
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
>> chrome/browser/prefs/chrome_pref_service_factory.cc:108: bool async) {
>> Maybe we should move the browser_prefs::RegisterLocalState call here?
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
>> File chrome/browser/prefs/chrome_pref_service_factory.h (right):
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
>> chrome/browser/prefs/chrome_pref_service_factory.h:36: //
>> CreateProfilePrefs yet).
>> Once we use it for profile prefs, there's a slight complication: The
>> incognito PrefService we derive from the regular PrefService also needs
>> a registry, but we can't have both PrefService instances own the
>> pointer. At that point we either have to copy, or make PrefRegistry
>> RefCounted (which would be in sync with the approach we took for
>> PrefStore, which is in the same situation).
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> chrome/browser/prefs/pref_notifier_impl_unittest.cc:100: true);
>> again, it would be nice if we could just use a CreateRegistry() helper
>> that creates and returns a registry so we can pass it into the
>> TestingPrefServiceSimple ctor.
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> File chrome/browser/prefs/pref_registry.cc (right):
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> chrome/browser/prefs/pref_registry.cc:17:
>> pref_service_->UnregisterPreference(path);
>> I'd rather not call into pref_service and back. Given that
>> PrefService::UnregisterPreference() is meant to go away eventually, how
>> about letting consumers call that function instead and have it call
>> PrefRegistry::UnregisterPreference()?
>>
>> That way, we should be able to remove the |pref_service_| pointer here
>> entirely (which would make the design much cleaner in that PrefRegistry
>> is now a true dependency of PrefService and we don't have inverse
>> arrows).
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> chrome/browser/prefs/pref_registry.cc:21: result->swap(default_prefs_);
>> // This releases the member reference.
>> Why would we want to release the pointer here?
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> chrome/browser/prefs/pref_registry.cc:30: }
>> It seems like instead of sometimes delegating to PrefService, we should
>> rather move the entire logic over to PrefRegistry (including the
>> RegisterXYZPref calls eventually).
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> File chrome/browser/prefs/pref_registry.h (right):
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> chrome/browser/prefs/pref_registry.h:9: #include
>> "base/memory/ref_counted.h"
>> once is enough ;)
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> chrome/browser/prefs/pref_registry.h:38: void
>> RetrieveDefaults(scoped_refptr<DefaultPrefStore>* result);
>> I'd make this public with the eventual goal of removing the friend class
>> PrefService declaration.
>>
>> Also, why isn't this just a public accessor like so:
>>
>> scoped_refptr<PrefStore> pref_store() { return default_prefs_; }
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> chrome/browser/prefs/pref_registry.h:53: friend class PrefValueStore;
>> I don't understand this comment, why can a PrefRegistry be used in place
>> of a PrefStore? Why would PrefValueStore be concerned with either
>> PrefStore or PrefRegistry?
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> File chrome/browser/prefs/pref_service.cc (right):
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> chrome/browser/prefs/pref_service.cc:332: // pref_map_ member for
>> UnregisterPreference.
>> The default_store_ it controls already! The user_pref_store_ piece is
>> unfortunate and the whole MarkNeedsEmptyValue thing seems like a big
>> hack to me. I just looked at the CL that added it, and I think the
>> proper way of handling this would have been to fix JsonPrefStore to
>> never discard prefs that are set, even if empty. I'll think a bit more
>> about this and see whether there's a good way to get rid of
>> PersistentPrefStore::MarkNeedsEmptyValue().
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> File chrome/browser/prefs/pref_service.h (right):
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> chrome/browser/prefs/pref_service.h:172: PrefRegistry*
>> DeprecatedGetPrefRegistry() const;
>> Does this have to be const? The caller can change state by manipulating
>> PrefRegistry, so the const is inaccurate.
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> chrome/browser/prefs/pref_service.h:198: virtual bool
>> AllowPrefRegistrarSimple() const;
>> Why would we need that? Subclasses that need more should take a pointer
>> of the appropriate type in their ctor.
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> File chrome/browser/prefs/pref_service_builder.h (right):
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> chrome/browser/prefs/pref_service_builder.h:53: virtual PrefService*
>> Create(scoped_ptr<PrefRegistry> registry);
>> Hm, it's a bit inconsistent to single out the registry parameter and
>> pass it in Create() instead of having a WithRegistry() function.
>> However, that's probably justified given PrefRegistry is essential -
>> every consumer will want to pass it. That's just my thoughts for your
>> consideration, I leave the decision to you.
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> File chrome/browser/prefs/pref_service_syncable.cc (right):
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> chrome/browser/prefs/pref_service_syncable.cc:97:
>> dummy_registry->default_prefs_ = default_store_;
>> shouldn't the two stores share the same registry?
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> File chrome/browser/prefs/pref_value_store.h (right):
>>
>>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> chrome/browser/prefs/pref_value_store.h:59: PrefRegistry* pref_registry,
>> PrefValueStore shouldn't have to know about PrefRegistry, PrefService
>> should just get the default pref store pointer from PrefRegistry and
>> pass it on here.
>>
>> https://codereview.chromium.org/11741003/

[Mattias Nissler (ping if slow), 2013-01-28 11:15:47]

Hi Joi,

I had your other mail in my inbox, let me try and add some thoughts.

On Mon, Jan 28, 2013 at 11:40 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> Hi Mattias,
>
> Thinking about this some more, I think there are two main things to
> discuss:
>
> a) Whether to extract the registration logic out of PrefService
> altogether.  I think this can be done, but I personally don't see a
> large amount of value in it, maybe slightly better separation of
> concerns.  It's certainly not necessary for switching to the
> PrefRegistry type of interface, to enforce usage of
> PrefRegistrySyncable for profile Prefs, and PrefRegistrySimple for
> local state prefs.
>

I'm OK with both as long as the code flow is clean enough.


> b) How to ensure that people do registration up front, or at least
> encourage it strongly.  Some of your feedback suggested that
> PrefRegistry should expose the DefaultPrefStore as a public member,
> and not release it once it has been given to the PrefService.
> Exposing it publicly would mean users could register preferences
> without any necessary secondary things happening (e.g. if we have a
> pref_map_ kind of optimization, this may need to be kept in sync).  In
> the current approach, not giving the DefaultPrefStore up when the
> PrefService has taken it over means we don't know whether registration
> functions should now fail or not; this is something that would change
> in a different implementation though.
>

I think this is the crucial point we need to settle. Note that in my
feedback, I said that we could have PrefRegistry provide a public member
returning a _PrefStore_ pointer (not DefaultPrefStore!). PrefStore only
declares a read-only interface, so we'd nicely express the fact that only
PrefRegistry can write to its PrefStore. And the fact that we give up the
sub-typed PrefRegistry pointer on PrefService construction would then
ensure that we can't write without casting. Would this address all your
concerns?


> Perhaps we should try to chat about this today so we can move forward
> on the change.
>
> Cheers,
> Jói
>
>
> On Fri, Jan 25, 2013 at 4:40 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> > Hi Mattias,
> >
> > Thanks.  I'll need to take a better look at some of the stuff before
> responding.
> >
> > On delegating to PrefService or not:  Note that PrefRegistry only
> > pre-populates the DefaultPrefStore and then relinquishes control of it
> > at PrefService construction time.  PrefRegistry doesn't really need to
> > use a DefaultPrefStore to store its registration details; it could
> > have used something else, e.g. a regular STL map from string to value.
> >  It just seemed wasteful to write into one map and then copy to
> > another, so I did it this way, but perhaps it would be cleaner to
> > fully separate them so it doesn't even use a DefaultPrefStore, and
> > just fulfills an abstract interface as you suggested initially (the
> > interface would just need some additional bits for enumerating all
> > preferences - it only had GetType and GetValue before).
>

See my comment above on returning DefaultPrefStore* vs. PrefStore*. If we
do copying here and PrefService maintains registrations after construction,
then I agree with you. However, I my gut feeling is we'll get a cleaner
result if registration moves to PrefRegistry. I guess I'm OK with calling
into PrefService for the benefit of post-construction registrations for
now, given that this is meant to go away anyways.

>
> > If we want to fully decouple the registration mechanics from
> > PrefService, then IMHO we would absolutely need to find a way to get
> > rid of needing a pref_map_ and the MarkNeedsEmptyValue thing.  I'm not
> > sure that it really matters though; there doesn't seem to me to be any
> > issue with PrefService keeping its own list of registered prefs things
> > (in a way that makes sense to PrefService, e.g. updating the pref_map_
> > which is much more a PrefService detail than something that
> > PrefRegistry should know about) based on information given to it at
> > construction time by PrefRegistry.
>

Agreed.


> The post-construction-time bits
> > can go away once we make a few incremental changes to get rid of the
> > non-conforming registrations etc.; they are an unfortunate relic at
> > the moment.
> >
> >> I think it would be clearer if we changed TestingPrefServiceSimple to
> >> take a scoped_ptr<PrefRegistry> in its ctor. And then we can have a
> >> CreatePrefRegistry() helper function that does the job of
> >> RegisterTestPrefs() and returns the registry.
> >
> > This will complicate a lot of tests I think, by requiring them to
> > handle both a service and a registry, whereas today they only need to
> > keep the service, which owns the registry.  On the other hand if we
> > want to clean up the tests to only do registration before construction
> > (do you think that will be possible?) then we'll need to get rid of
> > the .registry() thing anyway.  Need to look at this some more.
>

They don't need both handles - they just need to provide a function like
this:

scoped_ptr<PrefRegistry> CreatePrefRegistry();

that you can call inline when invoking the PrefService ctor or
PrefServiceBuilder::Create call.


>  >
> >> Once we use it for profile prefs, there's a slight complication: The
> >> incognito PrefService we derive from the regular PrefService also needs
> >> a registry, but we can't have both PrefService instances own the
> >> pointer. At that point we either have to copy, or make PrefRegistry
> >> RefCounted (which would be in sync with the approach we took for
> >> PrefStore, which is in the same situation).
> >
> > Ah, yes.  So even long-term the PrefRegistry instance cannot actually
> > be a construction-time thing only (that lives only briefly while
> > PrefService is constructed and initializes itself), it needs to be
> > owned by the PrefService.  Unless PrefService can create a new
> > PrefRegistry from its internal state (i.e. assuming it keeps its
> > internal registration logic) to initialize the incognito PrefService.
>

And then we're coping back and forth - why would we do that if we can avoid
it?


> >
> >> It seems like instead of sometimes delegating to PrefService, we should
> >> rather move the entire logic over to PrefRegistry (including the
> >> RegisterXYZPref calls eventually).
> >
> > I think this is the main bone of contention. I'm not convinced there
> > is any value in moving the actual registration mechanism (and the
> > associated optimizations e.g. pref_map_) out of PrefService.
>

That's true, and pref_map_ should stay in PrefService.


> The value
> > lies in removing knowledge of what the registration interface we
> > require users to use, and possible side aspects of that (e.g. sync in
> > the case of PrefRegistrySyncable).  If we move the registration logic
> > out of PrefService, it's not like we'll have separate implementations
> > of it in PrefRegistrySimple and PrefRegistrySyncable; we would just
> > have the same (or very similar) implementation in the PrefRegistry
> > base class.
>

So we're down to MarkNeedsEmptyValue here? That one I'd prefer to kill with
fire. It's probably OK to just treat all list and dictionary prefs like
that - I'll take a closer look after lunch.


> >
> > The RegisterXYZPref calls are already moved to PrefRegistrySimple for
> > the local state case, and would move to PrefRegistrySyncable for the
> > profile case.
> >
> >> AllowPrefRegistrarSimple() const;
> >> Why would we need that? Subclasses that need more should take a pointer
> >> of the appropriate type in their ctor.
> >
> > I simply forgot to remove this since the earlier patch version.
> >
> > Cheers,
> > Jói
> >
> >
> > On Fri, Jan 25, 2013 at 2:57 PM,  <mnissler@chromium.org> wrote:
> >> I only looked at the PrefService bits, not the consumers for now.
> >>
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
> >> File base/prefs/public/pref_member_unittest.cc (right):
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
> >> base/prefs/public/pref_member_unittest.cc:105:
> >> RegisterTestPrefs(prefs.registry());
> >> I think it would be clearer if we changed TestingPrefServiceSimple to
> >> take a scoped_ptr<PrefRegistry> in its ctor. And then we can have a
> >> CreatePrefRegistry() helper function that does the job of
> >> RegisterTestPrefs() and returns the registry.
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> >> File chrome/browser/prefs/browser_prefs.cc (right):
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> >> chrome/browser/prefs/browser_prefs.cc:188:
> >> NotificationPrefsManager::RegisterPrefs(local_state, registry);
> >> Ah, this is the reason. That set-to-default piece of code in the
> >> function looks weird and shouldn't be needed. Can you ask the author why
> >> this is there?
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> >> chrome/browser/prefs/browser_prefs.cc:337:
> >> registry->RegisterListPref(prefs::kDnsStartupPrefetchList);
> >> It seems like we could register these prefs through the regular paths
> >> just as well instead of doing it conditionally here.
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> >> File chrome/browser/prefs/browser_prefs.h (right):
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> >> chrome/browser/prefs/browser_prefs.h:16: void
> >> RegisterLocalState(PrefRegistrySimple* registry, PrefService*
> >> local_state);
> >> Why does this take the PrefService pointer?
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> >> File chrome/browser/prefs/chrome_pref_service_factory.cc (right):
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> >> chrome/browser/prefs/chrome_pref_service_factory.cc:108: bool async) {
> >> Maybe we should move the browser_prefs::RegisterLocalState call here?
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> >> File chrome/browser/prefs/chrome_pref_service_factory.h (right):
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> >> chrome/browser/prefs/chrome_pref_service_factory.h:36: //
> >> CreateProfilePrefs yet).
> >> Once we use it for profile prefs, there's a slight complication: The
> >> incognito PrefService we derive from the regular PrefService also needs
> >> a registry, but we can't have both PrefService instances own the
> >> pointer. At that point we either have to copy, or make PrefRegistry
> >> RefCounted (which would be in sync with the approach we took for
> >> PrefStore, which is in the same situation).
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> chrome/browser/prefs/pref_notifier_impl_unittest.cc:100: true);
> >> again, it would be nice if we could just use a CreateRegistry() helper
> >> that creates and returns a registry so we can pass it into the
> >> TestingPrefServiceSimple ctor.
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> File chrome/browser/prefs/pref_registry.cc (right):
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> chrome/browser/prefs/pref_registry.cc:17:
> >> pref_service_->UnregisterPreference(path);
> >> I'd rather not call into pref_service and back. Given that
> >> PrefService::UnregisterPreference() is meant to go away eventually, how
> >> about letting consumers call that function instead and have it call
> >> PrefRegistry::UnregisterPreference()?
> >>
> >> That way, we should be able to remove the |pref_service_| pointer here
> >> entirely (which would make the design much cleaner in that PrefRegistry
> >> is now a true dependency of PrefService and we don't have inverse
> >> arrows).
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> chrome/browser/prefs/pref_registry.cc:21: result->swap(default_prefs_);
> >> // This releases the member reference.
> >> Why would we want to release the pointer here?
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> chrome/browser/prefs/pref_registry.cc:30: }
> >> It seems like instead of sometimes delegating to PrefService, we should
> >> rather move the entire logic over to PrefRegistry (including the
> >> RegisterXYZPref calls eventually).
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> File chrome/browser/prefs/pref_registry.h (right):
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> chrome/browser/prefs/pref_registry.h:9: #include
> >> "base/memory/ref_counted.h"
> >> once is enough ;)
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> chrome/browser/prefs/pref_registry.h:38: void
> >> RetrieveDefaults(scoped_refptr<DefaultPrefStore>* result);
> >> I'd make this public with the eventual goal of removing the friend class
> >> PrefService declaration.
> >>
> >> Also, why isn't this just a public accessor like so:
> >>
> >> scoped_refptr<PrefStore> pref_store() { return default_prefs_; }
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> chrome/browser/prefs/pref_registry.h:53: friend class PrefValueStore;
> >> I don't understand this comment, why can a PrefRegistry be used in place
> >> of a PrefStore? Why would PrefValueStore be concerned with either
> >> PrefStore or PrefRegistry?
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> File chrome/browser/prefs/pref_service.cc (right):
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> chrome/browser/prefs/pref_service.cc:332: // pref_map_ member for
> >> UnregisterPreference.
> >> The default_store_ it controls already! The user_pref_store_ piece is
> >> unfortunate and the whole MarkNeedsEmptyValue thing seems like a big
> >> hack to me. I just looked at the CL that added it, and I think the
> >> proper way of handling this would have been to fix JsonPrefStore to
> >> never discard prefs that are set, even if empty. I'll think a bit more
> >> about this and see whether there's a good way to get rid of
> >> PersistentPrefStore::MarkNeedsEmptyValue().
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> File chrome/browser/prefs/pref_service.h (right):
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> chrome/browser/prefs/pref_service.h:172: PrefRegistry*
> >> DeprecatedGetPrefRegistry() const;
> >> Does this have to be const? The caller can change state by manipulating
> >> PrefRegistry, so the const is inaccurate.
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> chrome/browser/prefs/pref_service.h:198: virtual bool
> >> AllowPrefRegistrarSimple() const;
> >> Why would we need that? Subclasses that need more should take a pointer
> >> of the appropriate type in their ctor.
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> File chrome/browser/prefs/pref_service_builder.h (right):
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> chrome/browser/prefs/pref_service_builder.h:53: virtual PrefService*
> >> Create(scoped_ptr<PrefRegistry> registry);
> >> Hm, it's a bit inconsistent to single out the registry parameter and
> >> pass it in Create() instead of having a WithRegistry() function.
> >> However, that's probably justified given PrefRegistry is essential -
> >> every consumer will want to pass it. That's just my thoughts for your
> >> consideration, I leave the decision to you.
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> File chrome/browser/prefs/pref_service_syncable.cc (right):
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> chrome/browser/prefs/pref_service_syncable.cc:97:
> >> dummy_registry->default_prefs_ = default_store_;
> >> shouldn't the two stores share the same registry?
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> File chrome/browser/prefs/pref_value_store.h (right):
> >>
> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >> chrome/browser/prefs/pref_value_store.h:59: PrefRegistry* pref_registry,
> >> PrefValueStore shouldn't have to know about PrefRegistry, PrefService
> >> should just get the default pref store pointer from PrefRegistry and
> >> pass it on here.
> >>
> >> https://codereview.chromium.org/11741003/
>

[Jói, 2013-01-28 16:26:38]

Hi Mattias,

Thanks for the feedback, and for the draft change to remove
MarkNeedsEmptyValue.  I've been a bit distracted with another change
and sick family members today, but will try to take a better look at
this stuff tonight and tomorrow morning.  At a quick read I think I
misunderstood some of your comments and they make a lot of sense now,
e.g. providing only a read-only PrefStore.

Cheers,
Jói


On Mon, Jan 28, 2013 at 11:15 AM, Mattias Nissler <mnissler@chromium.org>
wrote:
> Hi Joi,
>
> I had your other mail in my inbox, let me try and add some thoughts.
>
> On Mon, Jan 28, 2013 at 11:40 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> Hi Mattias,
>>
>> Thinking about this some more, I think there are two main things to
>> discuss:
>>
>> a) Whether to extract the registration logic out of PrefService
>> altogether.  I think this can be done, but I personally don't see a
>> large amount of value in it, maybe slightly better separation of
>> concerns.  It's certainly not necessary for switching to the
>> PrefRegistry type of interface, to enforce usage of
>> PrefRegistrySyncable for profile Prefs, and PrefRegistrySimple for
>> local state prefs.
>
>
> I'm OK with both as long as the code flow is clean enough.
>
>>
>> b) How to ensure that people do registration up front, or at least
>> encourage it strongly.  Some of your feedback suggested that
>> PrefRegistry should expose the DefaultPrefStore as a public member,
>> and not release it once it has been given to the PrefService.
>> Exposing it publicly would mean users could register preferences
>> without any necessary secondary things happening (e.g. if we have a
>> pref_map_ kind of optimization, this may need to be kept in sync).  In
>> the current approach, not giving the DefaultPrefStore up when the
>> PrefService has taken it over means we don't know whether registration
>> functions should now fail or not; this is something that would change
>> in a different implementation though.
>
>
> I think this is the crucial point we need to settle. Note that in my
> feedback, I said that we could have PrefRegistry provide a public member
> returning a _PrefStore_ pointer (not DefaultPrefStore!). PrefStore only
> declares a read-only interface, so we'd nicely express the fact that only
> PrefRegistry can write to its PrefStore. And the fact that we give up the
> sub-typed PrefRegistry pointer on PrefService construction would then ensure
> that we can't write without casting. Would this address all your concerns?
>
>>
>> Perhaps we should try to chat about this today so we can move forward
>> on the change.
>>
>> Cheers,
>> Jói
>>
>>
>> On Fri, Jan 25, 2013 at 4:40 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> > Hi Mattias,
>> >
>> > Thanks.  I'll need to take a better look at some of the stuff before
>> > responding.
>> >
>> > On delegating to PrefService or not:  Note that PrefRegistry only
>> > pre-populates the DefaultPrefStore and then relinquishes control of it
>> > at PrefService construction time.  PrefRegistry doesn't really need to
>> > use a DefaultPrefStore to store its registration details; it could
>> > have used something else, e.g. a regular STL map from string to value.
>> >  It just seemed wasteful to write into one map and then copy to
>> > another, so I did it this way, but perhaps it would be cleaner to
>> > fully separate them so it doesn't even use a DefaultPrefStore, and
>> > just fulfills an abstract interface as you suggested initially (the
>> > interface would just need some additional bits for enumerating all
>> > preferences - it only had GetType and GetValue before).
>
>
> See my comment above on returning DefaultPrefStore* vs. PrefStore*. If we do
> copying here and PrefService maintains registrations after construction,
> then I agree with you. However, I my gut feeling is we'll get a cleaner
> result if registration moves to PrefRegistry. I guess I'm OK with calling
> into PrefService for the benefit of post-construction registrations for now,
> given that this is meant to go away anyways.
>
>> >
>> > If we want to fully decouple the registration mechanics from
>> > PrefService, then IMHO we would absolutely need to find a way to get
>> > rid of needing a pref_map_ and the MarkNeedsEmptyValue thing.  I'm not
>> > sure that it really matters though; there doesn't seem to me to be any
>> > issue with PrefService keeping its own list of registered prefs things
>> > (in a way that makes sense to PrefService, e.g. updating the pref_map_
>> > which is much more a PrefService detail than something that
>> > PrefRegistry should know about) based on information given to it at
>> > construction time by PrefRegistry.
>
>
> Agreed.
>
>>
>> The post-construction-time bits
>> > can go away once we make a few incremental changes to get rid of the
>> > non-conforming registrations etc.; they are an unfortunate relic at
>> > the moment.
>> >
>> >> I think it would be clearer if we changed TestingPrefServiceSimple to
>> >> take a scoped_ptr<PrefRegistry> in its ctor. And then we can have a
>> >> CreatePrefRegistry() helper function that does the job of
>> >> RegisterTestPrefs() and returns the registry.
>> >
>> > This will complicate a lot of tests I think, by requiring them to
>> > handle both a service and a registry, whereas today they only need to
>> > keep the service, which owns the registry.  On the other hand if we
>> > want to clean up the tests to only do registration before construction
>> > (do you think that will be possible?) then we'll need to get rid of
>> > the .registry() thing anyway.  Need to look at this some more.
>
>
> They don't need both handles - they just need to provide a function like
> this:
>
> scoped_ptr<PrefRegistry> CreatePrefRegistry();
>
> that you can call inline when invoking the PrefService ctor or
> PrefServiceBuilder::Create call.
>
>>
>> >
>> >> Once we use it for profile prefs, there's a slight complication: The
>> >> incognito PrefService we derive from the regular PrefService also needs
>> >> a registry, but we can't have both PrefService instances own the
>> >> pointer. At that point we either have to copy, or make PrefRegistry
>> >> RefCounted (which would be in sync with the approach we took for
>> >> PrefStore, which is in the same situation).
>> >
>> > Ah, yes.  So even long-term the PrefRegistry instance cannot actually
>> > be a construction-time thing only (that lives only briefly while
>> > PrefService is constructed and initializes itself), it needs to be
>> > owned by the PrefService.  Unless PrefService can create a new
>> > PrefRegistry from its internal state (i.e. assuming it keeps its
>> > internal registration logic) to initialize the incognito PrefService.
>
>
> And then we're coping back and forth - why would we do that if we can avoid
> it?
>
>>
>> >
>> >> It seems like instead of sometimes delegating to PrefService, we should
>> >> rather move the entire logic over to PrefRegistry (including the
>> >> RegisterXYZPref calls eventually).
>> >
>> > I think this is the main bone of contention. I'm not convinced there
>> > is any value in moving the actual registration mechanism (and the
>> > associated optimizations e.g. pref_map_) out of PrefService.
>
>
> That's true, and pref_map_ should stay in PrefService.
>
>>
>> The value
>> > lies in removing knowledge of what the registration interface we
>> > require users to use, and possible side aspects of that (e.g. sync in
>> > the case of PrefRegistrySyncable).  If we move the registration logic
>> > out of PrefService, it's not like we'll have separate implementations
>> > of it in PrefRegistrySimple and PrefRegistrySyncable; we would just
>> > have the same (or very similar) implementation in the PrefRegistry
>> > base class.
>
>
> So we're down to MarkNeedsEmptyValue here? That one I'd prefer to kill with
> fire. It's probably OK to just treat all list and dictionary prefs like that
> - I'll take a closer look after lunch.
>
>>
>> >
>> > The RegisterXYZPref calls are already moved to PrefRegistrySimple for
>> > the local state case, and would move to PrefRegistrySyncable for the
>> > profile case.
>> >
>> >> AllowPrefRegistrarSimple() const;
>> >> Why would we need that? Subclasses that need more should take a pointer
>> >> of the appropriate type in their ctor.
>> >
>> > I simply forgot to remove this since the earlier patch version.
>> >
>> > Cheers,
>> > Jói
>> >
>> >
>> > On Fri, Jan 25, 2013 at 2:57 PM,  <mnissler@chromium.org> wrote:
>> >> I only looked at the PrefService bits, not the consumers for now.
>> >>
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
>> >> File base/prefs/public/pref_member_unittest.cc (right):
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
>> >> base/prefs/public/pref_member_unittest.cc:105:
>> >> RegisterTestPrefs(prefs.registry());
>> >> I think it would be clearer if we changed TestingPrefServiceSimple to
>> >> take a scoped_ptr<PrefRegistry> in its ctor. And then we can have a
>> >> CreatePrefRegistry() helper function that does the job of
>> >> RegisterTestPrefs() and returns the registry.
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>> >> File chrome/browser/prefs/browser_prefs.cc (right):
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>> >> chrome/browser/prefs/browser_prefs.cc:188:
>> >> NotificationPrefsManager::RegisterPrefs(local_state, registry);
>> >> Ah, this is the reason. That set-to-default piece of code in the
>> >> function looks weird and shouldn't be needed. Can you ask the author
>> >> why
>> >> this is there?
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>> >> chrome/browser/prefs/browser_prefs.cc:337:
>> >> registry->RegisterListPref(prefs::kDnsStartupPrefetchList);
>> >> It seems like we could register these prefs through the regular paths
>> >> just as well instead of doing it conditionally here.
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>> >> File chrome/browser/prefs/browser_prefs.h (right):
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>> >> chrome/browser/prefs/browser_prefs.h:16: void
>> >> RegisterLocalState(PrefRegistrySimple* registry, PrefService*
>> >> local_state);
>> >> Why does this take the PrefService pointer?
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
>> >> File chrome/browser/prefs/chrome_pref_service_factory.cc (right):
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
>> >> chrome/browser/prefs/chrome_pref_service_factory.cc:108: bool async) {
>> >> Maybe we should move the browser_prefs::RegisterLocalState call here?
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
>> >> File chrome/browser/prefs/chrome_pref_service_factory.h (right):
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
>> >> chrome/browser/prefs/chrome_pref_service_factory.h:36: //
>> >> CreateProfilePrefs yet).
>> >> Once we use it for profile prefs, there's a slight complication: The
>> >> incognito PrefService we derive from the regular PrefService also needs
>> >> a registry, but we can't have both PrefService instances own the
>> >> pointer. At that point we either have to copy, or make PrefRegistry
>> >> RefCounted (which would be in sync with the approach we took for
>> >> PrefStore, which is in the same situation).
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> chrome/browser/prefs/pref_notifier_impl_unittest.cc:100: true);
>> >> again, it would be nice if we could just use a CreateRegistry() helper
>> >> that creates and returns a registry so we can pass it into the
>> >> TestingPrefServiceSimple ctor.
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> File chrome/browser/prefs/pref_registry.cc (right):
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> chrome/browser/prefs/pref_registry.cc:17:
>> >> pref_service_->UnregisterPreference(path);
>> >> I'd rather not call into pref_service and back. Given that
>> >> PrefService::UnregisterPreference() is meant to go away eventually, how
>> >> about letting consumers call that function instead and have it call
>> >> PrefRegistry::UnregisterPreference()?
>> >>
>> >> That way, we should be able to remove the |pref_service_| pointer here
>> >> entirely (which would make the design much cleaner in that PrefRegistry
>> >> is now a true dependency of PrefService and we don't have inverse
>> >> arrows).
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> chrome/browser/prefs/pref_registry.cc:21: result->swap(default_prefs_);
>> >> // This releases the member reference.
>> >> Why would we want to release the pointer here?
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> chrome/browser/prefs/pref_registry.cc:30: }
>> >> It seems like instead of sometimes delegating to PrefService, we should
>> >> rather move the entire logic over to PrefRegistry (including the
>> >> RegisterXYZPref calls eventually).
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> File chrome/browser/prefs/pref_registry.h (right):
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> chrome/browser/prefs/pref_registry.h:9: #include
>> >> "base/memory/ref_counted.h"
>> >> once is enough ;)
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> chrome/browser/prefs/pref_registry.h:38: void
>> >> RetrieveDefaults(scoped_refptr<DefaultPrefStore>* result);
>> >> I'd make this public with the eventual goal of removing the friend
>> >> class
>> >> PrefService declaration.
>> >>
>> >> Also, why isn't this just a public accessor like so:
>> >>
>> >> scoped_refptr<PrefStore> pref_store() { return default_prefs_; }
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> chrome/browser/prefs/pref_registry.h:53: friend class PrefValueStore;
>> >> I don't understand this comment, why can a PrefRegistry be used in
>> >> place
>> >> of a PrefStore? Why would PrefValueStore be concerned with either
>> >> PrefStore or PrefRegistry?
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> File chrome/browser/prefs/pref_service.cc (right):
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> chrome/browser/prefs/pref_service.cc:332: // pref_map_ member for
>> >> UnregisterPreference.
>> >> The default_store_ it controls already! The user_pref_store_ piece is
>> >> unfortunate and the whole MarkNeedsEmptyValue thing seems like a big
>> >> hack to me. I just looked at the CL that added it, and I think the
>> >> proper way of handling this would have been to fix JsonPrefStore to
>> >> never discard prefs that are set, even if empty. I'll think a bit more
>> >> about this and see whether there's a good way to get rid of
>> >> PersistentPrefStore::MarkNeedsEmptyValue().
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> File chrome/browser/prefs/pref_service.h (right):
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> chrome/browser/prefs/pref_service.h:172: PrefRegistry*
>> >> DeprecatedGetPrefRegistry() const;
>> >> Does this have to be const? The caller can change state by manipulating
>> >> PrefRegistry, so the const is inaccurate.
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> chrome/browser/prefs/pref_service.h:198: virtual bool
>> >> AllowPrefRegistrarSimple() const;
>> >> Why would we need that? Subclasses that need more should take a pointer
>> >> of the appropriate type in their ctor.
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> File chrome/browser/prefs/pref_service_builder.h (right):
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> chrome/browser/prefs/pref_service_builder.h:53: virtual PrefService*
>> >> Create(scoped_ptr<PrefRegistry> registry);
>> >> Hm, it's a bit inconsistent to single out the registry parameter and
>> >> pass it in Create() instead of having a WithRegistry() function.
>> >> However, that's probably justified given PrefRegistry is essential -
>> >> every consumer will want to pass it. That's just my thoughts for your
>> >> consideration, I leave the decision to you.
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> File chrome/browser/prefs/pref_service_syncable.cc (right):
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> chrome/browser/prefs/pref_service_syncable.cc:97:
>> >> dummy_registry->default_prefs_ = default_store_;
>> >> shouldn't the two stores share the same registry?
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> File chrome/browser/prefs/pref_value_store.h (right):
>> >>
>> >>
>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>> >> chrome/browser/prefs/pref_value_store.h:59: PrefRegistry*
>> >> pref_registry,
>> >> PrefValueStore shouldn't have to know about PrefRegistry, PrefService
>> >> should just get the default pref store pointer from PrefRegistry and
>> >> pass it on here.
>> >>
>> >> https://codereview.chromium.org/11741003/
>
>

[Jói, 2013-01-29 15:24:45]

Hi Mattias,

I'm working on this.  Quick question:  Are preferences never
registered on an incognito pref service?

Cheers,
Jói


On Mon, Jan 28, 2013 at 4:25 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Hi Mattias,
>
> Thanks for the feedback, and for the draft change to remove
> MarkNeedsEmptyValue.  I've been a bit distracted with another change
> and sick family members today, but will try to take a better look at
> this stuff tonight and tomorrow morning.  At a quick read I think I
> misunderstood some of your comments and they make a lot of sense now,
> e.g. providing only a read-only PrefStore.
>
> Cheers,
> Jói
>
>
> On Mon, Jan 28, 2013 at 11:15 AM, Mattias Nissler <mnissler@chromium.org>
wrote:
>> Hi Joi,
>>
>> I had your other mail in my inbox, let me try and add some thoughts.
>>
>> On Mon, Jan 28, 2013 at 11:40 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>>>
>>> Hi Mattias,
>>>
>>> Thinking about this some more, I think there are two main things to
>>> discuss:
>>>
>>> a) Whether to extract the registration logic out of PrefService
>>> altogether.  I think this can be done, but I personally don't see a
>>> large amount of value in it, maybe slightly better separation of
>>> concerns.  It's certainly not necessary for switching to the
>>> PrefRegistry type of interface, to enforce usage of
>>> PrefRegistrySyncable for profile Prefs, and PrefRegistrySimple for
>>> local state prefs.
>>
>>
>> I'm OK with both as long as the code flow is clean enough.
>>
>>>
>>> b) How to ensure that people do registration up front, or at least
>>> encourage it strongly.  Some of your feedback suggested that
>>> PrefRegistry should expose the DefaultPrefStore as a public member,
>>> and not release it once it has been given to the PrefService.
>>> Exposing it publicly would mean users could register preferences
>>> without any necessary secondary things happening (e.g. if we have a
>>> pref_map_ kind of optimization, this may need to be kept in sync).  In
>>> the current approach, not giving the DefaultPrefStore up when the
>>> PrefService has taken it over means we don't know whether registration
>>> functions should now fail or not; this is something that would change
>>> in a different implementation though.
>>
>>
>> I think this is the crucial point we need to settle. Note that in my
>> feedback, I said that we could have PrefRegistry provide a public member
>> returning a _PrefStore_ pointer (not DefaultPrefStore!). PrefStore only
>> declares a read-only interface, so we'd nicely express the fact that only
>> PrefRegistry can write to its PrefStore. And the fact that we give up the
>> sub-typed PrefRegistry pointer on PrefService construction would then ensure
>> that we can't write without casting. Would this address all your concerns?
>>
>>>
>>> Perhaps we should try to chat about this today so we can move forward
>>> on the change.
>>>
>>> Cheers,
>>> Jói
>>>
>>>
>>> On Fri, Jan 25, 2013 at 4:40 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>> > Hi Mattias,
>>> >
>>> > Thanks.  I'll need to take a better look at some of the stuff before
>>> > responding.
>>> >
>>> > On delegating to PrefService or not:  Note that PrefRegistry only
>>> > pre-populates the DefaultPrefStore and then relinquishes control of it
>>> > at PrefService construction time.  PrefRegistry doesn't really need to
>>> > use a DefaultPrefStore to store its registration details; it could
>>> > have used something else, e.g. a regular STL map from string to value.
>>> >  It just seemed wasteful to write into one map and then copy to
>>> > another, so I did it this way, but perhaps it would be cleaner to
>>> > fully separate them so it doesn't even use a DefaultPrefStore, and
>>> > just fulfills an abstract interface as you suggested initially (the
>>> > interface would just need some additional bits for enumerating all
>>> > preferences - it only had GetType and GetValue before).
>>
>>
>> See my comment above on returning DefaultPrefStore* vs. PrefStore*. If we do
>> copying here and PrefService maintains registrations after construction,
>> then I agree with you. However, I my gut feeling is we'll get a cleaner
>> result if registration moves to PrefRegistry. I guess I'm OK with calling
>> into PrefService for the benefit of post-construction registrations for now,
>> given that this is meant to go away anyways.
>>
>>> >
>>> > If we want to fully decouple the registration mechanics from
>>> > PrefService, then IMHO we would absolutely need to find a way to get
>>> > rid of needing a pref_map_ and the MarkNeedsEmptyValue thing.  I'm not
>>> > sure that it really matters though; there doesn't seem to me to be any
>>> > issue with PrefService keeping its own list of registered prefs things
>>> > (in a way that makes sense to PrefService, e.g. updating the pref_map_
>>> > which is much more a PrefService detail than something that
>>> > PrefRegistry should know about) based on information given to it at
>>> > construction time by PrefRegistry.
>>
>>
>> Agreed.
>>
>>>
>>> The post-construction-time bits
>>> > can go away once we make a few incremental changes to get rid of the
>>> > non-conforming registrations etc.; they are an unfortunate relic at
>>> > the moment.
>>> >
>>> >> I think it would be clearer if we changed TestingPrefServiceSimple to
>>> >> take a scoped_ptr<PrefRegistry> in its ctor. And then we can have a
>>> >> CreatePrefRegistry() helper function that does the job of
>>> >> RegisterTestPrefs() and returns the registry.
>>> >
>>> > This will complicate a lot of tests I think, by requiring them to
>>> > handle both a service and a registry, whereas today they only need to
>>> > keep the service, which owns the registry.  On the other hand if we
>>> > want to clean up the tests to only do registration before construction
>>> > (do you think that will be possible?) then we'll need to get rid of
>>> > the .registry() thing anyway.  Need to look at this some more.
>>
>>
>> They don't need both handles - they just need to provide a function like
>> this:
>>
>> scoped_ptr<PrefRegistry> CreatePrefRegistry();
>>
>> that you can call inline when invoking the PrefService ctor or
>> PrefServiceBuilder::Create call.
>>
>>>
>>> >
>>> >> Once we use it for profile prefs, there's a slight complication: The
>>> >> incognito PrefService we derive from the regular PrefService also needs
>>> >> a registry, but we can't have both PrefService instances own the
>>> >> pointer. At that point we either have to copy, or make PrefRegistry
>>> >> RefCounted (which would be in sync with the approach we took for
>>> >> PrefStore, which is in the same situation).
>>> >
>>> > Ah, yes.  So even long-term the PrefRegistry instance cannot actually
>>> > be a construction-time thing only (that lives only briefly while
>>> > PrefService is constructed and initializes itself), it needs to be
>>> > owned by the PrefService.  Unless PrefService can create a new
>>> > PrefRegistry from its internal state (i.e. assuming it keeps its
>>> > internal registration logic) to initialize the incognito PrefService.
>>
>>
>> And then we're coping back and forth - why would we do that if we can avoid
>> it?
>>
>>>
>>> >
>>> >> It seems like instead of sometimes delegating to PrefService, we should
>>> >> rather move the entire logic over to PrefRegistry (including the
>>> >> RegisterXYZPref calls eventually).
>>> >
>>> > I think this is the main bone of contention. I'm not convinced there
>>> > is any value in moving the actual registration mechanism (and the
>>> > associated optimizations e.g. pref_map_) out of PrefService.
>>
>>
>> That's true, and pref_map_ should stay in PrefService.
>>
>>>
>>> The value
>>> > lies in removing knowledge of what the registration interface we
>>> > require users to use, and possible side aspects of that (e.g. sync in
>>> > the case of PrefRegistrySyncable).  If we move the registration logic
>>> > out of PrefService, it's not like we'll have separate implementations
>>> > of it in PrefRegistrySimple and PrefRegistrySyncable; we would just
>>> > have the same (or very similar) implementation in the PrefRegistry
>>> > base class.
>>
>>
>> So we're down to MarkNeedsEmptyValue here? That one I'd prefer to kill with
>> fire. It's probably OK to just treat all list and dictionary prefs like that
>> - I'll take a closer look after lunch.
>>
>>>
>>> >
>>> > The RegisterXYZPref calls are already moved to PrefRegistrySimple for
>>> > the local state case, and would move to PrefRegistrySyncable for the
>>> > profile case.
>>> >
>>> >> AllowPrefRegistrarSimple() const;
>>> >> Why would we need that? Subclasses that need more should take a pointer
>>> >> of the appropriate type in their ctor.
>>> >
>>> > I simply forgot to remove this since the earlier patch version.
>>> >
>>> > Cheers,
>>> > Jói
>>> >
>>> >
>>> > On Fri, Jan 25, 2013 at 2:57 PM,  <mnissler@chromium.org> wrote:
>>> >> I only looked at the PrefService bits, not the consumers for now.
>>> >>
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
>>> >> File base/prefs/public/pref_member_unittest.cc (right):
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
>>> >> base/prefs/public/pref_member_unittest.cc:105:
>>> >> RegisterTestPrefs(prefs.registry());
>>> >> I think it would be clearer if we changed TestingPrefServiceSimple to
>>> >> take a scoped_ptr<PrefRegistry> in its ctor. And then we can have a
>>> >> CreatePrefRegistry() helper function that does the job of
>>> >> RegisterTestPrefs() and returns the registry.
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>>> >> File chrome/browser/prefs/browser_prefs.cc (right):
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>>> >> chrome/browser/prefs/browser_prefs.cc:188:
>>> >> NotificationPrefsManager::RegisterPrefs(local_state, registry);
>>> >> Ah, this is the reason. That set-to-default piece of code in the
>>> >> function looks weird and shouldn't be needed. Can you ask the author
>>> >> why
>>> >> this is there?
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>>> >> chrome/browser/prefs/browser_prefs.cc:337:
>>> >> registry->RegisterListPref(prefs::kDnsStartupPrefetchList);
>>> >> It seems like we could register these prefs through the regular paths
>>> >> just as well instead of doing it conditionally here.
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>>> >> File chrome/browser/prefs/browser_prefs.h (right):
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
>>> >> chrome/browser/prefs/browser_prefs.h:16: void
>>> >> RegisterLocalState(PrefRegistrySimple* registry, PrefService*
>>> >> local_state);
>>> >> Why does this take the PrefService pointer?
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
>>> >> File chrome/browser/prefs/chrome_pref_service_factory.cc (right):
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
>>> >> chrome/browser/prefs/chrome_pref_service_factory.cc:108: bool async) {
>>> >> Maybe we should move the browser_prefs::RegisterLocalState call here?
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
>>> >> File chrome/browser/prefs/chrome_pref_service_factory.h (right):
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
>>> >> chrome/browser/prefs/chrome_pref_service_factory.h:36: //
>>> >> CreateProfilePrefs yet).
>>> >> Once we use it for profile prefs, there's a slight complication: The
>>> >> incognito PrefService we derive from the regular PrefService also needs
>>> >> a registry, but we can't have both PrefService instances own the
>>> >> pointer. At that point we either have to copy, or make PrefRegistry
>>> >> RefCounted (which would be in sync with the approach we took for
>>> >> PrefStore, which is in the same situation).
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> chrome/browser/prefs/pref_notifier_impl_unittest.cc:100: true);
>>> >> again, it would be nice if we could just use a CreateRegistry() helper
>>> >> that creates and returns a registry so we can pass it into the
>>> >> TestingPrefServiceSimple ctor.
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> File chrome/browser/prefs/pref_registry.cc (right):
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> chrome/browser/prefs/pref_registry.cc:17:
>>> >> pref_service_->UnregisterPreference(path);
>>> >> I'd rather not call into pref_service and back. Given that
>>> >> PrefService::UnregisterPreference() is meant to go away eventually, how
>>> >> about letting consumers call that function instead and have it call
>>> >> PrefRegistry::UnregisterPreference()?
>>> >>
>>> >> That way, we should be able to remove the |pref_service_| pointer here
>>> >> entirely (which would make the design much cleaner in that PrefRegistry
>>> >> is now a true dependency of PrefService and we don't have inverse
>>> >> arrows).
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> chrome/browser/prefs/pref_registry.cc:21: result->swap(default_prefs_);
>>> >> // This releases the member reference.
>>> >> Why would we want to release the pointer here?
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> chrome/browser/prefs/pref_registry.cc:30: }
>>> >> It seems like instead of sometimes delegating to PrefService, we should
>>> >> rather move the entire logic over to PrefRegistry (including the
>>> >> RegisterXYZPref calls eventually).
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> File chrome/browser/prefs/pref_registry.h (right):
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> chrome/browser/prefs/pref_registry.h:9: #include
>>> >> "base/memory/ref_counted.h"
>>> >> once is enough ;)
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> chrome/browser/prefs/pref_registry.h:38: void
>>> >> RetrieveDefaults(scoped_refptr<DefaultPrefStore>* result);
>>> >> I'd make this public with the eventual goal of removing the friend
>>> >> class
>>> >> PrefService declaration.
>>> >>
>>> >> Also, why isn't this just a public accessor like so:
>>> >>
>>> >> scoped_refptr<PrefStore> pref_store() { return default_prefs_; }
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> chrome/browser/prefs/pref_registry.h:53: friend class PrefValueStore;
>>> >> I don't understand this comment, why can a PrefRegistry be used in
>>> >> place
>>> >> of a PrefStore? Why would PrefValueStore be concerned with either
>>> >> PrefStore or PrefRegistry?
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> File chrome/browser/prefs/pref_service.cc (right):
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> chrome/browser/prefs/pref_service.cc:332: // pref_map_ member for
>>> >> UnregisterPreference.
>>> >> The default_store_ it controls already! The user_pref_store_ piece is
>>> >> unfortunate and the whole MarkNeedsEmptyValue thing seems like a big
>>> >> hack to me. I just looked at the CL that added it, and I think the
>>> >> proper way of handling this would have been to fix JsonPrefStore to
>>> >> never discard prefs that are set, even if empty. I'll think a bit more
>>> >> about this and see whether there's a good way to get rid of
>>> >> PersistentPrefStore::MarkNeedsEmptyValue().
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> File chrome/browser/prefs/pref_service.h (right):
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> chrome/browser/prefs/pref_service.h:172: PrefRegistry*
>>> >> DeprecatedGetPrefRegistry() const;
>>> >> Does this have to be const? The caller can change state by manipulating
>>> >> PrefRegistry, so the const is inaccurate.
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> chrome/browser/prefs/pref_service.h:198: virtual bool
>>> >> AllowPrefRegistrarSimple() const;
>>> >> Why would we need that? Subclasses that need more should take a pointer
>>> >> of the appropriate type in their ctor.
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> File chrome/browser/prefs/pref_service_builder.h (right):
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> chrome/browser/prefs/pref_service_builder.h:53: virtual PrefService*
>>> >> Create(scoped_ptr<PrefRegistry> registry);
>>> >> Hm, it's a bit inconsistent to single out the registry parameter and
>>> >> pass it in Create() instead of having a WithRegistry() function.
>>> >> However, that's probably justified given PrefRegistry is essential -
>>> >> every consumer will want to pass it. That's just my thoughts for your
>>> >> consideration, I leave the decision to you.
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> File chrome/browser/prefs/pref_service_syncable.cc (right):
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> chrome/browser/prefs/pref_service_syncable.cc:97:
>>> >> dummy_registry->default_prefs_ = default_store_;
>>> >> shouldn't the two stores share the same registry?
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> File chrome/browser/prefs/pref_value_store.h (right):
>>> >>
>>> >>
>>> >>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
>>> >> chrome/browser/prefs/pref_value_store.h:59: PrefRegistry*
>>> >> pref_registry,
>>> >> PrefValueStore shouldn't have to know about PrefRegistry, PrefService
>>> >> should just get the default pref store pointer from PrefRegistry and
>>> >> pass it on here.
>>> >>
>>> >> https://codereview.chromium.org/11741003/
>>
>>

[Mattias Nissler (ping if slow), 2013-01-29 15:33:39]

On Tue, Jan 29, 2013 at 4:24 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> Hi Mattias,
>
> I'm working on this.  Quick question:  Are preferences never
> registered on an incognito pref service?
>

I certainly hope they are not. Hard to tell though whether there's a code
path that actually does it. If we share the PrefRegistry between the
regular an incognito PrefService, that shouldn't be an issue?

Cheers,
> Jói
>
>
> On Mon, Jan 28, 2013 at 4:25 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> > Hi Mattias,
> >
> > Thanks for the feedback, and for the draft change to remove
> > MarkNeedsEmptyValue.  I've been a bit distracted with another change
> > and sick family members today, but will try to take a better look at
> > this stuff tonight and tomorrow morning.  At a quick read I think I
> > misunderstood some of your comments and they make a lot of sense now,
> > e.g. providing only a read-only PrefStore.
> >
> > Cheers,
> > Jói
> >
> >
> > On Mon, Jan 28, 2013 at 11:15 AM, Mattias Nissler <mnissler@chromium.org>
> wrote:
> >> Hi Joi,
> >>
> >> I had your other mail in my inbox, let me try and add some thoughts.
> >>
> >> On Mon, Jan 28, 2013 at 11:40 AM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >>>
> >>> Hi Mattias,
> >>>
> >>> Thinking about this some more, I think there are two main things to
> >>> discuss:
> >>>
> >>> a) Whether to extract the registration logic out of PrefService
> >>> altogether.  I think this can be done, but I personally don't see a
> >>> large amount of value in it, maybe slightly better separation of
> >>> concerns.  It's certainly not necessary for switching to the
> >>> PrefRegistry type of interface, to enforce usage of
> >>> PrefRegistrySyncable for profile Prefs, and PrefRegistrySimple for
> >>> local state prefs.
> >>
> >>
> >> I'm OK with both as long as the code flow is clean enough.
> >>
> >>>
> >>> b) How to ensure that people do registration up front, or at least
> >>> encourage it strongly.  Some of your feedback suggested that
> >>> PrefRegistry should expose the DefaultPrefStore as a public member,
> >>> and not release it once it has been given to the PrefService.
> >>> Exposing it publicly would mean users could register preferences
> >>> without any necessary secondary things happening (e.g. if we have a
> >>> pref_map_ kind of optimization, this may need to be kept in sync).  In
> >>> the current approach, not giving the DefaultPrefStore up when the
> >>> PrefService has taken it over means we don't know whether registration
> >>> functions should now fail or not; this is something that would change
> >>> in a different implementation though.
> >>
> >>
> >> I think this is the crucial point we need to settle. Note that in my
> >> feedback, I said that we could have PrefRegistry provide a public member
> >> returning a _PrefStore_ pointer (not DefaultPrefStore!). PrefStore only
> >> declares a read-only interface, so we'd nicely express the fact that
> only
> >> PrefRegistry can write to its PrefStore. And the fact that we give up
> the
> >> sub-typed PrefRegistry pointer on PrefService construction would then
> ensure
> >> that we can't write without casting. Would this address all your
> concerns?
> >>
> >>>
> >>> Perhaps we should try to chat about this today so we can move forward
> >>> on the change.
> >>>
> >>> Cheers,
> >>> Jói
> >>>
> >>>
> >>> On Fri, Jan 25, 2013 at 4:40 PM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >>> > Hi Mattias,
> >>> >
> >>> > Thanks.  I'll need to take a better look at some of the stuff before
> >>> > responding.
> >>> >
> >>> > On delegating to PrefService or not:  Note that PrefRegistry only
> >>> > pre-populates the DefaultPrefStore and then relinquishes control of
> it
> >>> > at PrefService construction time.  PrefRegistry doesn't really need
> to
> >>> > use a DefaultPrefStore to store its registration details; it could
> >>> > have used something else, e.g. a regular STL map from string to
> value.
> >>> >  It just seemed wasteful to write into one map and then copy to
> >>> > another, so I did it this way, but perhaps it would be cleaner to
> >>> > fully separate them so it doesn't even use a DefaultPrefStore, and
> >>> > just fulfills an abstract interface as you suggested initially (the
> >>> > interface would just need some additional bits for enumerating all
> >>> > preferences - it only had GetType and GetValue before).
> >>
> >>
> >> See my comment above on returning DefaultPrefStore* vs. PrefStore*. If
> we do
> >> copying here and PrefService maintains registrations after construction,
> >> then I agree with you. However, I my gut feeling is we'll get a cleaner
> >> result if registration moves to PrefRegistry. I guess I'm OK with
> calling
> >> into PrefService for the benefit of post-construction registrations for
> now,
> >> given that this is meant to go away anyways.
> >>
> >>> >
> >>> > If we want to fully decouple the registration mechanics from
> >>> > PrefService, then IMHO we would absolutely need to find a way to get
> >>> > rid of needing a pref_map_ and the MarkNeedsEmptyValue thing.  I'm
> not
> >>> > sure that it really matters though; there doesn't seem to me to be
> any
> >>> > issue with PrefService keeping its own list of registered prefs
> things
> >>> > (in a way that makes sense to PrefService, e.g. updating the
> pref_map_
> >>> > which is much more a PrefService detail than something that
> >>> > PrefRegistry should know about) based on information given to it at
> >>> > construction time by PrefRegistry.
> >>
> >>
> >> Agreed.
> >>
> >>>
> >>> The post-construction-time bits
> >>> > can go away once we make a few incremental changes to get rid of the
> >>> > non-conforming registrations etc.; they are an unfortunate relic at
> >>> > the moment.
> >>> >
> >>> >> I think it would be clearer if we changed TestingPrefServiceSimple
> to
> >>> >> take a scoped_ptr<PrefRegistry> in its ctor. And then we can have a
> >>> >> CreatePrefRegistry() helper function that does the job of
> >>> >> RegisterTestPrefs() and returns the registry.
> >>> >
> >>> > This will complicate a lot of tests I think, by requiring them to
> >>> > handle both a service and a registry, whereas today they only need to
> >>> > keep the service, which owns the registry.  On the other hand if we
> >>> > want to clean up the tests to only do registration before
> construction
> >>> > (do you think that will be possible?) then we'll need to get rid of
> >>> > the .registry() thing anyway.  Need to look at this some more.
> >>
> >>
> >> They don't need both handles - they just need to provide a function like
> >> this:
> >>
> >> scoped_ptr<PrefRegistry> CreatePrefRegistry();
> >>
> >> that you can call inline when invoking the PrefService ctor or
> >> PrefServiceBuilder::Create call.
> >>
> >>>
> >>> >
> >>> >> Once we use it for profile prefs, there's a slight complication: The
> >>> >> incognito PrefService we derive from the regular PrefService also
> needs
> >>> >> a registry, but we can't have both PrefService instances own the
> >>> >> pointer. At that point we either have to copy, or make PrefRegistry
> >>> >> RefCounted (which would be in sync with the approach we took for
> >>> >> PrefStore, which is in the same situation).
> >>> >
> >>> > Ah, yes.  So even long-term the PrefRegistry instance cannot actually
> >>> > be a construction-time thing only (that lives only briefly while
> >>> > PrefService is constructed and initializes itself), it needs to be
> >>> > owned by the PrefService.  Unless PrefService can create a new
> >>> > PrefRegistry from its internal state (i.e. assuming it keeps its
> >>> > internal registration logic) to initialize the incognito PrefService.
> >>
> >>
> >> And then we're coping back and forth - why would we do that if we can
> avoid
> >> it?
> >>
> >>>
> >>> >
> >>> >> It seems like instead of sometimes delegating to PrefService, we
> should
> >>> >> rather move the entire logic over to PrefRegistry (including the
> >>> >> RegisterXYZPref calls eventually).
> >>> >
> >>> > I think this is the main bone of contention. I'm not convinced there
> >>> > is any value in moving the actual registration mechanism (and the
> >>> > associated optimizations e.g. pref_map_) out of PrefService.
> >>
> >>
> >> That's true, and pref_map_ should stay in PrefService.
> >>
> >>>
> >>> The value
> >>> > lies in removing knowledge of what the registration interface we
> >>> > require users to use, and possible side aspects of that (e.g. sync in
> >>> > the case of PrefRegistrySyncable).  If we move the registration logic
> >>> > out of PrefService, it's not like we'll have separate implementations
> >>> > of it in PrefRegistrySimple and PrefRegistrySyncable; we would just
> >>> > have the same (or very similar) implementation in the PrefRegistry
> >>> > base class.
> >>
> >>
> >> So we're down to MarkNeedsEmptyValue here? That one I'd prefer to kill
> with
> >> fire. It's probably OK to just treat all list and dictionary prefs like
> that
> >> - I'll take a closer look after lunch.
> >>
> >>>
> >>> >
> >>> > The RegisterXYZPref calls are already moved to PrefRegistrySimple for
> >>> > the local state case, and would move to PrefRegistrySyncable for the
> >>> > profile case.
> >>> >
> >>> >> AllowPrefRegistrarSimple() const;
> >>> >> Why would we need that? Subclasses that need more should take a
> pointer
> >>> >> of the appropriate type in their ctor.
> >>> >
> >>> > I simply forgot to remove this since the earlier patch version.
> >>> >
> >>> > Cheers,
> >>> > Jói
> >>> >
> >>> >
> >>> > On Fri, Jan 25, 2013 at 2:57 PM,  <mnissler@chromium.org> wrote:
> >>> >> I only looked at the PrefService bits, not the consumers for now.
> >>> >>
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
> >>> >> File base/prefs/public/pref_member_unittest.cc (right):
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
> >>> >> base/prefs/public/pref_member_unittest.cc:105:
> >>> >> RegisterTestPrefs(prefs.registry());
> >>> >> I think it would be clearer if we changed TestingPrefServiceSimple
> to
> >>> >> take a scoped_ptr<PrefRegistry> in its ctor. And then we can have a
> >>> >> CreatePrefRegistry() helper function that does the job of
> >>> >> RegisterTestPrefs() and returns the registry.
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> >>> >> File chrome/browser/prefs/browser_prefs.cc (right):
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> >>> >> chrome/browser/prefs/browser_prefs.cc:188:
> >>> >> NotificationPrefsManager::RegisterPrefs(local_state, registry);
> >>> >> Ah, this is the reason. That set-to-default piece of code in the
> >>> >> function looks weird and shouldn't be needed. Can you ask the author
> >>> >> why
> >>> >> this is there?
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> >>> >> chrome/browser/prefs/browser_prefs.cc:337:
> >>> >> registry->RegisterListPref(prefs::kDnsStartupPrefetchList);
> >>> >> It seems like we could register these prefs through the regular
> paths
> >>> >> just as well instead of doing it conditionally here.
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> >>> >> File chrome/browser/prefs/browser_prefs.h (right):
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> >>> >> chrome/browser/prefs/browser_prefs.h:16: void
> >>> >> RegisterLocalState(PrefRegistrySimple* registry, PrefService*
> >>> >> local_state);
> >>> >> Why does this take the PrefService pointer?
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> >>> >> File chrome/browser/prefs/chrome_pref_service_factory.cc (right):
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> >>> >> chrome/browser/prefs/chrome_pref_service_factory.cc:108: bool
> async) {
> >>> >> Maybe we should move the browser_prefs::RegisterLocalState call
> here?
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> >>> >> File chrome/browser/prefs/chrome_pref_service_factory.h (right):
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> >>> >> chrome/browser/prefs/chrome_pref_service_factory.h:36: //
> >>> >> CreateProfilePrefs yet).
> >>> >> Once we use it for profile prefs, there's a slight complication: The
> >>> >> incognito PrefService we derive from the regular PrefService also
> needs
> >>> >> a registry, but we can't have both PrefService instances own the
> >>> >> pointer. At that point we either have to copy, or make PrefRegistry
> >>> >> RefCounted (which would be in sync with the approach we took for
> >>> >> PrefStore, which is in the same situation).
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> chrome/browser/prefs/pref_notifier_impl_unittest.cc:100: true);
> >>> >> again, it would be nice if we could just use a CreateRegistry()
> helper
> >>> >> that creates and returns a registry so we can pass it into the
> >>> >> TestingPrefServiceSimple ctor.
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> File chrome/browser/prefs/pref_registry.cc (right):
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> chrome/browser/prefs/pref_registry.cc:17:
> >>> >> pref_service_->UnregisterPreference(path);
> >>> >> I'd rather not call into pref_service and back. Given that
> >>> >> PrefService::UnregisterPreference() is meant to go away eventually,
> how
> >>> >> about letting consumers call that function instead and have it call
> >>> >> PrefRegistry::UnregisterPreference()?
> >>> >>
> >>> >> That way, we should be able to remove the |pref_service_| pointer
> here
> >>> >> entirely (which would make the design much cleaner in that
> PrefRegistry
> >>> >> is now a true dependency of PrefService and we don't have inverse
> >>> >> arrows).
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> chrome/browser/prefs/pref_registry.cc:21:
> result->swap(default_prefs_);
> >>> >> // This releases the member reference.
> >>> >> Why would we want to release the pointer here?
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> chrome/browser/prefs/pref_registry.cc:30: }
> >>> >> It seems like instead of sometimes delegating to PrefService, we
> should
> >>> >> rather move the entire logic over to PrefRegistry (including the
> >>> >> RegisterXYZPref calls eventually).
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> File chrome/browser/prefs/pref_registry.h (right):
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> chrome/browser/prefs/pref_registry.h:9: #include
> >>> >> "base/memory/ref_counted.h"
> >>> >> once is enough ;)
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> chrome/browser/prefs/pref_registry.h:38: void
> >>> >> RetrieveDefaults(scoped_refptr<DefaultPrefStore>* result);
> >>> >> I'd make this public with the eventual goal of removing the friend
> >>> >> class
> >>> >> PrefService declaration.
> >>> >>
> >>> >> Also, why isn't this just a public accessor like so:
> >>> >>
> >>> >> scoped_refptr<PrefStore> pref_store() { return default_prefs_; }
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> chrome/browser/prefs/pref_registry.h:53: friend class
> PrefValueStore;
> >>> >> I don't understand this comment, why can a PrefRegistry be used in
> >>> >> place
> >>> >> of a PrefStore? Why would PrefValueStore be concerned with either
> >>> >> PrefStore or PrefRegistry?
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> File chrome/browser/prefs/pref_service.cc (right):
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> chrome/browser/prefs/pref_service.cc:332: // pref_map_ member for
> >>> >> UnregisterPreference.
> >>> >> The default_store_ it controls already! The user_pref_store_ piece
> is
> >>> >> unfortunate and the whole MarkNeedsEmptyValue thing seems like a big
> >>> >> hack to me. I just looked at the CL that added it, and I think the
> >>> >> proper way of handling this would have been to fix JsonPrefStore to
> >>> >> never discard prefs that are set, even if empty. I'll think a bit
> more
> >>> >> about this and see whether there's a good way to get rid of
> >>> >> PersistentPrefStore::MarkNeedsEmptyValue().
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> File chrome/browser/prefs/pref_service.h (right):
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> chrome/browser/prefs/pref_service.h:172: PrefRegistry*
> >>> >> DeprecatedGetPrefRegistry() const;
> >>> >> Does this have to be const? The caller can change state by
> manipulating
> >>> >> PrefRegistry, so the const is inaccurate.
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> chrome/browser/prefs/pref_service.h:198: virtual bool
> >>> >> AllowPrefRegistrarSimple() const;
> >>> >> Why would we need that? Subclasses that need more should take a
> pointer
> >>> >> of the appropriate type in their ctor.
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> File chrome/browser/prefs/pref_service_builder.h (right):
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> chrome/browser/prefs/pref_service_builder.h:53: virtual PrefService*
> >>> >> Create(scoped_ptr<PrefRegistry> registry);
> >>> >> Hm, it's a bit inconsistent to single out the registry parameter and
> >>> >> pass it in Create() instead of having a WithRegistry() function.
> >>> >> However, that's probably justified given PrefRegistry is essential -
> >>> >> every consumer will want to pass it. That's just my thoughts for
> your
> >>> >> consideration, I leave the decision to you.
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> File chrome/browser/prefs/pref_service_syncable.cc (right):
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> chrome/browser/prefs/pref_service_syncable.cc:97:
> >>> >> dummy_registry->default_prefs_ = default_store_;
> >>> >> shouldn't the two stores share the same registry?
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> File chrome/browser/prefs/pref_value_store.h (right):
> >>> >>
> >>> >>
> >>> >>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> >>> >> chrome/browser/prefs/pref_value_store.h:59: PrefRegistry*
> >>> >> pref_registry,
> >>> >> PrefValueStore shouldn't have to know about PrefRegistry,
> PrefService
> >>> >> should just get the default pref store pointer from PrefRegistry and
> >>> >> pass it on here.
> >>> >>
> >>> >> https://codereview.chromium.org/11741003/
> >>
> >>
>

[Jói, 2013-01-29 16:10:02]

Hi Mattias,

Please take another look.  I've addressed most of your comments on the
production side of things, but not on the test side yet.  Things are a lot more
beautiful now, I think.

If you like, you can look at the diff from patch set 3 to patch set 4 to see
what I changed in response to your review comments.

I'd like to defer the work to fix browser_prefs::RegisterLocalState to not
require the additional PrefService parameter, and do it in one or more
follow-ups since each change to a registration function that currently needs
PrefService will need owner review by different people.

On the test side, I'm wondering whether switching to requiring each test to
provide a PrefRegistry to construct the TestingPrefServiceXyz is the right move.
 There are two sides to this:

a) If we intend to fully remove after-the-fact registration, even for tests,
then we'll have to convert tests to do registration before the fact, which
implies we _have to_ change the constructor of TestingPrefServiceXyz to accept a
registry.

b) If we intend to allow tests to continue to do after-the-fact registration,
then the current approach seems more convenient:  Have each
TestingPrefServiceXyz (where Xyz is Simple or Syncable) to create a registry of
the appropriate type and provide a typed registry() accessor to it.

BTW, on the testing front, once I introduce PrefRegistrySyncable, there should
be an opportunity to further simplify the implementation around
TestingPrefService, stop using templates, and most importantly split up between
Chrome-specific and stuff that will move to base/prefs.

WDYT?

Cheers,
Jói

https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
File base/prefs/public/pref_member_unittest.cc (right):

https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
base/prefs/public/pref_member_unittest.cc:105:
RegisterTestPrefs(prefs.registry());
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> I think it would be clearer if we changed TestingPrefServiceSimple to take a
> scoped_ptr<PrefRegistry> in its ctor. And then we can have a
> CreatePrefRegistry() helper function that does the job of RegisterTestPrefs()
> and returns the registry.

I haven't started looking at how involved this is, but I plan to.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:188:
NotificationPrefsManager::RegisterPrefs(local_state, registry);
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> Ah, this is the reason. That set-to-default piece of code in the function
looks
> weird and shouldn't be needed. Can you ask the author why this is there?

Mind if we take care of this in a follow-up?

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:337:
registry->RegisterListPref(prefs::kDnsStartupPrefetchList);
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> It seems like we could register these prefs through the regular paths just as
> well instead of doing it conditionally here.

Mind if we take care of this in a follow-up?

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.h (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.h:16: void
RegisterLocalState(PrefRegistrySimple* registry, PrefService* local_state);
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> Why does this take the PrefService pointer?

Explained in .cc file.  Would like to fix in follow-ups rather than in this
change.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.cc:108: bool async) {
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> Maybe we should move the browser_prefs::RegisterLocalState call here?

That could be done once the TODO around line 263 of chrome_browser_main.cc (use
a JSONPrefStore instead of an entire PrefService) has been done. Until then,
this would cause an unnecessary call to browser_prefs::RegisterLocalState, which
is pretty expensive.

I've updated the TODO in chrome_browser_main.cc to say this can be fixed once
the TODO is addressed.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
File chrome/browser/prefs/chrome_pref_service_factory.h (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.h:36: // CreateProfilePrefs
yet).
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> Once we use it for profile prefs, there's a slight complication: The incognito
> PrefService we derive from the regular PrefService also needs a registry, but
we
> can't have both PrefService instances own the pointer. At that point we either
> have to copy, or make PrefRegistry RefCounted (which would be in sync with the
> approach we took for PrefStore, which is in the same situation).

Fixed this by having a separate PrefRegistry that owns the same
DefaultPrefStore. Currently accomplished by way of PrefServiceSyncable being a
friend of PrefRegistry but once that goes away I'll put something nicer (factory
function or some such).

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_notifier_impl_unittest.cc:100: true);
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> again, it would be nice if we could just use a CreateRegistry() helper that
> creates and returns a registry so we can pass it into the
> TestingPrefServiceSimple ctor.

Haven't started looking at this yet, but I plan to. Not sure yet how many tests
depend on doing registration after construction time, so I don't know yet if
this is a ton of work or quite easy.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_registry.cc (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.cc:17:
pref_service_->UnregisterPreference(path);
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> I'd rather not call into pref_service and back. Given that
> PrefService::UnregisterPreference() is meant to go away eventually, how about
> letting consumers call that function instead and have it call
> PrefRegistry::UnregisterPreference()?
> 
> That way, we should be able to remove the |pref_service_| pointer here
entirely
> (which would make the design much cleaner in that PrefRegistry is now a true
> dependency of PrefService and we don't have inverse arrows).

This needs to stay here (and be virtual) since PrefServiceSyncable will do more
than just unregister from the DefaultPrefStore.  In any case, registration is
now entirely in PrefRegistry.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.cc:21: result->swap(default_prefs_);  // This
releases the member reference.
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> Why would we want to release the pointer here?

This was meant as a belt-and-suspenders approach to ensure no registrations
after construction of the PrefService, unless they go to PrefService.  The new
approach is much nicer.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.cc:30: }
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> It seems like instead of sometimes delegating to PrefService, we should rather
> move the entire logic over to PrefRegistry (including the RegisterXYZPref
calls
> eventually).

Done, but PrefService still gets callbacks to update its internal state.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_registry.h (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.h:9: #include "base/memory/ref_counted.h"
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> once is enough ;)

Done.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.h:38: void
RetrieveDefaults(scoped_refptr<DefaultPrefStore>* result);
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> I'd make this public with the eventual goal of removing the friend class
> PrefService declaration.
> 
> Also, why isn't this just a public accessor like so:
> 
> scoped_refptr<PrefStore> pref_store() { return default_prefs_; }

Done.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.h:53: friend class PrefValueStore;
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> I don't understand this comment, why can a PrefRegistry be used in place of a
> PrefStore? Why would PrefValueStore be concerned with either PrefStore or
> PrefRegistry?

This bit of ugliness is gone.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service.cc (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.cc:332: // pref_map_ member for
UnregisterPreference.
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> The default_store_ it controls already! The user_pref_store_ piece is
> unfortunate and the whole MarkNeedsEmptyValue thing seems like a big hack to
me.
> I just looked at the CL that added it, and I think the proper way of handling
> this would have been to fix JsonPrefStore to never discard prefs that are set,
> even if empty. I'll think a bit more about this and see whether there's a good
> way to get rid of PersistentPrefStore::MarkNeedsEmptyValue().

Registration is now fully owned by PrefRegistry. PrefService registers for
registration and unregistration callbacks to update its internal caches when
registration is done after construction time, and on unregistration.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service.h (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.h:172: PrefRegistry*
DeprecatedGetPrefRegistry() const;
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> Does this have to be const? The caller can change state by manipulating
> PrefRegistry, so the const is inaccurate.

Done.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.h:198: virtual bool AllowPrefRegistrarSimple()
const;
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> Why would we need that? Subclasses that need more should take a pointer of the
> appropriate type in their ctor.

Gone.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_builder.h (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_builder.h:53: virtual PrefService*
Create(scoped_ptr<PrefRegistry> registry);
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> Hm, it's a bit inconsistent to single out the registry parameter and pass it
in
> Create() instead of having a WithRegistry() function. However, that's probably
> justified given PrefRegistry is essential - every consumer will want to pass
it.
> That's just my thoughts for your consideration, I leave the decision to you.

Initially I did this mainly to clarify the "pass ownership" semantics.  That no
longer matters since it's refcounted now.

The main reason I see to continue to have it passed, is to encourage
registration before the fact, rather than creating an empty PrefRegistry,
setting it with WithPrefRegistry, then retrieving it using
DeprecatedGetPrefRegistry.  Not a big deal; if you prefer the other approach I'm
happy to switch.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:97: dummy_registry->default_prefs_
= default_store_;
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> shouldn't the two stores share the same registry?

Done.

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_value_store.h (right):

https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_value_store.h:59: PrefRegistry* pref_registry,
On 2013/01/25 14:57:11, Mattias Nissler wrote:
> PrefValueStore shouldn't have to know about PrefRegistry, PrefService should
> just get the default pref store pointer from PrefRegistry and pass it on here.

Done.

[Jói, 2013-01-29 16:12:41]

Also, re using the same PrefRegistry for the incognito service, see
PrefServiceSyncable::CreateIncognitoPrefService.  We either need to do
something like I've done there (different registries, both share the
same DefaultPrefStore but only the non-incognito one actually allows
registration), or we would need to allow multiple
registration/unregistration observers on a PrefRegistry.  I think we
should only do the latter if we know that the incognito service
actually needs to do registrations.

Cheers,
Jói


On Tue, Jan 29, 2013 at 4:10 PM,  <joi@chromium.org> wrote:
> Hi Mattias,
>
> Please take another look.  I've addressed most of your comments on the
> production side of things, but not on the test side yet.  Things are a lot
> more
> beautiful now, I think.
>
> If you like, you can look at the diff from patch set 3 to patch set 4 to see
> what I changed in response to your review comments.
>
> I'd like to defer the work to fix browser_prefs::RegisterLocalState to not
> require the additional PrefService parameter, and do it in one or more
> follow-ups since each change to a registration function that currently needs
> PrefService will need owner review by different people.
>
> On the test side, I'm wondering whether switching to requiring each test to
> provide a PrefRegistry to construct the TestingPrefServiceXyz is the right
> move.
>  There are two sides to this:
>
> a) If we intend to fully remove after-the-fact registration, even for tests,
> then we'll have to convert tests to do registration before the fact, which
> implies we _have to_ change the constructor of TestingPrefServiceXyz to
> accept a
> registry.
>
> b) If we intend to allow tests to continue to do after-the-fact
> registration,
> then the current approach seems more convenient:  Have each
> TestingPrefServiceXyz (where Xyz is Simple or Syncable) to create a registry
> of
> the appropriate type and provide a typed registry() accessor to it.
>
> BTW, on the testing front, once I introduce PrefRegistrySyncable, there
> should
> be an opportunity to further simplify the implementation around
> TestingPrefService, stop using templates, and most importantly split up
> between
> Chrome-specific and stuff that will move to base/prefs.
>
> WDYT?
>
> Cheers,
> Jói
>
>
>
>
>
https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
> File base/prefs/public/pref_member_unittest.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_me...
> base/prefs/public/pref_member_unittest.cc:105:
> RegisterTestPrefs(prefs.registry());
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> I think it would be clearer if we changed TestingPrefServiceSimple to
>
> take a
>>
>> scoped_ptr<PrefRegistry> in its ctor. And then we can have a
>> CreatePrefRegistry() helper function that does the job of
>
> RegisterTestPrefs()
>>
>> and returns the registry.
>
>
> I haven't started looking at how involved this is, but I plan to.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> File chrome/browser/prefs/browser_prefs.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> chrome/browser/prefs/browser_prefs.cc:188:
> NotificationPrefsManager::RegisterPrefs(local_state, registry);
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> Ah, this is the reason. That set-to-default piece of code in the
>
> function looks
>>
>> weird and shouldn't be needed. Can you ask the author why this is
>
> there?
>
> Mind if we take care of this in a follow-up?
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> chrome/browser/prefs/browser_prefs.cc:337:
> registry->RegisterListPref(prefs::kDnsStartupPrefetchList);
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> It seems like we could register these prefs through the regular paths
>
> just as
>>
>> well instead of doing it conditionally here.
>
>
> Mind if we take care of this in a follow-up?
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> File chrome/browser/prefs/browser_prefs.h (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/brow...
> chrome/browser/prefs/browser_prefs.h:16: void
> RegisterLocalState(PrefRegistrySimple* registry, PrefService*
> local_state);
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> Why does this take the PrefService pointer?
>
>
> Explained in .cc file.  Would like to fix in follow-ups rather than in
> this change.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> File chrome/browser/prefs/chrome_pref_service_factory.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> chrome/browser/prefs/chrome_pref_service_factory.cc:108: bool async) {
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> Maybe we should move the browser_prefs::RegisterLocalState call here?
>
>
> That could be done once the TODO around line 263 of
> chrome_browser_main.cc (use a JSONPrefStore instead of an entire
> PrefService) has been done. Until then, this would cause an unnecessary
> call to browser_prefs::RegisterLocalState, which is pretty expensive.
>
> I've updated the TODO in chrome_browser_main.cc to say this can be fixed
> once the TODO is addressed.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> File chrome/browser/prefs/chrome_pref_service_factory.h (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chro...
> chrome/browser/prefs/chrome_pref_service_factory.h:36: //
> CreateProfilePrefs yet).
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> Once we use it for profile prefs, there's a slight complication: The
>
> incognito
>>
>> PrefService we derive from the regular PrefService also needs a
>
> registry, but we
>>
>> can't have both PrefService instances own the pointer. At that point
>
> we either
>>
>> have to copy, or make PrefRegistry RefCounted (which would be in sync
>
> with the
>>
>> approach we took for PrefStore, which is in the same situation).
>
>
> Fixed this by having a separate PrefRegistry that owns the same
> DefaultPrefStore. Currently accomplished by way of PrefServiceSyncable
> being a friend of PrefRegistry but once that goes away I'll put
> something nicer (factory function or some such).
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_notifier_impl_unittest.cc:100: true);
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> again, it would be nice if we could just use a CreateRegistry() helper
>
> that
>>
>> creates and returns a registry so we can pass it into the
>> TestingPrefServiceSimple ctor.
>
>
> Haven't started looking at this yet, but I plan to. Not sure yet how
> many tests depend on doing registration after construction time, so I
> don't know yet if this is a ton of work or quite easy.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_registry.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry.cc:17:
> pref_service_->UnregisterPreference(path);
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> I'd rather not call into pref_service and back. Given that
>> PrefService::UnregisterPreference() is meant to go away eventually,
>
> how about
>>
>> letting consumers call that function instead and have it call
>> PrefRegistry::UnregisterPreference()?
>
>
>> That way, we should be able to remove the |pref_service_| pointer here
>
> entirely
>>
>> (which would make the design much cleaner in that PrefRegistry is now
>
> a true
>>
>> dependency of PrefService and we don't have inverse arrows).
>
>
> This needs to stay here (and be virtual) since PrefServiceSyncable will
> do more than just unregister from the DefaultPrefStore.  In any case,
> registration is now entirely in PrefRegistry.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry.cc:21: result->swap(default_prefs_);
> // This releases the member reference.
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> Why would we want to release the pointer here?
>
>
> This was meant as a belt-and-suspenders approach to ensure no
> registrations after construction of the PrefService, unless they go to
> PrefService.  The new approach is much nicer.
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry.cc:30: }
>
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> It seems like instead of sometimes delegating to PrefService, we
>
> should rather
>>
>> move the entire logic over to PrefRegistry (including the
>
> RegisterXYZPref calls
>>
>> eventually).
>
>
> Done, but PrefService still gets callbacks to update its internal state.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_registry.h (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry.h:9: #include
> "base/memory/ref_counted.h"
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> once is enough ;)
>
>
> Done.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry.h:38: void
> RetrieveDefaults(scoped_refptr<DefaultPrefStore>* result);
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> I'd make this public with the eventual goal of removing the friend
>
> class
>>
>> PrefService declaration.
>
>
>> Also, why isn't this just a public accessor like so:
>
>
>> scoped_refptr<PrefStore> pref_store() { return default_prefs_; }
>
>
> Done.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry.h:53: friend class PrefValueStore;
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> I don't understand this comment, why can a PrefRegistry be used in
>
> place of a
>>
>> PrefStore? Why would PrefValueStore be concerned with either PrefStore
>
> or
>>
>> PrefRegistry?
>
>
> This bit of ugliness is gone.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_service.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service.cc:332: // pref_map_ member for
> UnregisterPreference.
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> The default_store_ it controls already! The user_pref_store_ piece is
>> unfortunate and the whole MarkNeedsEmptyValue thing seems like a big
>
> hack to me.
>>
>> I just looked at the CL that added it, and I think the proper way of
>
> handling
>>
>> this would have been to fix JsonPrefStore to never discard prefs that
>
> are set,
>>
>> even if empty. I'll think a bit more about this and see whether
>
> there's a good
>>
>> way to get rid of PersistentPrefStore::MarkNeedsEmptyValue().
>
>
> Registration is now fully owned by PrefRegistry. PrefService registers
> for registration and unregistration callbacks to update its internal
> caches when registration is done after construction time, and on
> unregistration.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_service.h (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service.h:172: PrefRegistry*
> DeprecatedGetPrefRegistry() const;
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> Does this have to be const? The caller can change state by
>
> manipulating
>>
>> PrefRegistry, so the const is inaccurate.
>
>
> Done.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service.h:198: virtual bool
> AllowPrefRegistrarSimple() const;
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> Why would we need that? Subclasses that need more should take a
>
> pointer of the
>>
>> appropriate type in their ctor.
>
>
> Gone.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_service_builder.h (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service_builder.h:53: virtual PrefService*
> Create(scoped_ptr<PrefRegistry> registry);
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> Hm, it's a bit inconsistent to single out the registry parameter and
>
> pass it in
>>
>> Create() instead of having a WithRegistry() function. However, that's
>
> probably
>>
>> justified given PrefRegistry is essential - every consumer will want
>
> to pass it.
>>
>> That's just my thoughts for your consideration, I leave the decision
>
> to you.
>
> Initially I did this mainly to clarify the "pass ownership" semantics.
> That no longer matters since it's refcounted now.
>
> The main reason I see to continue to have it passed, is to encourage
> registration before the fact, rather than creating an empty
> PrefRegistry, setting it with WithPrefRegistry, then retrieving it using
> DeprecatedGetPrefRegistry.  Not a big deal; if you prefer the other
> approach I'm happy to switch.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_service_syncable.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service_syncable.cc:97:
> dummy_registry->default_prefs_ = default_store_;
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> shouldn't the two stores share the same registry?
>
>
> Done.
>
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_value_store.h (right):
>
>
https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_value_store.h:59: PrefRegistry* pref_registry,
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>>
>> PrefValueStore shouldn't have to know about PrefRegistry, PrefService
>
> should
>>
>> just get the default pref store pointer from PrefRegistry and pass it
>
> on here.
>
> Done.
>
> https://codereview.chromium.org/11741003/

[Mattias Nissler (ping if slow), 2013-01-29 18:10:45]

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:378:
GoogleURLTracker::kDefaultGoogleHomepage);
nit: indentation

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:386: std::string());
nit: indentation

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
File chrome/browser/prefs/chrome_pref_service_factory.h (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.h:34: // |pref_registry| is the
registry object to retrieve registration
This sentence is a bit cloudy, can we just say that |pref_registry| keeps the
list of registered prefs and their default values?

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.h:35: // information from;
PrefService takes ownership (not used for
PrefRegistry is RefCounted vs. ownership transfer

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.h:48: PrefRegistry*
pref_registry,
Isn't this now refcounted? I think the rule is to pass the pointer wrapped in a
scoped_refptr then.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_notifier_impl_unittest.cc:98: true);
nit: no need for line break (also below).

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_registry.h (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.h:35: scoped_refptr<PrefStore> defaults();
forward-declare PrefStore?

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.h:68: scoped_refptr<DefaultPrefStore>
defaults_;
could be forward-declared?

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_registry_simple.cc (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry_simple.cc:16: RegisterPreference(path,
Value::CreateBooleanValue(default_value));
nit: namespace-qualify base::Value (here and elsewhere in the file)

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_registry_simple.h (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry_simple.h:33: const char* path,
base::DictionaryValue* default_value);
nit: parameters on separate lines

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service.cc (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.cc:61: weak_ptr_factory_.GetWeakPtr()));
Do we need the weak_ptr_factory_ here? We hold a references to pref_registry_,
so base::Unretained() should work as long as you reset the callbacks on
destruction.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.cc:64: weak_ptr_factory_.GetWeakPtr()));
So this will break if we're ever passing a registry to several PrefService
instances (which we might do for incognito?)

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service.h (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.h:173: friend class PrefRegistry;  // For
Add/RemoveRegisteredPreference.
Isn't that going through the callbacks now?

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:102: // any registrations.
I understand we are intending to remove the callbacks eventually? If so, can we
have a TODO here saying that this can go away once the callbacks are gone?

[Mattias Nissler (ping if slow), 2013-01-29 18:20:18]

On Tue, Jan 29, 2013 at 5:10 PM, <joi@chromium.org> wrote:

> Hi Mattias,
>
> Please take another look.  I've addressed most of your comments on the
> production side of things, but not on the test side yet.  Things are a lot
> more
> beautiful now, I think.


I agree, thanks!


> If you like, you can look at the diff from patch set 3 to patch set 4 to
> see
> what I changed in response to your review comments.
>
> I'd like to defer the work to fix browser_prefs::**RegisterLocalState to
> not
> require the additional PrefService parameter, and do it in one or more
> follow-ups since each change to a registration function that currently
> needs
> PrefService will need owner review by different people.
>

SGTM


> On the test side, I'm wondering whether switching to requiring each test to
> provide a PrefRegistry to construct the TestingPrefServiceXyz is the right
> move.
>  There are two sides to this:
>
> a) If we intend to fully remove after-the-fact registration, even for
> tests,
> then we'll have to convert tests to do registration before the fact, which
> implies we _have to_ change the constructor of TestingPrefServiceXyz to
> accept a
> registry.
>
> b) If we intend to allow tests to continue to do after-the-fact
> registration,
> then the current approach seems more convenient:  Have each
> TestingPrefServiceXyz (where Xyz is Simple or Syncable) to create a
> registry of
> the appropriate type and provide a typed registry() accessor to it.
>

I think I'm also fine with this. The only slight discomfort remaining on my
side is that this means pref registration works differently for tests as
compared to production prefs, which may confuse people, but I think that's
OK if well-documented.


>
> BTW, on the testing front, once I introduce PrefRegistrySyncable, there
> should
> be an opportunity to further simplify the implementation around
> TestingPrefService, stop using templates, and most importantly split up
> between
> Chrome-specific and stuff that will move to base/prefs.
>

SGTM


>
> WDYT?
>
> Cheers,
> Jói
>
>
>
>
> https://codereview.chromium.**org/11741003/diff/16001/base/**
>
prefs/public/pref_member_**unittest.cc<https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_member_unittest.cc>
> File base/prefs/public/pref_member_**unittest.cc (right):
>
> https://codereview.chromium.**org/11741003/diff/16001/base/**
>
prefs/public/pref_member_**unittest.cc#newcode105<https://codereview.chromium.org/11741003/diff/16001/base/prefs/public/pref_member_unittest.cc#newcode105>
> base/prefs/public/pref_member_**unittest.cc:105:
> RegisterTestPrefs(prefs.**registry());
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> I think it would be clearer if we changed TestingPrefServiceSimple to
>>
> take a
>
>> scoped_ptr<PrefRegistry> in its ctor. And then we can have a
>> CreatePrefRegistry() helper function that does the job of
>>
> RegisterTestPrefs()
>
>> and returns the registry.
>>
>
> I haven't started looking at how involved this is, but I plan to.
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/browser_**prefs.cc<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/browser_prefs.cc>
> File chrome/browser/prefs/browser_**prefs.cc (right):
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/browser_**prefs.cc#newcode188<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/browser_prefs.cc#newcode188>
> chrome/browser/prefs/browser_**prefs.cc:188:
> NotificationPrefsManager::**RegisterPrefs(local_state, registry);
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> Ah, this is the reason. That set-to-default piece of code in the
>>
> function looks
>
>> weird and shouldn't be needed. Can you ask the author why this is
>>
> there?
>
> Mind if we take care of this in a follow-up?
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/browser_**prefs.cc#newcode337<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/browser_prefs.cc#newcode337>
> chrome/browser/prefs/browser_**prefs.cc:337:
> registry->RegisterListPref(**prefs::**kDnsStartupPrefetchList);
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> It seems like we could register these prefs through the regular paths
>>
> just as
>
>> well instead of doing it conditionally here.
>>
>
> Mind if we take care of this in a follow-up?
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/browser_**prefs.h<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/browser_prefs.h>
> File chrome/browser/prefs/browser_**prefs.h (right):
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/browser_**prefs.h#newcode16<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/browser_prefs.h#newcode16>
> chrome/browser/prefs/browser_**prefs.h:16: void
> RegisterLocalState(**PrefRegistrySimple* registry, PrefService*
> local_state);
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> Why does this take the PrefService pointer?
>>
>
> Explained in .cc file.  Would like to fix in follow-ups rather than in
> this change.
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/chrome_**pref_service_factory.cc<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chrome_pref_service_factory.cc>
> File chrome/browser/prefs/chrome_**pref_service_factory.cc (right):
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/chrome_**pref_service_factory.cc#**newcode108<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chrome_pref_service_factory.cc#newcode108>
> chrome/browser/prefs/chrome_**pref_service_factory.cc:108: bool async) {
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> Maybe we should move the browser_prefs::**RegisterLocalState call here?
>>
>
> That could be done once the TODO around line 263 of
> chrome_browser_main.cc (use a JSONPrefStore instead of an entire
> PrefService) has been done. Until then, this would cause an unnecessary
> call to browser_prefs::**RegisterLocalState, which is pretty expensive.
>
> I've updated the TODO in chrome_browser_main.cc to say this can be fixed
> once the TODO is addressed.
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/chrome_**pref_service_factory.h<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chrome_pref_service_factory.h>
> File chrome/browser/prefs/chrome_**pref_service_factory.h (right):
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/chrome_**pref_service_factory.h#**newcode36<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/chrome_pref_service_factory.h#newcode36>
> chrome/browser/prefs/chrome_**pref_service_factory.h:36: //
> CreateProfilePrefs yet).
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> Once we use it for profile prefs, there's a slight complication: The
>>
> incognito
>
>> PrefService we derive from the regular PrefService also needs a
>>
> registry, but we
>
>> can't have both PrefService instances own the pointer. At that point
>>
> we either
>
>> have to copy, or make PrefRegistry RefCounted (which would be in sync
>>
> with the
>
>> approach we took for PrefStore, which is in the same situation).
>>
>
> Fixed this by having a separate PrefRegistry that owns the same
> DefaultPrefStore. Currently accomplished by way of PrefServiceSyncable
> being a friend of PrefRegistry but once that goes away I'll put
> something nicer (factory function or some such).
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**notifier_impl_unittest.cc<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_notifier_impl_unittest.cc>
> File chrome/browser/prefs/pref_**notifier_impl_unittest.cc (right):
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**notifier_impl_unittest.cc#**newcode100<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_notifier_impl_unittest.cc#newcode100>
> chrome/browser/prefs/pref_**notifier_impl_unittest.cc:100: true);
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> again, it would be nice if we could just use a CreateRegistry() helper
>>
> that
>
>> creates and returns a registry so we can pass it into the
>> TestingPrefServiceSimple ctor.
>>
>
> Haven't started looking at this yet, but I plan to. Not sure yet how
> many tests depend on doing registration after construction time, so I
> don't know yet if this is a ton of work or quite easy.
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**registry.cc<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_registry.cc>
> File chrome/browser/prefs/pref_**registry.cc (right):
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**registry.cc#newcode17<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_registry.cc#newcode17>
> chrome/browser/prefs/pref_**registry.cc:17:
> pref_service_->**UnregisterPreference(path);
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> I'd rather not call into pref_service and back. Given that
>> PrefService::**UnregisterPreference() is meant to go away eventually,
>>
> how about
>
>> letting consumers call that function instead and have it call
>> PrefRegistry::**UnregisterPreference()?
>>
>
>  That way, we should be able to remove the |pref_service_| pointer here
>>
> entirely
>
>> (which would make the design much cleaner in that PrefRegistry is now
>>
> a true
>
>> dependency of PrefService and we don't have inverse arrows).
>>
>
> This needs to stay here (and be virtual) since PrefServiceSyncable will
> do more than just unregister from the DefaultPrefStore.  In any case,
> registration is now entirely in PrefRegistry.
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**registry.cc#newcode21<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_registry.cc#newcode21>
> chrome/browser/prefs/pref_**registry.cc:21: result->swap(default_prefs_);
> // This releases the member reference.
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> Why would we want to release the pointer here?
>>
>
> This was meant as a belt-and-suspenders approach to ensure no
> registrations after construction of the PrefService, unless they go to
> PrefService.  The new approach is much nicer.
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**registry.cc#newcode30<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_registry.cc#newcode30>
> chrome/browser/prefs/pref_**registry.cc:30: }
>
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> It seems like instead of sometimes delegating to PrefService, we
>>
> should rather
>
>> move the entire logic over to PrefRegistry (including the
>>
> RegisterXYZPref calls
>
>> eventually).
>>
>
> Done, but PrefService still gets callbacks to update its internal state.
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**registry.h<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_registry.h>
> File chrome/browser/prefs/pref_**registry.h (right):
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**registry.h#newcode9<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_registry.h#newcode9>
> chrome/browser/prefs/pref_**registry.h:9: #include
> "base/memory/ref_counted.h"
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> once is enough ;)
>>
>
> Done.
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**registry.h#newcode38<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_registry.h#newcode38>
> chrome/browser/prefs/pref_**registry.h:38: void
> RetrieveDefaults(scoped_**refptr<DefaultPrefStore>* result);
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> I'd make this public with the eventual goal of removing the friend
>>
> class
>
>> PrefService declaration.
>>
>
>  Also, why isn't this just a public accessor like so:
>>
>
>  scoped_refptr<PrefStore> pref_store() { return default_prefs_; }
>>
>
> Done.
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**registry.h#newcode53<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_registry.h#newcode53>
> chrome/browser/prefs/pref_**registry.h:53: friend class PrefValueStore;
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> I don't understand this comment, why can a PrefRegistry be used in
>>
> place of a
>
>> PrefStore? Why would PrefValueStore be concerned with either PrefStore
>>
> or
>
>> PrefRegistry?
>>
>
> This bit of ugliness is gone.
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**service.cc<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_service.cc>
> File chrome/browser/prefs/pref_**service.cc (right):
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**service.cc#newcode332<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_service.cc#newcode332>
> chrome/browser/prefs/pref_**service.cc:332: // pref_map_ member for
> UnregisterPreference.
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> The default_store_ it controls already! The user_pref_store_ piece is
>> unfortunate and the whole MarkNeedsEmptyValue thing seems like a big
>>
> hack to me.
>
>> I just looked at the CL that added it, and I think the proper way of
>>
> handling
>
>> this would have been to fix JsonPrefStore to never discard prefs that
>>
> are set,
>
>> even if empty. I'll think a bit more about this and see whether
>>
> there's a good
>
>> way to get rid of PersistentPrefStore::**MarkNeedsEmptyValue().
>>
>
> Registration is now fully owned by PrefRegistry. PrefService registers
> for registration and unregistration callbacks to update its internal
> caches when registration is done after construction time, and on
> unregistration.
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**service.h<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_service.h>
> File chrome/browser/prefs/pref_**service.h (right):
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**service.h#newcode172<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_service.h#newcode172>
> chrome/browser/prefs/pref_**service.h:172: PrefRegistry*
> DeprecatedGetPrefRegistry() const;
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> Does this have to be const? The caller can change state by
>>
> manipulating
>
>> PrefRegistry, so the const is inaccurate.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**service.h#newcode198<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_service.h#newcode198>
> chrome/browser/prefs/pref_**service.h:198: virtual bool
> AllowPrefRegistrarSimple() const;
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> Why would we need that? Subclasses that need more should take a
>>
> pointer of the
>
>> appropriate type in their ctor.
>>
>
> Gone.
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**service_builder.h<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_service_builder.h>
> File chrome/browser/prefs/pref_**service_builder.h (right):
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**service_builder.h#newcode53<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_service_builder.h#newcode53>
> chrome/browser/prefs/pref_**service_builder.h:53: virtual PrefService*
> Create(scoped_ptr<**PrefRegistry> registry);
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> Hm, it's a bit inconsistent to single out the registry parameter and
>>
> pass it in
>
>> Create() instead of having a WithRegistry() function. However, that's
>>
> probably
>
>> justified given PrefRegistry is essential - every consumer will want
>>
> to pass it.
>
>> That's just my thoughts for your consideration, I leave the decision
>>
> to you.
>
> Initially I did this mainly to clarify the "pass ownership" semantics.
> That no longer matters since it's refcounted now.
>
> The main reason I see to continue to have it passed, is to encourage
> registration before the fact, rather than creating an empty
> PrefRegistry, setting it with WithPrefRegistry, then retrieving it using
> DeprecatedGetPrefRegistry.  Not a big deal; if you prefer the other
> approach I'm happy to switch.
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**service_syncable.cc<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_service_syncable.cc>
> File chrome/browser/prefs/pref_**service_syncable.cc (right):
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**service_syncable.cc#newcode97<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_service_syncable.cc#newcode97>
> chrome/browser/prefs/pref_**service_syncable.cc:97:
> dummy_registry->default_prefs_ = default_store_;
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> shouldn't the two stores share the same registry?
>>
>
> Done.
>
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**value_store.h<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_value_store.h>
> File chrome/browser/prefs/pref_**value_store.h (right):
>
> https://codereview.chromium.**org/11741003/diff/16001/**
>
chrome/browser/prefs/pref_**value_store.h#newcode59<https://codereview.chromium.org/11741003/diff/16001/chrome/browser/prefs/pref_value_store.h#newcode59>
> chrome/browser/prefs/pref_**value_store.h:59: PrefRegistry* pref_registry,
> On 2013/01/25 14:57:11, Mattias Nissler wrote:
>
>> PrefValueStore shouldn't have to know about PrefRegistry, PrefService
>>
> should
>
>> just get the default pref store pointer from PrefRegistry and pass it
>>
> on here.
>
> Done.
>
>
https://codereview.chromium.**org/11741003/<https://codereview.chromium.org/1...
>

[Jói, 2013-01-30 14:23:33]

Hi Mattias,

As requested, I added documentation on PrefRegistrySimple::registry().  See how
other comments were addressed below.

You can look at the diff from the second-to-last patch set (a pure merge to
LKGR) and the last patch set to see just the changes I made in response to
review feedback.

Cheers,
Jói

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:378:
GoogleURLTracker::kDefaultGoogleHomepage);
On 2013/01/29 18:10:45, Mattias Nissler wrote:
> nit: indentation

Done.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:386: std::string());
On 2013/01/29 18:10:45, Mattias Nissler wrote:
> nit: indentation

Done.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
File chrome/browser/prefs/chrome_pref_service_factory.h (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.h:34: // |pref_registry| is the
registry object to retrieve registration
On 2013/01/29 18:10:45, Mattias Nissler wrote:
> This sentence is a bit cloudy, can we just say that |pref_registry| keeps the
> list of registered prefs and their default values?

Done.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.h:35: // information from;
PrefService takes ownership (not used for
On 2013/01/29 18:10:45, Mattias Nissler wrote:
> PrefRegistry is RefCounted vs. ownership transfer

Done.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
chrome/browser/prefs/chrome_pref_service_factory.h:48: PrefRegistry*
pref_registry,
On 2013/01/29 18:10:45, Mattias Nissler wrote:
> Isn't this now refcounted? I think the rule is to pass the pointer wrapped in
a
> scoped_refptr then.

Done.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_notifier_impl_unittest.cc:98: true);
On 2013/01/29 18:10:45, Mattias Nissler wrote:
> nit: no need for line break (also below).

Done.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_registry.h (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.h:35: scoped_refptr<PrefStore> defaults();
On 2013/01/29 18:10:45, Mattias Nissler wrote:
> forward-declare PrefStore?

Done.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry.h:68: scoped_refptr<DefaultPrefStore>
defaults_;
On 2013/01/29 18:10:45, Mattias Nissler wrote:
> could be forward-declared?

Done.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_registry_simple.cc (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry_simple.cc:16: RegisterPreference(path,
Value::CreateBooleanValue(default_value));
On 2013/01/29 18:10:45, Mattias Nissler wrote:
> nit: namespace-qualify base::Value (here and elsewhere in the file)

Done.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_registry_simple.h (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry_simple.h:33: const char* path,
base::DictionaryValue* default_value);
On 2013/01/29 18:10:45, Mattias Nissler wrote:
> nit: parameters on separate lines

Done.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service.cc (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.cc:61: weak_ptr_factory_.GetWeakPtr()));
On 2013/01/29 18:10:45, Mattias Nissler wrote:
> Do we need the weak_ptr_factory_ here? We hold a references to pref_registry_,
> so base::Unretained() should work as long as you reset the callbacks on
> destruction.

That would be the alternative. Seems a bit cleaner, I've switched to that.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.cc:64: weak_ptr_factory_.GetWeakPtr()));
On 2013/01/29 18:10:45, Mattias Nissler wrote:
> So this will break if we're ever passing a registry to several PrefService
> instances (which we might do for incognito?)

Yes, which is why I used the approach you see in pref_service_syncable.cc for
the incognito PrefService.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service.h (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.h:173: friend class PrefRegistry;  // For
Add/RemoveRegisteredPreference.
On 2013/01/29 18:10:45, Mattias Nissler wrote:
> Isn't that going through the callbacks now?

Thanks, I missed that.

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:102: // any registrations.
On 2013/01/29 18:10:45, Mattias Nissler wrote:
> I understand we are intending to remove the callbacks eventually? If so, can
we
> have a TODO here saying that this can go away once the callbacks are gone?

Done.

[Jói, 2013-01-31 12:13:15]

Ping to see if you think this is looking good in general.  I'm working
on getting it through trybots, if there are significant changes I'd
like to do them ASAP.

Cheers,
Jói


On Wed, Jan 30, 2013 at 2:23 PM,  <joi@chromium.org> wrote:
> Hi Mattias,
>
> As requested, I added documentation on PrefRegistrySimple::registry().  See
> how
> other comments were addressed below.
>
> You can look at the diff from the second-to-last patch set (a pure merge to
> LKGR) and the last patch set to see just the changes I made in response to
> review feedback.
>
> Cheers,
> Jói
>
>
>
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
> File chrome/browser/prefs/browser_prefs.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
> chrome/browser/prefs/browser_prefs.cc:378:
> GoogleURLTracker::kDefaultGoogleHomepage);
> On 2013/01/29 18:10:45, Mattias Nissler wrote:
>>
>> nit: indentation
>
>
> Done.
>
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
> chrome/browser/prefs/browser_prefs.cc:386: std::string());
> On 2013/01/29 18:10:45, Mattias Nissler wrote:
>>
>> nit: indentation
>
>
> Done.
>
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
> File chrome/browser/prefs/chrome_pref_service_factory.h (right):
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
> chrome/browser/prefs/chrome_pref_service_factory.h:34: //
> |pref_registry| is the registry object to retrieve registration
> On 2013/01/29 18:10:45, Mattias Nissler wrote:
>>
>> This sentence is a bit cloudy, can we just say that |pref_registry|
>
> keeps the
>>
>> list of registered prefs and their default values?
>
>
> Done.
>
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
> chrome/browser/prefs/chrome_pref_service_factory.h:35: // information
> from; PrefService takes ownership (not used for
> On 2013/01/29 18:10:45, Mattias Nissler wrote:
>>
>> PrefRegistry is RefCounted vs. ownership transfer
>
>
> Done.
>
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
> chrome/browser/prefs/chrome_pref_service_factory.h:48: PrefRegistry*
> pref_registry,
> On 2013/01/29 18:10:45, Mattias Nissler wrote:
>>
>> Isn't this now refcounted? I think the rule is to pass the pointer
>
> wrapped in a
>>
>> scoped_refptr then.
>
>
> Done.
>
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_notifier_impl_unittest.cc:98: true);
> On 2013/01/29 18:10:45, Mattias Nissler wrote:
>>
>> nit: no need for line break (also below).
>
>
> Done.
>
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_registry.h (right):
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry.h:35: scoped_refptr<PrefStore>
> defaults();
> On 2013/01/29 18:10:45, Mattias Nissler wrote:
>>
>> forward-declare PrefStore?
>
>
> Done.
>
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry.h:68: scoped_refptr<DefaultPrefStore>
> defaults_;
> On 2013/01/29 18:10:45, Mattias Nissler wrote:
>>
>> could be forward-declared?
>
>
> Done.
>
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_registry_simple.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry_simple.cc:16:
> RegisterPreference(path, Value::CreateBooleanValue(default_value));
> On 2013/01/29 18:10:45, Mattias Nissler wrote:
>>
>> nit: namespace-qualify base::Value (here and elsewhere in the file)
>
>
> Done.
>
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_registry_simple.h (right):
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry_simple.h:33: const char* path,
> base::DictionaryValue* default_value);
> On 2013/01/29 18:10:45, Mattias Nissler wrote:
>>
>> nit: parameters on separate lines
>
>
> Done.
>
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_service.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service.cc:61:
> weak_ptr_factory_.GetWeakPtr()));
> On 2013/01/29 18:10:45, Mattias Nissler wrote:
>>
>> Do we need the weak_ptr_factory_ here? We hold a references to
>
> pref_registry_,
>>
>> so base::Unretained() should work as long as you reset the callbacks
>
> on
>>
>> destruction.
>
>
> That would be the alternative. Seems a bit cleaner, I've switched to
> that.
>
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service.cc:64:
> weak_ptr_factory_.GetWeakPtr()));
> On 2013/01/29 18:10:45, Mattias Nissler wrote:
>>
>> So this will break if we're ever passing a registry to several
>
> PrefService
>>
>> instances (which we might do for incognito?)
>
>
> Yes, which is why I used the approach you see in
> pref_service_syncable.cc for the incognito PrefService.
>
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_service.h (right):
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service.h:173: friend class PrefRegistry;  //
> For Add/RemoveRegisteredPreference.
> On 2013/01/29 18:10:45, Mattias Nissler wrote:
>>
>> Isn't that going through the callbacks now?
>
>
> Thanks, I missed that.
>
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_service_syncable.cc (right):
>
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service_syncable.cc:102: // any registrations.
> On 2013/01/29 18:10:45, Mattias Nissler wrote:
>>
>> I understand we are intending to remove the callbacks eventually? If
>
> so, can we
>>
>> have a TODO here saying that this can go away once the callbacks are
>
> gone?
>
> Done.
>
> https://codereview.chromium.org/11741003/

[Mattias Nissler (ping if slow), 2013-01-31 12:20:13]

I'm pending a full pass to the CL - will do that today. In general, I'm
happy with the change.

On Thu, Jan 31, 2013 at 1:12 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> Ping to see if you think this is looking good in general.  I'm working
> on getting it through trybots, if there are significant changes I'd
> like to do them ASAP.
>
> Cheers,
> Jói
>
>
> On Wed, Jan 30, 2013 at 2:23 PM,  <joi@chromium.org> wrote:
> > Hi Mattias,
> >
> > As requested, I added documentation on PrefRegistrySimple::registry().
>  See
> > how
> > other comments were addressed below.
> >
> > You can look at the diff from the second-to-last patch set (a pure merge
> to
> > LKGR) and the last patch set to see just the changes I made in response
> to
> > review feedback.
> >
> > Cheers,
> > Jói
> >
> >
> >
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
> > File chrome/browser/prefs/browser_prefs.cc (right):
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
> > chrome/browser/prefs/browser_prefs.cc:378:
> > GoogleURLTracker::kDefaultGoogleHomepage);
> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
> >>
> >> nit: indentation
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
> > chrome/browser/prefs/browser_prefs.cc:386: std::string());
> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
> >>
> >> nit: indentation
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
> > File chrome/browser/prefs/chrome_pref_service_factory.h (right):
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
> > chrome/browser/prefs/chrome_pref_service_factory.h:34: //
> > |pref_registry| is the registry object to retrieve registration
> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
> >>
> >> This sentence is a bit cloudy, can we just say that |pref_registry|
> >
> > keeps the
> >>
> >> list of registered prefs and their default values?
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
> > chrome/browser/prefs/chrome_pref_service_factory.h:35: // information
> > from; PrefService takes ownership (not used for
> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
> >>
> >> PrefRegistry is RefCounted vs. ownership transfer
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
> > chrome/browser/prefs/chrome_pref_service_factory.h:48: PrefRegistry*
> > pref_registry,
> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
> >>
> >> Isn't this now refcounted? I think the rule is to pass the pointer
> >
> > wrapped in a
> >>
> >> scoped_refptr then.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > chrome/browser/prefs/pref_notifier_impl_unittest.cc:98: true);
> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
> >>
> >> nit: no need for line break (also below).
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > File chrome/browser/prefs/pref_registry.h (right):
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > chrome/browser/prefs/pref_registry.h:35: scoped_refptr<PrefStore>
> > defaults();
> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
> >>
> >> forward-declare PrefStore?
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > chrome/browser/prefs/pref_registry.h:68: scoped_refptr<DefaultPrefStore>
> > defaults_;
> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
> >>
> >> could be forward-declared?
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > File chrome/browser/prefs/pref_registry_simple.cc (right):
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > chrome/browser/prefs/pref_registry_simple.cc:16:
> > RegisterPreference(path, Value::CreateBooleanValue(default_value));
> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
> >>
> >> nit: namespace-qualify base::Value (here and elsewhere in the file)
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > File chrome/browser/prefs/pref_registry_simple.h (right):
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > chrome/browser/prefs/pref_registry_simple.h:33: const char* path,
> > base::DictionaryValue* default_value);
> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
> >>
> >> nit: parameters on separate lines
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > File chrome/browser/prefs/pref_service.cc (right):
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > chrome/browser/prefs/pref_service.cc:61:
> > weak_ptr_factory_.GetWeakPtr()));
> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
> >>
> >> Do we need the weak_ptr_factory_ here? We hold a references to
> >
> > pref_registry_,
> >>
> >> so base::Unretained() should work as long as you reset the callbacks
> >
> > on
> >>
> >> destruction.
> >
> >
> > That would be the alternative. Seems a bit cleaner, I've switched to
> > that.
> >
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > chrome/browser/prefs/pref_service.cc:64:
> > weak_ptr_factory_.GetWeakPtr()));
> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
> >>
> >> So this will break if we're ever passing a registry to several
> >
> > PrefService
> >>
> >> instances (which we might do for incognito?)
> >
> >
> > Yes, which is why I used the approach you see in
> > pref_service_syncable.cc for the incognito PrefService.
> >
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > File chrome/browser/prefs/pref_service.h (right):
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > chrome/browser/prefs/pref_service.h:173: friend class PrefRegistry;  //
> > For Add/RemoveRegisteredPreference.
> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
> >>
> >> Isn't that going through the callbacks now?
> >
> >
> > Thanks, I missed that.
> >
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > File chrome/browser/prefs/pref_service_syncable.cc (right):
> >
> >
>
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
> > chrome/browser/prefs/pref_service_syncable.cc:102: // any registrations.
> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
> >>
> >> I understand we are intending to remove the callbacks eventually? If
> >
> > so, can we
> >>
> >> have a TODO here saying that this can go away once the callbacks are
> >
> > gone?
> >
> > Done.
> >
> > https://codereview.chromium.org/11741003/
>

[Jói, 2013-01-31 12:32:27]

Great, thanks Mattias.  I'll upload a new patch once it's passing all
trybots, but I expect only very tiny changes.

On Thu, Jan 31, 2013 at 12:19 PM, Mattias Nissler <mnissler@chromium.org>
wrote:
> I'm pending a full pass to the CL - will do that today. In general, I'm
> happy with the change.
>
>
> On Thu, Jan 31, 2013 at 1:12 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> Ping to see if you think this is looking good in general.  I'm working
>> on getting it through trybots, if there are significant changes I'd
>> like to do them ASAP.
>>
>> Cheers,
>> Jói
>>
>>
>> On Wed, Jan 30, 2013 at 2:23 PM,  <joi@chromium.org> wrote:
>> > Hi Mattias,
>> >
>> > As requested, I added documentation on PrefRegistrySimple::registry().
>> > See
>> > how
>> > other comments were addressed below.
>> >
>> > You can look at the diff from the second-to-last patch set (a pure merge
>> > to
>> > LKGR) and the last patch set to see just the changes I made in response
>> > to
>> > review feedback.
>> >
>> > Cheers,
>> > Jói
>> >
>> >
>> >
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
>> > File chrome/browser/prefs/browser_prefs.cc (right):
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
>> > chrome/browser/prefs/browser_prefs.cc:378:
>> > GoogleURLTracker::kDefaultGoogleHomepage);
>> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
>> >>
>> >> nit: indentation
>> >
>> >
>> > Done.
>> >
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/brow...
>> > chrome/browser/prefs/browser_prefs.cc:386: std::string());
>> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
>> >>
>> >> nit: indentation
>> >
>> >
>> > Done.
>> >
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
>> > File chrome/browser/prefs/chrome_pref_service_factory.h (right):
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
>> > chrome/browser/prefs/chrome_pref_service_factory.h:34: //
>> > |pref_registry| is the registry object to retrieve registration
>> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
>> >>
>> >> This sentence is a bit cloudy, can we just say that |pref_registry|
>> >
>> > keeps the
>> >>
>> >> list of registered prefs and their default values?
>> >
>> >
>> > Done.
>> >
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
>> > chrome/browser/prefs/chrome_pref_service_factory.h:35: // information
>> > from; PrefService takes ownership (not used for
>> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
>> >>
>> >> PrefRegistry is RefCounted vs. ownership transfer
>> >
>> >
>> > Done.
>> >
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/chro...
>> > chrome/browser/prefs/chrome_pref_service_factory.h:48: PrefRegistry*
>> > pref_registry,
>> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
>> >>
>> >> Isn't this now refcounted? I think the rule is to pass the pointer
>> >
>> > wrapped in a
>> >>
>> >> scoped_refptr then.
>> >
>> >
>> > Done.
>> >
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > chrome/browser/prefs/pref_notifier_impl_unittest.cc:98: true);
>> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
>> >>
>> >> nit: no need for line break (also below).
>> >
>> >
>> > Done.
>> >
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > File chrome/browser/prefs/pref_registry.h (right):
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > chrome/browser/prefs/pref_registry.h:35: scoped_refptr<PrefStore>
>> > defaults();
>> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
>> >>
>> >> forward-declare PrefStore?
>> >
>> >
>> > Done.
>> >
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > chrome/browser/prefs/pref_registry.h:68: scoped_refptr<DefaultPrefStore>
>> > defaults_;
>> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
>> >>
>> >> could be forward-declared?
>> >
>> >
>> > Done.
>> >
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > File chrome/browser/prefs/pref_registry_simple.cc (right):
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > chrome/browser/prefs/pref_registry_simple.cc:16:
>> > RegisterPreference(path, Value::CreateBooleanValue(default_value));
>> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
>> >>
>> >> nit: namespace-qualify base::Value (here and elsewhere in the file)
>> >
>> >
>> > Done.
>> >
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > File chrome/browser/prefs/pref_registry_simple.h (right):
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > chrome/browser/prefs/pref_registry_simple.h:33: const char* path,
>> > base::DictionaryValue* default_value);
>> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
>> >>
>> >> nit: parameters on separate lines
>> >
>> >
>> > Done.
>> >
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > File chrome/browser/prefs/pref_service.cc (right):
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > chrome/browser/prefs/pref_service.cc:61:
>> > weak_ptr_factory_.GetWeakPtr()));
>> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
>> >>
>> >> Do we need the weak_ptr_factory_ here? We hold a references to
>> >
>> > pref_registry_,
>> >>
>> >> so base::Unretained() should work as long as you reset the callbacks
>> >
>> > on
>> >>
>> >> destruction.
>> >
>> >
>> > That would be the alternative. Seems a bit cleaner, I've switched to
>> > that.
>> >
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > chrome/browser/prefs/pref_service.cc:64:
>> > weak_ptr_factory_.GetWeakPtr()));
>> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
>> >>
>> >> So this will break if we're ever passing a registry to several
>> >
>> > PrefService
>> >>
>> >> instances (which we might do for incognito?)
>> >
>> >
>> > Yes, which is why I used the approach you see in
>> > pref_service_syncable.cc for the incognito PrefService.
>> >
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > File chrome/browser/prefs/pref_service.h (right):
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > chrome/browser/prefs/pref_service.h:173: friend class PrefRegistry;  //
>> > For Add/RemoveRegisteredPreference.
>> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
>> >>
>> >> Isn't that going through the callbacks now?
>> >
>> >
>> > Thanks, I missed that.
>> >
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > File chrome/browser/prefs/pref_service_syncable.cc (right):
>> >
>> >
>> >
https://codereview.chromium.org/11741003/diff/39002/chrome/browser/prefs/pref...
>> > chrome/browser/prefs/pref_service_syncable.cc:102: // any registrations.
>> > On 2013/01/29 18:10:45, Mattias Nissler wrote:
>> >>
>> >> I understand we are intending to remove the callbacks eventually? If
>> >
>> > so, can we
>> >>
>> >> have a TODO here saying that this can go away once the callbacks are
>> >
>> > gone?
>> >
>> > Done.
>> >
>> > https://codereview.chromium.org/11741003/
>
>

[Mattias Nissler (ping if slow), 2013-01-31 13:06:25]

First half of the CL, I'll do the other half later.

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/browser_sh...
File chrome/browser/browser_shutdown.cc (right):

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/browser_sh...
chrome/browser/browser_shutdown.cc:81: void RegisterPrefs(PrefRegistrySimple*
local_state) {
I could nit here (and elsewhere) that the parameter name should be |registry|,
but I don't care much. Up to you.

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/chromeos/a...
File chrome/browser/chromeos/audio/audio_handler.cc (right):

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_handler.cc:21: #include
"chrome/browser/prefs/pref_service.h"
required?

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/external_p...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/external_p...
chrome/browser/external_protocol/external_protocol_handler.cc:18: #include
"chrome/browser/prefs/pref_registry_simple.h"
once is enough :)

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/metrics/va...
chrome/browser/metrics/variations/variations_service.cc:203: prefs
remove accidental newline.

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/net/pref_p...
File chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc (right):

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/net/pref_p...
chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc:81: virtual void
Init(PrefService* pref_service) {
Just change the parameter type to TestingPrefServiceSimple so you don't have to
do the cast.

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/notificati...
File chrome/browser/notifications/notification_prefs_manager.cc (right):

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/notificati...
chrome/browser/notifications/notification_prefs_manager.cc:16: #if
defined(OS_CHROMEOS)
Add a TODO saying that we shouldn't do this here?

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:152: PrefService* local_state) {
TODO here saying to remove the local_state parameter?

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:335: PrefRegistrySimple* registry =
static_cast<PrefRegistrySimple*>(
TODO here saying the registrations should happen elsewhere?

[Jói, 2013-01-31 13:43:42]

Thanks Mattias.  I've addressed all of your comments so far.

Also, I fixed an issue in PrefService::AddRegisteredPreference where the CHECK
on FindPreference would always trigger, since it was being done after the
preference had been added to the registry.  I now do the check in
PrefRegistry::RegisterPreference instead, where it belongs.

Apart from that, a tiny compile fix, there was a missing virtual on destructor
of PrefRegistrySimple since it derives from PrefRegistry which has a virtual
destructor.

Cheers,
Jói

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/browser_sh...
File chrome/browser/browser_shutdown.cc (right):

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/browser_sh...
chrome/browser/browser_shutdown.cc:81: void RegisterPrefs(PrefRegistrySimple*
local_state) {
On 2013/01/31 13:06:25, Mattias Nissler wrote:
> I could nit here (and elsewhere) that the parameter name should be |registry|,
> but I don't care much. Up to you.

I was thinking I might leave it, but nicer to do it right :)  Fixed.

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/chromeos/a...
File chrome/browser/chromeos/audio/audio_handler.cc (right):

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_handler.cc:21: #include
"chrome/browser/prefs/pref_service.h"
On 2013/01/31 13:06:25, Mattias Nissler wrote:
> required?

Removed.

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/external_p...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/external_p...
chrome/browser/external_protocol/external_protocol_handler.cc:18: #include
"chrome/browser/prefs/pref_registry_simple.h"
On 2013/01/31 13:06:25, Mattias Nissler wrote:
> once is enough :)

Done.

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/metrics/va...
chrome/browser/metrics/variations/variations_service.cc:203: prefs
On 2013/01/31 13:06:25, Mattias Nissler wrote:
> remove accidental newline.

Done. Found a couple of other places like this while renaming params to registry
and fixed those too.

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/net/pref_p...
File chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc (right):

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/net/pref_p...
chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc:81: virtual void
Init(PrefService* pref_service) {
On 2013/01/31 13:06:25, Mattias Nissler wrote:
> Just change the parameter type to TestingPrefServiceSimple so you don't have
to
> do the cast.

Done.

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/notificati...
File chrome/browser/notifications/notification_prefs_manager.cc (right):

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/notificati...
chrome/browser/notifications/notification_prefs_manager.cc:16: #if
defined(OS_CHROMEOS)
On 2013/01/31 13:06:25, Mattias Nissler wrote:
> Add a TODO saying that we shouldn't do this here?

Done.

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/11741003/diff/50002/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:152: PrefService* local_state) {
On 2013/01/31 13:06:25, Mattias Nissler wrote:
> TODO here saying to remove the local_state parameter?

Done.

[Mattias Nissler (ping if slow), 2013-01-31 14:50:44]

Made it through with a couple more nits. You have my tentative LGTM, in case
it's only nits trivial compile fixes that remain (the bots seem red).

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.cc:80: pref_notifier_.reset();
Let's reset the pref_registry_ pointer here as well.

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service.h (left):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.h:30: class PrefStore;
Technically we should keep this forward declaration since the headers still uses
the type.

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_builder.h (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_builder.h:11: #include
"base/memory/scoped_ptr.h"
not needed I guess?

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager_factory.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager_factory.cc:8: #include
"chrome/browser/prefs/pref_service.h"
needed?

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/task_manag...
File chrome/browser/task_manager/task_manager.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/task_manag...
chrome/browser/task_manager/task_manager.cc:25: #include
"chrome/browser/prefs/pref_service.h"
needed?

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/browser...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/browser...
chrome/browser/ui/browser_view_prefs.cc:8: #include
"chrome/browser/prefs/pref_service.h"
required?

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/cocoa/c...
File chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/cocoa/c...
chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm:16: #include
"chrome/browser/prefs/pref_service.h"
required?

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/search_...
File chrome/browser/ui/search_engines/keyword_editor_controller.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/search_...
chrome/browser/ui/search_engines/keyword_editor_controller.cc:9: #include
"chrome/browser/prefs/pref_service.h"
required?

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/tabs/ta...
File chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/tabs/ta...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:8: #include
"chrome/browser/prefs/pref_service.h"
required?

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/flags_ui.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.cc:18: #include
"chrome/browser/prefs/pref_service.h"
required?

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/upgrade_de...
File chrome/browser/upgrade_detector.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/upgrade_de...
chrome/browser/upgrade_detector.cc:11: #include
"chrome/browser/prefs/pref_service.h"
required?

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/web_resour...
File chrome/browser/web_resource/promo_resource_service.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/web_resour...
chrome/browser/web_resource/promo_resource_service.cc:14: #include
"chrome/browser/prefs/pref_service.h"
required?

https://codereview.chromium.org/11741003/diff/47006/chrome/test/base/testing_...
File chrome/test/base/testing_pref_service.h (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/test/base/testing_...
chrome/test/base/testing_pref_service.h:87: // This is provided as a
convenience; on a production PrefService
maybe elaborate: "as a convenience for registering preferences on an existing
TestingPrefServiceSimple instance"

https://codereview.chromium.org/11741003/diff/47006/chrome/test/reliability/p...
File chrome/test/reliability/page_load_test.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/test/reliability/p...
chrome/test/reliability/page_load_test.cc:764:
local_state->DeprecatedGetPrefRegistry());
I think we should convert this code to use a pre-initialized registry actually,
because it cannot use TestingPrefServiceSimple AFAICS.

https://codereview.chromium.org/11741003/diff/47006/chrome_frame/test/reliabi...
File chrome_frame/test/reliability/page_load_test.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome_frame/test/reliabi...
chrome_frame/test/reliability/page_load_test.cc:450:
local_state->DeprecatedGetPrefRegistry());
Same here for the copied code ;)

[Jói, 2013-01-31 16:23:36]

Thanks Mattias.

I've addressed all your comments, and I believe I've also sorted out the tests
that were failing (except I haven't seen test results on Windows yet, there was
a compile issue there that I've fixed).  Started another round of tryjobs just
now.

Note that I added a comment on pref_proxy_config_tracker_impl_unittest.cc.

Cheers,
Jói

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/net/pref_p...
File chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/net/pref_p...
chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc:81: virtual void
Init(TestingPrefServiceSimple* pref_service) {
Actually, this wasn't so simple because
PrefProxyConfigTrackerImplCommandLineTest needs to create a real PrefService to
initialize this class. Switched to taking PrefService* and PrefRegistrySimple*
as parameters here, and casting only where needed.

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.cc:80: pref_notifier_.reset();
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> Let's reset the pref_registry_ pointer here as well.

Done.

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service.h (left):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service.h:30: class PrefStore;
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> Technically we should keep this forward declaration since the headers still
uses
> the type.

Done.

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_builder.h (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_builder.h:11: #include
"base/memory/scoped_ptr.h"
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> not needed I guess?

Done.

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager_factory.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager_factory.cc:8: #include
"chrome/browser/prefs/pref_service.h"
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> needed?

Done.

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/task_manag...
File chrome/browser/task_manager/task_manager.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/task_manag...
chrome/browser/task_manager/task_manager.cc:25: #include
"chrome/browser/prefs/pref_service.h"
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> needed?

Done.

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/browser...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/browser...
chrome/browser/ui/browser_view_prefs.cc:8: #include
"chrome/browser/prefs/pref_service.h"
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> required?

Done.

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/search_...
File chrome/browser/ui/search_engines/keyword_editor_controller.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/search_...
chrome/browser/ui/search_engines/keyword_editor_controller.cc:9: #include
"chrome/browser/prefs/pref_service.h"
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> required?

Done.

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/tabs/ta...
File chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/tabs/ta...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:8: #include
"chrome/browser/prefs/pref_service.h"
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> required?

Done.

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/flags_ui.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.cc:18: #include
"chrome/browser/prefs/pref_service.h"
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> required?

Done.

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/upgrade_de...
File chrome/browser/upgrade_detector.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/upgrade_de...
chrome/browser/upgrade_detector.cc:11: #include
"chrome/browser/prefs/pref_service.h"
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> required?

Done.

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/web_resour...
File chrome/browser/web_resource/promo_resource_service.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/web_resour...
chrome/browser/web_resource/promo_resource_service.cc:14: #include
"chrome/browser/prefs/pref_service.h"
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> required?

Done.

https://codereview.chromium.org/11741003/diff/47006/chrome/test/base/testing_...
File chrome/test/base/testing_pref_service.h (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/test/base/testing_...
chrome/test/base/testing_pref_service.h:87: // This is provided as a
convenience; on a production PrefService
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> maybe elaborate: "as a convenience for registering preferences on an existing
> TestingPrefServiceSimple instance"

Done.

https://codereview.chromium.org/11741003/diff/47006/chrome/test/reliability/p...
File chrome/test/reliability/page_load_test.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/test/reliability/p...
chrome/test/reliability/page_load_test.cc:764:
local_state->DeprecatedGetPrefRegistry());
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> I think we should convert this code to use a pre-initialized registry
actually,
> because it cannot use TestingPrefServiceSimple AFAICS.

Done.

https://codereview.chromium.org/11741003/diff/47006/chrome_frame/test/reliabi...
File chrome_frame/test/reliability/page_load_test.cc (right):

https://codereview.chromium.org/11741003/diff/47006/chrome_frame/test/reliabi...
chrome_frame/test/reliability/page_load_test.cc:450:
local_state->DeprecatedGetPrefRegistry());
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> Same here for the copied code ;)

Done.

[Mattias Nissler (ping if slow), 2013-01-31 16:30:54]

LGTM with a suggestion how to avoid a downcast :)

https://codereview.chromium.org/11741003/diff/47019/chrome/browser/net/pref_p...
File chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc (right):

https://codereview.chromium.org/11741003/diff/47019/chrome/browser/net/pref_p...
chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc:344:
&command_line_).Create(new PrefRegistrySimple));
In this case, I'd rather create a PrefRegistrySimple up front, move the call to
PrefProxyConfigTrackerImpl::RegisterPrefs() out of Init() and pass just the
PrefService* to Init(). Note that the other call site is simpler, you can just
call RegisterPrefs on pref_service_->registry() there.

[Jói, 2013-01-31 17:29:32]

+ben for OWNERS stamp. Mattias has reviewed the full change, and the
modifications outside of the Prefs code are trivial usage updates.

Cheers,
Jói

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/cocoa/c...
File chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm (right):

https://codereview.chromium.org/11741003/diff/47006/chrome/browser/ui/cocoa/c...
chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm:16: #include
"chrome/browser/prefs/pref_service.h"
On 2013/01/31 14:50:45, Mattias Nissler wrote:
> required?

Done.

https://codereview.chromium.org/11741003/diff/47019/chrome/browser/net/pref_p...
File chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc (right):

https://codereview.chromium.org/11741003/diff/47019/chrome/browser/net/pref_p...
chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc:344:
&command_line_).Create(new PrefRegistrySimple));
On 2013/01/31 16:30:54, Mattias Nissler wrote:
> In this case, I'd rather create a PrefRegistrySimple up front, move the call
to
> PrefProxyConfigTrackerImpl::RegisterPrefs() out of Init() and pass just the
> PrefService* to Init(). Note that the other call site is simpler, you can just
> call RegisterPrefs on pref_service_->registry() there.

Done.

[commit-bot: I haz the power, 2013-01-31 17:31:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11741003/36192

[commit-bot: I haz the power, 2013-01-31 18:06:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Ben Goodger (Google), 2013-01-31 21:52:07]

lgtm

[commit-bot: I haz the power, 2013-02-01 14:34:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11741003/37183

[commit-bot: I haz the power, 2013-02-01 17:46:31]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2013-02-01 20:01:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11741003/37183

[commit-bot: I haz the power, 2013-02-04 12:37:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11741003/98001

[commit-bot: I haz the power, 2013-02-04 13:12:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11741003/92005

[commit-bot: I haz the power, 2013-02-04 13:29:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11741003/104001

[commit-bot: I haz the power, 2013-02-04 14:09:40]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-04 14:12:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11741003/89008

[commit-bot: I haz the power, 2013-02-04 17:09:31]

Retried try job too often on android_dbg_triggered_tests for step(s) build
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-02-05 15:03:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11741003/108004

[tfarina, 2013-02-05 16:55:16]

Joi, is the CL description up to date?

Is this still right: "replacing it with PrefService "?

I see PrefRegistry being added.

[Jói, 2013-02-05 16:57:59]

Good catch, thanks Thiago. I've updated the description to the following:

=== cut here ===
Remove PrefServiceSimple, replacing it with PrefService and PrefRegistrySimple.

This is a step towards removing registration methods from PrefService
and simplifying its class hierarchy.

TBR=ben@chromium.org
BUG=155525
=== cut here ===

Cheers,
Jói


On Tue, Feb 5, 2013 at 4:55 PM,  <tfarina@chromium.org> wrote:
> Joi, is the CL description up to date?
>
> Is this still right: "replacing it with PrefService "?
>
> I see PrefRegistry being added.
>
> https://codereview.chromium.org/11741003/

[commit-bot: I haz the power, 2013-02-05 18:44:14]

Retried try job too often on win_rel for step(s) chrome_frame_net_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-02-05 19:18:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11741003/108004

[commit-bot: I haz the power, 2013-02-05 22:51:57]

Retried try job too often on win_rel for step(s) chrome_frame_net_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-02-05 23:18:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11741003/121009

[commit-bot: I haz the power, 2013-02-06 02:45:51]

Change committed as 180873