Remove content::NotificationObserver dependency from most Prefs code.

Remove content::NotificationObserver dependency from most Prefs code.

Instead of using content::NotificationObserver, introduce specific
type-safe observer classes and update users to use them.  In a very
large number of cases this was the users' only reason for being a
content::NotificationObserver and they would have a lot of
boiler-plate code such as a DCHECK on the notification type and
unpacking of the generic NotificationDetails types, so this change
removes a bunch of boilerplate and introduces more type safety.

This is part of enabling more of the Prefs code to live in
base/prefs/.

TBR=ben@chromium.org,brettw@chromium.org
BUG=155525
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165414

[Jói, 2012-10-29 21:19:22]

Hi Mattias,

This one is pretty big, but most of it can be split up between a lot of people. 
Would you review the stuff that's under:
- base/prefs/,
- chrome/common/, and
- chrome/browser/prefs?

That is the meat of the change; the rest is updates to usage which I will ask
the respective OWNERS to review once you've OKed the basic change.

If you have time to review several of the usage changes just to see what they
look like and comment, that would be helpful as well.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2012-10-30 09:09:29]

One quick comment I want to get out before we dive into details :)

http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observer.h
File base/prefs/public/pref_observer.h (right):

http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observe...
base/prefs/public/pref_observer.h:15: const std::string& pref_name) = 0;
Any reason to not use base::Callback<void(PrefServiceBase*, const std::string&)
for this purpose? I guess this would also allow for reducing lots of complexity
in the consumers, because they can drop switch(pref_name)-style code and have
dedicated handlers for the different events.

[Jói, 2012-10-30 09:19:00]

> Any reason to not use base::Callback<void(PrefServiceBase*, const
> std::string&) for this purpose?

Having a dedicated delegate feels cleaner, as it creates an is-a
relationship for classes that are an observer.  Using a callback would
mean a Callback member variable, which is less obviously an is-a.  The
importance of the is-a relationship is increased because some class
hierarchies have multiple chained implementations of
OnPreferenceChanged, so being able to use polymorphism to find the
right implementation (that then chains back to the superclasses) is
good.

> I guess this would also allow for
> reducing lots of complexity in the consumers, because they can drop
> switch(pref_name)-style code and have dedicated handlers for the
> different events.

That would be possible if using callbacks, but at the expense of
harder chaining etc.  I think on the whole it would be more complex,
not less.

Cheers,
Jói



On Tue, Oct 30, 2012 at 9:09 AM,  <mnissler@chromium.org> wrote:
> One quick comment I want to get out before we dive into details :)
>
>
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observer.h
> File base/prefs/public/pref_observer.h (right):
>
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observe...
> base/prefs/public/pref_observer.h:15: const std::string& pref_name) = 0;
> Any reason to not use base::Callback<void(PrefServiceBase*, const
> std::string&) for this purpose? I guess this would also allow for
> reducing lots of complexity in the consumers, because they can drop
> switch(pref_name)-style code and have dedicated handlers for the
> different events.
>
> http://codereview.chromium.org/11345008/

[Mattias Nissler (ping if slow), 2012-10-30 09:33:04]

On 2012/10/30 09:19:00, Jói wrote:
> > Any reason to not use base::Callback<void(PrefServiceBase*, const
> > std::string&) for this purpose?
> 
> Having a dedicated delegate feels cleaner, as it creates an is-a
> relationship for classes that are an observer.  Using a callback would
> mean a Callback member variable, which is less obviously an is-a.  The
> importance of the is-a relationship is increased because some class
> hierarchies have multiple chained implementations of
> OnPreferenceChanged, so being able to use polymorphism to find the
> right implementation (that then chains back to the superclasses) is
> good.

You're assuming that the current chaining is inherent to the design of these
classes. Can you point me to a few examples that are complex? My hunch is that
the chaining only exists because there is only one way to receive PREF_CHANGED
notifications, and the implementations have to demultiplex.

Note that chaining can easily be accomplished by declaring handlers:

virtual void OnBananaPrefChanged();

That has the advantage that you can easily see in the class declaration what
events are relevant to the class, whereas I've found myself studying
NotificationObserver::Observe() implementations numerous times to find out why
the class is interested in pref notifications at all and what pref-triggered
events it reacts to.

Bottom line: My feeling is that the existing complexity is largely due to the
awkward interface, and my hope is that we could get rid of a significant amount
of complexity using callback-based handlers.

> 
> > I guess this would also allow for
> > reducing lots of complexity in the consumers, because they can drop
> > switch(pref_name)-style code and have dedicated handlers for the
> > different events.
> 
> That would be possible if using callbacks, but at the expense of
> harder chaining etc.  I think on the whole it would be more complex,
> not less.

I guess we're at the point where we need more data to make an informed decision.
Do you have a couple of complex examples you encountered that we could try with
either approach to test the waters?

One additional caveat with base::Callback-style notifications I hadn't thought
of is observer unregistration. base::Callback instances are often constructed on
the fly and passed by value, so it's not immediately obvious to me how to remove
previously-registered callback. One option I can see is forcing everybody to use
PrefChangeRegistrar and have it keep a handle that can be used to unregister the
callback later. WDYT?

> 
> Cheers,
> Jói
> 
> 
> 
> On Tue, Oct 30, 2012 at 9:09 AM,  <mailto:mnissler@chromium.org> wrote:
> > One quick comment I want to get out before we dive into details :)
> >
> >
> >
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observer.h
> > File base/prefs/public/pref_observer.h (right):
> >
> >
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observe...
> > base/prefs/public/pref_observer.h:15: const std::string& pref_name) = 0;
> > Any reason to not use base::Callback<void(PrefServiceBase*, const
> > std::string&) for this purpose? I guess this would also allow for
> > reducing lots of complexity in the consumers, because they can drop
> > switch(pref_name)-style code and have dedicated handlers for the
> > different events.
> >
> > http://codereview.chromium.org/11345008/

[Jói, 2012-10-30 09:47:35]

I would suggest doing this:
a) Leave the PrefObserver interface in place.
b) Have an implementation of it that lets you keep multiple
PrefObserverImpl objects (which are effectively just like callbacks)
and bind them to different methods on your class.

This gives you the best of both worlds.  In many cases, you only care
that one or more prefs changed and you don't even check their values.
For this case, it is much simpler to inherit from PrefObserver and
register using PrefChangeRegistrar, and override a single method.
Everything typesafe, no extra registration work, no new member
variable to add, etc.

In cases where users would prefer to have pref changes dispatched to
separate methods, they can create multiple PrefObserverImpl members
that bind to the methods they want to dispatch to, and use
PrefChangeRegistrar to register these members instead of registering
the 'this' pointer.

This also gets rid of the problem you mentioned about removing
previously-registered callbacks; in this case the things would have to
be members, not constructed on the fly, so deregistration works just
like it does today.

Another advantage is, this lets us convert usages one by one to the
dispatch-oriented (multiple OnXyzPrefChanged methods) approach,
instead of having to do it in one fell swoop.

I'm working on answering your question about inheritance hierarchies;
there were just a few and I'm trying to remember where they were.

Cheers,
Jói



On Tue, Oct 30, 2012 at 9:33 AM,  <mnissler@chromium.org> wrote:
> On 2012/10/30 09:19:00, Jói wrote:
>>
>> > Any reason to not use base::Callback<void(PrefServiceBase*, const
>> > std::string&) for this purpose?
>
>
>> Having a dedicated delegate feels cleaner, as it creates an is-a
>> relationship for classes that are an observer.  Using a callback would
>> mean a Callback member variable, which is less obviously an is-a.  The
>> importance of the is-a relationship is increased because some class
>> hierarchies have multiple chained implementations of
>> OnPreferenceChanged, so being able to use polymorphism to find the
>> right implementation (that then chains back to the superclasses) is
>> good.
>
>
> You're assuming that the current chaining is inherent to the design of these
> classes. Can you point me to a few examples that are complex? My hunch is
> that
> the chaining only exists because there is only one way to receive
> PREF_CHANGED
> notifications, and the implementations have to demultiplex.
>
> Note that chaining can easily be accomplished by declaring handlers:
>
> virtual void OnBananaPrefChanged();
>
> That has the advantage that you can easily see in the class declaration what
> events are relevant to the class, whereas I've found myself studying
> NotificationObserver::Observe() implementations numerous times to find out
> why
> the class is interested in pref notifications at all and what pref-triggered
> events it reacts to.
>
> Bottom line: My feeling is that the existing complexity is largely due to
> the
> awkward interface, and my hope is that we could get rid of a significant
> amount
> of complexity using callback-based handlers.
>
>
>
>> > I guess this would also allow for
>> > reducing lots of complexity in the consumers, because they can drop
>> > switch(pref_name)-style code and have dedicated handlers for the
>> > different events.
>
>
>> That would be possible if using callbacks, but at the expense of
>> harder chaining etc.  I think on the whole it would be more complex,
>> not less.
>
>
> I guess we're at the point where we need more data to make an informed
> decision.
> Do you have a couple of complex examples you encountered that we could try
> with
> either approach to test the waters?
>
> One additional caveat with base::Callback-style notifications I hadn't
> thought
> of is observer unregistration. base::Callback instances are often
> constructed on
> the fly and passed by value, so it's not immediately obvious to me how to
> remove
> previously-registered callback. One option I can see is forcing everybody to
> use
> PrefChangeRegistrar and have it keep a handle that can be used to unregister
> the
> callback later. WDYT?
>
>
>> Cheers,
>> Jói
>
>
>
>
>> On Tue, Oct 30, 2012 at 9:09 AM,  <mailto:mnissler@chromium.org> wrote:
>> > One quick comment I want to get out before we dive into details :)
>> >
>> >
>> >
>
>
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observer.h
>>
>> > File base/prefs/public/pref_observer.h (right):
>> >
>> >
>
>
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observe...
>>
>> > base/prefs/public/pref_observer.h:15: const std::string& pref_name) = 0;
>> > Any reason to not use base::Callback<void(PrefServiceBase*, const
>> > std::string&) for this purpose? I guess this would also allow for
>> > reducing lots of complexity in the consumers, because they can drop
>> > switch(pref_name)-style code and have dedicated handlers for the
>> > different events.
>> >
>> > http://codereview.chromium.org/11345008/
>
>
>
> http://codereview.chromium.org/11345008/

[Jói, 2012-10-30 09:57:59]

Here are two examples that I found given a quick search; I'm not sure
if there are more:
chrome/browser/chromeos/proxy_config_service_impl.h
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h

Cheers,
Jói


On Tue, Oct 30, 2012 at 9:46 AM, Jói Sigurðsson <joi@chromium.org> wrote:
> I would suggest doing this:
> a) Leave the PrefObserver interface in place.
> b) Have an implementation of it that lets you keep multiple
> PrefObserverImpl objects (which are effectively just like callbacks)
> and bind them to different methods on your class.
>
> This gives you the best of both worlds.  In many cases, you only care
> that one or more prefs changed and you don't even check their values.
> For this case, it is much simpler to inherit from PrefObserver and
> register using PrefChangeRegistrar, and override a single method.
> Everything typesafe, no extra registration work, no new member
> variable to add, etc.
>
> In cases where users would prefer to have pref changes dispatched to
> separate methods, they can create multiple PrefObserverImpl members
> that bind to the methods they want to dispatch to, and use
> PrefChangeRegistrar to register these members instead of registering
> the 'this' pointer.
>
> This also gets rid of the problem you mentioned about removing
> previously-registered callbacks; in this case the things would have to
> be members, not constructed on the fly, so deregistration works just
> like it does today.
>
> Another advantage is, this lets us convert usages one by one to the
> dispatch-oriented (multiple OnXyzPrefChanged methods) approach,
> instead of having to do it in one fell swoop.
>
> I'm working on answering your question about inheritance hierarchies;
> there were just a few and I'm trying to remember where they were.
>
> Cheers,
> Jói
>
>
>
> On Tue, Oct 30, 2012 at 9:33 AM,  <mnissler@chromium.org> wrote:
>> On 2012/10/30 09:19:00, Jói wrote:
>>>
>>> > Any reason to not use base::Callback<void(PrefServiceBase*, const
>>> > std::string&) for this purpose?
>>
>>
>>> Having a dedicated delegate feels cleaner, as it creates an is-a
>>> relationship for classes that are an observer.  Using a callback would
>>> mean a Callback member variable, which is less obviously an is-a.  The
>>> importance of the is-a relationship is increased because some class
>>> hierarchies have multiple chained implementations of
>>> OnPreferenceChanged, so being able to use polymorphism to find the
>>> right implementation (that then chains back to the superclasses) is
>>> good.
>>
>>
>> You're assuming that the current chaining is inherent to the design of these
>> classes. Can you point me to a few examples that are complex? My hunch is
>> that
>> the chaining only exists because there is only one way to receive
>> PREF_CHANGED
>> notifications, and the implementations have to demultiplex.
>>
>> Note that chaining can easily be accomplished by declaring handlers:
>>
>> virtual void OnBananaPrefChanged();
>>
>> That has the advantage that you can easily see in the class declaration what
>> events are relevant to the class, whereas I've found myself studying
>> NotificationObserver::Observe() implementations numerous times to find out
>> why
>> the class is interested in pref notifications at all and what pref-triggered
>> events it reacts to.
>>
>> Bottom line: My feeling is that the existing complexity is largely due to
>> the
>> awkward interface, and my hope is that we could get rid of a significant
>> amount
>> of complexity using callback-based handlers.
>>
>>
>>
>>> > I guess this would also allow for
>>> > reducing lots of complexity in the consumers, because they can drop
>>> > switch(pref_name)-style code and have dedicated handlers for the
>>> > different events.
>>
>>
>>> That would be possible if using callbacks, but at the expense of
>>> harder chaining etc.  I think on the whole it would be more complex,
>>> not less.
>>
>>
>> I guess we're at the point where we need more data to make an informed
>> decision.
>> Do you have a couple of complex examples you encountered that we could try
>> with
>> either approach to test the waters?
>>
>> One additional caveat with base::Callback-style notifications I hadn't
>> thought
>> of is observer unregistration. base::Callback instances are often
>> constructed on
>> the fly and passed by value, so it's not immediately obvious to me how to
>> remove
>> previously-registered callback. One option I can see is forcing everybody to
>> use
>> PrefChangeRegistrar and have it keep a handle that can be used to unregister
>> the
>> callback later. WDYT?
>>
>>
>>> Cheers,
>>> Jói
>>
>>
>>
>>
>>> On Tue, Oct 30, 2012 at 9:09 AM,  <mailto:mnissler@chromium.org> wrote:
>>> > One quick comment I want to get out before we dive into details :)
>>> >
>>> >
>>> >
>>
>>
>>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observer.h
>>>
>>> > File base/prefs/public/pref_observer.h (right):
>>> >
>>> >
>>
>>
>>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observe...
>>>
>>> > base/prefs/public/pref_observer.h:15: const std::string& pref_name) = 0;
>>> > Any reason to not use base::Callback<void(PrefServiceBase*, const
>>> > std::string&) for this purpose? I guess this would also allow for
>>> > reducing lots of complexity in the consumers, because they can drop
>>> > switch(pref_name)-style code and have dedicated handlers for the
>>> > different events.
>>> >
>>> > http://codereview.chromium.org/11345008/
>>
>>
>>
>> http://codereview.chromium.org/11345008/

[Mattias Nissler (ping if slow), 2012-10-30 10:00:32]

On Tue, Oct 30, 2012 at 10:46 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> I would suggest doing this:
> a) Leave the PrefObserver interface in place.
> b) Have an implementation of it that lets you keep multiple
> PrefObserverImpl objects (which are effectively just like callbacks)
> and bind them to different methods on your class.
>
> This gives you the best of both worlds.  In many cases, you only care
> that one or more prefs changed and you don't even check their values.
> For this case, it is much simpler to inherit from PrefObserver and
> register using PrefChangeRegistrar, and override a single method.
> Everything typesafe, no extra registration work, no new member
> variable to add, etc.
>

I don't see why this would be more difficult for the base::Callback-style
approach? Let me give an example.

First, PrefObserver-style:

class PrefWrangler : public PrefObserver {
 public:
  PrefWrangler(PrefServiceBase* prefs) {
    registrar_.Init(prefs);
    registrar_.Add(prefs::kBanana, this);
  }

  // PrefObserver:
  void OnPrefChanged(PrefServiceBase*, const std::string& pref_name) {
    // Do your thing.
  }

 private:
  PrefChangeRegistrar registrar_;
};

Second, base::Callback-style:

class PrefWrangler : public PrefObserver {
 public:
  PrefWrangler(PrefServiceBase* prefs) {
    registrar_.Init(prefs);
    registrar_.Add(prefs::kBanana,
base::Bind(&PrefWrangler::ShakeTreeOnBananaPrefChange,
base::Unretained(this));
  }

  // Shakes the tree after new bananas appear.
  void ShakeTreeOnBananaPrefChange() {
    // Do your thing.
  }

 private:
  PrefChangeRegistrar registrar_;
};

They are largely identical, with the big advantage in the second version
that you actually have proper function names that indicate what pref change
gets handled / what happens when the event fires.


> In cases where users would prefer to have pref changes dispatched to
> separate methods, they can create multiple PrefObserverImpl members
> that bind to the methods they want to dispatch to, and use
> PrefChangeRegistrar to register these members instead of registering
> the 'this' pointer.
>

You'd get that for free in the base::Callback-style approach.


>
> This also gets rid of the problem you mentioned about removing
> previously-registered callbacks; in this case the things would have to
> be members, not constructed on the fly, so deregistration works just
> like it does today.
>

I just talked to battre@ and we discussed this some more. I think the
proper solution would be to force everybody to go through
PrefChangeRegistrar (we already do that today), PrefChangeRegistrar could
manage unregistration behind the scenes. In PrefServiceBase, we'd do this:

class PrefServiceBase {
  // ...

 private:
  void AddObserver(const std::string& pref_name, PrefChangeRegistrar*
handler);
  void RemoveObserver(const std::string& pref_name, PrefChangeRegistrar*
handler);

  // Map of handlers to dispatch to, PrefChangeRegistrar keeps the
Callbacks internally.
  std::map<std::string, std::vector<PrefChangeRegistrar*> > handlers;
};


>
> Another advantage is, this lets us convert usages one by one to the
> dispatch-oriented (multiple OnXyzPrefChanged methods) approach,
> instead of having to do it in one fell swoop.
>

That's a good point :) Then again, you need to touch all of the call sites
anyways to remove NotificationObserver, so we could just as well introduce
the new base::Callback-style approach in parallel to what we have (just add
the additional pieces to PrefChangeRegistrar and PrefService), then convert
everything step by step and then removing the old stuff from
PrefServiceBase and PrefChangeRegistrar.


>
> I'm working on answering your question about inheritance hierarchies;
> there were just a few and I'm trying to remember where they were.
>

Thanks, that'll be helpful.


>
> Cheers,
> Jói
>
>
>
> On Tue, Oct 30, 2012 at 9:33 AM,  <mnissler@chromium.org> wrote:
> > On 2012/10/30 09:19:00, Jói wrote:
> >>
> >> > Any reason to not use base::Callback<void(PrefServiceBase*, const
> >> > std::string&) for this purpose?
> >
> >
> >> Having a dedicated delegate feels cleaner, as it creates an is-a
> >> relationship for classes that are an observer.  Using a callback would
> >> mean a Callback member variable, which is less obviously an is-a.  The
> >> importance of the is-a relationship is increased because some class
> >> hierarchies have multiple chained implementations of
> >> OnPreferenceChanged, so being able to use polymorphism to find the
> >> right implementation (that then chains back to the superclasses) is
> >> good.
> >
> >
> > You're assuming that the current chaining is inherent to the design of
> these
> > classes. Can you point me to a few examples that are complex? My hunch is
> > that
> > the chaining only exists because there is only one way to receive
> > PREF_CHANGED
> > notifications, and the implementations have to demultiplex.
> >
> > Note that chaining can easily be accomplished by declaring handlers:
> >
> > virtual void OnBananaPrefChanged();
> >
> > That has the advantage that you can easily see in the class declaration
> what
> > events are relevant to the class, whereas I've found myself studying
> > NotificationObserver::Observe() implementations numerous times to find
> out
> > why
> > the class is interested in pref notifications at all and what
> pref-triggered
> > events it reacts to.
> >
> > Bottom line: My feeling is that the existing complexity is largely due to
> > the
> > awkward interface, and my hope is that we could get rid of a significant
> > amount
> > of complexity using callback-based handlers.
> >
> >
> >
> >> > I guess this would also allow for
> >> > reducing lots of complexity in the consumers, because they can drop
> >> > switch(pref_name)-style code and have dedicated handlers for the
> >> > different events.
> >
> >
> >> That would be possible if using callbacks, but at the expense of
> >> harder chaining etc.  I think on the whole it would be more complex,
> >> not less.
> >
> >
> > I guess we're at the point where we need more data to make an informed
> > decision.
> > Do you have a couple of complex examples you encountered that we could
> try
> > with
> > either approach to test the waters?
> >
> > One additional caveat with base::Callback-style notifications I hadn't
> > thought
> > of is observer unregistration. base::Callback instances are often
> > constructed on
> > the fly and passed by value, so it's not immediately obvious to me how to
> > remove
> > previously-registered callback. One option I can see is forcing
> everybody to
> > use
> > PrefChangeRegistrar and have it keep a handle that can be used to
> unregister
> > the
> > callback later. WDYT?
> >
> >
> >> Cheers,
> >> Jói
> >
> >
> >
> >
> >> On Tue, Oct 30, 2012 at 9:09 AM,  <mailto:mnissler@chromium.org> wrote:
> >> > One quick comment I want to get out before we dive into details :)
> >> >
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observer.h
> >>
> >> > File base/prefs/public/pref_observer.h (right):
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observe...
> >>
> >> > base/prefs/public/pref_observer.h:15: const std::string& pref_name) =
> 0;
> >> > Any reason to not use base::Callback<void(PrefServiceBase*, const
> >> > std::string&) for this purpose? I guess this would also allow for
> >> > reducing lots of complexity in the consumers, because they can drop
> >> > switch(pref_name)-style code and have dedicated handlers for the
> >> > different events.
> >> >
> >> > http://codereview.chromium.org/11345008/
> >
> >
> >
> > http://codereview.chromium.org/11345008/
>

[Mattias Nissler (ping if slow), 2012-10-30 10:06:56]

On Tue, Oct 30, 2012 at 10:57 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> Here are two examples that I found given a quick search; I'm not sure
> if there are more:
> chrome/browser/chromeos/proxy_config_service_impl.h
>

That's a case in which the prefs that the two classes observe are disjoint.
In fact, there's a missing pref_name == prefs::kProxy check in
pref_proxy_config_tracker_impl.cc, which would have probably not crept in
if we had separate callback functions.


> chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h
>

In this case it would probably be cleaner to wire up the special case pref
change code to separate handlers. A pref that behaves "the ordinary way"
would just be wired up using NotifyPrefChanged() as the callback.


>
> Cheers,
> Jói
>
>
> On Tue, Oct 30, 2012 at 9:46 AM, Jói Sigurðsson <joi@chromium.org> wrote:
> > I would suggest doing this:
> > a) Leave the PrefObserver interface in place.
> > b) Have an implementation of it that lets you keep multiple
> > PrefObserverImpl objects (which are effectively just like callbacks)
> > and bind them to different methods on your class.
> >
> > This gives you the best of both worlds.  In many cases, you only care
> > that one or more prefs changed and you don't even check their values.
> > For this case, it is much simpler to inherit from PrefObserver and
> > register using PrefChangeRegistrar, and override a single method.
> > Everything typesafe, no extra registration work, no new member
> > variable to add, etc.
> >
> > In cases where users would prefer to have pref changes dispatched to
> > separate methods, they can create multiple PrefObserverImpl members
> > that bind to the methods they want to dispatch to, and use
> > PrefChangeRegistrar to register these members instead of registering
> > the 'this' pointer.
> >
> > This also gets rid of the problem you mentioned about removing
> > previously-registered callbacks; in this case the things would have to
> > be members, not constructed on the fly, so deregistration works just
> > like it does today.
> >
> > Another advantage is, this lets us convert usages one by one to the
> > dispatch-oriented (multiple OnXyzPrefChanged methods) approach,
> > instead of having to do it in one fell swoop.
> >
> > I'm working on answering your question about inheritance hierarchies;
> > there were just a few and I'm trying to remember where they were.
> >
> > Cheers,
> > Jói
> >
> >
> >
> > On Tue, Oct 30, 2012 at 9:33 AM,  <mnissler@chromium.org> wrote:
> >> On 2012/10/30 09:19:00, Jói wrote:
> >>>
> >>> > Any reason to not use base::Callback<void(PrefServiceBase*, const
> >>> > std::string&) for this purpose?
> >>
> >>
> >>> Having a dedicated delegate feels cleaner, as it creates an is-a
> >>> relationship for classes that are an observer.  Using a callback would
> >>> mean a Callback member variable, which is less obviously an is-a.  The
> >>> importance of the is-a relationship is increased because some class
> >>> hierarchies have multiple chained implementations of
> >>> OnPreferenceChanged, so being able to use polymorphism to find the
> >>> right implementation (that then chains back to the superclasses) is
> >>> good.
> >>
> >>
> >> You're assuming that the current chaining is inherent to the design of
> these
> >> classes. Can you point me to a few examples that are complex? My hunch
> is
> >> that
> >> the chaining only exists because there is only one way to receive
> >> PREF_CHANGED
> >> notifications, and the implementations have to demultiplex.
> >>
> >> Note that chaining can easily be accomplished by declaring handlers:
> >>
> >> virtual void OnBananaPrefChanged();
> >>
> >> That has the advantage that you can easily see in the class declaration
> what
> >> events are relevant to the class, whereas I've found myself studying
> >> NotificationObserver::Observe() implementations numerous times to find
> out
> >> why
> >> the class is interested in pref notifications at all and what
> pref-triggered
> >> events it reacts to.
> >>
> >> Bottom line: My feeling is that the existing complexity is largely due
> to
> >> the
> >> awkward interface, and my hope is that we could get rid of a significant
> >> amount
> >> of complexity using callback-based handlers.
> >>
> >>
> >>
> >>> > I guess this would also allow for
> >>> > reducing lots of complexity in the consumers, because they can drop
> >>> > switch(pref_name)-style code and have dedicated handlers for the
> >>> > different events.
> >>
> >>
> >>> That would be possible if using callbacks, but at the expense of
> >>> harder chaining etc.  I think on the whole it would be more complex,
> >>> not less.
> >>
> >>
> >> I guess we're at the point where we need more data to make an informed
> >> decision.
> >> Do you have a couple of complex examples you encountered that we could
> try
> >> with
> >> either approach to test the waters?
> >>
> >> One additional caveat with base::Callback-style notifications I hadn't
> >> thought
> >> of is observer unregistration. base::Callback instances are often
> >> constructed on
> >> the fly and passed by value, so it's not immediately obvious to me how
> to
> >> remove
> >> previously-registered callback. One option I can see is forcing
> everybody to
> >> use
> >> PrefChangeRegistrar and have it keep a handle that can be used to
> unregister
> >> the
> >> callback later. WDYT?
> >>
> >>
> >>> Cheers,
> >>> Jói
> >>
> >>
> >>
> >>
> >>> On Tue, Oct 30, 2012 at 9:09 AM,  <mailto:mnissler@chromium.org>
> wrote:
> >>> > One quick comment I want to get out before we dive into details :)
> >>> >
> >>> >
> >>> >
> >>
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observer.h
> >>>
> >>> > File base/prefs/public/pref_observer.h (right):
> >>> >
> >>> >
> >>
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observe...
> >>>
> >>> > base/prefs/public/pref_observer.h:15: const std::string& pref_name)
> = 0;
> >>> > Any reason to not use base::Callback<void(PrefServiceBase*, const
> >>> > std::string&) for this purpose? I guess this would also allow for
> >>> > reducing lots of complexity in the consumers, because they can drop
> >>> > switch(pref_name)-style code and have dedicated handlers for the
> >>> > different events.
> >>> >
> >>> > http://codereview.chromium.org/11345008/
> >>
> >>
> >>
> >> http://codereview.chromium.org/11345008/
>

[Jói, 2012-10-30 10:24:34]

Honestly, I think having a PrefObserver class for users to inherit
from, so that there is an is-a relationship, is cleaner.  You guys are
the owners though, so if you want to convert to a Callback based
thing, that's fine by me, although while I might have time at some
point to help with that, I'd prefer to focus on finishing the
extraction of Prefs out of chrome/ before working on larger interface
changes such as that.  In either case, I think it's best to make the
change to using Callback in incremental follow-up changes, rather than
as part of this change, and it sounds like you agree.

My current change breaks the dependency on
content::NotificationObserver, and if I get this checked in it will be
a lot easier to convert to the approach you're suggesting when you
have time to do so.  This is because the current change goes through
all of the usages of NotificationObserver and either removes
inheritance of it (where the pref notification was the only one being
listened for), or removes the NOTIFICATION_PREF_CHANGED case from
switch statements in NotificationObserver::Observe implementations.
This is work that is very useful as a precursor for the Callback-based
approach you desire, and took quite a lot of time to perform.

If you agree we can do this step by step, I'd like to go ahead and
land the current approach as quickly as possible, since it's a huge
change and will get out of sync pretty quickly.  As you suggested, the
switch to a Callback-based solution can be done piecemeal, but as far
as I can tell this can only happen if we leave a PrefObserver base
class in there for now (which means landing my curent approach or
something similar to it), then once you've done the switch piecemeal
for all users you could remove the PrefObserver class.

Cheers,
Jói


On Tue, Oct 30, 2012 at 10:06 AM, Mattias Nissler <mnissler@chromium.org>
wrote:
>
>
> On Tue, Oct 30, 2012 at 10:57 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> Here are two examples that I found given a quick search; I'm not sure
>> if there are more:
>> chrome/browser/chromeos/proxy_config_service_impl.h
>
>
> That's a case in which the prefs that the two classes observe are disjoint.
> In fact, there's a missing pref_name == prefs::kProxy check in
> pref_proxy_config_tracker_impl.cc, which would have probably not crept in if
> we had separate callback functions.
>
>>
>> chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h
>
>
> In this case it would probably be cleaner to wire up the special case pref
> change code to separate handlers. A pref that behaves "the ordinary way"
> would just be wired up using NotifyPrefChanged() as the callback.
>
>>
>>
>> Cheers,
>> Jói
>>
>>
>> On Tue, Oct 30, 2012 at 9:46 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>> > I would suggest doing this:
>> > a) Leave the PrefObserver interface in place.
>> > b) Have an implementation of it that lets you keep multiple
>> > PrefObserverImpl objects (which are effectively just like callbacks)
>> > and bind them to different methods on your class.
>> >
>> > This gives you the best of both worlds.  In many cases, you only care
>> > that one or more prefs changed and you don't even check their values.
>> > For this case, it is much simpler to inherit from PrefObserver and
>> > register using PrefChangeRegistrar, and override a single method.
>> > Everything typesafe, no extra registration work, no new member
>> > variable to add, etc.
>> >
>> > In cases where users would prefer to have pref changes dispatched to
>> > separate methods, they can create multiple PrefObserverImpl members
>> > that bind to the methods they want to dispatch to, and use
>> > PrefChangeRegistrar to register these members instead of registering
>> > the 'this' pointer.
>> >
>> > This also gets rid of the problem you mentioned about removing
>> > previously-registered callbacks; in this case the things would have to
>> > be members, not constructed on the fly, so deregistration works just
>> > like it does today.
>> >
>> > Another advantage is, this lets us convert usages one by one to the
>> > dispatch-oriented (multiple OnXyzPrefChanged methods) approach,
>> > instead of having to do it in one fell swoop.
>> >
>> > I'm working on answering your question about inheritance hierarchies;
>> > there were just a few and I'm trying to remember where they were.
>> >
>> > Cheers,
>> > Jói
>> >
>> >
>> >
>> > On Tue, Oct 30, 2012 at 9:33 AM,  <mnissler@chromium.org> wrote:
>> >> On 2012/10/30 09:19:00, Jói wrote:
>> >>>
>> >>> > Any reason to not use base::Callback<void(PrefServiceBase*, const
>> >>> > std::string&) for this purpose?
>> >>
>> >>
>> >>> Having a dedicated delegate feels cleaner, as it creates an is-a
>> >>> relationship for classes that are an observer.  Using a callback would
>> >>> mean a Callback member variable, which is less obviously an is-a.  The
>> >>> importance of the is-a relationship is increased because some class
>> >>> hierarchies have multiple chained implementations of
>> >>> OnPreferenceChanged, so being able to use polymorphism to find the
>> >>> right implementation (that then chains back to the superclasses) is
>> >>> good.
>> >>
>> >>
>> >> You're assuming that the current chaining is inherent to the design of
>> >> these
>> >> classes. Can you point me to a few examples that are complex? My hunch
>> >> is
>> >> that
>> >> the chaining only exists because there is only one way to receive
>> >> PREF_CHANGED
>> >> notifications, and the implementations have to demultiplex.
>> >>
>> >> Note that chaining can easily be accomplished by declaring handlers:
>> >>
>> >> virtual void OnBananaPrefChanged();
>> >>
>> >> That has the advantage that you can easily see in the class declaration
>> >> what
>> >> events are relevant to the class, whereas I've found myself studying
>> >> NotificationObserver::Observe() implementations numerous times to find
>> >> out
>> >> why
>> >> the class is interested in pref notifications at all and what
>> >> pref-triggered
>> >> events it reacts to.
>> >>
>> >> Bottom line: My feeling is that the existing complexity is largely due
>> >> to
>> >> the
>> >> awkward interface, and my hope is that we could get rid of a
>> >> significant
>> >> amount
>> >> of complexity using callback-based handlers.
>> >>
>> >>
>> >>
>> >>> > I guess this would also allow for
>> >>> > reducing lots of complexity in the consumers, because they can drop
>> >>> > switch(pref_name)-style code and have dedicated handlers for the
>> >>> > different events.
>> >>
>> >>
>> >>> That would be possible if using callbacks, but at the expense of
>> >>> harder chaining etc.  I think on the whole it would be more complex,
>> >>> not less.
>> >>
>> >>
>> >> I guess we're at the point where we need more data to make an informed
>> >> decision.
>> >> Do you have a couple of complex examples you encountered that we could
>> >> try
>> >> with
>> >> either approach to test the waters?
>> >>
>> >> One additional caveat with base::Callback-style notifications I hadn't
>> >> thought
>> >> of is observer unregistration. base::Callback instances are often
>> >> constructed on
>> >> the fly and passed by value, so it's not immediately obvious to me how
>> >> to
>> >> remove
>> >> previously-registered callback. One option I can see is forcing
>> >> everybody to
>> >> use
>> >> PrefChangeRegistrar and have it keep a handle that can be used to
>> >> unregister
>> >> the
>> >> callback later. WDYT?
>> >>
>> >>
>> >>> Cheers,
>> >>> Jói
>> >>
>> >>
>> >>
>> >>
>> >>> On Tue, Oct 30, 2012 at 9:09 AM,  <mailto:mnissler@chromium.org>
>> >>> wrote:
>> >>> > One quick comment I want to get out before we dive into details :)
>> >>> >
>> >>> >
>> >>> >
>> >>
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observer.h
>> >>>
>> >>> > File base/prefs/public/pref_observer.h (right):
>> >>> >
>> >>> >
>> >>
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observe...
>> >>>
>> >>> > base/prefs/public/pref_observer.h:15: const std::string& pref_name)
>> >>> > = 0;
>> >>> > Any reason to not use base::Callback<void(PrefServiceBase*, const
>> >>> > std::string&) for this purpose? I guess this would also allow for
>> >>> > reducing lots of complexity in the consumers, because they can drop
>> >>> > switch(pref_name)-style code and have dedicated handlers for the
>> >>> > different events.
>> >>> >
>> >>> > http://codereview.chromium.org/11345008/
>> >>
>> >>
>> >>
>> >> http://codereview.chromium.org/11345008/
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "browser-components-watch" group.
> To post to this group, send email to browser-components-watch@chromium.org.
> To unsubscribe from this group, send email to
> browser-components-watch+unsubscribe@chromium.org.
> For more options, visit
> https://groups.google.com/a/chromium.org/groups/opt_out.
>
>

[Jói, 2012-10-30 11:32:36]

Hi again,

TL;DR:  I am willing to work on the Callback-based interface if you
wish, but I think we should first land and try this change.

I looked through about half of the OnPreferenceChanged implementations
to get a clearer picture of what makes sense.  Looking at this stuff,
it would probably not be too much work to switch over to the
Callback-based interface you are suggesting, so I would be OK with
taking care of that in a follow-up.  I don't think I would switch over
all methods though, but certainly in cases where just a single pref is
being handled it can reduce boiler-plate code (eliminating a DCHECK)
and in cases where there is a simple switch to dispatch to multiple
functions it can also help.  There are some outliers though where I
wouldn't necessarily switch, where there is multiple dispatch but a
bunch of common code before or after or on both sides of the dispatch,
so dispatching to individual methods would not necessarily make things
more readable.  See some examples below:

Examples where you might want to continue doing manual dispatch
through a single function:
FontSettingsEventRouter::OnPreferenceChanged
PolicyProvider::OnPreferenceChanged
SSLConfigServiceManagerPref::OnPreferenceChanged

An example where it does help with multiple dispatch (there are
several of this kind of usage):
ProfileImpl::OnPreferenceChanged

An examples where it maybe helps (it makes the code a bit more "type
safe" but perhaps not more readable; not sure I would necessarily
switch e.g. this one):
CredentialCacheService::OnPreferenceChanged

It looks like a lot of the usages require the pref_name parameter
(they may e.g. be forwarding it on to another function), and some of
them also require the PrefService* parameter, so even if we enable a
parameterless dispatch via PrefChangeRegistrar, we should also allow a
dispatch with the same parameters as OnPreferenceChanged currently
has.

In summary:
- I would be willing to work on a Callback-based interface after this
change lands.
- I think we should land this change first, otherwise I'm worried that
(a) it will get out of sync and (b) the significant changes needed to
switch to the new type of dispatch will introduce further possibility
of breaking things, so landing this change first reduces risk and I
think it also reduces the overall amount of work.

Cheers,
Jói



On Tue, Oct 30, 2012 at 10:23 AM, Jói Sigurðsson <joi@chromium.org> wrote:
> Honestly, I think having a PrefObserver class for users to inherit
> from, so that there is an is-a relationship, is cleaner.  You guys are
> the owners though, so if you want to convert to a Callback based
> thing, that's fine by me, although while I might have time at some
> point to help with that, I'd prefer to focus on finishing the
> extraction of Prefs out of chrome/ before working on larger interface
> changes such as that.  In either case, I think it's best to make the
> change to using Callback in incremental follow-up changes, rather than
> as part of this change, and it sounds like you agree.
>
> My current change breaks the dependency on
> content::NotificationObserver, and if I get this checked in it will be
> a lot easier to convert to the approach you're suggesting when you
> have time to do so.  This is because the current change goes through
> all of the usages of NotificationObserver and either removes
> inheritance of it (where the pref notification was the only one being
> listened for), or removes the NOTIFICATION_PREF_CHANGED case from
> switch statements in NotificationObserver::Observe implementations.
> This is work that is very useful as a precursor for the Callback-based
> approach you desire, and took quite a lot of time to perform.
>
> If you agree we can do this step by step, I'd like to go ahead and
> land the current approach as quickly as possible, since it's a huge
> change and will get out of sync pretty quickly.  As you suggested, the
> switch to a Callback-based solution can be done piecemeal, but as far
> as I can tell this can only happen if we leave a PrefObserver base
> class in there for now (which means landing my curent approach or
> something similar to it), then once you've done the switch piecemeal
> for all users you could remove the PrefObserver class.
>
> Cheers,
> Jói
>
>
> On Tue, Oct 30, 2012 at 10:06 AM, Mattias Nissler <mnissler@chromium.org>
wrote:
>>
>>
>> On Tue, Oct 30, 2012 at 10:57 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>>>
>>> Here are two examples that I found given a quick search; I'm not sure
>>> if there are more:
>>> chrome/browser/chromeos/proxy_config_service_impl.h
>>
>>
>> That's a case in which the prefs that the two classes observe are disjoint.
>> In fact, there's a missing pref_name == prefs::kProxy check in
>> pref_proxy_config_tracker_impl.cc, which would have probably not crept in if
>> we had separate callback functions.
>>
>>>
>>> chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h
>>
>>
>> In this case it would probably be cleaner to wire up the special case pref
>> change code to separate handlers. A pref that behaves "the ordinary way"
>> would just be wired up using NotifyPrefChanged() as the callback.
>>
>>>
>>>
>>> Cheers,
>>> Jói
>>>
>>>
>>> On Tue, Oct 30, 2012 at 9:46 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>>> > I would suggest doing this:
>>> > a) Leave the PrefObserver interface in place.
>>> > b) Have an implementation of it that lets you keep multiple
>>> > PrefObserverImpl objects (which are effectively just like callbacks)
>>> > and bind them to different methods on your class.
>>> >
>>> > This gives you the best of both worlds.  In many cases, you only care
>>> > that one or more prefs changed and you don't even check their values.
>>> > For this case, it is much simpler to inherit from PrefObserver and
>>> > register using PrefChangeRegistrar, and override a single method.
>>> > Everything typesafe, no extra registration work, no new member
>>> > variable to add, etc.
>>> >
>>> > In cases where users would prefer to have pref changes dispatched to
>>> > separate methods, they can create multiple PrefObserverImpl members
>>> > that bind to the methods they want to dispatch to, and use
>>> > PrefChangeRegistrar to register these members instead of registering
>>> > the 'this' pointer.
>>> >
>>> > This also gets rid of the problem you mentioned about removing
>>> > previously-registered callbacks; in this case the things would have to
>>> > be members, not constructed on the fly, so deregistration works just
>>> > like it does today.
>>> >
>>> > Another advantage is, this lets us convert usages one by one to the
>>> > dispatch-oriented (multiple OnXyzPrefChanged methods) approach,
>>> > instead of having to do it in one fell swoop.
>>> >
>>> > I'm working on answering your question about inheritance hierarchies;
>>> > there were just a few and I'm trying to remember where they were.
>>> >
>>> > Cheers,
>>> > Jói
>>> >
>>> >
>>> >
>>> > On Tue, Oct 30, 2012 at 9:33 AM,  <mnissler@chromium.org> wrote:
>>> >> On 2012/10/30 09:19:00, Jói wrote:
>>> >>>
>>> >>> > Any reason to not use base::Callback<void(PrefServiceBase*, const
>>> >>> > std::string&) for this purpose?
>>> >>
>>> >>
>>> >>> Having a dedicated delegate feels cleaner, as it creates an is-a
>>> >>> relationship for classes that are an observer.  Using a callback would
>>> >>> mean a Callback member variable, which is less obviously an is-a.  The
>>> >>> importance of the is-a relationship is increased because some class
>>> >>> hierarchies have multiple chained implementations of
>>> >>> OnPreferenceChanged, so being able to use polymorphism to find the
>>> >>> right implementation (that then chains back to the superclasses) is
>>> >>> good.
>>> >>
>>> >>
>>> >> You're assuming that the current chaining is inherent to the design of
>>> >> these
>>> >> classes. Can you point me to a few examples that are complex? My hunch
>>> >> is
>>> >> that
>>> >> the chaining only exists because there is only one way to receive
>>> >> PREF_CHANGED
>>> >> notifications, and the implementations have to demultiplex.
>>> >>
>>> >> Note that chaining can easily be accomplished by declaring handlers:
>>> >>
>>> >> virtual void OnBananaPrefChanged();
>>> >>
>>> >> That has the advantage that you can easily see in the class declaration
>>> >> what
>>> >> events are relevant to the class, whereas I've found myself studying
>>> >> NotificationObserver::Observe() implementations numerous times to find
>>> >> out
>>> >> why
>>> >> the class is interested in pref notifications at all and what
>>> >> pref-triggered
>>> >> events it reacts to.
>>> >>
>>> >> Bottom line: My feeling is that the existing complexity is largely due
>>> >> to
>>> >> the
>>> >> awkward interface, and my hope is that we could get rid of a
>>> >> significant
>>> >> amount
>>> >> of complexity using callback-based handlers.
>>> >>
>>> >>
>>> >>
>>> >>> > I guess this would also allow for
>>> >>> > reducing lots of complexity in the consumers, because they can drop
>>> >>> > switch(pref_name)-style code and have dedicated handlers for the
>>> >>> > different events.
>>> >>
>>> >>
>>> >>> That would be possible if using callbacks, but at the expense of
>>> >>> harder chaining etc.  I think on the whole it would be more complex,
>>> >>> not less.
>>> >>
>>> >>
>>> >> I guess we're at the point where we need more data to make an informed
>>> >> decision.
>>> >> Do you have a couple of complex examples you encountered that we could
>>> >> try
>>> >> with
>>> >> either approach to test the waters?
>>> >>
>>> >> One additional caveat with base::Callback-style notifications I hadn't
>>> >> thought
>>> >> of is observer unregistration. base::Callback instances are often
>>> >> constructed on
>>> >> the fly and passed by value, so it's not immediately obvious to me how
>>> >> to
>>> >> remove
>>> >> previously-registered callback. One option I can see is forcing
>>> >> everybody to
>>> >> use
>>> >> PrefChangeRegistrar and have it keep a handle that can be used to
>>> >> unregister
>>> >> the
>>> >> callback later. WDYT?
>>> >>
>>> >>
>>> >>> Cheers,
>>> >>> Jói
>>> >>
>>> >>
>>> >>
>>> >>
>>> >>> On Tue, Oct 30, 2012 at 9:09 AM,  <mailto:mnissler@chromium.org>
>>> >>> wrote:
>>> >>> > One quick comment I want to get out before we dive into details :)
>>> >>> >
>>> >>> >
>>> >>> >
>>> >>
>>> >>
>>> >>
>>> >>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observer.h
>>> >>>
>>> >>> > File base/prefs/public/pref_observer.h (right):
>>> >>> >
>>> >>> >
>>> >>
>>> >>
>>> >>
>>> >>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observe...
>>> >>>
>>> >>> > base/prefs/public/pref_observer.h:15: const std::string& pref_name)
>>> >>> > = 0;
>>> >>> > Any reason to not use base::Callback<void(PrefServiceBase*, const
>>> >>> > std::string&) for this purpose? I guess this would also allow for
>>> >>> > reducing lots of complexity in the consumers, because they can drop
>>> >>> > switch(pref_name)-style code and have dedicated handlers for the
>>> >>> > different events.
>>> >>> >
>>> >>> > http://codereview.chromium.org/11345008/
>>> >>
>>> >>
>>> >>
>>> >> http://codereview.chromium.org/11345008/
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "browser-components-watch" group.
>> To post to this group, send email to browser-components-watch@chromium.org.
>> To unsubscribe from this group, send email to
>> browser-components-watch+unsubscribe@chromium.org.
>> For more options, visit
>> https://groups.google.com/a/chromium.org/groups/opt_out.
>>
>>

[Mattias Nissler (ping if slow), 2012-10-30 13:37:05]

I only looked at the main changes in chrome/browser/prefs and base/prefs and a
couple of arbitrarily chosen consumers.

http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_init_ob...
File base/prefs/public/pref_init_observer.h (right):

http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_init_ob...
base/prefs/public/pref_init_observer.h:13: public:
Please add a comment here explaining when this gets called and what it is good
for.

http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observer.h
File base/prefs/public/pref_observer.h (right):

http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observe...
base/prefs/public/pref_observer.h:13: public:
should have a comment, and you should put a TODO saying that we'd like to
replace that by a base::Callback-style interface.

http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/audio/...
File chrome/browser/chromeos/audio/audio_handler.cc (right):

http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/audio/...
chrome/browser/chromeos/audio/audio_handler.cc:166: pref_name ==
prefs::kAudioCaptureAllowed);
This should probably be

if(..)
  ApplyAudioPolicy()
else
  NOTREACHED();

http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_file_system.cc (right):

http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_file_system.cc:2190: DCHECK(pref_name ==
prefs::kDisableDriveHostedFiles);
same comment regarding if() {} else NOTREACHED();

http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):

http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
chrome/browser/prefs/pref_notifier_impl_unittest.cc:101:
notifier.AddInitObserver(&observer);
The init observer would also be better served through a callback as we can just
clear the list of observers/callbacks after the event fires (i.e. Init only
happens once).

http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
chrome/browser/prefs/pref_notifier_impl_unittest.cc:107:
notifier.RemoveInitObserver(&observer);
So this shouldn't be necessary.

http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_serv...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service.h:276: void
AddPrefInitObserver(PrefInitObserver* obs);
needs documentation

[Jói, 2012-10-30 13:46:35]

Thanks Mattias.

As discussed off-line, I plan to switch to the Callback-based
interface and send this for review again.  I think for this change, I
will always have the Callback-based interface invoke on
OnPreferenceChanged, and then in follow-up changes fix the places
where OnPreferenceChanged would better be done as multiple
OnXyzPreferenceChanged with independent dispatching.  That way we are
switching to the right interface now, while letting me minimize the
amount of change so that I can get this checked in soonish, before I
end up in merge hell.  The follow-ups will then just be local
improvements to how dispatching is done.

>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/audio/...
> chrome/browser/chromeos/audio/audio_handler.cc:166: pref_name ==
> prefs::kAudioCaptureAllowed);
> This should probably be
>
> if(..)
>   ApplyAudioPolicy()
> else
>   NOTREACHED();

I disagree; the DCHECK makes our production code more efficient, and
represents what is actually being asserted on.  Having a DCHECK up
front when it was always the same pref (or one of a set of prefs) was
also a fairly common pattern throughout the code, so I guess we could
go either way, but for me the up-front DCHECK is clearer and less
verbose.

Cheers,
Jói


On Tue, Oct 30, 2012 at 1:37 PM,  <mnissler@chromium.org> wrote:
> I only looked at the main changes in chrome/browser/prefs and base/prefs and
> a
> couple of arbitrarily chosen consumers.
>
>
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_init_ob...
> File base/prefs/public/pref_init_observer.h (right):
>
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_init_ob...
> base/prefs/public/pref_init_observer.h:13: public:
> Please add a comment here explaining when this gets called and what it
> is good for.
>
>
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observer.h
> File base/prefs/public/pref_observer.h (right):
>
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observe...
> base/prefs/public/pref_observer.h:13: public:
> should have a comment, and you should put a TODO saying that we'd like
> to replace that by a base::Callback-style interface.
>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/audio/...
> File chrome/browser/chromeos/audio/audio_handler.cc (right):
>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/audio/...
> chrome/browser/chromeos/audio/audio_handler.cc:166: pref_name ==
> prefs::kAudioCaptureAllowed);
> This should probably be
>
> if(..)
>   ApplyAudioPolicy()
> else
>   NOTREACHED();
>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/drive/...
> File chrome/browser/chromeos/drive/drive_file_system.cc (right):
>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/drive/...
> chrome/browser/chromeos/drive/drive_file_system.cc:2190:
> DCHECK(pref_name == prefs::kDisableDriveHostedFiles);
> same comment regarding if() {} else NOTREACHED();
>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
> File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):
>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
> chrome/browser/prefs/pref_notifier_impl_unittest.cc:101:
> notifier.AddInitObserver(&observer);
> The init observer would also be better served through a callback as we
> can just clear the list of observers/callbacks after the event fires
> (i.e. Init only happens once).
>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
> chrome/browser/prefs/pref_notifier_impl_unittest.cc:107:
> notifier.RemoveInitObserver(&observer);
> So this shouldn't be necessary.
>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_serv...
> File chrome/browser/prefs/pref_service.h (right):
>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_serv...
> chrome/browser/prefs/pref_service.h:276: void
> AddPrefInitObserver(PrefInitObserver* obs);
> needs documentation
>
> http://codereview.chromium.org/11345008/

[Jói, 2012-10-30 13:46:58]

Also:  I will send a preview of the intended interface changes before
switching all the usages over.

On Tue, Oct 30, 2012 at 1:45 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Thanks Mattias.
>
> As discussed off-line, I plan to switch to the Callback-based
> interface and send this for review again.  I think for this change, I
> will always have the Callback-based interface invoke on
> OnPreferenceChanged, and then in follow-up changes fix the places
> where OnPreferenceChanged would better be done as multiple
> OnXyzPreferenceChanged with independent dispatching.  That way we are
> switching to the right interface now, while letting me minimize the
> amount of change so that I can get this checked in soonish, before I
> end up in merge hell.  The follow-ups will then just be local
> improvements to how dispatching is done.
>
>>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/audio/...
>> chrome/browser/chromeos/audio/audio_handler.cc:166: pref_name ==
>> prefs::kAudioCaptureAllowed);
>> This should probably be
>>
>> if(..)
>>   ApplyAudioPolicy()
>> else
>>   NOTREACHED();
>
> I disagree; the DCHECK makes our production code more efficient, and
> represents what is actually being asserted on.  Having a DCHECK up
> front when it was always the same pref (or one of a set of prefs) was
> also a fairly common pattern throughout the code, so I guess we could
> go either way, but for me the up-front DCHECK is clearer and less
> verbose.
>
> Cheers,
> Jói
>
>
> On Tue, Oct 30, 2012 at 1:37 PM,  <mnissler@chromium.org> wrote:
>> I only looked at the main changes in chrome/browser/prefs and base/prefs and
>> a
>> couple of arbitrarily chosen consumers.
>>
>>
>>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_init_ob...
>> File base/prefs/public/pref_init_observer.h (right):
>>
>>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_init_ob...
>> base/prefs/public/pref_init_observer.h:13: public:
>> Please add a comment here explaining when this gets called and what it
>> is good for.
>>
>>
>>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observer.h
>> File base/prefs/public/pref_observer.h (right):
>>
>>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observe...
>> base/prefs/public/pref_observer.h:13: public:
>> should have a comment, and you should put a TODO saying that we'd like
>> to replace that by a base::Callback-style interface.
>>
>>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/audio/...
>> File chrome/browser/chromeos/audio/audio_handler.cc (right):
>>
>>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/audio/...
>> chrome/browser/chromeos/audio/audio_handler.cc:166: pref_name ==
>> prefs::kAudioCaptureAllowed);
>> This should probably be
>>
>> if(..)
>>   ApplyAudioPolicy()
>> else
>>   NOTREACHED();
>>
>>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/drive/...
>> File chrome/browser/chromeos/drive/drive_file_system.cc (right):
>>
>>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/drive/...
>> chrome/browser/chromeos/drive/drive_file_system.cc:2190:
>> DCHECK(pref_name == prefs::kDisableDriveHostedFiles);
>> same comment regarding if() {} else NOTREACHED();
>>
>>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
>> File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):
>>
>>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
>> chrome/browser/prefs/pref_notifier_impl_unittest.cc:101:
>> notifier.AddInitObserver(&observer);
>> The init observer would also be better served through a callback as we
>> can just clear the list of observers/callbacks after the event fires
>> (i.e. Init only happens once).
>>
>>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
>> chrome/browser/prefs/pref_notifier_impl_unittest.cc:107:
>> notifier.RemoveInitObserver(&observer);
>> So this shouldn't be necessary.
>>
>>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_serv...
>> File chrome/browser/prefs/pref_service.h (right):
>>
>>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_serv...
>> chrome/browser/prefs/pref_service.h:276: void
>> AddPrefInitObserver(PrefInitObserver* obs);
>> needs documentation
>>
>> http://codereview.chromium.org/11345008/

[Mattias Nissler (ping if slow), 2012-10-30 14:22:17]

On Tue, Oct 30, 2012 at 2:46 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> Also:  I will send a preview of the intended interface changes before
> switching all the usages over.
>
> On Tue, Oct 30, 2012 at 1:45 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> > Thanks Mattias.
> >
> > As discussed off-line, I plan to switch to the Callback-based
> > interface and send this for review again.  I think for this change, I
> > will always have the Callback-based interface invoke on
> > OnPreferenceChanged, and then in follow-up changes fix the places
> > where OnPreferenceChanged would better be done as multiple
> > OnXyzPreferenceChanged with independent dispatching.  That way we are
> > switching to the right interface now, while letting me minimize the
> > amount of change so that I can get this checked in soonish, before I
> > end up in merge hell.  The follow-ups will then just be local
> > improvements to how dispatching is done.
>

Sounds good to me, thanks!


>  >
> >>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/audio/...
> >> chrome/browser/chromeos/audio/audio_handler.cc:166: pref_name ==
> >> prefs::kAudioCaptureAllowed);
> >> This should probably be
> >>
> >> if(..)
> >>   ApplyAudioPolicy()
> >> else
> >>   NOTREACHED();
> >
> > I disagree; the DCHECK makes our production code more efficient, and
> > represents what is actually being asserted on.  Having a DCHECK up
> > front when it was always the same pref (or one of a set of prefs) was
> > also a fairly common pattern throughout the code, so I guess we could
> > go either way, but for me the up-front DCHECK is clearer and less
> > verbose.
> >
> > Cheers,
> > Jói
> >
> >
> > On Tue, Oct 30, 2012 at 1:37 PM,  <mnissler@chromium.org> wrote:
> >> I only looked at the main changes in chrome/browser/prefs and
> base/prefs and
> >> a
> >> couple of arbitrarily chosen consumers.
> >>
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_init_ob...
> >> File base/prefs/public/pref_init_observer.h (right):
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_init_ob...
> >> base/prefs/public/pref_init_observer.h:13: public:
> >> Please add a comment here explaining when this gets called and what it
> >> is good for.
> >>
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observer.h
> >> File base/prefs/public/pref_observer.h (right):
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observe...
> >> base/prefs/public/pref_observer.h:13: public:
> >> should have a comment, and you should put a TODO saying that we'd like
> >> to replace that by a base::Callback-style interface.
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/audio/...
> >> File chrome/browser/chromeos/audio/audio_handler.cc (right):
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/audio/...
> >> chrome/browser/chromeos/audio/audio_handler.cc:166: pref_name ==
> >> prefs::kAudioCaptureAllowed);
> >> This should probably be
> >>
> >> if(..)
> >>   ApplyAudioPolicy()
> >> else
> >>   NOTREACHED();
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/drive/...
> >> File chrome/browser/chromeos/drive/drive_file_system.cc (right):
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/drive/...
> >> chrome/browser/chromeos/drive/drive_file_system.cc:2190:
> >> DCHECK(pref_name == prefs::kDisableDriveHostedFiles);
> >> same comment regarding if() {} else NOTREACHED();
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
> >> File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
> >> chrome/browser/prefs/pref_notifier_impl_unittest.cc:101:
> >> notifier.AddInitObserver(&observer);
> >> The init observer would also be better served through a callback as we
> >> can just clear the list of observers/callbacks after the event fires
> >> (i.e. Init only happens once).
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
> >> chrome/browser/prefs/pref_notifier_impl_unittest.cc:107:
> >> notifier.RemoveInitObserver(&observer);
> >> So this shouldn't be necessary.
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_serv...
> >> File chrome/browser/prefs/pref_service.h (right):
> >>
> >>
>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_serv...
> >> chrome/browser/prefs/pref_service.h:276: void
> >> AddPrefInitObserver(PrefInitObserver* obs);
> >> needs documentation
> >>
> >> http://codereview.chromium.org/11345008/
>

[Jói, 2012-10-30 14:51:39]

Looking at this some more, I am back to thinking it would be best to
land the current change with the PrefObserver and PrefInitObserver
things in place, and then we can take the time to revisit and fix the
interface (e.g. using base::Callback) if desired.

Things that are unclear in my mind and might take some time to figure out:
a) Should PrefChangeRegistrar really take a base::Callback rather than
echoing PrefStore::Callback?
b) If the answer to (a) is yes, then I guess you also want to get rid
of PrefStore::Callback?
c) If we switch PrefChangeRegistrar to taking base::Callback, then the
interface between it and PrefService (and ultimately PrefNotifierImpl)
needs to change; should this be base::Callback based?  Is that safe?
If not, what interface to use?  Back to PrefObserver?  Reuse
PrefStore::Observer?
d) Is it really a good thing to switch to passing a base::Callback for
an observer that is never manually removed?  Are we OK with using a
base::WeakPtr for that, or reasoning about base::Unretained lifetime
management?
e) Given the new relationship between PrefChangeRegistrar and
PrefService, perhaps PrefNotifier and PrefNotifierImpl can go away
altogether?  Since each PrefChangeRegistrar is already going to be
registering once with PrefService and iterating through pref names to
determine which callback to invoke, that duplicates what is currently
PrefNotifierImpl's responsibility, so we should probably collapse the
two and not have to iterate twice.

Since all of these might take some time to figure out, and the current
change makes things better by removing an unwanted dependency and
extracting pref notifications out of Observe() implementations and to
their own functions (even if follow-up changes would further allow
dispatching to individual functions), and since keeping a ~200 file
change alive for long will be very hard, I think the incremental
approach is best.  I wouldn't want us to rush through decisions on the
points above.

What do you think?  If you believe we can settle the above issues
today or no later than early tomorrow, then it might be possible to
keep the change open and only send it for review after all of the work
is done; otherwise I think it needs to be done incrementally.  Another
option is to abandon the current change, but I think since it is ready
and moves us in a useful direction, that would be wasteful.  I concede
that I could have planned my work better and sent out a preview of the
change earlier, but what's done is done.  I didn't imagine you would
ask for substantial changes to the semantics of the interface
(dispatching on pref names instead of passing names to a notification
handler).

Cheers,
Jói



On Tue, Oct 30, 2012 at 2:21 PM, Mattias Nissler <mnissler@chromium.org> wrote:
>
>
> On Tue, Oct 30, 2012 at 2:46 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> Also:  I will send a preview of the intended interface changes before
>> switching all the usages over.
>>
>> On Tue, Oct 30, 2012 at 1:45 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> > Thanks Mattias.
>> >
>> > As discussed off-line, I plan to switch to the Callback-based
>> > interface and send this for review again.  I think for this change, I
>> > will always have the Callback-based interface invoke on
>> > OnPreferenceChanged, and then in follow-up changes fix the places
>> > where OnPreferenceChanged would better be done as multiple
>> > OnXyzPreferenceChanged with independent dispatching.  That way we are
>> > switching to the right interface now, while letting me minimize the
>> > amount of change so that I can get this checked in soonish, before I
>> > end up in merge hell.  The follow-ups will then just be local
>> > improvements to how dispatching is done.
>
>
> Sounds good to me, thanks!
>
>>
>> >
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/audio/...
>> >> chrome/browser/chromeos/audio/audio_handler.cc:166: pref_name ==
>> >> prefs::kAudioCaptureAllowed);
>> >> This should probably be
>> >>
>> >> if(..)
>> >>   ApplyAudioPolicy()
>> >> else
>> >>   NOTREACHED();
>> >
>> > I disagree; the DCHECK makes our production code more efficient, and
>> > represents what is actually being asserted on.  Having a DCHECK up
>> > front when it was always the same pref (or one of a set of prefs) was
>> > also a fairly common pattern throughout the code, so I guess we could
>> > go either way, but for me the up-front DCHECK is clearer and less
>> > verbose.
>> >
>> > Cheers,
>> > Jói
>> >
>> >
>> > On Tue, Oct 30, 2012 at 1:37 PM,  <mnissler@chromium.org> wrote:
>> >> I only looked at the main changes in chrome/browser/prefs and
>> >> base/prefs and
>> >> a
>> >> couple of arbitrarily chosen consumers.
>> >>
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_init_ob...
>> >> File base/prefs/public/pref_init_observer.h (right):
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_init_ob...
>> >> base/prefs/public/pref_init_observer.h:13: public:
>> >> Please add a comment here explaining when this gets called and what it
>> >> is good for.
>> >>
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observer.h
>> >> File base/prefs/public/pref_observer.h (right):
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/base/prefs/public/pref_observe...
>> >> base/prefs/public/pref_observer.h:13: public:
>> >> should have a comment, and you should put a TODO saying that we'd like
>> >> to replace that by a base::Callback-style interface.
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/audio/...
>> >> File chrome/browser/chromeos/audio/audio_handler.cc (right):
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/audio/...
>> >> chrome/browser/chromeos/audio/audio_handler.cc:166: pref_name ==
>> >> prefs::kAudioCaptureAllowed);
>> >> This should probably be
>> >>
>> >> if(..)
>> >>   ApplyAudioPolicy()
>> >> else
>> >>   NOTREACHED();
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/drive/...
>> >> File chrome/browser/chromeos/drive/drive_file_system.cc (right):
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/chromeos/drive/...
>> >> chrome/browser/chromeos/drive/drive_file_system.cc:2190:
>> >> DCHECK(pref_name == prefs::kDisableDriveHostedFiles);
>> >> same comment regarding if() {} else NOTREACHED();
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
>> >> File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
>> >> chrome/browser/prefs/pref_notifier_impl_unittest.cc:101:
>> >> notifier.AddInitObserver(&observer);
>> >> The init observer would also be better served through a callback as we
>> >> can just clear the list of observers/callbacks after the event fires
>> >> (i.e. Init only happens once).
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_noti...
>> >> chrome/browser/prefs/pref_notifier_impl_unittest.cc:107:
>> >> notifier.RemoveInitObserver(&observer);
>> >> So this shouldn't be necessary.
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_serv...
>> >> File chrome/browser/prefs/pref_service.h (right):
>> >>
>> >>
>> >>
http://codereview.chromium.org/11345008/diff/1/chrome/browser/prefs/pref_serv...
>> >> chrome/browser/prefs/pref_service.h:276: void
>> >> AddPrefInitObserver(PrefInitObserver* obs);
>> >> needs documentation
>> >>
>> >> http://codereview.chromium.org/11345008/
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "browser-components-watch" group.
> To post to this group, send email to browser-components-watch@chromium.org.
> To unsubscribe from this group, send email to
> browser-components-watch+unsubscribe@chromium.org.
> For more options, visit
> https://groups.google.com/a/chromium.org/groups/opt_out.
>
>

[jam, 2012-10-30 15:15:04]

For the record, I had a draft written yesterday saying that these-single method
interfaces would be better as callbacks. But then I decided not to send it to
see what the owners of the code say first :)

In general, before we had the current callback system we tended to use
interfaces everywhere. While for interfaces with more than one method it's
sometimes awkward to switch all of them to callbacks, for an interface for one
method I think it's generally better to switch to a callback. Some reasons are:
-more flexibility, i.e. to dispatch to different methods as has been pointed out
-adding extra information to the callback via currying
-even dispatching to stating methods etc
-helps with lifetime issues: i.e. objects can bind using WeakPtrFactory. When
PrefService is calling the objects, it can look to see which callbacks are now
null and remove it automatically. This means no unregistration method is needed.

btw when I was refactoring content, I switched single-method interfaces to
callbacks.

One other minor comment: does the callback need a PrefService pointer? There'll
only be one PrefService up at a time (which is easily accessible using the
static getter), so this seems unnecessary.


To be clear, my preference is to have callbacks but it's up to you guys on
when/how to do this. Given how much usage there is, I agree that trying to do it
in one shot will be quite painful. Even ignoring merge-hell, one subtle bug that
sneaks in can mean a very large change gets reverted which is a time sink. Much
better to figure out a way to incrementally switch over.

[Jói, 2012-10-30 15:20:37]

Thanks for the input John.

After discussing with Mattias, I also agree that switching to
callbacks is nicer.  It's just after looking at trying to do it as
part of this change, too many questions came up for me to think we can
do it all in one shot without getting into merge hell, or bugs that
lead to reverts, as you say.  Some of the questions are just around a
coherent design for notifications from Prefs, since it looks like we
can perhaps simplify further than just the PrefChangeRegistrar object.

I think we're probably all getting onto the same page here.  Mattias,
I know I've gone back and forth once already today on trying to switch
to callbacks all in one fell swoop, but I think you had tacitly agreed
earlier today that we could land this change first and then switch to
the "correct" interface in follow-up changes.  Are you still OK with
that?

Cheers,
Jói



On Tue, Oct 30, 2012 at 3:15 PM,  <jam@chromium.org> wrote:
> For the record, I had a draft written yesterday saying that these-single
> method
> interfaces would be better as callbacks. But then I decided not to send it
> to
> see what the owners of the code say first :)
>
> In general, before we had the current callback system we tended to use
> interfaces everywhere. While for interfaces with more than one method it's
> sometimes awkward to switch all of them to callbacks, for an interface for
> one
> method I think it's generally better to switch to a callback. Some reasons
> are:
> -more flexibility, i.e. to dispatch to different methods as has been pointed
> out
> -adding extra information to the callback via currying
> -even dispatching to stating methods etc
> -helps with lifetime issues: i.e. objects can bind using WeakPtrFactory.
> When
> PrefService is calling the objects, it can look to see which callbacks are
> now
> null and remove it automatically. This means no unregistration method is
> needed.
>
> btw when I was refactoring content, I switched single-method interfaces to
> callbacks.
>
> One other minor comment: does the callback need a PrefService pointer?
> There'll
> only be one PrefService up at a time (which is easily accessible using the
> static getter), so this seems unnecessary.
>
>
> To be clear, my preference is to have callbacks but it's up to you guys on
> when/how to do this. Given how much usage there is, I agree that trying to
> do it
> in one shot will be quite painful. Even ignoring merge-hell, one subtle bug
> that
> sneaks in can mean a very large change gets reverted which is a time sink.
> Much
> better to figure out a way to incrementally switch over.
>
> http://codereview.chromium.org/11345008/

[jam, 2012-10-30 15:34:06]

On Tue, Oct 30, 2012 at 8:19 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> Thanks for the input John.
>
> After discussing with Mattias, I also agree that switching to
> callbacks is nicer.  It's just after looking at trying to do it as
> part of this change, too many questions came up for me to think we can
> do it all in one shot without getting into merge hell, or bugs that
> lead to reverts, as you say.  Some of the questions are just around a
> coherent design for notifications from Prefs, since it looks like we
> can perhaps simplify further than just the PrefChangeRegistrar object.
>
> I think we're probably all getting onto the same page here.  Mattias,
> I know I've gone back and forth once already today on trying to switch
> to callbacks all in one fell swoop, but I think you had tacitly agreed
> earlier today that we could land this change first and then switch to
> the "correct" interface in follow-up changes.  Are you still OK with
> that?
>

btw I agree this will be the only sane way of doing this :)

Personally I have been burnt a number of times when I try to do something
big like this in one change. I'm sure you're already thinking about this: i
suggest that in followups, both teh interface and callback approach are
supported by pref service so that the code can be switched in many parallel
changes instead of one. That way if one change is problematic, only that
change gets reverted.


> Cheers,
> Jói
>
>
>
> On Tue, Oct 30, 2012 at 3:15 PM,  <jam@chromium.org> wrote:
> > For the record, I had a draft written yesterday saying that these-single
> > method
> > interfaces would be better as callbacks. But then I decided not to send
> it
> > to
> > see what the owners of the code say first :)
> >
> > In general, before we had the current callback system we tended to use
> > interfaces everywhere. While for interfaces with more than one method
> it's
> > sometimes awkward to switch all of them to callbacks, for an interface
> for
> > one
> > method I think it's generally better to switch to a callback. Some
> reasons
> > are:
> > -more flexibility, i.e. to dispatch to different methods as has been
> pointed
> > out
> > -adding extra information to the callback via currying
> > -even dispatching to stating methods etc
> > -helps with lifetime issues: i.e. objects can bind using WeakPtrFactory.
> > When
> > PrefService is calling the objects, it can look to see which callbacks
> are
> > now
> > null and remove it automatically. This means no unregistration method is
> > needed.
> >
> > btw when I was refactoring content, I switched single-method interfaces
> to
> > callbacks.
> >
> > One other minor comment: does the callback need a PrefService pointer?
> > There'll
> > only be one PrefService up at a time (which is easily accessible using
> the
> > static getter), so this seems unnecessary.
> >
> >
> > To be clear, my preference is to have callbacks but it's up to you guys
> on
> > when/how to do this. Given how much usage there is, I agree that trying
> to
> > do it
> > in one shot will be quite painful. Even ignoring merge-hell, one subtle
> bug
> > that
> > sneaks in can mean a very large change gets reverted which is a time
> sink.
> > Much
> > better to figure out a way to incrementally switch over.
> >
> > http://codereview.chromium.org/11345008/
>

[Jói, 2012-10-30 15:37:05]

Yep, that's the plan.  In the follow-ups, we would leave the
PrefObserver approach of getting notified enabled, and add a
base::Callback approach, and only remove the PrefObserver approach
once everything is converted.

Cheers,
Jói


On Tue, Oct 30, 2012 at 3:34 PM, John Abd-El-Malek <jam@chromium.org> wrote:
>
>
> On Tue, Oct 30, 2012 at 8:19 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> Thanks for the input John.
>>
>> After discussing with Mattias, I also agree that switching to
>> callbacks is nicer.  It's just after looking at trying to do it as
>> part of this change, too many questions came up for me to think we can
>> do it all in one shot without getting into merge hell, or bugs that
>> lead to reverts, as you say.  Some of the questions are just around a
>> coherent design for notifications from Prefs, since it looks like we
>> can perhaps simplify further than just the PrefChangeRegistrar object.
>>
>> I think we're probably all getting onto the same page here.  Mattias,
>> I know I've gone back and forth once already today on trying to switch
>> to callbacks all in one fell swoop, but I think you had tacitly agreed
>> earlier today that we could land this change first and then switch to
>> the "correct" interface in follow-up changes.  Are you still OK with
>> that?
>
>
> btw I agree this will be the only sane way of doing this :)
>
> Personally I have been burnt a number of times when I try to do something
> big like this in one change. I'm sure you're already thinking about this: i
> suggest that in followups, both teh interface and callback approach are
> supported by pref service so that the code can be switched in many parallel
> changes instead of one. That way if one change is problematic, only that
> change gets reverted.
>
>>
>> Cheers,
>> Jói
>>
>>
>>
>> On Tue, Oct 30, 2012 at 3:15 PM,  <jam@chromium.org> wrote:
>> > For the record, I had a draft written yesterday saying that these-single
>> > method
>> > interfaces would be better as callbacks. But then I decided not to send
>> > it
>> > to
>> > see what the owners of the code say first :)
>> >
>> > In general, before we had the current callback system we tended to use
>> > interfaces everywhere. While for interfaces with more than one method
>> > it's
>> > sometimes awkward to switch all of them to callbacks, for an interface
>> > for
>> > one
>> > method I think it's generally better to switch to a callback. Some
>> > reasons
>> > are:
>> > -more flexibility, i.e. to dispatch to different methods as has been
>> > pointed
>> > out
>> > -adding extra information to the callback via currying
>> > -even dispatching to stating methods etc
>> > -helps with lifetime issues: i.e. objects can bind using WeakPtrFactory.
>> > When
>> > PrefService is calling the objects, it can look to see which callbacks
>> > are
>> > now
>> > null and remove it automatically. This means no unregistration method is
>> > needed.
>> >
>> > btw when I was refactoring content, I switched single-method interfaces
>> > to
>> > callbacks.
>> >
>> > One other minor comment: does the callback need a PrefService pointer?
>> > There'll
>> > only be one PrefService up at a time (which is easily accessible using
>> > the
>> > static getter), so this seems unnecessary.
>> >
>> >
>> > To be clear, my preference is to have callbacks but it's up to you guys
>> > on
>> > when/how to do this. Given how much usage there is, I agree that trying
>> > to
>> > do it
>> > in one shot will be quite painful. Even ignoring merge-hell, one subtle
>> > bug
>> > that
>> > sneaks in can mean a very large change gets reverted which is a time
>> > sink.
>> > Much
>> > better to figure out a way to incrementally switch over.
>> >
>> > http://codereview.chromium.org/11345008/
>
>

[Mattias Nissler (ping if slow), 2012-10-30 15:58:28]

On Tue, Oct 30, 2012 at 4:36 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> Yep, that's the plan.  In the follow-ups, we would leave the
> PrefObserver approach of getting notified enabled, and add a
> base::Callback approach, and only remove the PrefObserver approach
> once everything is converted.
>
> Cheers,
> Jói
>
>
> On Tue, Oct 30, 2012 at 3:34 PM, John Abd-El-Malek <jam@chromium.org>
> wrote:
> >
> >
> > On Tue, Oct 30, 2012 at 8:19 AM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >>
> >> Thanks for the input John.
> >>
> >> After discussing with Mattias, I also agree that switching to
> >> callbacks is nicer.  It's just after looking at trying to do it as
> >> part of this change, too many questions came up for me to think we can
> >> do it all in one shot without getting into merge hell, or bugs that
> >> lead to reverts, as you say.  Some of the questions are just around a
> >> coherent design for notifications from Prefs, since it looks like we
> >> can perhaps simplify further than just the PrefChangeRegistrar object.
> >>
> >> I think we're probably all getting onto the same page here.  Mattias,
> >> I know I've gone back and forth once already today on trying to switch
> >> to callbacks all in one fell swoop, but I think you had tacitly agreed
> >> earlier today that we could land this change first and then switch to
> >> the "correct" interface in follow-up changes.  Are you still OK with
> >> that?
>

I'm still OK with that, but I honestly think it'll make your life harder
because (as you noted) you have to get OWNERS approval twice for all
consumers instead of once. Note that in either approach, you can keep the
old and the new interface in the tree at the same time and convert
individual consumers one-by-one, so this work doesn't require monster CLs
at all.

>
> >
> > btw I agree this will be the only sane way of doing this :)
> >
> > Personally I have been burnt a number of times when I try to do something
> > big like this in one change. I'm sure you're already thinking about
> this: i
> > suggest that in followups, both teh interface and callback approach are
> > supported by pref service so that the code can be switched in many
> parallel
> > changes instead of one. That way if one change is problematic, only that
> > change gets reverted.
> >
> >>
> >> Cheers,
> >> Jói
> >>
> >>
> >>
> >> On Tue, Oct 30, 2012 at 3:15 PM,  <jam@chromium.org> wrote:
> >> > For the record, I had a draft written yesterday saying that
> these-single
> >> > method
> >> > interfaces would be better as callbacks. But then I decided not to
> send
> >> > it
> >> > to
> >> > see what the owners of the code say first :)
> >> >
> >> > In general, before we had the current callback system we tended to use
> >> > interfaces everywhere. While for interfaces with more than one method
> >> > it's
> >> > sometimes awkward to switch all of them to callbacks, for an interface
> >> > for
> >> > one
> >> > method I think it's generally better to switch to a callback. Some
> >> > reasons
> >> > are:
> >> > -more flexibility, i.e. to dispatch to different methods as has been
> >> > pointed
> >> > out
> >> > -adding extra information to the callback via currying
> >> > -even dispatching to stating methods etc
> >> > -helps with lifetime issues: i.e. objects can bind using
> WeakPtrFactory.
> >> > When
> >> > PrefService is calling the objects, it can look to see which callbacks
> >> > are
> >> > now
> >> > null and remove it automatically. This means no unregistration method
> is
> >> > needed.
> >> >
> >> > btw when I was refactoring content, I switched single-method
> interfaces
> >> > to
> >> > callbacks.
> >> >
> >> > One other minor comment: does the callback need a PrefService pointer?
> >> > There'll
> >> > only be one PrefService up at a time (which is easily accessible using
> >> > the
> >> > static getter), so this seems unnecessary.
> >> >
> >> >
> >> > To be clear, my preference is to have callbacks but it's up to you
> guys
> >> > on
> >> > when/how to do this. Given how much usage there is, I agree that
> trying
> >> > to
> >> > do it
> >> > in one shot will be quite painful. Even ignoring merge-hell, one
> subtle
> >> > bug
> >> > that
> >> > sneaks in can mean a very large change gets reverted which is a time
> >> > sink.
> >> > Much
> >> > better to figure out a way to incrementally switch over.
> >> >
> >> > http://codereview.chromium.org/11345008/
> >
> >
>

[Jói, 2012-10-30 16:02:45]

Thanks Mattias.

I'd like to go ahead with the current change, just extracting the
stuff from switch statements etc. was already close to a couple of
days of work so I don't want to re-do that bit, even if it means two
rounds of OWNERS approvals.  I realize I should have planned this
change more carefully.

I've uploaded a new patch that does the following to address your
earlier review comments:
a) Removes PrefInitObserver, replacing it with base::Callback<void(bool)>.
b) Adds a TODO on PrefObserver.
c) Improves documentation as requested.

Cheers,
Jói


On Tue, Oct 30, 2012 at 3:58 PM, Mattias Nissler <mnissler@chromium.org> wrote:
>
>
> On Tue, Oct 30, 2012 at 4:36 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> Yep, that's the plan.  In the follow-ups, we would leave the
>> PrefObserver approach of getting notified enabled, and add a
>> base::Callback approach, and only remove the PrefObserver approach
>> once everything is converted.
>>
>> Cheers,
>> Jói
>>
>>
>> On Tue, Oct 30, 2012 at 3:34 PM, John Abd-El-Malek <jam@chromium.org>
>> wrote:
>> >
>> >
>> > On Tue, Oct 30, 2012 at 8:19 AM, Jói Sigurðsson <joi@chromium.org>
>> > wrote:
>> >>
>> >> Thanks for the input John.
>> >>
>> >> After discussing with Mattias, I also agree that switching to
>> >> callbacks is nicer.  It's just after looking at trying to do it as
>> >> part of this change, too many questions came up for me to think we can
>> >> do it all in one shot without getting into merge hell, or bugs that
>> >> lead to reverts, as you say.  Some of the questions are just around a
>> >> coherent design for notifications from Prefs, since it looks like we
>> >> can perhaps simplify further than just the PrefChangeRegistrar object.
>> >>
>> >> I think we're probably all getting onto the same page here.  Mattias,
>> >> I know I've gone back and forth once already today on trying to switch
>> >> to callbacks all in one fell swoop, but I think you had tacitly agreed
>> >> earlier today that we could land this change first and then switch to
>> >> the "correct" interface in follow-up changes.  Are you still OK with
>> >> that?
>
>
> I'm still OK with that, but I honestly think it'll make your life harder
> because (as you noted) you have to get OWNERS approval twice for all
> consumers instead of once. Note that in either approach, you can keep the
> old and the new interface in the tree at the same time and convert
> individual consumers one-by-one, so this work doesn't require monster CLs at
> all.
>
>> >
>> >
>> > btw I agree this will be the only sane way of doing this :)
>> >
>> > Personally I have been burnt a number of times when I try to do
>> > something
>> > big like this in one change. I'm sure you're already thinking about
>> > this: i
>> > suggest that in followups, both teh interface and callback approach are
>> > supported by pref service so that the code can be switched in many
>> > parallel
>> > changes instead of one. That way if one change is problematic, only that
>> > change gets reverted.
>> >
>> >>
>> >> Cheers,
>> >> Jói
>> >>
>> >>
>> >>
>> >> On Tue, Oct 30, 2012 at 3:15 PM,  <jam@chromium.org> wrote:
>> >> > For the record, I had a draft written yesterday saying that
>> >> > these-single
>> >> > method
>> >> > interfaces would be better as callbacks. But then I decided not to
>> >> > send
>> >> > it
>> >> > to
>> >> > see what the owners of the code say first :)
>> >> >
>> >> > In general, before we had the current callback system we tended to
>> >> > use
>> >> > interfaces everywhere. While for interfaces with more than one method
>> >> > it's
>> >> > sometimes awkward to switch all of them to callbacks, for an
>> >> > interface
>> >> > for
>> >> > one
>> >> > method I think it's generally better to switch to a callback. Some
>> >> > reasons
>> >> > are:
>> >> > -more flexibility, i.e. to dispatch to different methods as has been
>> >> > pointed
>> >> > out
>> >> > -adding extra information to the callback via currying
>> >> > -even dispatching to stating methods etc
>> >> > -helps with lifetime issues: i.e. objects can bind using
>> >> > WeakPtrFactory.
>> >> > When
>> >> > PrefService is calling the objects, it can look to see which
>> >> > callbacks
>> >> > are
>> >> > now
>> >> > null and remove it automatically. This means no unregistration method
>> >> > is
>> >> > needed.
>> >> >
>> >> > btw when I was refactoring content, I switched single-method
>> >> > interfaces
>> >> > to
>> >> > callbacks.
>> >> >
>> >> > One other minor comment: does the callback need a PrefService
>> >> > pointer?
>> >> > There'll
>> >> > only be one PrefService up at a time (which is easily accessible
>> >> > using
>> >> > the
>> >> > static getter), so this seems unnecessary.
>> >> >
>> >> >
>> >> > To be clear, my preference is to have callbacks but it's up to you
>> >> > guys
>> >> > on
>> >> > when/how to do this. Given how much usage there is, I agree that
>> >> > trying
>> >> > to
>> >> > do it
>> >> > in one shot will be quite painful. Even ignoring merge-hell, one
>> >> > subtle
>> >> > bug
>> >> > that
>> >> > sneaks in can mean a very large change gets reverted which is a time
>> >> > sink.
>> >> > Much
>> >> > better to figure out a way to incrementally switch over.
>> >> >
>> >> > http://codereview.chromium.org/11345008/
>> >
>> >
>
>

[Mattias Nissler (ping if slow), 2012-10-30 16:04:39]

Fine with me, let me review the CL.

On Tue, Oct 30, 2012 at 5:02 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> Thanks Mattias.
>
> I'd like to go ahead with the current change, just extracting the
> stuff from switch statements etc. was already close to a couple of
> days of work so I don't want to re-do that bit, even if it means two
> rounds of OWNERS approvals.  I realize I should have planned this
> change more carefully.
>
> I've uploaded a new patch that does the following to address your
> earlier review comments:
> a) Removes PrefInitObserver, replacing it with base::Callback<void(bool)>.
> b) Adds a TODO on PrefObserver.
> c) Improves documentation as requested.
>
> Cheers,
> Jói
>
>
> On Tue, Oct 30, 2012 at 3:58 PM, Mattias Nissler <mnissler@chromium.org>
> wrote:
> >
> >
> > On Tue, Oct 30, 2012 at 4:36 PM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >>
> >> Yep, that's the plan.  In the follow-ups, we would leave the
> >> PrefObserver approach of getting notified enabled, and add a
> >> base::Callback approach, and only remove the PrefObserver approach
> >> once everything is converted.
> >>
> >> Cheers,
> >> Jói
> >>
> >>
> >> On Tue, Oct 30, 2012 at 3:34 PM, John Abd-El-Malek <jam@chromium.org>
> >> wrote:
> >> >
> >> >
> >> > On Tue, Oct 30, 2012 at 8:19 AM, Jói Sigurðsson <joi@chromium.org>
> >> > wrote:
> >> >>
> >> >> Thanks for the input John.
> >> >>
> >> >> After discussing with Mattias, I also agree that switching to
> >> >> callbacks is nicer.  It's just after looking at trying to do it as
> >> >> part of this change, too many questions came up for me to think we
> can
> >> >> do it all in one shot without getting into merge hell, or bugs that
> >> >> lead to reverts, as you say.  Some of the questions are just around a
> >> >> coherent design for notifications from Prefs, since it looks like we
> >> >> can perhaps simplify further than just the PrefChangeRegistrar
> object.
> >> >>
> >> >> I think we're probably all getting onto the same page here.  Mattias,
> >> >> I know I've gone back and forth once already today on trying to
> switch
> >> >> to callbacks all in one fell swoop, but I think you had tacitly
> agreed
> >> >> earlier today that we could land this change first and then switch to
> >> >> the "correct" interface in follow-up changes.  Are you still OK with
> >> >> that?
> >
> >
> > I'm still OK with that, but I honestly think it'll make your life harder
> > because (as you noted) you have to get OWNERS approval twice for all
> > consumers instead of once. Note that in either approach, you can keep the
> > old and the new interface in the tree at the same time and convert
> > individual consumers one-by-one, so this work doesn't require monster
> CLs at
> > all.
> >
> >> >
> >> >
> >> > btw I agree this will be the only sane way of doing this :)
> >> >
> >> > Personally I have been burnt a number of times when I try to do
> >> > something
> >> > big like this in one change. I'm sure you're already thinking about
> >> > this: i
> >> > suggest that in followups, both teh interface and callback approach
> are
> >> > supported by pref service so that the code can be switched in many
> >> > parallel
> >> > changes instead of one. That way if one change is problematic, only
> that
> >> > change gets reverted.
> >> >
> >> >>
> >> >> Cheers,
> >> >> Jói
> >> >>
> >> >>
> >> >>
> >> >> On Tue, Oct 30, 2012 at 3:15 PM,  <jam@chromium.org> wrote:
> >> >> > For the record, I had a draft written yesterday saying that
> >> >> > these-single
> >> >> > method
> >> >> > interfaces would be better as callbacks. But then I decided not to
> >> >> > send
> >> >> > it
> >> >> > to
> >> >> > see what the owners of the code say first :)
> >> >> >
> >> >> > In general, before we had the current callback system we tended to
> >> >> > use
> >> >> > interfaces everywhere. While for interfaces with more than one
> method
> >> >> > it's
> >> >> > sometimes awkward to switch all of them to callbacks, for an
> >> >> > interface
> >> >> > for
> >> >> > one
> >> >> > method I think it's generally better to switch to a callback. Some
> >> >> > reasons
> >> >> > are:
> >> >> > -more flexibility, i.e. to dispatch to different methods as has
> been
> >> >> > pointed
> >> >> > out
> >> >> > -adding extra information to the callback via currying
> >> >> > -even dispatching to stating methods etc
> >> >> > -helps with lifetime issues: i.e. objects can bind using
> >> >> > WeakPtrFactory.
> >> >> > When
> >> >> > PrefService is calling the objects, it can look to see which
> >> >> > callbacks
> >> >> > are
> >> >> > now
> >> >> > null and remove it automatically. This means no unregistration
> method
> >> >> > is
> >> >> > needed.
> >> >> >
> >> >> > btw when I was refactoring content, I switched single-method
> >> >> > interfaces
> >> >> > to
> >> >> > callbacks.
> >> >> >
> >> >> > One other minor comment: does the callback need a PrefService
> >> >> > pointer?
> >> >> > There'll
> >> >> > only be one PrefService up at a time (which is easily accessible
> >> >> > using
> >> >> > the
> >> >> > static getter), so this seems unnecessary.
> >> >> >
> >> >> >
> >> >> > To be clear, my preference is to have callbacks but it's up to you
> >> >> > guys
> >> >> > on
> >> >> > when/how to do this. Given how much usage there is, I agree that
> >> >> > trying
> >> >> > to
> >> >> > do it
> >> >> > in one shot will be quite painful. Even ignoring merge-hell, one
> >> >> > subtle
> >> >> > bug
> >> >> > that
> >> >> > sneaks in can mean a very large change gets reverted which is a
> time
> >> >> > sink.
> >> >> > Much
> >> >> > better to figure out a way to incrementally switch over.
> >> >> >
> >> >> > http://codereview.chromium.org/11345008/
> >> >
> >> >
> >
> >
>

[jam, 2012-10-30 16:09:01]

On Tue, Oct 30, 2012 at 9:02 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> Thanks Mattias.
>
> I'd like to go ahead with the current change, just extracting the
> stuff from switch statements etc. was already close to a couple of
> days of work so I don't want to re-do that bit, even if it means two
> rounds of OWNERS approvals.  I realize I should have planned this
> change more carefully.
>

small note: when OWNERS got added, it was pointed out that it would hinder
refactorings. i.e. if someone changes a function in base, they would need
to get OWNERS approval for all the places that call it, which is tedious
and a waste of time. What really matters is that an owner of the base API
lgtm'd the changes to the API. After that, the changes to convert every
caller can be TBR'd to an owner, and just have any other person review it.
This is the model I used with content refactoring. I suggest the same here.


> I've uploaded a new patch that does the following to address your
> earlier review comments:
> a) Removes PrefInitObserver, replacing it with base::Callback<void(bool)>.
> b) Adds a TODO on PrefObserver.
> c) Improves documentation as requested.
>
> Cheers,
> Jói
>
>
> On Tue, Oct 30, 2012 at 3:58 PM, Mattias Nissler <mnissler@chromium.org>
> wrote:
> >
> >
> > On Tue, Oct 30, 2012 at 4:36 PM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >>
> >> Yep, that's the plan.  In the follow-ups, we would leave the
> >> PrefObserver approach of getting notified enabled, and add a
> >> base::Callback approach, and only remove the PrefObserver approach
> >> once everything is converted.
> >>
> >> Cheers,
> >> Jói
> >>
> >>
> >> On Tue, Oct 30, 2012 at 3:34 PM, John Abd-El-Malek <jam@chromium.org>
> >> wrote:
> >> >
> >> >
> >> > On Tue, Oct 30, 2012 at 8:19 AM, Jói Sigurðsson <joi@chromium.org>
> >> > wrote:
> >> >>
> >> >> Thanks for the input John.
> >> >>
> >> >> After discussing with Mattias, I also agree that switching to
> >> >> callbacks is nicer.  It's just after looking at trying to do it as
> >> >> part of this change, too many questions came up for me to think we
> can
> >> >> do it all in one shot without getting into merge hell, or bugs that
> >> >> lead to reverts, as you say.  Some of the questions are just around a
> >> >> coherent design for notifications from Prefs, since it looks like we
> >> >> can perhaps simplify further than just the PrefChangeRegistrar
> object.
> >> >>
> >> >> I think we're probably all getting onto the same page here.  Mattias,
> >> >> I know I've gone back and forth once already today on trying to
> switch
> >> >> to callbacks all in one fell swoop, but I think you had tacitly
> agreed
> >> >> earlier today that we could land this change first and then switch to
> >> >> the "correct" interface in follow-up changes.  Are you still OK with
> >> >> that?
> >
> >
> > I'm still OK with that, but I honestly think it'll make your life harder
> > because (as you noted) you have to get OWNERS approval twice for all
> > consumers instead of once. Note that in either approach, you can keep the
> > old and the new interface in the tree at the same time and convert
> > individual consumers one-by-one, so this work doesn't require monster
> CLs at
> > all.
> >
> >> >
> >> >
> >> > btw I agree this will be the only sane way of doing this :)
> >> >
> >> > Personally I have been burnt a number of times when I try to do
> >> > something
> >> > big like this in one change. I'm sure you're already thinking about
> >> > this: i
> >> > suggest that in followups, both teh interface and callback approach
> are
> >> > supported by pref service so that the code can be switched in many
> >> > parallel
> >> > changes instead of one. That way if one change is problematic, only
> that
> >> > change gets reverted.
> >> >
> >> >>
> >> >> Cheers,
> >> >> Jói
> >> >>
> >> >>
> >> >>
> >> >> On Tue, Oct 30, 2012 at 3:15 PM,  <jam@chromium.org> wrote:
> >> >> > For the record, I had a draft written yesterday saying that
> >> >> > these-single
> >> >> > method
> >> >> > interfaces would be better as callbacks. But then I decided not to
> >> >> > send
> >> >> > it
> >> >> > to
> >> >> > see what the owners of the code say first :)
> >> >> >
> >> >> > In general, before we had the current callback system we tended to
> >> >> > use
> >> >> > interfaces everywhere. While for interfaces with more than one
> method
> >> >> > it's
> >> >> > sometimes awkward to switch all of them to callbacks, for an
> >> >> > interface
> >> >> > for
> >> >> > one
> >> >> > method I think it's generally better to switch to a callback. Some
> >> >> > reasons
> >> >> > are:
> >> >> > -more flexibility, i.e. to dispatch to different methods as has
> been
> >> >> > pointed
> >> >> > out
> >> >> > -adding extra information to the callback via currying
> >> >> > -even dispatching to stating methods etc
> >> >> > -helps with lifetime issues: i.e. objects can bind using
> >> >> > WeakPtrFactory.
> >> >> > When
> >> >> > PrefService is calling the objects, it can look to see which
> >> >> > callbacks
> >> >> > are
> >> >> > now
> >> >> > null and remove it automatically. This means no unregistration
> method
> >> >> > is
> >> >> > needed.
> >> >> >
> >> >> > btw when I was refactoring content, I switched single-method
> >> >> > interfaces
> >> >> > to
> >> >> > callbacks.
> >> >> >
> >> >> > One other minor comment: does the callback need a PrefService
> >> >> > pointer?
> >> >> > There'll
> >> >> > only be one PrefService up at a time (which is easily accessible
> >> >> > using
> >> >> > the
> >> >> > static getter), so this seems unnecessary.
> >> >> >
> >> >> >
> >> >> > To be clear, my preference is to have callbacks but it's up to you
> >> >> > guys
> >> >> > on
> >> >> > when/how to do this. Given how much usage there is, I agree that
> >> >> > trying
> >> >> > to
> >> >> > do it
> >> >> > in one shot will be quite painful. Even ignoring merge-hell, one
> >> >> > subtle
> >> >> > bug
> >> >> > that
> >> >> > sneaks in can mean a very large change gets reverted which is a
> time
> >> >> > sink.
> >> >> > Much
> >> >> > better to figure out a way to incrementally switch over.
> >> >> >
> >> >> > http://codereview.chromium.org/11345008/
> >> >
> >> >
> >
> >
>

[Jói, 2012-10-30 16:10:38]

That makes sense.

Mattias, if you're up for looking through the usage updates so that I
can TBR it to OWNERS, that would be great.  If not, I can ask someone
on the Browser Components team.

Cheers,
Jói


On Tue, Oct 30, 2012 at 4:08 PM, John Abd-El-Malek <jam@chromium.org> wrote:
>
>
> On Tue, Oct 30, 2012 at 9:02 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> Thanks Mattias.
>>
>> I'd like to go ahead with the current change, just extracting the
>> stuff from switch statements etc. was already close to a couple of
>> days of work so I don't want to re-do that bit, even if it means two
>> rounds of OWNERS approvals.  I realize I should have planned this
>> change more carefully.
>
>
> small note: when OWNERS got added, it was pointed out that it would hinder
> refactorings. i.e. if someone changes a function in base, they would need to
> get OWNERS approval for all the places that call it, which is tedious and a
> waste of time. What really matters is that an owner of the base API lgtm'd
> the changes to the API. After that, the changes to convert every caller can
> be TBR'd to an owner, and just have any other person review it. This is the
> model I used with content refactoring. I suggest the same here.
>
>>
>> I've uploaded a new patch that does the following to address your
>> earlier review comments:
>> a) Removes PrefInitObserver, replacing it with base::Callback<void(bool)>.
>> b) Adds a TODO on PrefObserver.
>> c) Improves documentation as requested.
>>
>> Cheers,
>> Jói
>>
>>
>> On Tue, Oct 30, 2012 at 3:58 PM, Mattias Nissler <mnissler@chromium.org>
>> wrote:
>> >
>> >
>> > On Tue, Oct 30, 2012 at 4:36 PM, Jói Sigurðsson <joi@chromium.org>
>> > wrote:
>> >>
>> >> Yep, that's the plan.  In the follow-ups, we would leave the
>> >> PrefObserver approach of getting notified enabled, and add a
>> >> base::Callback approach, and only remove the PrefObserver approach
>> >> once everything is converted.
>> >>
>> >> Cheers,
>> >> Jói
>> >>
>> >>
>> >> On Tue, Oct 30, 2012 at 3:34 PM, John Abd-El-Malek <jam@chromium.org>
>> >> wrote:
>> >> >
>> >> >
>> >> > On Tue, Oct 30, 2012 at 8:19 AM, Jói Sigurðsson <joi@chromium.org>
>> >> > wrote:
>> >> >>
>> >> >> Thanks for the input John.
>> >> >>
>> >> >> After discussing with Mattias, I also agree that switching to
>> >> >> callbacks is nicer.  It's just after looking at trying to do it as
>> >> >> part of this change, too many questions came up for me to think we
>> >> >> can
>> >> >> do it all in one shot without getting into merge hell, or bugs that
>> >> >> lead to reverts, as you say.  Some of the questions are just around
>> >> >> a
>> >> >> coherent design for notifications from Prefs, since it looks like we
>> >> >> can perhaps simplify further than just the PrefChangeRegistrar
>> >> >> object.
>> >> >>
>> >> >> I think we're probably all getting onto the same page here.
>> >> >> Mattias,
>> >> >> I know I've gone back and forth once already today on trying to
>> >> >> switch
>> >> >> to callbacks all in one fell swoop, but I think you had tacitly
>> >> >> agreed
>> >> >> earlier today that we could land this change first and then switch
>> >> >> to
>> >> >> the "correct" interface in follow-up changes.  Are you still OK with
>> >> >> that?
>> >
>> >
>> > I'm still OK with that, but I honestly think it'll make your life harder
>> > because (as you noted) you have to get OWNERS approval twice for all
>> > consumers instead of once. Note that in either approach, you can keep
>> > the
>> > old and the new interface in the tree at the same time and convert
>> > individual consumers one-by-one, so this work doesn't require monster
>> > CLs at
>> > all.
>> >
>> >> >
>> >> >
>> >> > btw I agree this will be the only sane way of doing this :)
>> >> >
>> >> > Personally I have been burnt a number of times when I try to do
>> >> > something
>> >> > big like this in one change. I'm sure you're already thinking about
>> >> > this: i
>> >> > suggest that in followups, both teh interface and callback approach
>> >> > are
>> >> > supported by pref service so that the code can be switched in many
>> >> > parallel
>> >> > changes instead of one. That way if one change is problematic, only
>> >> > that
>> >> > change gets reverted.
>> >> >
>> >> >>
>> >> >> Cheers,
>> >> >> Jói
>> >> >>
>> >> >>
>> >> >>
>> >> >> On Tue, Oct 30, 2012 at 3:15 PM,  <jam@chromium.org> wrote:
>> >> >> > For the record, I had a draft written yesterday saying that
>> >> >> > these-single
>> >> >> > method
>> >> >> > interfaces would be better as callbacks. But then I decided not to
>> >> >> > send
>> >> >> > it
>> >> >> > to
>> >> >> > see what the owners of the code say first :)
>> >> >> >
>> >> >> > In general, before we had the current callback system we tended to
>> >> >> > use
>> >> >> > interfaces everywhere. While for interfaces with more than one
>> >> >> > method
>> >> >> > it's
>> >> >> > sometimes awkward to switch all of them to callbacks, for an
>> >> >> > interface
>> >> >> > for
>> >> >> > one
>> >> >> > method I think it's generally better to switch to a callback. Some
>> >> >> > reasons
>> >> >> > are:
>> >> >> > -more flexibility, i.e. to dispatch to different methods as has
>> >> >> > been
>> >> >> > pointed
>> >> >> > out
>> >> >> > -adding extra information to the callback via currying
>> >> >> > -even dispatching to stating methods etc
>> >> >> > -helps with lifetime issues: i.e. objects can bind using
>> >> >> > WeakPtrFactory.
>> >> >> > When
>> >> >> > PrefService is calling the objects, it can look to see which
>> >> >> > callbacks
>> >> >> > are
>> >> >> > now
>> >> >> > null and remove it automatically. This means no unregistration
>> >> >> > method
>> >> >> > is
>> >> >> > needed.
>> >> >> >
>> >> >> > btw when I was refactoring content, I switched single-method
>> >> >> > interfaces
>> >> >> > to
>> >> >> > callbacks.
>> >> >> >
>> >> >> > One other minor comment: does the callback need a PrefService
>> >> >> > pointer?
>> >> >> > There'll
>> >> >> > only be one PrefService up at a time (which is easily accessible
>> >> >> > using
>> >> >> > the
>> >> >> > static getter), so this seems unnecessary.
>> >> >> >
>> >> >> >
>> >> >> > To be clear, my preference is to have callbacks but it's up to you
>> >> >> > guys
>> >> >> > on
>> >> >> > when/how to do this. Given how much usage there is, I agree that
>> >> >> > trying
>> >> >> > to
>> >> >> > do it
>> >> >> > in one shot will be quite painful. Even ignoring merge-hell, one
>> >> >> > subtle
>> >> >> > bug
>> >> >> > that
>> >> >> > sneaks in can mean a very large change gets reverted which is a
>> >> >> > time
>> >> >> > sink.
>> >> >> > Much
>> >> >> > better to figure out a way to incrementally switch over.
>> >> >> >
>> >> >> > http://codereview.chromium.org/11345008/
>> >> >
>> >> >
>> >
>> >
>
>

[Mattias Nissler (ping if slow), 2012-10-30 16:15:51]

On Tue, Oct 30, 2012 at 5:09 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> That makes sense.
>
> Mattias, if you're up for looking through the usage updates so that I
> can TBR it to OWNERS, that would be great.  If not, I can ask someone
> on the Browser Components team.
>

I can do that, but not today ;) Happy to take a look at the consumers
tomorrow.


>
> Cheers,
> Jói
>
>
> On Tue, Oct 30, 2012 at 4:08 PM, John Abd-El-Malek <jam@chromium.org>
> wrote:
> >
> >
> > On Tue, Oct 30, 2012 at 9:02 AM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >>
> >> Thanks Mattias.
> >>
> >> I'd like to go ahead with the current change, just extracting the
> >> stuff from switch statements etc. was already close to a couple of
> >> days of work so I don't want to re-do that bit, even if it means two
> >> rounds of OWNERS approvals.  I realize I should have planned this
> >> change more carefully.
> >
> >
> > small note: when OWNERS got added, it was pointed out that it would
> hinder
> > refactorings. i.e. if someone changes a function in base, they would
> need to
> > get OWNERS approval for all the places that call it, which is tedious
> and a
> > waste of time. What really matters is that an owner of the base API
> lgtm'd
> > the changes to the API. After that, the changes to convert every caller
> can
> > be TBR'd to an owner, and just have any other person review it. This is
> the
> > model I used with content refactoring. I suggest the same here.
> >
> >>
> >> I've uploaded a new patch that does the following to address your
> >> earlier review comments:
> >> a) Removes PrefInitObserver, replacing it with
> base::Callback<void(bool)>.
> >> b) Adds a TODO on PrefObserver.
> >> c) Improves documentation as requested.
> >>
> >> Cheers,
> >> Jói
> >>
> >>
> >> On Tue, Oct 30, 2012 at 3:58 PM, Mattias Nissler <mnissler@chromium.org
> >
> >> wrote:
> >> >
> >> >
> >> > On Tue, Oct 30, 2012 at 4:36 PM, Jói Sigurðsson <joi@chromium.org>
> >> > wrote:
> >> >>
> >> >> Yep, that's the plan.  In the follow-ups, we would leave the
> >> >> PrefObserver approach of getting notified enabled, and add a
> >> >> base::Callback approach, and only remove the PrefObserver approach
> >> >> once everything is converted.
> >> >>
> >> >> Cheers,
> >> >> Jói
> >> >>
> >> >>
> >> >> On Tue, Oct 30, 2012 at 3:34 PM, John Abd-El-Malek <jam@chromium.org
> >
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> > On Tue, Oct 30, 2012 at 8:19 AM, Jói Sigurðsson <joi@chromium.org>
> >> >> > wrote:
> >> >> >>
> >> >> >> Thanks for the input John.
> >> >> >>
> >> >> >> After discussing with Mattias, I also agree that switching to
> >> >> >> callbacks is nicer.  It's just after looking at trying to do it as
> >> >> >> part of this change, too many questions came up for me to think we
> >> >> >> can
> >> >> >> do it all in one shot without getting into merge hell, or bugs
> that
> >> >> >> lead to reverts, as you say.  Some of the questions are just
> around
> >> >> >> a
> >> >> >> coherent design for notifications from Prefs, since it looks like
> we
> >> >> >> can perhaps simplify further than just the PrefChangeRegistrar
> >> >> >> object.
> >> >> >>
> >> >> >> I think we're probably all getting onto the same page here.
> >> >> >> Mattias,
> >> >> >> I know I've gone back and forth once already today on trying to
> >> >> >> switch
> >> >> >> to callbacks all in one fell swoop, but I think you had tacitly
> >> >> >> agreed
> >> >> >> earlier today that we could land this change first and then switch
> >> >> >> to
> >> >> >> the "correct" interface in follow-up changes.  Are you still OK
> with
> >> >> >> that?
> >> >
> >> >
> >> > I'm still OK with that, but I honestly think it'll make your life
> harder
> >> > because (as you noted) you have to get OWNERS approval twice for all
> >> > consumers instead of once. Note that in either approach, you can keep
> >> > the
> >> > old and the new interface in the tree at the same time and convert
> >> > individual consumers one-by-one, so this work doesn't require monster
> >> > CLs at
> >> > all.
> >> >
> >> >> >
> >> >> >
> >> >> > btw I agree this will be the only sane way of doing this :)
> >> >> >
> >> >> > Personally I have been burnt a number of times when I try to do
> >> >> > something
> >> >> > big like this in one change. I'm sure you're already thinking about
> >> >> > this: i
> >> >> > suggest that in followups, both teh interface and callback approach
> >> >> > are
> >> >> > supported by pref service so that the code can be switched in many
> >> >> > parallel
> >> >> > changes instead of one. That way if one change is problematic, only
> >> >> > that
> >> >> > change gets reverted.
> >> >> >
> >> >> >>
> >> >> >> Cheers,
> >> >> >> Jói
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >> On Tue, Oct 30, 2012 at 3:15 PM,  <jam@chromium.org> wrote:
> >> >> >> > For the record, I had a draft written yesterday saying that
> >> >> >> > these-single
> >> >> >> > method
> >> >> >> > interfaces would be better as callbacks. But then I decided not
> to
> >> >> >> > send
> >> >> >> > it
> >> >> >> > to
> >> >> >> > see what the owners of the code say first :)
> >> >> >> >
> >> >> >> > In general, before we had the current callback system we tended
> to
> >> >> >> > use
> >> >> >> > interfaces everywhere. While for interfaces with more than one
> >> >> >> > method
> >> >> >> > it's
> >> >> >> > sometimes awkward to switch all of them to callbacks, for an
> >> >> >> > interface
> >> >> >> > for
> >> >> >> > one
> >> >> >> > method I think it's generally better to switch to a callback.
> Some
> >> >> >> > reasons
> >> >> >> > are:
> >> >> >> > -more flexibility, i.e. to dispatch to different methods as has
> >> >> >> > been
> >> >> >> > pointed
> >> >> >> > out
> >> >> >> > -adding extra information to the callback via currying
> >> >> >> > -even dispatching to stating methods etc
> >> >> >> > -helps with lifetime issues: i.e. objects can bind using
> >> >> >> > WeakPtrFactory.
> >> >> >> > When
> >> >> >> > PrefService is calling the objects, it can look to see which
> >> >> >> > callbacks
> >> >> >> > are
> >> >> >> > now
> >> >> >> > null and remove it automatically. This means no unregistration
> >> >> >> > method
> >> >> >> > is
> >> >> >> > needed.
> >> >> >> >
> >> >> >> > btw when I was refactoring content, I switched single-method
> >> >> >> > interfaces
> >> >> >> > to
> >> >> >> > callbacks.
> >> >> >> >
> >> >> >> > One other minor comment: does the callback need a PrefService
> >> >> >> > pointer?
> >> >> >> > There'll
> >> >> >> > only be one PrefService up at a time (which is easily accessible
> >> >> >> > using
> >> >> >> > the
> >> >> >> > static getter), so this seems unnecessary.
> >> >> >> >
> >> >> >> >
> >> >> >> > To be clear, my preference is to have callbacks but it's up to
> you
> >> >> >> > guys
> >> >> >> > on
> >> >> >> > when/how to do this. Given how much usage there is, I agree that
> >> >> >> > trying
> >> >> >> > to
> >> >> >> > do it
> >> >> >> > in one shot will be quite painful. Even ignoring merge-hell, one
> >> >> >> > subtle
> >> >> >> > bug
> >> >> >> > that
> >> >> >> > sneaks in can mean a very large change gets reverted which is a
> >> >> >> > time
> >> >> >> > sink.
> >> >> >> > Much
> >> >> >> > better to figure out a way to incrementally switch over.
> >> >> >> >
> >> >> >> > http://codereview.chromium.org/11345008/
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>

[jam, 2012-10-30 16:17:40]

http://codereview.chromium.org/11345008/diff/8002/base/prefs/public/pref_obse...
File base/prefs/public/pref_observer.h (right):

http://codereview.chromium.org/11345008/diff/8002/base/prefs/public/pref_obse...
base/prefs/public/pref_observer.h:15: virtual void
OnPreferenceChanged(PrefServiceBase* service,
nit: so do do we really need the pref service pointer here?

[Jói, 2012-10-30 16:23:04]

I was planning to remove it in the follow-ups, i.e. when we get rid of
PrefObserver.  This way it's closer to a "safe refactoring" since in
some cases code was using the service pointer sent as the
NotificationSource through NotificationObserver::Observe.

Cheers,
Jói


On Tue, Oct 30, 2012 at 4:17 PM,  <jam@chromium.org> wrote:
>
>
http://codereview.chromium.org/11345008/diff/8002/base/prefs/public/pref_obse...
> File base/prefs/public/pref_observer.h (right):
>
>
http://codereview.chromium.org/11345008/diff/8002/base/prefs/public/pref_obse...
> base/prefs/public/pref_observer.h:15: virtual void
> OnPreferenceChanged(PrefServiceBase* service,
> nit: so do do we really need the pref service pointer here?
>
> http://codereview.chromium.org/11345008/

[Jói, 2012-10-30 16:23:22]

> I can do that, but not today ;) Happy to take a look at the consumers
> tomorrow.

Awesome, thanks Mattias.

Cheers,
Jói


On Tue, Oct 30, 2012 at 4:15 PM, Mattias Nissler <mnissler@chromium.org> wrote:
>
>
> On Tue, Oct 30, 2012 at 5:09 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> That makes sense.
>>
>> Mattias, if you're up for looking through the usage updates so that I
>> can TBR it to OWNERS, that would be great.  If not, I can ask someone
>> on the Browser Components team.
>
>
> I can do that, but not today ;) Happy to take a look at the consumers
> tomorrow.
>
>>
>>
>> Cheers,
>> Jói
>>
>>
>> On Tue, Oct 30, 2012 at 4:08 PM, John Abd-El-Malek <jam@chromium.org>
>> wrote:
>> >
>> >
>> > On Tue, Oct 30, 2012 at 9:02 AM, Jói Sigurðsson <joi@chromium.org>
>> > wrote:
>> >>
>> >> Thanks Mattias.
>> >>
>> >> I'd like to go ahead with the current change, just extracting the
>> >> stuff from switch statements etc. was already close to a couple of
>> >> days of work so I don't want to re-do that bit, even if it means two
>> >> rounds of OWNERS approvals.  I realize I should have planned this
>> >> change more carefully.
>> >
>> >
>> > small note: when OWNERS got added, it was pointed out that it would
>> > hinder
>> > refactorings. i.e. if someone changes a function in base, they would
>> > need to
>> > get OWNERS approval for all the places that call it, which is tedious
>> > and a
>> > waste of time. What really matters is that an owner of the base API
>> > lgtm'd
>> > the changes to the API. After that, the changes to convert every caller
>> > can
>> > be TBR'd to an owner, and just have any other person review it. This is
>> > the
>> > model I used with content refactoring. I suggest the same here.
>> >
>> >>
>> >> I've uploaded a new patch that does the following to address your
>> >> earlier review comments:
>> >> a) Removes PrefInitObserver, replacing it with
>> >> base::Callback<void(bool)>.
>> >> b) Adds a TODO on PrefObserver.
>> >> c) Improves documentation as requested.
>> >>
>> >> Cheers,
>> >> Jói
>> >>
>> >>
>> >> On Tue, Oct 30, 2012 at 3:58 PM, Mattias Nissler
>> >> <mnissler@chromium.org>
>> >> wrote:
>> >> >
>> >> >
>> >> > On Tue, Oct 30, 2012 at 4:36 PM, Jói Sigurðsson <joi@chromium.org>
>> >> > wrote:
>> >> >>
>> >> >> Yep, that's the plan.  In the follow-ups, we would leave the
>> >> >> PrefObserver approach of getting notified enabled, and add a
>> >> >> base::Callback approach, and only remove the PrefObserver approach
>> >> >> once everything is converted.
>> >> >>
>> >> >> Cheers,
>> >> >> Jói
>> >> >>
>> >> >>
>> >> >> On Tue, Oct 30, 2012 at 3:34 PM, John Abd-El-Malek
>> >> >> <jam@chromium.org>
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Tue, Oct 30, 2012 at 8:19 AM, Jói Sigurðsson <joi@chromium.org>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> Thanks for the input John.
>> >> >> >>
>> >> >> >> After discussing with Mattias, I also agree that switching to
>> >> >> >> callbacks is nicer.  It's just after looking at trying to do it
>> >> >> >> as
>> >> >> >> part of this change, too many questions came up for me to think
>> >> >> >> we
>> >> >> >> can
>> >> >> >> do it all in one shot without getting into merge hell, or bugs
>> >> >> >> that
>> >> >> >> lead to reverts, as you say.  Some of the questions are just
>> >> >> >> around
>> >> >> >> a
>> >> >> >> coherent design for notifications from Prefs, since it looks like
>> >> >> >> we
>> >> >> >> can perhaps simplify further than just the PrefChangeRegistrar
>> >> >> >> object.
>> >> >> >>
>> >> >> >> I think we're probably all getting onto the same page here.
>> >> >> >> Mattias,
>> >> >> >> I know I've gone back and forth once already today on trying to
>> >> >> >> switch
>> >> >> >> to callbacks all in one fell swoop, but I think you had tacitly
>> >> >> >> agreed
>> >> >> >> earlier today that we could land this change first and then
>> >> >> >> switch
>> >> >> >> to
>> >> >> >> the "correct" interface in follow-up changes.  Are you still OK
>> >> >> >> with
>> >> >> >> that?
>> >> >
>> >> >
>> >> > I'm still OK with that, but I honestly think it'll make your life
>> >> > harder
>> >> > because (as you noted) you have to get OWNERS approval twice for all
>> >> > consumers instead of once. Note that in either approach, you can keep
>> >> > the
>> >> > old and the new interface in the tree at the same time and convert
>> >> > individual consumers one-by-one, so this work doesn't require monster
>> >> > CLs at
>> >> > all.
>> >> >
>> >> >> >
>> >> >> >
>> >> >> > btw I agree this will be the only sane way of doing this :)
>> >> >> >
>> >> >> > Personally I have been burnt a number of times when I try to do
>> >> >> > something
>> >> >> > big like this in one change. I'm sure you're already thinking
>> >> >> > about
>> >> >> > this: i
>> >> >> > suggest that in followups, both teh interface and callback
>> >> >> > approach
>> >> >> > are
>> >> >> > supported by pref service so that the code can be switched in many
>> >> >> > parallel
>> >> >> > changes instead of one. That way if one change is problematic,
>> >> >> > only
>> >> >> > that
>> >> >> > change gets reverted.
>> >> >> >
>> >> >> >>
>> >> >> >> Cheers,
>> >> >> >> Jói
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >> On Tue, Oct 30, 2012 at 3:15 PM,  <jam@chromium.org> wrote:
>> >> >> >> > For the record, I had a draft written yesterday saying that
>> >> >> >> > these-single
>> >> >> >> > method
>> >> >> >> > interfaces would be better as callbacks. But then I decided not
>> >> >> >> > to
>> >> >> >> > send
>> >> >> >> > it
>> >> >> >> > to
>> >> >> >> > see what the owners of the code say first :)
>> >> >> >> >
>> >> >> >> > In general, before we had the current callback system we tended
>> >> >> >> > to
>> >> >> >> > use
>> >> >> >> > interfaces everywhere. While for interfaces with more than one
>> >> >> >> > method
>> >> >> >> > it's
>> >> >> >> > sometimes awkward to switch all of them to callbacks, for an
>> >> >> >> > interface
>> >> >> >> > for
>> >> >> >> > one
>> >> >> >> > method I think it's generally better to switch to a callback.
>> >> >> >> > Some
>> >> >> >> > reasons
>> >> >> >> > are:
>> >> >> >> > -more flexibility, i.e. to dispatch to different methods as has
>> >> >> >> > been
>> >> >> >> > pointed
>> >> >> >> > out
>> >> >> >> > -adding extra information to the callback via currying
>> >> >> >> > -even dispatching to stating methods etc
>> >> >> >> > -helps with lifetime issues: i.e. objects can bind using
>> >> >> >> > WeakPtrFactory.
>> >> >> >> > When
>> >> >> >> > PrefService is calling the objects, it can look to see which
>> >> >> >> > callbacks
>> >> >> >> > are
>> >> >> >> > now
>> >> >> >> > null and remove it automatically. This means no unregistration
>> >> >> >> > method
>> >> >> >> > is
>> >> >> >> > needed.
>> >> >> >> >
>> >> >> >> > btw when I was refactoring content, I switched single-method
>> >> >> >> > interfaces
>> >> >> >> > to
>> >> >> >> > callbacks.
>> >> >> >> >
>> >> >> >> > One other minor comment: does the callback need a PrefService
>> >> >> >> > pointer?
>> >> >> >> > There'll
>> >> >> >> > only be one PrefService up at a time (which is easily
>> >> >> >> > accessible
>> >> >> >> > using
>> >> >> >> > the
>> >> >> >> > static getter), so this seems unnecessary.
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > To be clear, my preference is to have callbacks but it's up to
>> >> >> >> > you
>> >> >> >> > guys
>> >> >> >> > on
>> >> >> >> > when/how to do this. Given how much usage there is, I agree
>> >> >> >> > that
>> >> >> >> > trying
>> >> >> >> > to
>> >> >> >> > do it
>> >> >> >> > in one shot will be quite painful. Even ignoring merge-hell,
>> >> >> >> > one
>> >> >> >> > subtle
>> >> >> >> > bug
>> >> >> >> > that
>> >> >> >> > sneaks in can mean a very large change gets reverted which is a
>> >> >> >> > time
>> >> >> >> > sink.
>> >> >> >> > Much
>> >> >> >> > better to figure out a way to incrementally switch over.
>> >> >> >> >
>> >> >> >> > http://codereview.chromium.org/11345008/
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>

[jam, 2012-10-30 16:30:34]

On Tue, Oct 30, 2012 at 9:22 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> I was planning to remove it in the follow-ups, i.e. when we get rid of
> PrefObserver.  This way it's closer to a "safe refactoring" since in
> some cases code was using the service pointer sent as the
> NotificationSource through NotificationObserver::Observe.
>

sgtm

>
> Cheers,
> Jói
>
>
> On Tue, Oct 30, 2012 at 4:17 PM,  <jam@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/11345008/diff/8002/base/prefs/public/pref_obse...
> > File base/prefs/public/pref_observer.h (right):
> >
> >
>
http://codereview.chromium.org/11345008/diff/8002/base/prefs/public/pref_obse...
> > base/prefs/public/pref_observer.h:15: virtual void
> > OnPreferenceChanged(PrefServiceBase* service,
> > nit: so do do we really need the pref service pointer here?
> >
> > http://codereview.chromium.org/11345008/
>

[Mattias Nissler (ping if slow), 2012-10-30 16:32:33]

base/prefs, chrome/browser/prefs LGTM, but I didn't look at the rest of the code
yet.

Regarding the parameters to OnPreferenceChanged(), I don't feel strong about the
state you commit in this CL, since the plan is to fix them up properly later
anyways.

http://codereview.chromium.org/11345008/diff/8002/base/prefs/public/pref_obse...
File base/prefs/public/pref_observer.h (right):

http://codereview.chromium.org/11345008/diff/8002/base/prefs/public/pref_obse...
base/prefs/public/pref_observer.h:15: virtual void
OnPreferenceChanged(PrefServiceBase* service,
On 2012/10/30 16:17:41, John Abd-El-Malek wrote:
> nit: so do do we really need the pref service pointer here?

I don't think so, but Jói has looked at all the implementations recently ;) In
the final base::Callback-based solution, I'd prefer to use no arguments (i.e. a
base::Closure), as that will give us the most flexibility. For callers that need
the name, they can Bind it when constructing the callback.

http://codereview.chromium.org/11345008/diff/8002/chrome/browser/prefs/pref_n...
File chrome/browser/prefs/pref_notifier_impl.h (right):

http://codereview.chromium.org/11345008/diff/8002/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.h:51: typedef
std::list<base::Callback<void(bool)> > PrefInitObserverList;
any good reason to use std::list instead of std::vector?

[Jói, 2012-10-30 16:37:58]

Thanks Mattias, answers below.

http://codereview.chromium.org/11345008/diff/8002/base/prefs/public/pref_obse...
File base/prefs/public/pref_observer.h (right):

http://codereview.chromium.org/11345008/diff/8002/base/prefs/public/pref_obse...
base/prefs/public/pref_observer.h:15: virtual void
OnPreferenceChanged(PrefServiceBase* service,
On 2012/10/30 16:32:33, Mattias Nissler wrote:
> On 2012/10/30 16:17:41, John Abd-El-Malek wrote:
> > nit: so do do we really need the pref service pointer here?
> 
> I don't think so, but Jói has looked at all the implementations recently ;) In
> the final base::Callback-based solution, I'd prefer to use no arguments (i.e.
a
> base::Closure), as that will give us the most flexibility. For callers that
need
> the name, they can Bind it when constructing the callback.

Agreed, in the final implementation it will be a base::Callback<void(void)>,
although some callers will definitely want to bind parameters at registration
time.

http://codereview.chromium.org/11345008/diff/8002/chrome/browser/prefs/pref_n...
File chrome/browser/prefs/pref_notifier_impl.h (right):

http://codereview.chromium.org/11345008/diff/8002/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.h:51: typedef
std::list<base::Callback<void(bool)> > PrefInitObserverList;
On 2012/10/30 16:32:33, Mattias Nissler wrote:
> any good reason to use std::list instead of std::vector?

Not really.  In practice I think we only get one registration, and we only need
push_back, iterate and clear semantics.  It could just as well be a vector I
guess.

[Mattias Nissler (ping if slow), 2012-10-31 13:29:35]

Did a full pass, mostly looks good.

Halfway through the review, I switched to a newer patch version that appeared
out of the blue ;)

Also, I understood at some point that you tried to be consistent with the file
when it comes to

// PrefObserver:

comments on the overrides, so feel free to ignore these comments.

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/browser_state_monitor.cc (left):

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/browser_state_monitor.cc:158: }
It seems this is entirely dead code. I guess we should check with the OWNERS.

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (left):

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:597:
PrefProxyConfigTrackerImpl::Observe(type, source, details);
I think this breaks, the parent class also needs to observe prefs.

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/extensions/a...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/extensions/a...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:151: const
char* event_name,
nit: indentation

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/extensions/a...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:175:
OnFontNamePrefChanged(static_cast<PrefService*>(pref_service), pref_name,
The cast is not necessary.

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_tracker_impl.cc (right):

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:294: if (service ==
pref_service_) {
I think this check is unnecessary. Remove or convert to DCHECK?

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:308: NOTREACHED() <<
"Unexpected prefs service.";
nit: s/prefs service/PrefService/

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/ssl_conf...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/ssl_conf...
chrome/browser/net/ssl_config_service_manager_pref.cc:261: PrefService* prefs =
static_cast<PrefService*>(service);
Can we do with a PrefServiceBase* instead?

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

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:367: // because the prefs service is
owned by this class and lives on
nit: s/prefs service/PrefService/

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

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.h:140: // PrefInitObserver implementation.
Comment misplaced :)

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service.h (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.h:268: 
// PrefChangeObserver:

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/translate/t...
File chrome/browser/translate/translate_manager.cc (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/translate/t...
chrome/browser/translate/translate_manager.cc:420:
InitAcceptLanguages(static_cast<PrefService*>(service));
Can we do without the cast?

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:38: 
remove extra newline.

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.cc:1200: :
ui::SHOW_TITLEBAR_WHEN_MAXIMIZED);
indentation

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/gtk/gtk_...
File chrome/browser/ui/gtk/gtk_theme_service.cc (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/gtk/gtk_...
chrome/browser/ui/gtk/gtk_theme_service.cc:377: // forward to the base class.
This comment does make sense for people who are aware of history, but will
confuse others. Let's just remove it.

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:288: "true" : "false");
indentation

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/new_tab_ui.h (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/new_tab_ui.h:107: 
// PrefObserver:

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/content_settings_handler.h (left):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/content_settings_handler.h:26: public
PepperFlashSettingsManager::Client {
Any reasons to reformat this? I think this is legal per the style guide.

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/font_settings_handler.h (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/font_settings_handler.h:22: public PrefObserver
{
why reformatted (also in a few other places)?

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/password_manager_handler.cc:117: // No need to
forward to base class as it is not a PrefObserver.
See my other comment, let's remove this.

[Jói, 2012-10-31 13:33:01]

Thanks Mattias.

Sorry about the new patch, to void merge hell I'm trying to merge to
LKGR at least daily.

Will address your comments and upload a new version ASAP.

Cheers,
Jói


On Wed, Oct 31, 2012 at 1:29 PM,  <mnissler@chromium.org> wrote:
> Did a full pass, mostly looks good.
>
> Halfway through the review, I switched to a newer patch version that
> appeared
> out of the blue ;)
>
> Also, I understood at some point that you tried to be consistent with the
> file
> when it comes to
>
> // PrefObserver:
>
> comments on the overrides, so feel free to ignore these comments.
>
>
>
http://codereview.chromium.org/11345008/diff/5013/chrome/browser/chromeos/inp...
> File chrome/browser/chromeos/input_method/browser_state_monitor.cc
> (left):
>
>
http://codereview.chromium.org/11345008/diff/5013/chrome/browser/chromeos/inp...
> chrome/browser/chromeos/input_method/browser_state_monitor.cc:158: }
> It seems this is entirely dead code. I guess we should check with the
> OWNERS.
>
>
http://codereview.chromium.org/11345008/diff/5013/chrome/browser/chromeos/pro...
> File chrome/browser/chromeos/proxy_config_service_impl.cc (left):
>
>
http://codereview.chromium.org/11345008/diff/5013/chrome/browser/chromeos/pro...
> chrome/browser/chromeos/proxy_config_service_impl.cc:597:
> PrefProxyConfigTrackerImpl::Observe(type, source, details);
> I think this breaks, the parent class also needs to observe prefs.
>
>
http://codereview.chromium.org/11345008/diff/5013/chrome/browser/extensions/a...
> File chrome/browser/extensions/api/font_settings/font_settings_api.cc
> (right):
>
>
http://codereview.chromium.org/11345008/diff/5013/chrome/browser/extensions/a...
> chrome/browser/extensions/api/font_settings/font_settings_api.cc:151:
> const char* event_name,
> nit: indentation
>
>
http://codereview.chromium.org/11345008/diff/5013/chrome/browser/extensions/a...
> chrome/browser/extensions/api/font_settings/font_settings_api.cc:175:
> OnFontNamePrefChanged(static_cast<PrefService*>(pref_service),
> pref_name,
> The cast is not necessary.
>
>
http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/pref_pro...
> File chrome/browser/net/pref_proxy_config_tracker_impl.cc (right):
>
>
http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/pref_pro...
> chrome/browser/net/pref_proxy_config_tracker_impl.cc:294: if (service ==
> pref_service_) {
> I think this check is unnecessary. Remove or convert to DCHECK?
>
>
http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/pref_pro...
> chrome/browser/net/pref_proxy_config_tracker_impl.cc:308: NOTREACHED()
> << "Unexpected prefs service.";
> nit: s/prefs service/PrefService/
>
>
http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/ssl_conf...
> File chrome/browser/net/ssl_config_service_manager_pref.cc (right):
>
>
http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/ssl_conf...
> chrome/browser/net/ssl_config_service_manager_pref.cc:261: PrefService*
> prefs = static_cast<PrefService*>(service);
> Can we do with a PrefServiceBase* instead?
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/profiles/pr...
> File chrome/browser/profiles/profile_impl.cc (right):
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/profiles/pr...
> chrome/browser/profiles/profile_impl.cc:367: // because the prefs
> service is owned by this class and lives on
> nit: s/prefs service/PrefService/
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/profiles/pr...
> File chrome/browser/profiles/profile_impl.h (right):
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/profiles/pr...
> chrome/browser/profiles/profile_impl.h:140: // PrefInitObserver
> implementation.
> Comment misplaced :)
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/search_engi...
> File chrome/browser/search_engines/template_url_service.h (right):
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/search_engi...
> chrome/browser/search_engines/template_url_service.h:268:
> // PrefChangeObserver:
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/translate/t...
> File chrome/browser/translate/translate_manager.cc (right):
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/translate/t...
> chrome/browser/translate/translate_manager.cc:420:
> InitAcceptLanguages(static_cast<PrefService*>(service));
> Can we do without the cast?
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/cocoa/br...
> File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right):
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/cocoa/br...
> chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:38:
> remove extra newline.
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/gtk/brow...
> File chrome/browser/ui/gtk/browser_window_gtk.cc (right):
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/gtk/brow...
> chrome/browser/ui/gtk/browser_window_gtk.cc:1200: :
> ui::SHOW_TITLEBAR_WHEN_MAXIMIZED);
> indentation
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/gtk/gtk_...
> File chrome/browser/ui/gtk/gtk_theme_service.cc (right):
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/gtk/gtk_...
> chrome/browser/ui/gtk/gtk_theme_service.cc:377: // forward to the base
> class.
> This comment does make sense for people who are aware of history, but
> will confuse others. Let's just remove it.
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/nt...
> File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/nt...
> chrome/browser/ui/webui/ntp/new_tab_ui.cc:288: "true" : "false");
> indentation
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/nt...
> File chrome/browser/ui/webui/ntp/new_tab_ui.h (right):
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/nt...
> chrome/browser/ui/webui/ntp/new_tab_ui.h:107:
> // PrefObserver:
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
> File chrome/browser/ui/webui/options/content_settings_handler.h (left):
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
> chrome/browser/ui/webui/options/content_settings_handler.h:26: public
> PepperFlashSettingsManager::Client {
> Any reasons to reformat this? I think this is legal per the style guide.
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
> File chrome/browser/ui/webui/options/font_settings_handler.h (right):
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
> chrome/browser/ui/webui/options/font_settings_handler.h:22: public
> PrefObserver {
> why reformatted (also in a few other places)?
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
> File chrome/browser/ui/webui/options/password_manager_handler.cc
> (right):
>
>
http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
> chrome/browser/ui/webui/options/password_manager_handler.cc:117: // No
> need to forward to base class as it is not a PrefObserver.
> See my other comment, let's remove this.
>
> http://codereview.chromium.org/11345008/

[Jói, 2012-10-31 14:56:25]

PTAL, I've addressed your review comments.

You can look at the diff from patch set 6 to patch set 7 if you want to see just
what I changed based on your feedback.

Cheers,
Jói

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/browser_state_monitor.cc (left):

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/browser_state_monitor.cc:158: }
On 2012/10/31 13:29:35, Mattias Nissler wrote:
> It seems this is entirely dead code. I guess we should check with the OWNERS.

It's possible they received this in an inheritance hierarchy via a superclass or
subclass or something.  It's dead though, the NOTIFICATION_PREF_CHANGED doesn't
even exist any more and the code did nothing before this change anyway.

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (left):

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:597:
PrefProxyConfigTrackerImpl::Observe(type, source, details);
On 2012/10/31 13:29:35, Mattias Nissler wrote:
> I think this breaks, the parent class also needs to observe prefs.

You're right, this was caught by a tryjob and is fixed in the newer patch.  It's
interesting that the DCHECK I put on line 578 did not fire though, yet chaining
to the parent class did fix the problem.

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/extensions/a...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/extensions/a...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:151: const
char* event_name,
On 2012/10/31 13:29:35, Mattias Nissler wrote:
> nit: indentation

Done.

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/extensions/a...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:175:
OnFontNamePrefChanged(static_cast<PrefService*>(pref_service), pref_name,
On 2012/10/31 13:29:35, Mattias Nissler wrote:
> The cast is not necessary.

It was (this is OnFontNamePrefChanged, not OnFontNameChanged) but a better fix
was to switch OnFontNamePrefChanged to use a PrefServiceBase.

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_tracker_impl.cc (right):

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:294: if (service ==
pref_service_) {
On 2012/10/31 13:29:35, Mattias Nissler wrote:
> I think this check is unnecessary. Remove or convert to DCHECK?

I agree this should be unnecessary.  I would like to keep it in for this
revision, to keep it closer to a "safe" refactoring, but this is one of the
things that should go away everywhere in the next round, where we'll probably
remove the 'service' pointer altogether.

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:308: NOTREACHED() <<
"Unexpected prefs service.";
On 2012/10/31 13:29:35, Mattias Nissler wrote:
> nit: s/prefs service/PrefService/

Done.

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/ssl_conf...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

http://codereview.chromium.org/11345008/diff/5013/chrome/browser/net/ssl_conf...
chrome/browser/net/ssl_config_service_manager_pref.cc:261: PrefService* prefs =
static_cast<PrefService*>(service);
On 2012/10/31 13:29:35, Mattias Nissler wrote:
> Can we do with a PrefServiceBase* instead?

Done.

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

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:367: // because the prefs service is
owned by this class and lives on
On 2012/10/31 13:29:36, Mattias Nissler wrote:
> nit: s/prefs service/PrefService/

Done.

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

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.h:140: // PrefInitObserver implementation.
On 2012/10/31 13:29:36, Mattias Nissler wrote:
> Comment misplaced :)

Done.

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service.h (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.h:268: 
On 2012/10/31 13:29:36, Mattias Nissler wrote:
> // PrefChangeObserver:

Done.

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/translate/t...
File chrome/browser/translate/translate_manager.cc (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/translate/t...
chrome/browser/translate/translate_manager.cc:420:
InitAcceptLanguages(static_cast<PrefService*>(service));
On 2012/10/31 13:29:36, Mattias Nissler wrote:
> Can we do without the cast?

Done.

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:38: 
On 2012/10/31 13:29:36, Mattias Nissler wrote:
> remove extra newline.

Done.

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.cc:1200: :
ui::SHOW_TITLEBAR_WHEN_MAXIMIZED);
On 2012/10/31 13:29:36, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/gtk/gtk_...
File chrome/browser/ui/gtk/gtk_theme_service.cc (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/gtk/gtk_...
chrome/browser/ui/gtk/gtk_theme_service.cc:377: // forward to the base class.
On 2012/10/31 13:29:36, Mattias Nissler wrote:
> This comment does make sense for people who are aware of history, but will
> confuse others. Let's just remove it.

Done.

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:288: "true" : "false");
On 2012/10/31 13:29:36, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/new_tab_ui.h (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/new_tab_ui.h:107: 
On 2012/10/31 13:29:36, Mattias Nissler wrote:
> // PrefObserver:

Done.

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/content_settings_handler.h (left):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/content_settings_handler.h:26: public
PepperFlashSettingsManager::Client {
On 2012/10/31 13:29:36, Mattias Nissler wrote:
> Any reasons to reformat this? I think this is legal per the style guide.

I don't have to reformat it (or the others where it changed).  I'm changing them
back.

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/font_settings_handler.h (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/font_settings_handler.h:22: public PrefObserver
{
On 2012/10/31 13:29:36, Mattias Nissler wrote:
> why reformatted (also in a few other places)?

Ditto.

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

http://codereview.chromium.org/11345008/diff/19002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/password_manager_handler.cc:117: // No need to
forward to base class as it is not a PrefObserver.
On 2012/10/31 13:29:36, Mattias Nissler wrote:
> See my other comment, let's remove this.

Done.

[Mattias Nissler (ping if slow), 2012-10-31 15:13:01]

LGTM

[Jói, 2012-11-01 14:34:22]

TBRs to OWNERS for usage updates (full change reviewed by mnissler@ who is an
OWNER for the subsystem that is changing):

ben@ for chrome/ updates to usage (PrefObserver instead of
NOTIFICATION_PREF_CHANGED via content::NotificationObserver).

brettw@ for one source file added to base/base.gyp

Cheers,
Jói

[commit-bot: I haz the power, 2012-11-01 15:03:34]

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

[commit-bot: I haz the power, 2012-11-01 17:24:24]

Change committed as 165414

[Ben Goodger (Google), 2012-11-01 17:24:31]

cool! lgtm.