Draft change to use base::Closure instead of PrefObserver in PrefChangeRegistrar.

Add Closure-based API to PrefChangeObserver and PrefMember, deprecate PrefObserver-based API.

This switches the API with minimal implementation changes; PrefNotifierImpl still uses PrefObserver to accept registrations, and still filters on pref names (which PrefChangeObserver looks up again when it receives callbacks via its PrefObserver implementation).

This approach is chosen for now to establish the new API so that usages can be switched, because:

a) We need a way for PrefNotifierImpl to dispatch to both PrefChangeObserver and PrefMember, and it's unclear to me whether we want to switch that interface to base::Callback as well (if so, how to unregister?), or whether we want PrefMember to have a PrefChangeObserver instance (probably inefficient?) or something else.

b) There are plans to do performance measurements of a few different implementation approaches in how PrefNotifierImpl and PrefChangeObserver interact; that interaction can be changed "under the hood" while the new API stays unchanged, and this lets us start switching users to the new API and removing the now-deprecated PrefObserver-based API.

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

[Jói, 2012-11-06 12:58:05]

Hi Mattias,

This doesn't quite compile, as you can't store a base::Callback in a set (no
operator<), but is this roughly what you guys were thinking?

Note I'm not 100% sure how base::Callback equality works, but I think if you
call base::Bind with identical parameters you get objects for which .Equals()
evaluates to true.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2012-11-06 13:26:48]

See comments for thoughts :)

http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
File base/prefs/public/pref_change_registrar.cc (right):

http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
base/prefs/public/pref_change_registrar.cc:51: base::Unretained(obs), service_,
path));
Hm, comparing callbacks... not sure whether that's a good idea.

In the discussion with battre@ last week, we figured we'd probably change the
interface here to allow only a single callback per pref (we thought that's a
reasonable restriction, haven't checked all consumers though). Also we'd change
the model to not forward the callback registration with PrefService, but store a
std::map<std::string, base::Closure> in PrefChangeRegistrar. We would then have
PrefService keep a list of PrefChangeRegistrars, which it forwards pref change
events to, and PrefChangeRegistrar would figure out the callback to invoke using
its map. Remove would not take a closure as a parameter. WDYT? Makes sense?

http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
File chrome/browser/api/prefs/pref_member.cc (right):

http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
chrome/browser/api/prefs/pref_member.cc:65: const std::string& pref_name) {
doesn't require these parameters any longer, can just use |prefs_| and
|pref_name_|

[Jói, 2012-11-06 13:35:50]

If I understand you correctly, you mean PrefNotifierImpl would no
longer filter on the pref name and would just forward to all
registered PrefChangeRegistrars.  (Would those be registered as
base::Callbacks, or as PrefChangeRegistrar, or as a base interface
like PrefObserver?)

If that's the intent, then yes that works but seems potentially a bit
less performant than the current situation, which creates a list of
PrefObservers (in PrefNotifierImpl) to notify per pref name.

I guess an alternative would be for PrefNotifierImpl to continue
building a map from pref name to list of observers (which would now be
PrefChangeRegistrar pointers), but again I think this is a bit less
efficient in terms of memory usage than the current model (you are
storing both a pointer to a PrefChangeRegistrar per registration, plus
the callback object per registration).

Perhaps it is best I leave it to you guys to come up with the new
interface and bindings from PrefChangeRegistrar to PrefService to
PrefNotifierImpl, while I continue to focus on breaking dependencies
and moving the Prefs code to base/prefs?  If we do it that way, I am
still happy to help convert callers to the new interface when it is
ready, as I had promised.

Cheers,
Jói



On Tue, Nov 6, 2012 at 1:26 PM,  <mnissler@chromium.org> wrote:
> See comments for thoughts :)
>
>
>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
> File base/prefs/public/pref_change_registrar.cc (right):
>
>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
> base/prefs/public/pref_change_registrar.cc:51: base::Unretained(obs),
> service_, path));
> Hm, comparing callbacks... not sure whether that's a good idea.
>
> In the discussion with battre@ last week, we figured we'd probably
> change the interface here to allow only a single callback per pref (we
> thought that's a reasonable restriction, haven't checked all consumers
> though). Also we'd change the model to not forward the callback
> registration with PrefService, but store a std::map<std::string,
> base::Closure> in PrefChangeRegistrar. We would then have PrefService
> keep a list of PrefChangeRegistrars, which it forwards pref change
> events to, and PrefChangeRegistrar would figure out the callback to
> invoke using its map. Remove would not take a closure as a parameter.
> WDYT? Makes sense?
>
>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
> File chrome/browser/api/prefs/pref_member.cc (right):
>
>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
> chrome/browser/api/prefs/pref_member.cc:65: const std::string&
> pref_name) {
> doesn't require these parameters any longer, can just use |prefs_| and
> |pref_name_|
>
> http://codereview.chromium.org/11368098/

[Jói, 2012-11-06 14:25:59]

For discussion, I uploaded a very rough draft (does not compile) of having an
Add-only, one-registration-per-name interface on PrefChangeRegistrar, and doing
the dispatch-by-name there instead of in PrefNotifierImpl.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2012-11-06 14:43:14]

On 2012/11/06 13:35:50, Jói wrote:
> If I understand you correctly, you mean PrefNotifierImpl would no
> longer filter on the pref name and would just forward to all
> registered PrefChangeRegistrars.  (Would those be registered as
> base::Callbacks, or as PrefChangeRegistrar, or as a base interface
> like PrefObserver?)
> 
> If that's the intent, then yes that works but seems potentially a bit
> less performant than the current situation, which creates a list of
> PrefObservers (in PrefNotifierImpl) to notify per pref name.

Yes, that was the intent behind my comment. Performance-wise, I think it's hard
to say what's better with out measuring this. Note that we're now doing less
work on pref registration, but more on dispatch. I wouldn't be confident to say
either change is significant without data though :)

> 
> I guess an alternative would be for PrefNotifierImpl to continue
> building a map from pref name to list of observers (which would now be
> PrefChangeRegistrar pointers), but again I think this is a bit less
> efficient in terms of memory usage than the current model (you are
> storing both a pointer to a PrefChangeRegistrar per registration, plus
> the callback object per registration).

That seems like the worst of both worlds :)

> 
> Perhaps it is best I leave it to you guys to come up with the new
> interface and bindings from PrefChangeRegistrar to PrefService to
> PrefNotifierImpl, while I continue to focus on breaking dependencies
> and moving the Prefs code to base/prefs?  

I'm clogged with other stuff, so I may be a bit slow. Dominic, would you mind
taking a look at the current code and offer your opinion?

> If we do it that way, I am
> still happy to help convert callers to the new interface when it is
> ready, as I had promised.
> 
> Cheers,
> Jói
> 
> 
> 
> On Tue, Nov 6, 2012 at 1:26 PM,  <mailto:mnissler@chromium.org> wrote:
> > See comments for thoughts :)
> >
> >
> >
>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
> > File base/prefs/public/pref_change_registrar.cc (right):
> >
> >
>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
> > base/prefs/public/pref_change_registrar.cc:51: base::Unretained(obs),
> > service_, path));
> > Hm, comparing callbacks... not sure whether that's a good idea.
> >
> > In the discussion with battre@ last week, we figured we'd probably
> > change the interface here to allow only a single callback per pref (we
> > thought that's a reasonable restriction, haven't checked all consumers
> > though). Also we'd change the model to not forward the callback
> > registration with PrefService, but store a std::map<std::string,
> > base::Closure> in PrefChangeRegistrar. We would then have PrefService
> > keep a list of PrefChangeRegistrars, which it forwards pref change
> > events to, and PrefChangeRegistrar would figure out the callback to
> > invoke using its map. Remove would not take a closure as a parameter.
> > WDYT? Makes sense?
> >
> >
>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
> > File chrome/browser/api/prefs/pref_member.cc (right):
> >
> >
>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
> > chrome/browser/api/prefs/pref_member.cc:65: const std::string&
> > pref_name) {
> > doesn't require these parameters any longer, can just use |prefs_| and
> > |pref_name_|
> >
> > <crxref style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">http://codereview.chromium.org/11368098/</crxref>

[Jói, 2012-11-06 14:51:08]

> I'm clogged with other stuff, so I may be a bit slow. Dominic, would you
> mind
> taking a look at the current code and offer your opinion?

An alternative is to leave this for now, until you guys have more time.

My primary goal is to extract components out of src/chrome/, Prefs
being the first.  That means breaking dependencies, which has already
happened in this case (PrefObserver instead of
content::NotificationObserver).  I hear you that you want to take the
opportunity to improve the interface further (per-pref-name
registration of base::Callback instead of PrefObserver), and I think
it's a worth-while thing to do at some point, but I don't think that's
as urgent a goal (at least it's not to me) so we could leave it for a
bit.

Whether we do the PrefObserver removal now or later, I don't think it
makes it any harder or easier.

For making Prefs a component, there is lots more work, and it's the
first of many components we are trying to move out of src/chrome/.
The next stop is probably to look at splitting PrefService up so that
the generic part can move to base/prefs/ and the stuff that is
Chrome-specific (e.g. setting up the priority list of various
PrefStores, some of which are Chrome-specific) can stay in
chrome/browser/prefs/.

Cheers,
Jói



On Tue, Nov 6, 2012 at 2:43 PM,  <mnissler@chromium.org> wrote:
> On 2012/11/06 13:35:50, Jói wrote:
>>
>> If I understand you correctly, you mean PrefNotifierImpl would no
>> longer filter on the pref name and would just forward to all
>> registered PrefChangeRegistrars.  (Would those be registered as
>> base::Callbacks, or as PrefChangeRegistrar, or as a base interface
>> like PrefObserver?)
>
>
>> If that's the intent, then yes that works but seems potentially a bit
>> less performant than the current situation, which creates a list of
>> PrefObservers (in PrefNotifierImpl) to notify per pref name.
>
>
> Yes, that was the intent behind my comment. Performance-wise, I think it's
> hard
> to say what's better with out measuring this. Note that we're now doing less
> work on pref registration, but more on dispatch. I wouldn't be confident to
> say
> either change is significant without data though :)
>
>
>
>> I guess an alternative would be for PrefNotifierImpl to continue
>> building a map from pref name to list of observers (which would now be
>> PrefChangeRegistrar pointers), but again I think this is a bit less
>> efficient in terms of memory usage than the current model (you are
>> storing both a pointer to a PrefChangeRegistrar per registration, plus
>> the callback object per registration).
>
>
> That seems like the worst of both worlds :)
>
>
>
>> Perhaps it is best I leave it to you guys to come up with the new
>> interface and bindings from PrefChangeRegistrar to PrefService to
>> PrefNotifierImpl, while I continue to focus on breaking dependencies
>> and moving the Prefs code to base/prefs?
>
>
> I'm clogged with other stuff, so I may be a bit slow. Dominic, would you
> mind
> taking a look at the current code and offer your opinion?
>
>
>> If we do it that way, I am
>> still happy to help convert callers to the new interface when it is
>> ready, as I had promised.
>
>
>> Cheers,
>> Jói
>
>
>
>
>> On Tue, Nov 6, 2012 at 1:26 PM,  <mailto:mnissler@chromium.org> wrote:
>> > See comments for thoughts :)
>> >
>> >
>> >
>
>
>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
>>
>> > File base/prefs/public/pref_change_registrar.cc (right):
>> >
>> >
>
>
>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
>>
>> > base/prefs/public/pref_change_registrar.cc:51: base::Unretained(obs),
>> > service_, path));
>> > Hm, comparing callbacks... not sure whether that's a good idea.
>> >
>> > In the discussion with battre@ last week, we figured we'd probably
>> > change the interface here to allow only a single callback per pref (we
>> > thought that's a reasonable restriction, haven't checked all consumers
>> > though). Also we'd change the model to not forward the callback
>> > registration with PrefService, but store a std::map<std::string,
>> > base::Closure> in PrefChangeRegistrar. We would then have PrefService
>> > keep a list of PrefChangeRegistrars, which it forwards pref change
>> > events to, and PrefChangeRegistrar would figure out the callback to
>> > invoke using its map. Remove would not take a closure as a parameter.
>> > WDYT? Makes sense?
>> >
>> >
>
>
>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
>>
>> > File chrome/browser/api/prefs/pref_member.cc (right):
>> >
>> >
>
>
>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
>>
>> > chrome/browser/api/prefs/pref_member.cc:65: const std::string&
>> > pref_name) {
>> > doesn't require these parameters any longer, can just use |prefs_| and
>> > |pref_name_|
>> >
>> > <crxref style="background-image:
>
>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">http://codereview.chromium.org/11368098/</crxref>
>
>
>
> http://codereview.chromium.org/11368098/

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

On 2012/11/06 14:51:08, Jói wrote:
> > I'm clogged with other stuff, so I may be a bit slow. Dominic, would you
> > mind
> > taking a look at the current code and offer your opinion?
> 
> An alternative is to leave this for now, until you guys have more time.
> 
> My primary goal is to extract components out of src/chrome/, Prefs
> being the first.  That means breaking dependencies, which has already
> happened in this case (PrefObserver instead of
> content::NotificationObserver).  I hear you that you want to take the
> opportunity to improve the interface further (per-pref-name
> registration of base::Callback instead of PrefObserver), and I think
> it's a worth-while thing to do at some point, but I don't think that's
> as urgent a goal (at least it's not to me) so we could leave it for a
> bit.
> 
> Whether we do the PrefObserver removal now or later, I don't think it
> makes it any harder or easier.

TBH, I'm a bit disappointed by this answer. I stamped the PrefObserver CL mainly
to make your life a little simpler, and now you're arguing it's a solution
that's good for a while (so it'll likely stay in the tree for the time being).
While that's true from a technical point of view, we're now forcing PrefService
consumers to re-learn the interface twice instead of figuring out a good
solution and then converting everything to that in one sweep. I understand that
you may not be motivated to refactor pref observing, and that's fine, but then
please be honest in the first place (maybe I misunderstood our earlier
communication on this?). 

> 
> For making Prefs a component, there is lots more work, and it's the
> first of many components we are trying to move out of src/chrome/.
> The next stop is probably to look at splitting PrefService up so that
> the generic part can move to base/prefs/ and the stuff that is
> Chrome-specific (e.g. setting up the priority list of various
> PrefStores, some of which are Chrome-specific) can stay in
> chrome/browser/prefs/.

Agreed.

> 
> Cheers,
> Jói
> 
> 
> 
> On Tue, Nov 6, 2012 at 2:43 PM,  <mailto:mnissler@chromium.org> wrote:
> > On 2012/11/06 13:35:50, Jói wrote:
> >>
> >> If I understand you correctly, you mean PrefNotifierImpl would no
> >> longer filter on the pref name and would just forward to all
> >> registered PrefChangeRegistrars.  (Would those be registered as
> >> base::Callbacks, or as PrefChangeRegistrar, or as a base interface
> >> like PrefObserver?)
> >
> >
> >> If that's the intent, then yes that works but seems potentially a bit
> >> less performant than the current situation, which creates a list of
> >> PrefObservers (in PrefNotifierImpl) to notify per pref name.
> >
> >
> > Yes, that was the intent behind my comment. Performance-wise, I think it's
> > hard
> > to say what's better with out measuring this. Note that we're now doing less
> > work on pref registration, but more on dispatch. I wouldn't be confident to
> > say
> > either change is significant without data though :)
> >
> >
> >
> >> I guess an alternative would be for PrefNotifierImpl to continue
> >> building a map from pref name to list of observers (which would now be
> >> PrefChangeRegistrar pointers), but again I think this is a bit less
> >> efficient in terms of memory usage than the current model (you are
> >> storing both a pointer to a PrefChangeRegistrar per registration, plus
> >> the callback object per registration).
> >
> >
> > That seems like the worst of both worlds :)
> >
> >
> >
> >> Perhaps it is best I leave it to you guys to come up with the new
> >> interface and bindings from PrefChangeRegistrar to PrefService to
> >> PrefNotifierImpl, while I continue to focus on breaking dependencies
> >> and moving the Prefs code to base/prefs?
> >
> >
> > I'm clogged with other stuff, so I may be a bit slow. Dominic, would you
> > mind
> > taking a look at the current code and offer your opinion?
> >
> >
> >> If we do it that way, I am
> >> still happy to help convert callers to the new interface when it is
> >> ready, as I had promised.
> >
> >
> >> Cheers,
> >> Jói
> >
> >
> >
> >
> >> On Tue, Nov 6, 2012 at 1:26 PM,  <mailto:mnissler@chromium.org> wrote:
> >> > See comments for thoughts :)
> >> >
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
> >>
> >> > File base/prefs/public/pref_change_registrar.cc (right):
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
> >>
> >> > base/prefs/public/pref_change_registrar.cc:51: base::Unretained(obs),
> >> > service_, path));
> >> > Hm, comparing callbacks... not sure whether that's a good idea.
> >> >
> >> > In the discussion with battre@ last week, we figured we'd probably
> >> > change the interface here to allow only a single callback per pref (we
> >> > thought that's a reasonable restriction, haven't checked all consumers
> >> > though). Also we'd change the model to not forward the callback
> >> > registration with PrefService, but store a std::map<std::string,
> >> > base::Closure> in PrefChangeRegistrar. We would then have PrefService
> >> > keep a list of PrefChangeRegistrars, which it forwards pref change
> >> > events to, and PrefChangeRegistrar would figure out the callback to
> >> > invoke using its map. Remove would not take a closure as a parameter.
> >> > WDYT? Makes sense?
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
> >>
> >> > File chrome/browser/api/prefs/pref_member.cc (right):
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
> >>
> >> > chrome/browser/api/prefs/pref_member.cc:65: const std::string&
> >> > pref_name) {
> >> > doesn't require these parameters any longer, can just use |prefs_| and
> >> > |pref_name_|
> >> >
> >> > <crxref style="background-image:
> >
> >
>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);"><crxref
style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">http://codereview.chromium.org/11368098/</crxref></crxref>
> >
> >
> >
> > <crxref style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">http://codereview.chromium.org/11368098/</crxref>

[Jói, 2012-11-06 17:07:24]

Hi Mattias,

I respect your decision as an OWNER about what the shape of the API
should be, and I don't intend to renege on my promise to bring about
the Callback-based approach; I have already spent the better part of
the day working on it, and a good part of yesterday.

My suggestion to defer this work was in the context of you saying you
did not have time to work on it with me.  If either you or Dominic has
time to help me figure out the new API, then that should probably be
fine and I can take care of this end to end, as I think was the
implicit promise from the change you reviewed.

(As an aside, I'm not sure yet whether or not you agree with my
suggestion that it might be more efficient if you and Dominic design
the API and then I can help switch usages over.  If you don't agree,
that's fine, but I will still definitely need some time from one of
you for reviewing my suggestions.)

Since we are being frank, let me provide some background on my
thinking.  Again, this does not change what I promised to do and still
intend to do; it's just for clarification and for us to keep in mind
in future discussions:  My team's mandate is to extract components out
of src/chrome/, and it's as part of this mandate that I am trying to
make this happen for Prefs.  This means I favor forward progress on
the goal of extracting components by using reasonable,
straight-forward means to break dependencies.  In fact there has been
the explicit intent, discussed e.g. with Ben Goodger, that
improvements in APIs should be left to OWNERS, but of course there
will be special cases like this one, where the case can be made that
it doesn't make sense to change an API to one thing if it's soon going
to change to another.  So I chose the PrefObserver paradigm as the
most straight-forward possible change I could see that would break the
dependency.  In retrospect, as I have said before, I should have
discussed with you before starting and we might possibly have arrived
at the API you desire with less total work.

To move forward, if I can get feedback on the latest draft, that
should be enough for now.

Cheers,
Jói



On Tue, Nov 6, 2012 at 3:31 PM,  <mnissler@chromium.org> wrote:
> On 2012/11/06 14:51:08, Jói wrote:
>>
>> > I'm clogged with other stuff, so I may be a bit slow. Dominic, would you
>> > mind
>> > taking a look at the current code and offer your opinion?
>
>
>> An alternative is to leave this for now, until you guys have more time.
>
>
>> My primary goal is to extract components out of src/chrome/, Prefs
>> being the first.  That means breaking dependencies, which has already
>> happened in this case (PrefObserver instead of
>> content::NotificationObserver).  I hear you that you want to take the
>> opportunity to improve the interface further (per-pref-name
>> registration of base::Callback instead of PrefObserver), and I think
>> it's a worth-while thing to do at some point, but I don't think that's
>> as urgent a goal (at least it's not to me) so we could leave it for a
>> bit.
>
>
>> Whether we do the PrefObserver removal now or later, I don't think it
>> makes it any harder or easier.
>
>
> TBH, I'm a bit disappointed by this answer. I stamped the PrefObserver CL
> mainly
> to make your life a little simpler, and now you're arguing it's a solution
> that's good for a while (so it'll likely stay in the tree for the time
> being).
> While that's true from a technical point of view, we're now forcing
> PrefService
> consumers to re-learn the interface twice instead of figuring out a good
> solution and then converting everything to that in one sweep. I understand
> that
> you may not be motivated to refactor pref observing, and that's fine, but
> then
> please be honest in the first place (maybe I misunderstood our earlier
> communication on this?).
>
>
>
>> For making Prefs a component, there is lots more work, and it's the
>> first of many components we are trying to move out of src/chrome/.
>> The next stop is probably to look at splitting PrefService up so that
>> the generic part can move to base/prefs/ and the stuff that is
>> Chrome-specific (e.g. setting up the priority list of various
>> PrefStores, some of which are Chrome-specific) can stay in
>> chrome/browser/prefs/.
>
>
> Agreed.
>
>
>> Cheers,
>> Jói
>
>
>
>
>> On Tue, Nov 6, 2012 at 2:43 PM,  <mailto:mnissler@chromium.org> wrote:
>> > On 2012/11/06 13:35:50, Jói wrote:
>> >>
>> >> If I understand you correctly, you mean PrefNotifierImpl would no
>> >> longer filter on the pref name and would just forward to all
>> >> registered PrefChangeRegistrars.  (Would those be registered as
>> >> base::Callbacks, or as PrefChangeRegistrar, or as a base interface
>> >> like PrefObserver?)
>> >
>> >
>> >> If that's the intent, then yes that works but seems potentially a bit
>> >> less performant than the current situation, which creates a list of
>> >> PrefObservers (in PrefNotifierImpl) to notify per pref name.
>> >
>> >
>> > Yes, that was the intent behind my comment. Performance-wise, I think
>> > it's
>> > hard
>> > to say what's better with out measuring this. Note that we're now doing
>> > less
>> > work on pref registration, but more on dispatch. I wouldn't be confident
>> > to
>> > say
>> > either change is significant without data though :)
>> >
>> >
>> >
>> >> I guess an alternative would be for PrefNotifierImpl to continue
>> >> building a map from pref name to list of observers (which would now be
>> >> PrefChangeRegistrar pointers), but again I think this is a bit less
>> >> efficient in terms of memory usage than the current model (you are
>> >> storing both a pointer to a PrefChangeRegistrar per registration, plus
>> >> the callback object per registration).
>> >
>> >
>> > That seems like the worst of both worlds :)
>> >
>> >
>> >
>> >> Perhaps it is best I leave it to you guys to come up with the new
>> >> interface and bindings from PrefChangeRegistrar to PrefService to
>> >> PrefNotifierImpl, while I continue to focus on breaking dependencies
>> >> and moving the Prefs code to base/prefs?
>> >
>> >
>> > I'm clogged with other stuff, so I may be a bit slow. Dominic, would you
>> > mind
>> > taking a look at the current code and offer your opinion?
>> >
>> >
>> >> If we do it that way, I am
>> >> still happy to help convert callers to the new interface when it is
>> >> ready, as I had promised.
>> >
>> >
>> >> Cheers,
>> >> Jói
>> >
>> >
>> >
>> >
>> >> On Tue, Nov 6, 2012 at 1:26 PM,  <mailto:mnissler@chromium.org> wrote:
>> >> > See comments for thoughts :)
>> >> >
>> >> >
>> >> >
>> >
>> >
>> >
>
>
>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
>>
>> >>
>> >> > File base/prefs/public/pref_change_registrar.cc (right):
>> >> >
>> >> >
>> >
>> >
>> >
>
>
>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
>>
>> >>
>> >> > base/prefs/public/pref_change_registrar.cc:51: base::Unretained(obs),
>> >> > service_, path));
>> >> > Hm, comparing callbacks... not sure whether that's a good idea.
>> >> >
>> >> > In the discussion with battre@ last week, we figured we'd probably
>> >> > change the interface here to allow only a single callback per pref
>> >> > (we
>> >> > thought that's a reasonable restriction, haven't checked all
>> >> > consumers
>> >> > though). Also we'd change the model to not forward the callback
>> >> > registration with PrefService, but store a std::map<std::string,
>> >> > base::Closure> in PrefChangeRegistrar. We would then have PrefService
>> >> > keep a list of PrefChangeRegistrars, which it forwards pref change
>> >> > events to, and PrefChangeRegistrar would figure out the callback to
>> >> > invoke using its map. Remove would not take a closure as a parameter.
>> >> > WDYT? Makes sense?
>> >> >
>> >> >
>> >
>> >
>> >
>
>
>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
>>
>> >>
>> >> > File chrome/browser/api/prefs/pref_member.cc (right):
>> >> >
>> >> >
>> >
>> >
>> >
>
>
>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
>>
>> >>
>> >> > chrome/browser/api/prefs/pref_member.cc:65: const std::string&
>> >> > pref_name) {
>> >> > doesn't require these parameters any longer, can just use |prefs_|
>> >> > and
>> >> > |pref_name_|
>> >> >
>> >> > <crxref style="background-image:
>> >
>> >
>
>
>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);"><crxref
> style="background-image:
>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">http://codereview.chromium.org/11368098/</crxref></crxref>
>
>> >
>> >
>> >
>> > <crxref style="background-image:
>
>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">http://codereview.chromium.org/11368098/</crxref>
>
>
>
> http://codereview.chromium.org/11368098/

[Mattias Nissler (ping if slow), 2012-11-06 17:33:05]

On Tue, Nov 6, 2012 at 6:06 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> Hi Mattias,
>
> I respect your decision as an OWNER about what the shape of the API
> should be, and I don't intend to renege on my promise to bring about
> the Callback-based approach; I have already spent the better part of
> the day working on it, and a good part of yesterday.
>

That work is much appreciated!


>
> My suggestion to defer this work was in the context of you saying you
> did not have time to work on it with me.  If either you or Dominic has
> time to help me figure out the new API, then that should probably be
> fine and I can take care of this end to end, as I think was the
> implicit promise from the change you reviewed.
>

Ah, so that's were we got our wires crossed. I had mistakenly understood
that you were suggesting we should do the new-API work entirely on our
side, which was obviously not what you was saying. All good then. I'll talk
with Dominic tomorrow and we'll hopefully come to a conclusion on how the
API should look like.


> (As an aside, I'm not sure yet whether or not you agree with my
> suggestion that it might be more efficient if you and Dominic design
> the API and then I can help switch usages over.  If you don't agree,
> that's fine, but I will still definitely need some time from one of
> you for reviewing my suggestions.)
>

Happy to provide that.


>
> Since we are being frank, let me provide some background on my
> thinking.  Again, this does not change what I promised to do and still
> intend to do; it's just for clarification and for us to keep in mind
> in future discussions:  My team's mandate is to extract components out
> of src/chrome/, and it's as part of this mandate that I am trying to
> make this happen for Prefs.  This means I favor forward progress on
> the goal of extracting components by using reasonable,
> straight-forward means to break dependencies.  In fact there has been
> the explicit intent, discussed e.g. with Ben Goodger, that
> improvements in APIs should be left to OWNERS, but of course there
> will be special cases like this one, where the case can be made that
> it doesn't make sense to change an API to one thing if it's soon going
> to change to another.  So I chose the PrefObserver paradigm as the
> most straight-forward possible change I could see that would break the
> dependency.  In retrospect, as I have said before, I should have
> discussed with you before starting and we might possibly have arrived
> at the API you desire with less total work.
>

Understood.


>
> To move forward, if I can get feedback on the latest draft, that
> should be enough for now.
>

I'll send that tomorrow after discussion with Dominic.


>
> Cheers,
> Jói
>
>
>
> On Tue, Nov 6, 2012 at 3:31 PM,  <mnissler@chromium.org> wrote:
> > On 2012/11/06 14:51:08, Jói wrote:
> >>
> >> > I'm clogged with other stuff, so I may be a bit slow. Dominic, would
> you
> >> > mind
> >> > taking a look at the current code and offer your opinion?
> >
> >
> >> An alternative is to leave this for now, until you guys have more time.
> >
> >
> >> My primary goal is to extract components out of src/chrome/, Prefs
> >> being the first.  That means breaking dependencies, which has already
> >> happened in this case (PrefObserver instead of
> >> content::NotificationObserver).  I hear you that you want to take the
> >> opportunity to improve the interface further (per-pref-name
> >> registration of base::Callback instead of PrefObserver), and I think
> >> it's a worth-while thing to do at some point, but I don't think that's
> >> as urgent a goal (at least it's not to me) so we could leave it for a
> >> bit.
> >
> >
> >> Whether we do the PrefObserver removal now or later, I don't think it
> >> makes it any harder or easier.
> >
> >
> > TBH, I'm a bit disappointed by this answer. I stamped the PrefObserver CL
> > mainly
> > to make your life a little simpler, and now you're arguing it's a
> solution
> > that's good for a while (so it'll likely stay in the tree for the time
> > being).
> > While that's true from a technical point of view, we're now forcing
> > PrefService
> > consumers to re-learn the interface twice instead of figuring out a good
> > solution and then converting everything to that in one sweep. I
> understand
> > that
> > you may not be motivated to refactor pref observing, and that's fine, but
> > then
> > please be honest in the first place (maybe I misunderstood our earlier
> > communication on this?).
> >
> >
> >
> >> For making Prefs a component, there is lots more work, and it's the
> >> first of many components we are trying to move out of src/chrome/.
> >> The next stop is probably to look at splitting PrefService up so that
> >> the generic part can move to base/prefs/ and the stuff that is
> >> Chrome-specific (e.g. setting up the priority list of various
> >> PrefStores, some of which are Chrome-specific) can stay in
> >> chrome/browser/prefs/.
> >
> >
> > Agreed.
> >
> >
> >> Cheers,
> >> Jói
> >
> >
> >
> >
> >> On Tue, Nov 6, 2012 at 2:43 PM,  <mailto:mnissler@chromium.org> wrote:
> >> > On 2012/11/06 13:35:50, Jói wrote:
> >> >>
> >> >> If I understand you correctly, you mean PrefNotifierImpl would no
> >> >> longer filter on the pref name and would just forward to all
> >> >> registered PrefChangeRegistrars.  (Would those be registered as
> >> >> base::Callbacks, or as PrefChangeRegistrar, or as a base interface
> >> >> like PrefObserver?)
> >> >
> >> >
> >> >> If that's the intent, then yes that works but seems potentially a bit
> >> >> less performant than the current situation, which creates a list of
> >> >> PrefObservers (in PrefNotifierImpl) to notify per pref name.
> >> >
> >> >
> >> > Yes, that was the intent behind my comment. Performance-wise, I think
> >> > it's
> >> > hard
> >> > to say what's better with out measuring this. Note that we're now
> doing
> >> > less
> >> > work on pref registration, but more on dispatch. I wouldn't be
> confident
> >> > to
> >> > say
> >> > either change is significant without data though :)
> >> >
> >> >
> >> >
> >> >> I guess an alternative would be for PrefNotifierImpl to continue
> >> >> building a map from pref name to list of observers (which would now
> be
> >> >> PrefChangeRegistrar pointers), but again I think this is a bit less
> >> >> efficient in terms of memory usage than the current model (you are
> >> >> storing both a pointer to a PrefChangeRegistrar per registration,
> plus
> >> >> the callback object per registration).
> >> >
> >> >
> >> > That seems like the worst of both worlds :)
> >> >
> >> >
> >> >
> >> >> Perhaps it is best I leave it to you guys to come up with the new
> >> >> interface and bindings from PrefChangeRegistrar to PrefService to
> >> >> PrefNotifierImpl, while I continue to focus on breaking dependencies
> >> >> and moving the Prefs code to base/prefs?
> >> >
> >> >
> >> > I'm clogged with other stuff, so I may be a bit slow. Dominic, would
> you
> >> > mind
> >> > taking a look at the current code and offer your opinion?
> >> >
> >> >
> >> >> If we do it that way, I am
> >> >> still happy to help convert callers to the new interface when it is
> >> >> ready, as I had promised.
> >> >
> >> >
> >> >> Cheers,
> >> >> Jói
> >> >
> >> >
> >> >
> >> >
> >> >> On Tue, Nov 6, 2012 at 1:26 PM,  <mailto:mnissler@chromium.org>
> wrote:
> >> >> > See comments for thoughts :)
> >> >> >
> >> >> >
> >> >> >
> >> >
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
> >>
> >> >>
> >> >> > File base/prefs/public/pref_change_registrar.cc (right):
> >> >> >
> >> >> >
> >> >
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
> >>
> >> >>
> >> >> > base/prefs/public/pref_change_registrar.cc:51:
> base::Unretained(obs),
> >> >> > service_, path));
> >> >> > Hm, comparing callbacks... not sure whether that's a good idea.
> >> >> >
> >> >> > In the discussion with battre@ last week, we figured we'd probably
> >> >> > change the interface here to allow only a single callback per pref
> >> >> > (we
> >> >> > thought that's a reasonable restriction, haven't checked all
> >> >> > consumers
> >> >> > though). Also we'd change the model to not forward the callback
> >> >> > registration with PrefService, but store a std::map<std::string,
> >> >> > base::Closure> in PrefChangeRegistrar. We would then have
> PrefService
> >> >> > keep a list of PrefChangeRegistrars, which it forwards pref change
> >> >> > events to, and PrefChangeRegistrar would figure out the callback to
> >> >> > invoke using its map. Remove would not take a closure as a
> parameter.
> >> >> > WDYT? Makes sense?
> >> >> >
> >> >> >
> >> >
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
> >>
> >> >>
> >> >> > File chrome/browser/api/prefs/pref_member.cc (right):
> >> >> >
> >> >> >
> >> >
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
> >>
> >> >>
> >> >> > chrome/browser/api/prefs/pref_member.cc:65: const std::string&
> >> >> > pref_name) {
> >> >> > doesn't require these parameters any longer, can just use |prefs_|
> >> >> > and
> >> >> > |pref_name_|
> >> >> >
> >> >> > <crxref style="background-image:
> >> >
> >> >
> >
> >
> >
>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);"><crxref
> > style="background-image:
> >
>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">
> http://codereview.chromium.org/11368098/</crxref></crxref>
> >
> >> >
> >> >
> >> >
> >> > <crxref style="background-image:
> >
> >
>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">
> http://codereview.chromium.org/11368098/</crxref>
> >
> >
> >
> > http://codereview.chromium.org/11368098/
>

[Jói, 2012-11-06 17:35:58]

Great, thanks Mattias.

(typed on tiny keyboard)
On Nov 6, 2012 5:33 PM, "Mattias Nissler" <mnissler@chromium.org> wrote:

>
>
> On Tue, Nov 6, 2012 at 6:06 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>
>> Hi Mattias,
>>
>> I respect your decision as an OWNER about what the shape of the API
>> should be, and I don't intend to renege on my promise to bring about
>> the Callback-based approach; I have already spent the better part of
>> the day working on it, and a good part of yesterday.
>>
>
> That work is much appreciated!
>
>
>>
>> My suggestion to defer this work was in the context of you saying you
>> did not have time to work on it with me.  If either you or Dominic has
>> time to help me figure out the new API, then that should probably be
>> fine and I can take care of this end to end, as I think was the
>> implicit promise from the change you reviewed.
>>
>
> Ah, so that's were we got our wires crossed. I had mistakenly understood
> that you were suggesting we should do the new-API work entirely on our
> side, which was obviously not what you was saying. All good then. I'll talk
> with Dominic tomorrow and we'll hopefully come to a conclusion on how the
> API should look like.
>
>
>> (As an aside, I'm not sure yet whether or not you agree with my
>> suggestion that it might be more efficient if you and Dominic design
>> the API and then I can help switch usages over.  If you don't agree,
>> that's fine, but I will still definitely need some time from one of
>> you for reviewing my suggestions.)
>>
>
> Happy to provide that.
>
>
>>
>> Since we are being frank, let me provide some background on my
>> thinking.  Again, this does not change what I promised to do and still
>> intend to do; it's just for clarification and for us to keep in mind
>> in future discussions:  My team's mandate is to extract components out
>> of src/chrome/, and it's as part of this mandate that I am trying to
>> make this happen for Prefs.  This means I favor forward progress on
>> the goal of extracting components by using reasonable,
>> straight-forward means to break dependencies.  In fact there has been
>> the explicit intent, discussed e.g. with Ben Goodger, that
>> improvements in APIs should be left to OWNERS, but of course there
>> will be special cases like this one, where the case can be made that
>> it doesn't make sense to change an API to one thing if it's soon going
>> to change to another.  So I chose the PrefObserver paradigm as the
>> most straight-forward possible change I could see that would break the
>> dependency.  In retrospect, as I have said before, I should have
>> discussed with you before starting and we might possibly have arrived
>> at the API you desire with less total work.
>>
>
> Understood.
>
>
>>
>> To move forward, if I can get feedback on the latest draft, that
>> should be enough for now.
>>
>
> I'll send that tomorrow after discussion with Dominic.
>
>
>>
>> Cheers,
>> Jói
>>
>>
>>
>> On Tue, Nov 6, 2012 at 3:31 PM,  <mnissler@chromium.org> wrote:
>> > On 2012/11/06 14:51:08, Jói wrote:
>> >>
>> >> > I'm clogged with other stuff, so I may be a bit slow. Dominic, would
>> you
>> >> > mind
>> >> > taking a look at the current code and offer your opinion?
>> >
>> >
>> >> An alternative is to leave this for now, until you guys have more time.
>> >
>> >
>> >> My primary goal is to extract components out of src/chrome/, Prefs
>> >> being the first.  That means breaking dependencies, which has already
>> >> happened in this case (PrefObserver instead of
>> >> content::NotificationObserver).  I hear you that you want to take the
>> >> opportunity to improve the interface further (per-pref-name
>> >> registration of base::Callback instead of PrefObserver), and I think
>> >> it's a worth-while thing to do at some point, but I don't think that's
>> >> as urgent a goal (at least it's not to me) so we could leave it for a
>> >> bit.
>> >
>> >
>> >> Whether we do the PrefObserver removal now or later, I don't think it
>> >> makes it any harder or easier.
>> >
>> >
>> > TBH, I'm a bit disappointed by this answer. I stamped the PrefObserver
>> CL
>> > mainly
>> > to make your life a little simpler, and now you're arguing it's a
>> solution
>> > that's good for a while (so it'll likely stay in the tree for the time
>> > being).
>> > While that's true from a technical point of view, we're now forcing
>> > PrefService
>> > consumers to re-learn the interface twice instead of figuring out a good
>> > solution and then converting everything to that in one sweep. I
>> understand
>> > that
>> > you may not be motivated to refactor pref observing, and that's fine,
>> but
>> > then
>> > please be honest in the first place (maybe I misunderstood our earlier
>> > communication on this?).
>> >
>> >
>> >
>> >> For making Prefs a component, there is lots more work, and it's the
>> >> first of many components we are trying to move out of src/chrome/.
>> >> The next stop is probably to look at splitting PrefService up so that
>> >> the generic part can move to base/prefs/ and the stuff that is
>> >> Chrome-specific (e.g. setting up the priority list of various
>> >> PrefStores, some of which are Chrome-specific) can stay in
>> >> chrome/browser/prefs/.
>> >
>> >
>> > Agreed.
>> >
>> >
>> >> Cheers,
>> >> Jói
>> >
>> >
>> >
>> >
>> >> On Tue, Nov 6, 2012 at 2:43 PM,  <mailto:mnissler@chromium.org> wrote:
>> >> > On 2012/11/06 13:35:50, Jói wrote:
>> >> >>
>> >> >> If I understand you correctly, you mean PrefNotifierImpl would no
>> >> >> longer filter on the pref name and would just forward to all
>> >> >> registered PrefChangeRegistrars.  (Would those be registered as
>> >> >> base::Callbacks, or as PrefChangeRegistrar, or as a base interface
>> >> >> like PrefObserver?)
>> >> >
>> >> >
>> >> >> If that's the intent, then yes that works but seems potentially a
>> bit
>> >> >> less performant than the current situation, which creates a list of
>> >> >> PrefObservers (in PrefNotifierImpl) to notify per pref name.
>> >> >
>> >> >
>> >> > Yes, that was the intent behind my comment. Performance-wise, I think
>> >> > it's
>> >> > hard
>> >> > to say what's better with out measuring this. Note that we're now
>> doing
>> >> > less
>> >> > work on pref registration, but more on dispatch. I wouldn't be
>> confident
>> >> > to
>> >> > say
>> >> > either change is significant without data though :)
>> >> >
>> >> >
>> >> >
>> >> >> I guess an alternative would be for PrefNotifierImpl to continue
>> >> >> building a map from pref name to list of observers (which would now
>> be
>> >> >> PrefChangeRegistrar pointers), but again I think this is a bit less
>> >> >> efficient in terms of memory usage than the current model (you are
>> >> >> storing both a pointer to a PrefChangeRegistrar per registration,
>> plus
>> >> >> the callback object per registration).
>> >> >
>> >> >
>> >> > That seems like the worst of both worlds :)
>> >> >
>> >> >
>> >> >
>> >> >> Perhaps it is best I leave it to you guys to come up with the new
>> >> >> interface and bindings from PrefChangeRegistrar to PrefService to
>> >> >> PrefNotifierImpl, while I continue to focus on breaking dependencies
>> >> >> and moving the Prefs code to base/prefs?
>> >> >
>> >> >
>> >> > I'm clogged with other stuff, so I may be a bit slow. Dominic, would
>> you
>> >> > mind
>> >> > taking a look at the current code and offer your opinion?
>> >> >
>> >> >
>> >> >> If we do it that way, I am
>> >> >> still happy to help convert callers to the new interface when it is
>> >> >> ready, as I had promised.
>> >> >
>> >> >
>> >> >> Cheers,
>> >> >> Jói
>> >> >
>> >> >
>> >> >
>> >> >
>> >> >> On Tue, Nov 6, 2012 at 1:26 PM,  <mailto:mnissler@chromium.org>
>> wrote:
>> >> >> > See comments for thoughts :)
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >
>> >
>> >
>>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
>> >>
>> >> >>
>> >> >> > File base/prefs/public/pref_change_registrar.cc (right):
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >
>> >
>> >
>>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
>> >>
>> >> >>
>> >> >> > base/prefs/public/pref_change_registrar.cc:51:
>> base::Unretained(obs),
>> >> >> > service_, path));
>> >> >> > Hm, comparing callbacks... not sure whether that's a good idea.
>> >> >> >
>> >> >> > In the discussion with battre@ last week, we figured we'd
>> probably
>> >> >> > change the interface here to allow only a single callback per pref
>> >> >> > (we
>> >> >> > thought that's a reasonable restriction, haven't checked all
>> >> >> > consumers
>> >> >> > though). Also we'd change the model to not forward the callback
>> >> >> > registration with PrefService, but store a std::map<std::string,
>> >> >> > base::Closure> in PrefChangeRegistrar. We would then have
>> PrefService
>> >> >> > keep a list of PrefChangeRegistrars, which it forwards pref change
>> >> >> > events to, and PrefChangeRegistrar would figure out the callback
>> to
>> >> >> > invoke using its map. Remove would not take a closure as a
>> parameter.
>> >> >> > WDYT? Makes sense?
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >
>> >
>> >
>>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
>> >>
>> >> >>
>> >> >> > File chrome/browser/api/prefs/pref_member.cc (right):
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >
>> >
>> >
>>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
>> >>
>> >> >>
>> >> >> > chrome/browser/api/prefs/pref_member.cc:65: const std::string&
>> >> >> > pref_name) {
>> >> >> > doesn't require these parameters any longer, can just use |prefs_|
>> >> >> > and
>> >> >> > |pref_name_|
>> >> >> >
>> >> >> > <crxref style="background-image:
>> >> >
>> >> >
>> >
>> >
>> >
>>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);"><crxref
>> > style="background-image:
>> >
>>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">
>> http://codereview.chromium.org/11368098/</crxref></crxref>
>> >
>> >> >
>> >> >
>> >> >
>> >> > <crxref style="background-image:
>> >
>> >
>>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">
>> http://codereview.chromium.org/11368098/</crxref>
>> >
>> >
>> >
>> > http://codereview.chromium.org/11368098/
>>
>
>

[Mattias Nissler (ping if slow), 2012-11-07 10:59:41]

Just chatted with Dominic. We're both favoring the following
PrefChangeRegistrar interface:

class PrefChangeRegistrar {
  // ...

  // Allows registration of only one callback per |path|, CHECK()s
otherwise.
  void Add(const char* path, const base::Closure& callback);
  void Remove(const char* path);

  // ...
}

Regarding how to implement that interface behind the scenes and potential
performance implications, we feel we should run some measurements (based on
numbers on how many registrars/registration/pref changes show up in a
typical browser run) to make sure performance doesn't degrade. In
particular, we'd like to measure the following options:

   1. Keep a std::vector<PrefChangeRegistrar> in PrefNotifierImpl, loop
   through all of them when dispatching a notification, let the
   PrefChangeRegistrar keep a std::map<std::string, base::Closure> for
   dispatching to individual callbacks.
   2. Keep a std::map<std::string, std::pair<PrefChangeRegistrar*,
   base::Closure> > in PrefNotifierImpl. Dispatching would not call into the
   registrars, and we would use the first element in the pair for implementing
   Remove().
   3. Keep a std::map<std::string, PrefChangeRegistrar*> in
   PrefNotifierImpl and a std::map<std::string, base::Closure> in
   PrefChangeRegistrar. Dispatch would work in two stages, first to the
   registrar, and then within the registrar to the correct callback.

We're trying to find cycles for the measurements later this week or next
week. Comments and help are welcome!

Thanks,
Mattias

On Tue, Nov 6, 2012 at 6:32 PM, Mattias Nissler <mnissler@chromium.org>wrote:

>
>
> On Tue, Nov 6, 2012 at 6:06 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>
>> Hi Mattias,
>>
>> I respect your decision as an OWNER about what the shape of the API
>> should be, and I don't intend to renege on my promise to bring about
>> the Callback-based approach; I have already spent the better part of
>> the day working on it, and a good part of yesterday.
>>
>
> That work is much appreciated!
>
>
>>
>> My suggestion to defer this work was in the context of you saying you
>> did not have time to work on it with me.  If either you or Dominic has
>> time to help me figure out the new API, then that should probably be
>> fine and I can take care of this end to end, as I think was the
>> implicit promise from the change you reviewed.
>>
>
> Ah, so that's were we got our wires crossed. I had mistakenly understood
> that you were suggesting we should do the new-API work entirely on our
> side, which was obviously not what you was saying. All good then. I'll talk
> with Dominic tomorrow and we'll hopefully come to a conclusion on how the
> API should look like.
>
>
>> (As an aside, I'm not sure yet whether or not you agree with my
>> suggestion that it might be more efficient if you and Dominic design
>> the API and then I can help switch usages over.  If you don't agree,
>> that's fine, but I will still definitely need some time from one of
>> you for reviewing my suggestions.)
>>
>
> Happy to provide that.
>
>
>>
>> Since we are being frank, let me provide some background on my
>> thinking.  Again, this does not change what I promised to do and still
>> intend to do; it's just for clarification and for us to keep in mind
>> in future discussions:  My team's mandate is to extract components out
>> of src/chrome/, and it's as part of this mandate that I am trying to
>> make this happen for Prefs.  This means I favor forward progress on
>> the goal of extracting components by using reasonable,
>> straight-forward means to break dependencies.  In fact there has been
>> the explicit intent, discussed e.g. with Ben Goodger, that
>> improvements in APIs should be left to OWNERS, but of course there
>> will be special cases like this one, where the case can be made that
>> it doesn't make sense to change an API to one thing if it's soon going
>> to change to another.  So I chose the PrefObserver paradigm as the
>> most straight-forward possible change I could see that would break the
>> dependency.  In retrospect, as I have said before, I should have
>> discussed with you before starting and we might possibly have arrived
>> at the API you desire with less total work.
>>
>
> Understood.
>
>
>>
>> To move forward, if I can get feedback on the latest draft, that
>> should be enough for now.
>>
>
> I'll send that tomorrow after discussion with Dominic.
>
>
>>
>> Cheers,
>> Jói
>>
>>
>>
>> On Tue, Nov 6, 2012 at 3:31 PM,  <mnissler@chromium.org> wrote:
>> > On 2012/11/06 14:51:08, Jói wrote:
>> >>
>> >> > I'm clogged with other stuff, so I may be a bit slow. Dominic, would
>> you
>> >> > mind
>> >> > taking a look at the current code and offer your opinion?
>> >
>> >
>> >> An alternative is to leave this for now, until you guys have more time.
>> >
>> >
>> >> My primary goal is to extract components out of src/chrome/, Prefs
>> >> being the first.  That means breaking dependencies, which has already
>> >> happened in this case (PrefObserver instead of
>> >> content::NotificationObserver).  I hear you that you want to take the
>> >> opportunity to improve the interface further (per-pref-name
>> >> registration of base::Callback instead of PrefObserver), and I think
>> >> it's a worth-while thing to do at some point, but I don't think that's
>> >> as urgent a goal (at least it's not to me) so we could leave it for a
>> >> bit.
>> >
>> >
>> >> Whether we do the PrefObserver removal now or later, I don't think it
>> >> makes it any harder or easier.
>> >
>> >
>> > TBH, I'm a bit disappointed by this answer. I stamped the PrefObserver
>> CL
>> > mainly
>> > to make your life a little simpler, and now you're arguing it's a
>> solution
>> > that's good for a while (so it'll likely stay in the tree for the time
>> > being).
>> > While that's true from a technical point of view, we're now forcing
>> > PrefService
>> > consumers to re-learn the interface twice instead of figuring out a good
>> > solution and then converting everything to that in one sweep. I
>> understand
>> > that
>> > you may not be motivated to refactor pref observing, and that's fine,
>> but
>> > then
>> > please be honest in the first place (maybe I misunderstood our earlier
>> > communication on this?).
>> >
>> >
>> >
>> >> For making Prefs a component, there is lots more work, and it's the
>> >> first of many components we are trying to move out of src/chrome/.
>> >> The next stop is probably to look at splitting PrefService up so that
>> >> the generic part can move to base/prefs/ and the stuff that is
>> >> Chrome-specific (e.g. setting up the priority list of various
>> >> PrefStores, some of which are Chrome-specific) can stay in
>> >> chrome/browser/prefs/.
>> >
>> >
>> > Agreed.
>> >
>> >
>> >> Cheers,
>> >> Jói
>> >
>> >
>> >
>> >
>> >> On Tue, Nov 6, 2012 at 2:43 PM,  <mailto:mnissler@chromium.org> wrote:
>> >> > On 2012/11/06 13:35:50, Jói wrote:
>> >> >>
>> >> >> If I understand you correctly, you mean PrefNotifierImpl would no
>> >> >> longer filter on the pref name and would just forward to all
>> >> >> registered PrefChangeRegistrars.  (Would those be registered as
>> >> >> base::Callbacks, or as PrefChangeRegistrar, or as a base interface
>> >> >> like PrefObserver?)
>> >> >
>> >> >
>> >> >> If that's the intent, then yes that works but seems potentially a
>> bit
>> >> >> less performant than the current situation, which creates a list of
>> >> >> PrefObservers (in PrefNotifierImpl) to notify per pref name.
>> >> >
>> >> >
>> >> > Yes, that was the intent behind my comment. Performance-wise, I think
>> >> > it's
>> >> > hard
>> >> > to say what's better with out measuring this. Note that we're now
>> doing
>> >> > less
>> >> > work on pref registration, but more on dispatch. I wouldn't be
>> confident
>> >> > to
>> >> > say
>> >> > either change is significant without data though :)
>> >> >
>> >> >
>> >> >
>> >> >> I guess an alternative would be for PrefNotifierImpl to continue
>> >> >> building a map from pref name to list of observers (which would now
>> be
>> >> >> PrefChangeRegistrar pointers), but again I think this is a bit less
>> >> >> efficient in terms of memory usage than the current model (you are
>> >> >> storing both a pointer to a PrefChangeRegistrar per registration,
>> plus
>> >> >> the callback object per registration).
>> >> >
>> >> >
>> >> > That seems like the worst of both worlds :)
>> >> >
>> >> >
>> >> >
>> >> >> Perhaps it is best I leave it to you guys to come up with the new
>> >> >> interface and bindings from PrefChangeRegistrar to PrefService to
>> >> >> PrefNotifierImpl, while I continue to focus on breaking dependencies
>> >> >> and moving the Prefs code to base/prefs?
>> >> >
>> >> >
>> >> > I'm clogged with other stuff, so I may be a bit slow. Dominic, would
>> you
>> >> > mind
>> >> > taking a look at the current code and offer your opinion?
>> >> >
>> >> >
>> >> >> If we do it that way, I am
>> >> >> still happy to help convert callers to the new interface when it is
>> >> >> ready, as I had promised.
>> >> >
>> >> >
>> >> >> Cheers,
>> >> >> Jói
>> >> >
>> >> >
>> >> >
>> >> >
>> >> >> On Tue, Nov 6, 2012 at 1:26 PM,  <mailto:mnissler@chromium.org>
>> wrote:
>> >> >> > See comments for thoughts :)
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >
>> >
>> >
>>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
>> >>
>> >> >>
>> >> >> > File base/prefs/public/pref_change_registrar.cc (right):
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >
>> >
>> >
>>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
>> >>
>> >> >>
>> >> >> > base/prefs/public/pref_change_registrar.cc:51:
>> base::Unretained(obs),
>> >> >> > service_, path));
>> >> >> > Hm, comparing callbacks... not sure whether that's a good idea.
>> >> >> >
>> >> >> > In the discussion with battre@ last week, we figured we'd
>> probably
>> >> >> > change the interface here to allow only a single callback per pref
>> >> >> > (we
>> >> >> > thought that's a reasonable restriction, haven't checked all
>> >> >> > consumers
>> >> >> > though). Also we'd change the model to not forward the callback
>> >> >> > registration with PrefService, but store a std::map<std::string,
>> >> >> > base::Closure> in PrefChangeRegistrar. We would then have
>> PrefService
>> >> >> > keep a list of PrefChangeRegistrars, which it forwards pref change
>> >> >> > events to, and PrefChangeRegistrar would figure out the callback
>> to
>> >> >> > invoke using its map. Remove would not take a closure as a
>> parameter.
>> >> >> > WDYT? Makes sense?
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >
>> >
>> >
>>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
>> >>
>> >> >>
>> >> >> > File chrome/browser/api/prefs/pref_member.cc (right):
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >
>> >
>> >
>>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
>> >>
>> >> >>
>> >> >> > chrome/browser/api/prefs/pref_member.cc:65: const std::string&
>> >> >> > pref_name) {
>> >> >> > doesn't require these parameters any longer, can just use |prefs_|
>> >> >> > and
>> >> >> > |pref_name_|
>> >> >> >
>> >> >> > <crxref style="background-image:
>> >> >
>> >> >
>> >
>> >
>> >
>>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);"><crxref
>> > style="background-image:
>> >
>>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">
>> http://codereview.chromium.org/11368098/</crxref></crxref>
>> >
>> >> >
>> >> >
>> >> >
>> >> > <crxref style="background-image:
>> >
>> >
>>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">
>> http://codereview.chromium.org/11368098/</crxref>
>> >
>> >
>> >
>> > http://codereview.chromium.org/11368098/
>>
>
>

[Jói, 2012-11-07 11:20:04]

Cool, thanks guys.

> Keep a std::vector<PrefChangeRegistrar> in PrefNotifierImpl, loop through
> all of them when dispatching a notification, let the PrefChangeRegistrar
> keep a std::map<std::string, base::Closure> for dispatching to individual
> callbacks.

This is roughly what I started drafting, and I think it would work.
It is also the most obvious mapping to the objects, from an OO
standpoint.  Given that the interface is the same in all these cases
(and hence changes to unit tests and consumers of the interface), if I
have time I might just wrap up this implementation to see if it works,
and start switching users over.  You could then switch the
implementation to one of the others later if it proves faster; does
that make sense?

> Keep a std::map<std::string, std::pair<PrefChangeRegistrar*, base::Closure>
>> in PrefNotifierImpl. Dispatching would not call into the registrars, and
> we would use the first element in the pair for implementing Remove().

This would need to be a multi-map or keep a list of such pairs behind
each key.  On the surface it seems equivalent to the above, i.e. a
lookup followed by an iteration, with the difference that there are M
fewer method calls where M is the number of PrefChangeRegistrar
objects registered (since you are calling the closure directly from
PrefNotifierImpl instead of calling the PrefChangeRegistrar which then
calls the correct Closure).  Probably half as many method calls, not
sure this makes an appreciable difference though.

> Keep a std::map<std::string, PrefChangeRegistrar*> in PrefNotifierImpl and a
> std::map<std::string, base::Closure> in PrefChangeRegistrar. Dispatch would
> work in two stages, first to the registrar, and then within the registrar to
> the correct callback.

This is twice the number of map look-ups and feels like it would be
slower, but intuition may be wrong.

Cheers,
Jói



On Wed, Nov 7, 2012 at 10:59 AM, Mattias Nissler <mnissler@chromium.org> wrote:
> Just chatted with Dominic. We're both favoring the following
> PrefChangeRegistrar interface:
>
> class PrefChangeRegistrar {
>   // ...
>
>   // Allows registration of only one callback per |path|, CHECK()s
> otherwise.
>   void Add(const char* path, const base::Closure& callback);
>   void Remove(const char* path);
>
>   // ...
> }
>
> Regarding how to implement that interface behind the scenes and potential
> performance implications, we feel we should run some measurements (based on
> numbers on how many registrars/registration/pref changes show up in a
> typical browser run) to make sure performance doesn't degrade. In
> particular, we'd like to measure the following options:
>
> Keep a std::vector<PrefChangeRegistrar> in PrefNotifierImpl, loop through
> all of them when dispatching a notification, let the PrefChangeRegistrar
> keep a std::map<std::string, base::Closure> for dispatching to individual
> callbacks.
> Keep a std::map<std::string, std::pair<PrefChangeRegistrar*, base::Closure>
>> in PrefNotifierImpl. Dispatching would not call into the registrars, and
> we would use the first element in the pair for implementing Remove().
> Keep a std::map<std::string, PrefChangeRegistrar*> in PrefNotifierImpl and a
> std::map<std::string, base::Closure> in PrefChangeRegistrar. Dispatch would
> work in two stages, first to the registrar, and then within the registrar to
> the correct callback.
>
> We're trying to find cycles for the measurements later this week or next
> week. Comments and help are welcome!
>
> Thanks,
> Mattias
>
> On Tue, Nov 6, 2012 at 6:32 PM, Mattias Nissler <mnissler@chromium.org>
> wrote:
>>
>>
>>
>> On Tue, Nov 6, 2012 at 6:06 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>>
>>> Hi Mattias,
>>>
>>> I respect your decision as an OWNER about what the shape of the API
>>> should be, and I don't intend to renege on my promise to bring about
>>> the Callback-based approach; I have already spent the better part of
>>> the day working on it, and a good part of yesterday.
>>
>>
>> That work is much appreciated!
>>
>>>
>>>
>>> My suggestion to defer this work was in the context of you saying you
>>> did not have time to work on it with me.  If either you or Dominic has
>>> time to help me figure out the new API, then that should probably be
>>> fine and I can take care of this end to end, as I think was the
>>> implicit promise from the change you reviewed.
>>
>>
>> Ah, so that's were we got our wires crossed. I had mistakenly understood
>> that you were suggesting we should do the new-API work entirely on our side,
>> which was obviously not what you was saying. All good then. I'll talk with
>> Dominic tomorrow and we'll hopefully come to a conclusion on how the API
>> should look like.
>>
>>>
>>> (As an aside, I'm not sure yet whether or not you agree with my
>>> suggestion that it might be more efficient if you and Dominic design
>>> the API and then I can help switch usages over.  If you don't agree,
>>> that's fine, but I will still definitely need some time from one of
>>> you for reviewing my suggestions.)
>>
>>
>> Happy to provide that.
>>
>>>
>>>
>>> Since we are being frank, let me provide some background on my
>>> thinking.  Again, this does not change what I promised to do and still
>>> intend to do; it's just for clarification and for us to keep in mind
>>> in future discussions:  My team's mandate is to extract components out
>>> of src/chrome/, and it's as part of this mandate that I am trying to
>>> make this happen for Prefs.  This means I favor forward progress on
>>> the goal of extracting components by using reasonable,
>>> straight-forward means to break dependencies.  In fact there has been
>>> the explicit intent, discussed e.g. with Ben Goodger, that
>>> improvements in APIs should be left to OWNERS, but of course there
>>> will be special cases like this one, where the case can be made that
>>> it doesn't make sense to change an API to one thing if it's soon going
>>> to change to another.  So I chose the PrefObserver paradigm as the
>>> most straight-forward possible change I could see that would break the
>>> dependency.  In retrospect, as I have said before, I should have
>>> discussed with you before starting and we might possibly have arrived
>>> at the API you desire with less total work.
>>
>>
>> Understood.
>>
>>>
>>>
>>> To move forward, if I can get feedback on the latest draft, that
>>> should be enough for now.
>>
>>
>> I'll send that tomorrow after discussion with Dominic.
>>
>>>
>>>
>>> Cheers,
>>> Jói
>>>
>>>
>>>
>>> On Tue, Nov 6, 2012 at 3:31 PM,  <mnissler@chromium.org> wrote:
>>> > On 2012/11/06 14:51:08, Jói wrote:
>>> >>
>>> >> > I'm clogged with other stuff, so I may be a bit slow. Dominic, would
>>> >> > you
>>> >> > mind
>>> >> > taking a look at the current code and offer your opinion?
>>> >
>>> >
>>> >> An alternative is to leave this for now, until you guys have more
>>> >> time.
>>> >
>>> >
>>> >> My primary goal is to extract components out of src/chrome/, Prefs
>>> >> being the first.  That means breaking dependencies, which has already
>>> >> happened in this case (PrefObserver instead of
>>> >> content::NotificationObserver).  I hear you that you want to take the
>>> >> opportunity to improve the interface further (per-pref-name
>>> >> registration of base::Callback instead of PrefObserver), and I think
>>> >> it's a worth-while thing to do at some point, but I don't think that's
>>> >> as urgent a goal (at least it's not to me) so we could leave it for a
>>> >> bit.
>>> >
>>> >
>>> >> Whether we do the PrefObserver removal now or later, I don't think it
>>> >> makes it any harder or easier.
>>> >
>>> >
>>> > TBH, I'm a bit disappointed by this answer. I stamped the PrefObserver
>>> > CL
>>> > mainly
>>> > to make your life a little simpler, and now you're arguing it's a
>>> > solution
>>> > that's good for a while (so it'll likely stay in the tree for the time
>>> > being).
>>> > While that's true from a technical point of view, we're now forcing
>>> > PrefService
>>> > consumers to re-learn the interface twice instead of figuring out a
>>> > good
>>> > solution and then converting everything to that in one sweep. I
>>> > understand
>>> > that
>>> > you may not be motivated to refactor pref observing, and that's fine,
>>> > but
>>> > then
>>> > please be honest in the first place (maybe I misunderstood our earlier
>>> > communication on this?).
>>> >
>>> >
>>> >
>>> >> For making Prefs a component, there is lots more work, and it's the
>>> >> first of many components we are trying to move out of src/chrome/.
>>> >> The next stop is probably to look at splitting PrefService up so that
>>> >> the generic part can move to base/prefs/ and the stuff that is
>>> >> Chrome-specific (e.g. setting up the priority list of various
>>> >> PrefStores, some of which are Chrome-specific) can stay in
>>> >> chrome/browser/prefs/.
>>> >
>>> >
>>> > Agreed.
>>> >
>>> >
>>> >> Cheers,
>>> >> Jói
>>> >
>>> >
>>> >
>>> >
>>> >> On Tue, Nov 6, 2012 at 2:43 PM,  <mailto:mnissler@chromium.org> wrote:
>>> >> > On 2012/11/06 13:35:50, Jói wrote:
>>> >> >>
>>> >> >> If I understand you correctly, you mean PrefNotifierImpl would no
>>> >> >> longer filter on the pref name and would just forward to all
>>> >> >> registered PrefChangeRegistrars.  (Would those be registered as
>>> >> >> base::Callbacks, or as PrefChangeRegistrar, or as a base interface
>>> >> >> like PrefObserver?)
>>> >> >
>>> >> >
>>> >> >> If that's the intent, then yes that works but seems potentially a
>>> >> >> bit
>>> >> >> less performant than the current situation, which creates a list of
>>> >> >> PrefObservers (in PrefNotifierImpl) to notify per pref name.
>>> >> >
>>> >> >
>>> >> > Yes, that was the intent behind my comment. Performance-wise, I
>>> >> > think
>>> >> > it's
>>> >> > hard
>>> >> > to say what's better with out measuring this. Note that we're now
>>> >> > doing
>>> >> > less
>>> >> > work on pref registration, but more on dispatch. I wouldn't be
>>> >> > confident
>>> >> > to
>>> >> > say
>>> >> > either change is significant without data though :)
>>> >> >
>>> >> >
>>> >> >
>>> >> >> I guess an alternative would be for PrefNotifierImpl to continue
>>> >> >> building a map from pref name to list of observers (which would now
>>> >> >> be
>>> >> >> PrefChangeRegistrar pointers), but again I think this is a bit less
>>> >> >> efficient in terms of memory usage than the current model (you are
>>> >> >> storing both a pointer to a PrefChangeRegistrar per registration,
>>> >> >> plus
>>> >> >> the callback object per registration).
>>> >> >
>>> >> >
>>> >> > That seems like the worst of both worlds :)
>>> >> >
>>> >> >
>>> >> >
>>> >> >> Perhaps it is best I leave it to you guys to come up with the new
>>> >> >> interface and bindings from PrefChangeRegistrar to PrefService to
>>> >> >> PrefNotifierImpl, while I continue to focus on breaking
>>> >> >> dependencies
>>> >> >> and moving the Prefs code to base/prefs?
>>> >> >
>>> >> >
>>> >> > I'm clogged with other stuff, so I may be a bit slow. Dominic, would
>>> >> > you
>>> >> > mind
>>> >> > taking a look at the current code and offer your opinion?
>>> >> >
>>> >> >
>>> >> >> If we do it that way, I am
>>> >> >> still happy to help convert callers to the new interface when it is
>>> >> >> ready, as I had promised.
>>> >> >
>>> >> >
>>> >> >> Cheers,
>>> >> >> Jói
>>> >> >
>>> >> >
>>> >> >
>>> >> >
>>> >> >> On Tue, Nov 6, 2012 at 1:26 PM,  <mailto:mnissler@chromium.org>
>>> >> >> wrote:
>>> >> >> > See comments for thoughts :)
>>> >> >> >
>>> >> >> >
>>> >> >> >
>>> >> >
>>> >> >
>>> >> >
>>> >
>>> >
>>> >
>>> >
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
>>> >>
>>> >> >>
>>> >> >> > File base/prefs/public/pref_change_registrar.cc (right):
>>> >> >> >
>>> >> >> >
>>> >> >
>>> >> >
>>> >> >
>>> >
>>> >
>>> >
>>> >
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
>>> >>
>>> >> >>
>>> >> >> > base/prefs/public/pref_change_registrar.cc:51:
>>> >> >> > base::Unretained(obs),
>>> >> >> > service_, path));
>>> >> >> > Hm, comparing callbacks... not sure whether that's a good idea.
>>> >> >> >
>>> >> >> > In the discussion with battre@ last week, we figured we'd
>>> >> >> > probably
>>> >> >> > change the interface here to allow only a single callback per
>>> >> >> > pref
>>> >> >> > (we
>>> >> >> > thought that's a reasonable restriction, haven't checked all
>>> >> >> > consumers
>>> >> >> > though). Also we'd change the model to not forward the callback
>>> >> >> > registration with PrefService, but store a std::map<std::string,
>>> >> >> > base::Closure> in PrefChangeRegistrar. We would then have
>>> >> >> > PrefService
>>> >> >> > keep a list of PrefChangeRegistrars, which it forwards pref
>>> >> >> > change
>>> >> >> > events to, and PrefChangeRegistrar would figure out the callback
>>> >> >> > to
>>> >> >> > invoke using its map. Remove would not take a closure as a
>>> >> >> > parameter.
>>> >> >> > WDYT? Makes sense?
>>> >> >> >
>>> >> >> >
>>> >> >
>>> >> >
>>> >> >
>>> >
>>> >
>>> >
>>> >
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
>>> >>
>>> >> >>
>>> >> >> > File chrome/browser/api/prefs/pref_member.cc (right):
>>> >> >> >
>>> >> >> >
>>> >> >
>>> >> >
>>> >> >
>>> >
>>> >
>>> >
>>> >
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
>>> >>
>>> >> >>
>>> >> >> > chrome/browser/api/prefs/pref_member.cc:65: const std::string&
>>> >> >> > pref_name) {
>>> >> >> > doesn't require these parameters any longer, can just use
>>> >> >> > |prefs_|
>>> >> >> > and
>>> >> >> > |pref_name_|
>>> >> >> >
>>> >> >> > <crxref style="background-image:
>>> >> >
>>> >> >
>>> >
>>> >
>>> >
>>> >
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);"><crxref
>>> > style="background-image:
>>> >
>>> >
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">http://codereview.chromium.org/11368098/</crxref></crxref>
>>> >
>>> >> >
>>> >> >
>>> >> >
>>> >> > <crxref style="background-image:
>>> >
>>> >
>>> >
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">http://codereview.chromium.org/11368098/</crxref>
>>> >
>>> >
>>> >
>>> > http://codereview.chromium.org/11368098/
>>
>>
>

[Mattias Nissler (ping if slow), 2012-11-07 13:03:41]

On Wed, Nov 7, 2012 at 12:19 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> Cool, thanks guys.
>
> > Keep a std::vector<PrefChangeRegistrar> in PrefNotifierImpl, loop through
> > all of them when dispatching a notification, let the PrefChangeRegistrar
> > keep a std::map<std::string, base::Closure> for dispatching to individual
> > callbacks.
>
> This is roughly what I started drafting, and I think it would work.
> It is also the most obvious mapping to the objects, from an OO
> standpoint.  Given that the interface is the same in all these cases
> (and hence changes to unit tests and consumers of the interface), if I
> have time I might just wrap up this implementation to see if it works,
> and start switching users over.  You could then switch the
> implementation to one of the others later if it proves faster; does
> that make sense?
>

Makes sense to me. If you're willing to start working on this, I shall be
the last person to stop you!


>
> > Keep a std::map<std::string, std::pair<PrefChangeRegistrar*,
> base::Closure>
> >> in PrefNotifierImpl. Dispatching would not call into the registrars, and
> > we would use the first element in the pair for implementing Remove().
>
> This would need to be a multi-map or keep a list of such pairs behind
> each key.  On the surface it seems equivalent to the above, i.e. a
> lookup followed by an iteration, with the difference that there are M
> fewer method calls where M is the number of PrefChangeRegistrar
> objects registered (since you are calling the closure directly from
> PrefNotifierImpl instead of calling the PrefChangeRegistrar which then
> calls the correct Closure).  Probably half as many method calls, not
> sure this makes an appreciable difference though.
>
> > Keep a std::map<std::string, PrefChangeRegistrar*> in PrefNotifierImpl
> and a
> > std::map<std::string, base::Closure> in PrefChangeRegistrar. Dispatch
> would
> > work in two stages, first to the registrar, and then within the
> registrar to
> > the correct callback.
>
> This is twice the number of map look-ups and feels like it would be
> slower, but intuition may be wrong.
>

Then again, the maps have fewer entries. We need data ;)


>
> Cheers,
> Jói
>
>
>
> On Wed, Nov 7, 2012 at 10:59 AM, Mattias Nissler <mnissler@chromium.org>
> wrote:
> > Just chatted with Dominic. We're both favoring the following
> > PrefChangeRegistrar interface:
> >
> > class PrefChangeRegistrar {
> >   // ...
> >
> >   // Allows registration of only one callback per |path|, CHECK()s
> > otherwise.
> >   void Add(const char* path, const base::Closure& callback);
> >   void Remove(const char* path);
> >
> >   // ...
> > }
> >
> > Regarding how to implement that interface behind the scenes and potential
> > performance implications, we feel we should run some measurements (based
> on
> > numbers on how many registrars/registration/pref changes show up in a
> > typical browser run) to make sure performance doesn't degrade. In
> > particular, we'd like to measure the following options:
> >
> > Keep a std::vector<PrefChangeRegistrar> in PrefNotifierImpl, loop through
> > all of them when dispatching a notification, let the PrefChangeRegistrar
> > keep a std::map<std::string, base::Closure> for dispatching to individual
> > callbacks.
> > Keep a std::map<std::string, std::pair<PrefChangeRegistrar*,
> base::Closure>
> >> in PrefNotifierImpl. Dispatching would not call into the registrars, and
> > we would use the first element in the pair for implementing Remove().
> > Keep a std::map<std::string, PrefChangeRegistrar*> in PrefNotifierImpl
> and a
> > std::map<std::string, base::Closure> in PrefChangeRegistrar. Dispatch
> would
> > work in two stages, first to the registrar, and then within the
> registrar to
> > the correct callback.
> >
> > We're trying to find cycles for the measurements later this week or next
> > week. Comments and help are welcome!
> >
> > Thanks,
> > Mattias
> >
> > On Tue, Nov 6, 2012 at 6:32 PM, Mattias Nissler <mnissler@chromium.org>
> > wrote:
> >>
> >>
> >>
> >> On Tue, Nov 6, 2012 at 6:06 PM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >>>
> >>> Hi Mattias,
> >>>
> >>> I respect your decision as an OWNER about what the shape of the API
> >>> should be, and I don't intend to renege on my promise to bring about
> >>> the Callback-based approach; I have already spent the better part of
> >>> the day working on it, and a good part of yesterday.
> >>
> >>
> >> That work is much appreciated!
> >>
> >>>
> >>>
> >>> My suggestion to defer this work was in the context of you saying you
> >>> did not have time to work on it with me.  If either you or Dominic has
> >>> time to help me figure out the new API, then that should probably be
> >>> fine and I can take care of this end to end, as I think was the
> >>> implicit promise from the change you reviewed.
> >>
> >>
> >> Ah, so that's were we got our wires crossed. I had mistakenly understood
> >> that you were suggesting we should do the new-API work entirely on our
> side,
> >> which was obviously not what you was saying. All good then. I'll talk
> with
> >> Dominic tomorrow and we'll hopefully come to a conclusion on how the API
> >> should look like.
> >>
> >>>
> >>> (As an aside, I'm not sure yet whether or not you agree with my
> >>> suggestion that it might be more efficient if you and Dominic design
> >>> the API and then I can help switch usages over.  If you don't agree,
> >>> that's fine, but I will still definitely need some time from one of
> >>> you for reviewing my suggestions.)
> >>
> >>
> >> Happy to provide that.
> >>
> >>>
> >>>
> >>> Since we are being frank, let me provide some background on my
> >>> thinking.  Again, this does not change what I promised to do and still
> >>> intend to do; it's just for clarification and for us to keep in mind
> >>> in future discussions:  My team's mandate is to extract components out
> >>> of src/chrome/, and it's as part of this mandate that I am trying to
> >>> make this happen for Prefs.  This means I favor forward progress on
> >>> the goal of extracting components by using reasonable,
> >>> straight-forward means to break dependencies.  In fact there has been
> >>> the explicit intent, discussed e.g. with Ben Goodger, that
> >>> improvements in APIs should be left to OWNERS, but of course there
> >>> will be special cases like this one, where the case can be made that
> >>> it doesn't make sense to change an API to one thing if it's soon going
> >>> to change to another.  So I chose the PrefObserver paradigm as the
> >>> most straight-forward possible change I could see that would break the
> >>> dependency.  In retrospect, as I have said before, I should have
> >>> discussed with you before starting and we might possibly have arrived
> >>> at the API you desire with less total work.
> >>
> >>
> >> Understood.
> >>
> >>>
> >>>
> >>> To move forward, if I can get feedback on the latest draft, that
> >>> should be enough for now.
> >>
> >>
> >> I'll send that tomorrow after discussion with Dominic.
> >>
> >>>
> >>>
> >>> Cheers,
> >>> Jói
> >>>
> >>>
> >>>
> >>> On Tue, Nov 6, 2012 at 3:31 PM,  <mnissler@chromium.org> wrote:
> >>> > On 2012/11/06 14:51:08, Jói wrote:
> >>> >>
> >>> >> > I'm clogged with other stuff, so I may be a bit slow. Dominic,
> would
> >>> >> > you
> >>> >> > mind
> >>> >> > taking a look at the current code and offer your opinion?
> >>> >
> >>> >
> >>> >> An alternative is to leave this for now, until you guys have more
> >>> >> time.
> >>> >
> >>> >
> >>> >> My primary goal is to extract components out of src/chrome/, Prefs
> >>> >> being the first.  That means breaking dependencies, which has
> already
> >>> >> happened in this case (PrefObserver instead of
> >>> >> content::NotificationObserver).  I hear you that you want to take
> the
> >>> >> opportunity to improve the interface further (per-pref-name
> >>> >> registration of base::Callback instead of PrefObserver), and I think
> >>> >> it's a worth-while thing to do at some point, but I don't think
> that's
> >>> >> as urgent a goal (at least it's not to me) so we could leave it for
> a
> >>> >> bit.
> >>> >
> >>> >
> >>> >> Whether we do the PrefObserver removal now or later, I don't think
> it
> >>> >> makes it any harder or easier.
> >>> >
> >>> >
> >>> > TBH, I'm a bit disappointed by this answer. I stamped the
> PrefObserver
> >>> > CL
> >>> > mainly
> >>> > to make your life a little simpler, and now you're arguing it's a
> >>> > solution
> >>> > that's good for a while (so it'll likely stay in the tree for the
> time
> >>> > being).
> >>> > While that's true from a technical point of view, we're now forcing
> >>> > PrefService
> >>> > consumers to re-learn the interface twice instead of figuring out a
> >>> > good
> >>> > solution and then converting everything to that in one sweep. I
> >>> > understand
> >>> > that
> >>> > you may not be motivated to refactor pref observing, and that's fine,
> >>> > but
> >>> > then
> >>> > please be honest in the first place (maybe I misunderstood our
> earlier
> >>> > communication on this?).
> >>> >
> >>> >
> >>> >
> >>> >> For making Prefs a component, there is lots more work, and it's the
> >>> >> first of many components we are trying to move out of src/chrome/.
> >>> >> The next stop is probably to look at splitting PrefService up so
> that
> >>> >> the generic part can move to base/prefs/ and the stuff that is
> >>> >> Chrome-specific (e.g. setting up the priority list of various
> >>> >> PrefStores, some of which are Chrome-specific) can stay in
> >>> >> chrome/browser/prefs/.
> >>> >
> >>> >
> >>> > Agreed.
> >>> >
> >>> >
> >>> >> Cheers,
> >>> >> Jói
> >>> >
> >>> >
> >>> >
> >>> >
> >>> >> On Tue, Nov 6, 2012 at 2:43 PM,  <mailto:mnissler@chromium.org>
> wrote:
> >>> >> > On 2012/11/06 13:35:50, Jói wrote:
> >>> >> >>
> >>> >> >> If I understand you correctly, you mean PrefNotifierImpl would no
> >>> >> >> longer filter on the pref name and would just forward to all
> >>> >> >> registered PrefChangeRegistrars.  (Would those be registered as
> >>> >> >> base::Callbacks, or as PrefChangeRegistrar, or as a base
> interface
> >>> >> >> like PrefObserver?)
> >>> >> >
> >>> >> >
> >>> >> >> If that's the intent, then yes that works but seems potentially a
> >>> >> >> bit
> >>> >> >> less performant than the current situation, which creates a list
> of
> >>> >> >> PrefObservers (in PrefNotifierImpl) to notify per pref name.
> >>> >> >
> >>> >> >
> >>> >> > Yes, that was the intent behind my comment. Performance-wise, I
> >>> >> > think
> >>> >> > it's
> >>> >> > hard
> >>> >> > to say what's better with out measuring this. Note that we're now
> >>> >> > doing
> >>> >> > less
> >>> >> > work on pref registration, but more on dispatch. I wouldn't be
> >>> >> > confident
> >>> >> > to
> >>> >> > say
> >>> >> > either change is significant without data though :)
> >>> >> >
> >>> >> >
> >>> >> >
> >>> >> >> I guess an alternative would be for PrefNotifierImpl to continue
> >>> >> >> building a map from pref name to list of observers (which would
> now
> >>> >> >> be
> >>> >> >> PrefChangeRegistrar pointers), but again I think this is a bit
> less
> >>> >> >> efficient in terms of memory usage than the current model (you
> are
> >>> >> >> storing both a pointer to a PrefChangeRegistrar per registration,
> >>> >> >> plus
> >>> >> >> the callback object per registration).
> >>> >> >
> >>> >> >
> >>> >> > That seems like the worst of both worlds :)
> >>> >> >
> >>> >> >
> >>> >> >
> >>> >> >> Perhaps it is best I leave it to you guys to come up with the new
> >>> >> >> interface and bindings from PrefChangeRegistrar to PrefService to
> >>> >> >> PrefNotifierImpl, while I continue to focus on breaking
> >>> >> >> dependencies
> >>> >> >> and moving the Prefs code to base/prefs?
> >>> >> >
> >>> >> >
> >>> >> > I'm clogged with other stuff, so I may be a bit slow. Dominic,
> would
> >>> >> > you
> >>> >> > mind
> >>> >> > taking a look at the current code and offer your opinion?
> >>> >> >
> >>> >> >
> >>> >> >> If we do it that way, I am
> >>> >> >> still happy to help convert callers to the new interface when it
> is
> >>> >> >> ready, as I had promised.
> >>> >> >
> >>> >> >
> >>> >> >> Cheers,
> >>> >> >> Jói
> >>> >> >
> >>> >> >
> >>> >> >
> >>> >> >
> >>> >> >> On Tue, Nov 6, 2012 at 1:26 PM,  <mailto:mnissler@chromium.org>
> >>> >> >> wrote:
> >>> >> >> > See comments for thoughts :)
> >>> >> >> >
> >>> >> >> >
> >>> >> >> >
> >>> >> >
> >>> >> >
> >>> >> >
> >>> >
> >>> >
> >>> >
> >>> >
>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
> >>> >>
> >>> >> >>
> >>> >> >> > File base/prefs/public/pref_change_registrar.cc (right):
> >>> >> >> >
> >>> >> >> >
> >>> >> >
> >>> >> >
> >>> >> >
> >>> >
> >>> >
> >>> >
> >>> >
>
http://codereview.chromium.org/11368098/diff/2001/base/prefs/public/pref_chan...
> >>> >>
> >>> >> >>
> >>> >> >> > base/prefs/public/pref_change_registrar.cc:51:
> >>> >> >> > base::Unretained(obs),
> >>> >> >> > service_, path));
> >>> >> >> > Hm, comparing callbacks... not sure whether that's a good idea.
> >>> >> >> >
> >>> >> >> > In the discussion with battre@ last week, we figured we'd
> >>> >> >> > probably
> >>> >> >> > change the interface here to allow only a single callback per
> >>> >> >> > pref
> >>> >> >> > (we
> >>> >> >> > thought that's a reasonable restriction, haven't checked all
> >>> >> >> > consumers
> >>> >> >> > though). Also we'd change the model to not forward the callback
> >>> >> >> > registration with PrefService, but store a
> std::map<std::string,
> >>> >> >> > base::Closure> in PrefChangeRegistrar. We would then have
> >>> >> >> > PrefService
> >>> >> >> > keep a list of PrefChangeRegistrars, which it forwards pref
> >>> >> >> > change
> >>> >> >> > events to, and PrefChangeRegistrar would figure out the
> callback
> >>> >> >> > to
> >>> >> >> > invoke using its map. Remove would not take a closure as a
> >>> >> >> > parameter.
> >>> >> >> > WDYT? Makes sense?
> >>> >> >> >
> >>> >> >> >
> >>> >> >
> >>> >> >
> >>> >> >
> >>> >
> >>> >
> >>> >
> >>> >
>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
> >>> >>
> >>> >> >>
> >>> >> >> > File chrome/browser/api/prefs/pref_member.cc (right):
> >>> >> >> >
> >>> >> >> >
> >>> >> >
> >>> >> >
> >>> >> >
> >>> >
> >>> >
> >>> >
> >>> >
>
http://codereview.chromium.org/11368098/diff/2001/chrome/browser/api/prefs/pr...
> >>> >>
> >>> >> >>
> >>> >> >> > chrome/browser/api/prefs/pref_member.cc:65: const std::string&
> >>> >> >> > pref_name) {
> >>> >> >> > doesn't require these parameters any longer, can just use
> >>> >> >> > |prefs_|
> >>> >> >> > and
> >>> >> >> > |pref_name_|
> >>> >> >> >
> >>> >> >> > <crxref style="background-image:
> >>> >> >
> >>> >> >
> >>> >
> >>> >
> >>> >
> >>> >
>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);"><crxref
> >>> > style="background-image:
> >>> >
> >>> >
>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">
> http://codereview.chromium.org/11368098/</crxref></crxref>
> >>> >
> >>> >> >
> >>> >> >
> >>> >> >
> >>> >> > <crxref style="background-image:
> >>> >
> >>> >
> >>> >
>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">
> http://codereview.chromium.org/11368098/</crxref>
> >>> >
> >>> >
> >>> >
> >>> > http://codereview.chromium.org/11368098/
> >>
> >>
> >
>

[Jói, 2012-11-07 15:20:01]

I think this is ready for review.  Note the new change description, and a
slightly different approach implementation-wise.

I will start working on follow-ups to this change to switch PrefChangeRegistrar
users over; it seems clear that this will be the new API, regardless of how the
implementation might change.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2012-11-07 16:02:24]

Joi, thanks for your time working on this. Looks mostly good, just a couple
minor comments.

http://codereview.chromium.org/11368098/diff/8002/base/prefs/public/pref_chan...
File base/prefs/public/pref_change_registrar.cc (right):

http://codereview.chromium.org/11368098/diff/8002/base/prefs/public/pref_chan...
base/prefs/public/pref_change_registrar.cc:80: if (IsObserved(pref)) {
nit: no need for curlies.

http://codereview.chromium.org/11368098/diff/8002/chrome/browser/api/prefs/pr...
File chrome/browser/api/prefs/pref_member.h (right):

http://codereview.chromium.org/11368098/diff/8002/chrome/browser/api/prefs/pr...
chrome/browser/api/prefs/pref_member.h:131: void DoNothing() {}
base/bind_helpers.h has a DoNothing that you can use.

http://codereview.chromium.org/11368098/diff/8002/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/11368098/diff/8002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_unittest.cc:124: registrar.Add(pref_name_two,
&obs2);
I think we should just declare a second registrar here, the point of the test is
to check that two consumers watching the same pref work.

[Jói, 2012-11-08 11:03:14]

Thanks Mattias.  I addressed your concerns and fixed a bug caused by behavior of
base::Bind that I did not expect (need to wrap the pref_name param with
std::string or it seems to curry the raw const char*).

TBR=ben@chromium.org,finnur@chromium.org for trivial usage updates here:
    chrome/browser/ui/webui/options
    chrome/browser/ui/ash/launcher
    chrome/browser/extensions

Pulling the CQ trigger now.

Cheers,
Jói

http://codereview.chromium.org/11368098/diff/8002/base/prefs/public/pref_chan...
File base/prefs/public/pref_change_registrar.cc (right):

http://codereview.chromium.org/11368098/diff/8002/base/prefs/public/pref_chan...
base/prefs/public/pref_change_registrar.cc:80: if (IsObserved(pref)) {
On 2012/11/07 16:02:24, Mattias Nissler wrote:
> nit: no need for curlies.

Done.

http://codereview.chromium.org/11368098/diff/8002/chrome/browser/api/prefs/pr...
File chrome/browser/api/prefs/pref_member.h (right):

http://codereview.chromium.org/11368098/diff/8002/chrome/browser/api/prefs/pr...
chrome/browser/api/prefs/pref_member.h:131: void DoNothing() {}
On 2012/11/07 16:02:24, Mattias Nissler wrote:
> base/bind_helpers.h has a DoNothing that you can use.

Thanks, switched.

http://codereview.chromium.org/11368098/diff/8002/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/11368098/diff/8002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_unittest.cc:124: registrar.Add(pref_name_two,
&obs2);
On 2012/11/07 16:02:24, Mattias Nissler wrote:
> I think we should just declare a second registrar here, the point of the test
is
> to check that two consumers watching the same pref work.

Done.

[commit-bot: I haz the power, 2012-11-08 11:03:30]

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

[Finnur, 2012-11-08 12:00:28]

chrome/browser/extensions LGTM.

[commit-bot: I haz the power, 2012-11-08 13:06:20]

Change committed as 166670