Move PrefMember and PrefChangeRegistrar to api directory.

Move PrefMember and PrefChangeRegistrar to api directory.

This is the first step in breaking Autofill's concrete dependency on
prefs.  These two classes are essentially utilities with very small
dependencies back to PrefService (which I will break in the next
change, by introducing a virtual PrefServiceBase) so they are suitable
for api.

TBRing other owners for mechanical changes.
TBR=ben@chromium.org
BUG=140037
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152544

[Jói, 2012-08-16 11:34:27]

erikwright: Main reviewer.
mnissler:  For chrome/browser/prefs/OWNERS.

I plan to TBR other owners since changes outside of chrome/browser/api,
chrome/browser/autofill and chrome/browser/prefs are purely mechanical to update
include paths.

Mattias, for background:  This is purely a mechanical change to allow Browser
Components to use these two classes; such components will not be allowed to
depend on anything in chrome/browser/ except stuff under chrome/browser/api. 
See http://www.chromium.org/developers/design-documents/browser-components for
details.

Moving these two classes whole-sale is consistent with significant discussions
that have taken place in the last couple of weeks about how to approach the
chrome/browser/api directory.  In summary, simple utility classes whose
dependencies outside of api are easy to break (and I will break their dependency
on PrefService in my very next change) can be moved wholesale to live in the api
directory, whereas classes with significant dependencies on the implementation
of Chrome are handled by extracting a virtual interface and using that in
Browser Components, so that the implementation details are hidden and the
implementation could be replaced on some of our platforms.  This is what I will
do in my next change where I will introduce a PrefServiceBase abstract base
class with just the fundamental API of PrefService, and use that within
Autofill.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2012-08-16 11:49:51]

Sounds reasonable, just two questions:

1. Should we have a chrome/browser/api/prefs/OWNERS file that mirrors
chrome/browser/prefs/OWNERS? I think it's important to have people who know the
code review API changes.

2. Do you have a preliminary version of the CL extracting PrefServiceBase (or
can you share which parts of PrefService you'd like to pull into
PrefServiceBase)?

[tfarina, 2012-08-16 12:03:09]

Instead of PrefServiceBase, can you do the reverse?

PrefService  # the interface with pure virtual methods
PrefServiceImpl # the concrete implementation of PrefService

That is the convention used in content iirc.

On Thursday, August 16, 2012, wrote:

> Sounds reasonable, just two questions:
>
> 1. Should we have a chrome/browser/api/prefs/**OWNERS file that mirrors
> chrome/browser/prefs/OWNERS? I think it's important to have people who
> know the
> code review API changes.
>
> 2. Do you have a preliminary version of the CL extracting PrefServiceBase
> (or
> can you share which parts of PrefService you'd like to pull into
> PrefServiceBase)?
>
>
http://codereview.chromium.**org/10857031/<http://codereview.chromium.org/108...
>


-- 
Thiago

[Jói, 2012-08-16 12:04:53]

Thanks Mattias.

Not adding the OWNERS file was an oversight on my part, this is
exactly the plan to mirror OWNERS files.  We don't have the capability
yet to link files do we?

As a nuance regarding OWNERS files:  The tradition we have had in
feature directories that we are making into Browser Components, and in
the api directory, is that pure refactoring changes that do not change
the API in any meaningful way and do not change behavior at all, go to
reviewers who are working full-time on Browser Components.  This is so
that we can move quickly; many of the changes touch dozens or even
hundreds of files and getting them checked in quickly can mean the
difference between checking them in the same day they are prepared,
vs. multiple days cycling through merge-resolve-try-it-again cycles.
Of course, changes that affect behavior or introduce a new API or
modify existing APIs in significant ways go to OWNERS for the specific
code in question.  Please let me know if you are OK with us continuing
this tradition w.r.t. the prefs code.  I should say though that I
don't expect major changes after these first couple of changes, at
least not until we see further need in the next features we
componentize.

I don't have a preview of the PrefServiceBase change yet, but probably
later today.  At the moment it looks like the definition of the
Preferences class, plus most of the Register/Unregister/Get/Set/Find
methods forprefs would be pure virtual methods in PrefServiceBase,
with the existing implementations in PrefService unmodified except to
make them virtual overrides.

Cheers,
Jói



On Thu, Aug 16, 2012 at 11:49 AM,  <mnissler@chromium.org> wrote:
> Sounds reasonable, just two questions:
>
> 1. Should we have a chrome/browser/api/prefs/OWNERS file that mirrors
> chrome/browser/prefs/OWNERS? I think it's important to have people who know
> the
> code review API changes.
>
> 2. Do you have a preliminary version of the CL extracting PrefServiceBase
> (or
> can you share which parts of PrefService you'd like to pull into
> PrefServiceBase)?
>
> http://codereview.chromium.org/10857031/

[Jói, 2012-08-16 12:11:27]

Hi Thiago,

> PrefService  # the interface with pure virtual methods
> PrefServiceImpl # the concrete implementation of PrefService
>
> That is the convention used in content iirc.

You're right that this is the convention in content, and thanks for
bringing it up because it does bear some discussion.

I don't think we should follow the same convention for
chrome/browser/api, at least not yet.  My rationale is as follows:
a) I believe one of the reasons we wanted that convention in content
was so that code in chrome/ wouldn't need to be updated with new names
all over the place (e.g. if you were using Foo before, then you are
now using content::Foo, which incidentally is a virtual interface),
and so that developers who mostly touch chrome/ could keep using the
same names for the concepts they are used to;
b) A large portion of the chrome/ code will not be changed into
Browser Components, at least not nearly yet, so if we want to achieve
what was desired for content as per (a), we actually shouldn't switch
the names, at least not right away;
c) We want to move as fast as possible getting the biggest easy wins
we can in Browser Components;
d) Switching the names around like that, while fairly mechanical, is
actually a ton of work given the current tools we have, and so is in
conflict with (c);
e) There is no penalty whatsoever in going through all the names in
chrome/browser/api at some later date, and switching them if desired;
this helps us on point (c).  An appropriate time to switch the names,
if ever, might be e.g. once a significant portion of the code under
chrome/browser has been made into Browser Components.

Cheers,
Jói


On Thu, Aug 16, 2012 at 12:03 PM, Thiago Farina <tfarina@chromium.org> wrote:
> Instead of PrefServiceBase, can you do the reverse?
>
> PrefService  # the interface with pure virtual methods
> PrefServiceImpl # the concrete implementation of PrefService
>
> That is the convention used in content iirc.
>
>
> On Thursday, August 16, 2012, wrote:
>>
>> Sounds reasonable, just two questions:
>>
>> 1. Should we have a chrome/browser/api/prefs/OWNERS file that mirrors
>> chrome/browser/prefs/OWNERS? I think it's important to have people who
>> know the
>> code review API changes.
>>
>> 2. Do you have a preliminary version of the CL extracting PrefServiceBase
>> (or
>> can you share which parts of PrefService you'd like to pull into
>> PrefServiceBase)?
>>
>> http://codereview.chromium.org/10857031/
>
>
>
> --
> Thiago

[Jói, 2012-08-16 12:15:14]

Mattias:  I uploaded a new patchset that has the OWNERS file.

On Thu, Aug 16, 2012 at 12:10 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Hi Thiago,
>
>> PrefService  # the interface with pure virtual methods
>> PrefServiceImpl # the concrete implementation of PrefService
>>
>> That is the convention used in content iirc.
>
> You're right that this is the convention in content, and thanks for
> bringing it up because it does bear some discussion.
>
> I don't think we should follow the same convention for
> chrome/browser/api, at least not yet.  My rationale is as follows:
> a) I believe one of the reasons we wanted that convention in content
> was so that code in chrome/ wouldn't need to be updated with new names
> all over the place (e.g. if you were using Foo before, then you are
> now using content::Foo, which incidentally is a virtual interface),
> and so that developers who mostly touch chrome/ could keep using the
> same names for the concepts they are used to;
> b) A large portion of the chrome/ code will not be changed into
> Browser Components, at least not nearly yet, so if we want to achieve
> what was desired for content as per (a), we actually shouldn't switch
> the names, at least not right away;
> c) We want to move as fast as possible getting the biggest easy wins
> we can in Browser Components;
> d) Switching the names around like that, while fairly mechanical, is
> actually a ton of work given the current tools we have, and so is in
> conflict with (c);
> e) There is no penalty whatsoever in going through all the names in
> chrome/browser/api at some later date, and switching them if desired;
> this helps us on point (c).  An appropriate time to switch the names,
> if ever, might be e.g. once a significant portion of the code under
> chrome/browser has been made into Browser Components.
>
> Cheers,
> Jói
>
>
> On Thu, Aug 16, 2012 at 12:03 PM, Thiago Farina <tfarina@chromium.org> wrote:
>> Instead of PrefServiceBase, can you do the reverse?
>>
>> PrefService  # the interface with pure virtual methods
>> PrefServiceImpl # the concrete implementation of PrefService
>>
>> That is the convention used in content iirc.
>>
>>
>> On Thursday, August 16, 2012, wrote:
>>>
>>> Sounds reasonable, just two questions:
>>>
>>> 1. Should we have a chrome/browser/api/prefs/OWNERS file that mirrors
>>> chrome/browser/prefs/OWNERS? I think it's important to have people who
>>> know the
>>> code review API changes.
>>>
>>> 2. Do you have a preliminary version of the CL extracting PrefServiceBase
>>> (or
>>> can you share which parts of PrefService you'd like to pull into
>>> PrefServiceBase)?
>>>
>>> http://codereview.chromium.org/10857031/
>>
>>
>>
>> --
>> Thiago

[Jói, 2012-08-16 12:27:16]

Mattias:  Looking more, it looks like I can probably leave out
FindPreference and thus make the Preference inner class an
implementation detail of PrefService and not expose it in
PrefServiceBase.

On Thu, Aug 16, 2012 at 12:14 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Mattias:  I uploaded a new patchset that has the OWNERS file.
>
> On Thu, Aug 16, 2012 at 12:10 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> Hi Thiago,
>>
>>> PrefService  # the interface with pure virtual methods
>>> PrefServiceImpl # the concrete implementation of PrefService
>>>
>>> That is the convention used in content iirc.
>>
>> You're right that this is the convention in content, and thanks for
>> bringing it up because it does bear some discussion.
>>
>> I don't think we should follow the same convention for
>> chrome/browser/api, at least not yet.  My rationale is as follows:
>> a) I believe one of the reasons we wanted that convention in content
>> was so that code in chrome/ wouldn't need to be updated with new names
>> all over the place (e.g. if you were using Foo before, then you are
>> now using content::Foo, which incidentally is a virtual interface),
>> and so that developers who mostly touch chrome/ could keep using the
>> same names for the concepts they are used to;
>> b) A large portion of the chrome/ code will not be changed into
>> Browser Components, at least not nearly yet, so if we want to achieve
>> what was desired for content as per (a), we actually shouldn't switch
>> the names, at least not right away;
>> c) We want to move as fast as possible getting the biggest easy wins
>> we can in Browser Components;
>> d) Switching the names around like that, while fairly mechanical, is
>> actually a ton of work given the current tools we have, and so is in
>> conflict with (c);
>> e) There is no penalty whatsoever in going through all the names in
>> chrome/browser/api at some later date, and switching them if desired;
>> this helps us on point (c).  An appropriate time to switch the names,
>> if ever, might be e.g. once a significant portion of the code under
>> chrome/browser has been made into Browser Components.
>>
>> Cheers,
>> Jói
>>
>>
>> On Thu, Aug 16, 2012 at 12:03 PM, Thiago Farina <tfarina@chromium.org>
wrote:
>>> Instead of PrefServiceBase, can you do the reverse?
>>>
>>> PrefService  # the interface with pure virtual methods
>>> PrefServiceImpl # the concrete implementation of PrefService
>>>
>>> That is the convention used in content iirc.
>>>
>>>
>>> On Thursday, August 16, 2012, wrote:
>>>>
>>>> Sounds reasonable, just two questions:
>>>>
>>>> 1. Should we have a chrome/browser/api/prefs/OWNERS file that mirrors
>>>> chrome/browser/prefs/OWNERS? I think it's important to have people who
>>>> know the
>>>> code review API changes.
>>>>
>>>> 2. Do you have a preliminary version of the CL extracting PrefServiceBase
>>>> (or
>>>> can you share which parts of PrefService you'd like to pull into
>>>> PrefServiceBase)?
>>>>
>>>> http://codereview.chromium.org/10857031/
>>>
>>>
>>>
>>> --
>>> Thiago

[Mattias Nissler (ping if slow), 2012-08-16 12:42:50]

On 2012/08/16 12:04:53, Jói wrote:
> Thanks Mattias.
> 
> Not adding the OWNERS file was an oversight on my part, this is
> exactly the plan to mirror OWNERS files.  We don't have the capability
> yet to link files do we?

I don't think so, copying is fine.

> 
> As a nuance regarding OWNERS files:  The tradition we have had in
> feature directories that we are making into Browser Components, and in
> the api directory, is that pure refactoring changes that do not change
> the API in any meaningful way and do not change behavior at all, go to
> reviewers who are working full-time on Browser Components.  This is so
> that we can move quickly; many of the changes touch dozens or even
> hundreds of files and getting them checked in quickly can mean the
> difference between checking them in the same day they are prepared,
> vs. multiple days cycling through merge-resolve-try-it-again cycles.
> Of course, changes that affect behavior or introduce a new API or
> modify existing APIs in significant ways go to OWNERS for the specific
> code in question.  Please let me know if you are OK with us continuing
> this tradition w.r.t. the prefs code.  I should say though that I
> don't expect major changes after these first couple of changes, at
> least not until we see further need in the next features we
> componentize.

I'm fine with that, just TBR one of us so we can follow what you're doing.

> 
> I don't have a preview of the PrefServiceBase change yet, but probably
> later today.  At the moment it looks like the definition of the
> Preferences class, plus most of the Register/Unregister/Get/Set/Find
> methods forprefs would be pure virtual methods in PrefServiceBase,
> with the existing implementations in PrefService unmodified except to
> make them virtual overrides.

Aha, there's an additional caveat here regarding Register/Unregister: There are
other plans for splitting up PrefService, namely factoring out all the
sync-specific bits into a subclass as the basic PrefService implementation
shouldn't have a sync dependency. One unfortunate detail of the sync stuff in
the current PrefService implementation is the RegisterXYZPref() calls which
takes an argument that says whether to sync or not. That parameter doesn't make
sense for the g_browser_process->local_state() PrefService instance, so the
Register calls would probably have to be different for the basic PrefService and
the subclass that supports syncing.

Long story short: There are plans to use the subtype axis for another
abstraction within PrefService that might collide with your plans. One way to
solve this could be to have a PrefServiceAPI that has a basic implementation in
PrefService and then a second SyncablePrefServiceAPI that has an implementation
in SyncablePrefService, where SyncablePrefService would ideally derive from
PrefService. The API versions would have to be virtual interfaces then.

I guess you have the situation of browser API exposing class hierarchies
elsewhere, how do you handle that in other cases?

> 
> Cheers,
> Jói
> 
> 
> 
> On Thu, Aug 16, 2012 at 11:49 AM,  <mailto:mnissler@chromium.org> wrote:
> > Sounds reasonable, just two questions:
> >
> > 1. Should we have a chrome/browser/api/prefs/OWNERS file that mirrors
> > chrome/browser/prefs/OWNERS? I think it's important to have people who know
> > the
> > code review API changes.
> >
> > 2. Do you have a preliminary version of the CL extracting PrefServiceBase
> > (or
> > can you share which parts of PrefService you'd like to pull into
> > PrefServiceBase)?
> >
> > <crxref style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);
">http://codereview.chromium.org/10857031/</crxref>

[Jói, 2012-08-16 12:55:03]

Cool, thanks Mattias.  I think I can easily split up the syncable vs.
non-syncable in the API, and then splitting up the implementations can
be done later.  In terms of the class hierarchies, from a Browser
Component's point of view there will probably just be a static
accessor method like GetInstance(Profile*) that will get the relevant
service, so the class hierarchies might not make life too complicated
from that standpoint, but that's something that can be dealt with once
the details of the concrete implementation are known.  I'll make a
stab at splitting up the API in the change I'm preparing.

As a note on what the concrete split might look like down the road, we
have a couple of parallel interface/concrete class class hierarchies
in content, like RenderView which inherits from RenderWidget, and
RenderViewImpl which inherits from RenderWidgetImpl and from the
RenderView interface (and RenderWidgetImpl which inherits from
RenderWidget, but that case is simple).  This kind of parallel
inheritance is slightly messy because you end up inheriting the top
interface two different ways in the bottom class, so you either need
to go with composition instead, or use virtual inheritance.  Virtual
inheritance seems to work out OK, as long as there is only one
concrete class being inherited.

Cheers,
Jói


On Thu, Aug 16, 2012 at 12:42 PM,  <mnissler@chromium.org> wrote:
> On 2012/08/16 12:04:53, Jói wrote:
>>
>> Thanks Mattias.
>
>
>> Not adding the OWNERS file was an oversight on my part, this is
>> exactly the plan to mirror OWNERS files.  We don't have the capability
>> yet to link files do we?
>
>
> I don't think so, copying is fine.
>
>
>
>> As a nuance regarding OWNERS files:  The tradition we have had in
>> feature directories that we are making into Browser Components, and in
>> the api directory, is that pure refactoring changes that do not change
>> the API in any meaningful way and do not change behavior at all, go to
>> reviewers who are working full-time on Browser Components.  This is so
>> that we can move quickly; many of the changes touch dozens or even
>> hundreds of files and getting them checked in quickly can mean the
>> difference between checking them in the same day they are prepared,
>> vs. multiple days cycling through merge-resolve-try-it-again cycles.
>> Of course, changes that affect behavior or introduce a new API or
>> modify existing APIs in significant ways go to OWNERS for the specific
>> code in question.  Please let me know if you are OK with us continuing
>> this tradition w.r.t. the prefs code.  I should say though that I
>> don't expect major changes after these first couple of changes, at
>> least not until we see further need in the next features we
>> componentize.
>
>
> I'm fine with that, just TBR one of us so we can follow what you're doing.
>
>
>
>> I don't have a preview of the PrefServiceBase change yet, but probably
>> later today.  At the moment it looks like the definition of the
>> Preferences class, plus most of the Register/Unregister/Get/Set/Find
>> methods forprefs would be pure virtual methods in PrefServiceBase,
>> with the existing implementations in PrefService unmodified except to
>> make them virtual overrides.
>
>
> Aha, there's an additional caveat here regarding Register/Unregister: There
> are
> other plans for splitting up PrefService, namely factoring out all the
> sync-specific bits into a subclass as the basic PrefService implementation
> shouldn't have a sync dependency. One unfortunate detail of the sync stuff
> in
> the current PrefService implementation is the RegisterXYZPref() calls which
> takes an argument that says whether to sync or not. That parameter doesn't
> make
> sense for the g_browser_process->local_state() PrefService instance, so the
> Register calls would probably have to be different for the basic PrefService
> and
> the subclass that supports syncing.
>
> Long story short: There are plans to use the subtype axis for another
> abstraction within PrefService that might collide with your plans. One way
> to
> solve this could be to have a PrefServiceAPI that has a basic implementation
> in
> PrefService and then a second SyncablePrefServiceAPI that has an
> implementation
> in SyncablePrefService, where SyncablePrefService would ideally derive from
> PrefService. The API versions would have to be virtual interfaces then.
>
> I guess you have the situation of browser API exposing class hierarchies
> elsewhere, how do you handle that in other cases?
>
>
>> Cheers,
>> Jói
>
>
>
>
>> On Thu, Aug 16, 2012 at 11:49 AM,  <mailto:mnissler@chromium.org> wrote:
>> > Sounds reasonable, just two questions:
>> >
>> > 1. Should we have a chrome/browser/api/prefs/OWNERS file that mirrors
>> > chrome/browser/prefs/OWNERS? I think it's important to have people who
>> > know
>> > the
>> > code review API changes.
>> >
>> > 2. Do you have a preliminary version of the CL extracting
>> > PrefServiceBase
>> > (or
>> > can you share which parts of PrefService you'd like to pull into
>> > PrefServiceBase)?
>> >
>> > <crxref style="background-image:
>
>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);
> ">http://codereview.chromium.org/10857031/</crxref>
>
>
>
> http://codereview.chromium.org/10857031/

[Mattias Nissler (ping if slow), 2012-08-16 12:58:21]

LGTM for chrome/browser/prefs and chrome/browser/policy

On 2012/08/16 12:55:03, Jói wrote:
> Cool, thanks Mattias.  I think I can easily split up the syncable vs.
> non-syncable in the API, and then splitting up the implementations can
> be done later.  In terms of the class hierarchies, from a Browser
> Component's point of view there will probably just be a static
> accessor method like GetInstance(Profile*) that will get the relevant
> service, so the class hierarchies might not make life too complicated
> from that standpoint, but that's something that can be dealt with once
> the details of the concrete implementation are known.  I'll make a
> stab at splitting up the API in the change I'm preparing.

Thanks. I meant to refer to the bug but forgot:
http://code.google.com/p/chromium/issues/detail?id=85906

> 
> As a note on what the concrete split might look like down the road, we
> have a couple of parallel interface/concrete class class hierarchies
> in content, like RenderView which inherits from RenderWidget, and
> RenderViewImpl which inherits from RenderWidgetImpl and from the
> RenderView interface (and RenderWidgetImpl which inherits from
> RenderWidget, but that case is simple).  This kind of parallel
> inheritance is slightly messy because you end up inheriting the top
> interface two different ways in the bottom class, so you either need
> to go with composition instead, or use virtual inheritance.  Virtual
> inheritance seems to work out OK, as long as there is only one
> concrete class being inherited.

Ah, virtual inheritance... Do the style guide and the Chrome elders approve?

> 
> Cheers,
> Jói
> 
> 
> On Thu, Aug 16, 2012 at 12:42 PM,  <mailto:mnissler@chromium.org> wrote:
> > On 2012/08/16 12:04:53, Jói wrote:
> >>
> >> Thanks Mattias.
> >
> >
> >> Not adding the OWNERS file was an oversight on my part, this is
> >> exactly the plan to mirror OWNERS files.  We don't have the capability
> >> yet to link files do we?
> >
> >
> > I don't think so, copying is fine.
> >
> >
> >
> >> As a nuance regarding OWNERS files:  The tradition we have had in
> >> feature directories that we are making into Browser Components, and in
> >> the api directory, is that pure refactoring changes that do not change
> >> the API in any meaningful way and do not change behavior at all, go to
> >> reviewers who are working full-time on Browser Components.  This is so
> >> that we can move quickly; many of the changes touch dozens or even
> >> hundreds of files and getting them checked in quickly can mean the
> >> difference between checking them in the same day they are prepared,
> >> vs. multiple days cycling through merge-resolve-try-it-again cycles.
> >> Of course, changes that affect behavior or introduce a new API or
> >> modify existing APIs in significant ways go to OWNERS for the specific
> >> code in question.  Please let me know if you are OK with us continuing
> >> this tradition w.r.t. the prefs code.  I should say though that I
> >> don't expect major changes after these first couple of changes, at
> >> least not until we see further need in the next features we
> >> componentize.
> >
> >
> > I'm fine with that, just TBR one of us so we can follow what you're doing.
> >
> >
> >
> >> I don't have a preview of the PrefServiceBase change yet, but probably
> >> later today.  At the moment it looks like the definition of the
> >> Preferences class, plus most of the Register/Unregister/Get/Set/Find
> >> methods forprefs would be pure virtual methods in PrefServiceBase,
> >> with the existing implementations in PrefService unmodified except to
> >> make them virtual overrides.
> >
> >
> > Aha, there's an additional caveat here regarding Register/Unregister: There
> > are
> > other plans for splitting up PrefService, namely factoring out all the
> > sync-specific bits into a subclass as the basic PrefService implementation
> > shouldn't have a sync dependency. One unfortunate detail of the sync stuff
> > in
> > the current PrefService implementation is the RegisterXYZPref() calls which
> > takes an argument that says whether to sync or not. That parameter doesn't
> > make
> > sense for the g_browser_process->local_state() PrefService instance, so the
> > Register calls would probably have to be different for the basic PrefService
> > and
> > the subclass that supports syncing.
> >
> > Long story short: There are plans to use the subtype axis for another
> > abstraction within PrefService that might collide with your plans. One way
> > to
> > solve this could be to have a PrefServiceAPI that has a basic implementation
> > in
> > PrefService and then a second SyncablePrefServiceAPI that has an
> > implementation
> > in SyncablePrefService, where SyncablePrefService would ideally derive from
> > PrefService. The API versions would have to be virtual interfaces then.
> >
> > I guess you have the situation of browser API exposing class hierarchies
> > elsewhere, how do you handle that in other cases?
> >
> >
> >> Cheers,
> >> Jói
> >
> >
> >
> >
> >> On Thu, Aug 16, 2012 at 11:49 AM,  <mailto:mnissler@chromium.org> wrote:
> >> > Sounds reasonable, just two questions:
> >> >
> >> > 1. Should we have a chrome/browser/api/prefs/OWNERS file that mirrors
> >> > chrome/browser/prefs/OWNERS? I think it's important to have people who
> >> > know
> >> > the
> >> > code review API changes.
> >> >
> >> > 2. Do you have a preliminary version of the CL extracting
> >> > PrefServiceBase
> >> > (or
> >> > can you share which parts of PrefService you'd like to pull into
> >> > PrefServiceBase)?
> >> >
> >> > <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/10857031/</crxref></crxref>
> >
> >
> >
> > <crxref style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);
">http://codereview.chromium.org/10857031/</crxref>

[Jói, 2012-08-16 13:01:34]

> Ah, virtual inheritance... Do the style guide and the Chrome elders approve?

On the latter question, they have but only if there is just a single
concrete class in the multiple inheritance layout.  On the former, I
believe so; if not it should probably be updated since that is the
consensus and the convention being followed.

Cheers,
Jói


On Thu, Aug 16, 2012 at 12:58 PM,  <mnissler@chromium.org> wrote:
> LGTM for chrome/browser/prefs and chrome/browser/policy
>
>
> On 2012/08/16 12:55:03, Jói wrote:
>>
>> Cool, thanks Mattias.  I think I can easily split up the syncable vs.
>> non-syncable in the API, and then splitting up the implementations can
>> be done later.  In terms of the class hierarchies, from a Browser
>> Component's point of view there will probably just be a static
>> accessor method like GetInstance(Profile*) that will get the relevant
>> service, so the class hierarchies might not make life too complicated
>> from that standpoint, but that's something that can be dealt with once
>> the details of the concrete implementation are known.  I'll make a
>> stab at splitting up the API in the change I'm preparing.
>
>
> Thanks. I meant to refer to the bug but forgot:
> http://code.google.com/p/chromium/issues/detail?id=85906
>
>
>
>> As a note on what the concrete split might look like down the road, we
>> have a couple of parallel interface/concrete class class hierarchies
>> in content, like RenderView which inherits from RenderWidget, and
>> RenderViewImpl which inherits from RenderWidgetImpl and from the
>> RenderView interface (and RenderWidgetImpl which inherits from
>> RenderWidget, but that case is simple).  This kind of parallel
>> inheritance is slightly messy because you end up inheriting the top
>> interface two different ways in the bottom class, so you either need
>> to go with composition instead, or use virtual inheritance.  Virtual
>> inheritance seems to work out OK, as long as there is only one
>> concrete class being inherited.
>
>
> Ah, virtual inheritance... Do the style guide and the Chrome elders approve?
>
>
>> Cheers,
>> Jói
>
>
>
>> On Thu, Aug 16, 2012 at 12:42 PM,  <mailto:mnissler@chromium.org> wrote:
>> > On 2012/08/16 12:04:53, Jói wrote:
>> >>
>> >> Thanks Mattias.
>> >
>> >
>> >> Not adding the OWNERS file was an oversight on my part, this is
>> >> exactly the plan to mirror OWNERS files.  We don't have the capability
>> >> yet to link files do we?
>> >
>> >
>> > I don't think so, copying is fine.
>> >
>> >
>> >
>> >> As a nuance regarding OWNERS files:  The tradition we have had in
>> >> feature directories that we are making into Browser Components, and in
>> >> the api directory, is that pure refactoring changes that do not change
>> >> the API in any meaningful way and do not change behavior at all, go to
>> >> reviewers who are working full-time on Browser Components.  This is so
>> >> that we can move quickly; many of the changes touch dozens or even
>> >> hundreds of files and getting them checked in quickly can mean the
>> >> difference between checking them in the same day they are prepared,
>> >> vs. multiple days cycling through merge-resolve-try-it-again cycles.
>> >> Of course, changes that affect behavior or introduce a new API or
>> >> modify existing APIs in significant ways go to OWNERS for the specific
>> >> code in question.  Please let me know if you are OK with us continuing
>> >> this tradition w.r.t. the prefs code.  I should say though that I
>> >> don't expect major changes after these first couple of changes, at
>> >> least not until we see further need in the next features we
>> >> componentize.
>> >
>> >
>> > I'm fine with that, just TBR one of us so we can follow what you're
>> > doing.
>> >
>> >
>> >
>> >> I don't have a preview of the PrefServiceBase change yet, but probably
>> >> later today.  At the moment it looks like the definition of the
>> >> Preferences class, plus most of the Register/Unregister/Get/Set/Find
>> >> methods forprefs would be pure virtual methods in PrefServiceBase,
>> >> with the existing implementations in PrefService unmodified except to
>> >> make them virtual overrides.
>> >
>> >
>> > Aha, there's an additional caveat here regarding Register/Unregister:
>> > There
>> > are
>> > other plans for splitting up PrefService, namely factoring out all the
>> > sync-specific bits into a subclass as the basic PrefService
>> > implementation
>> > shouldn't have a sync dependency. One unfortunate detail of the sync
>> > stuff
>> > in
>> > the current PrefService implementation is the RegisterXYZPref() calls
>> > which
>> > takes an argument that says whether to sync or not. That parameter
>> > doesn't
>> > make
>> > sense for the g_browser_process->local_state() PrefService instance, so
>> > the
>> > Register calls would probably have to be different for the basic
>> > PrefService
>> > and
>> > the subclass that supports syncing.
>> >
>> > Long story short: There are plans to use the subtype axis for another
>> > abstraction within PrefService that might collide with your plans. One
>> > way
>> > to
>> > solve this could be to have a PrefServiceAPI that has a basic
>> > implementation
>> > in
>> > PrefService and then a second SyncablePrefServiceAPI that has an
>> > implementation
>> > in SyncablePrefService, where SyncablePrefService would ideally derive
>> > from
>> > PrefService. The API versions would have to be virtual interfaces then.
>> >
>> > I guess you have the situation of browser API exposing class hierarchies
>> > elsewhere, how do you handle that in other cases?
>> >
>> >
>> >> Cheers,
>> >> Jói
>> >
>> >
>> >
>> >
>> >> On Thu, Aug 16, 2012 at 11:49 AM,  <mailto:mnissler@chromium.org>
>> >> wrote:
>> >> > Sounds reasonable, just two questions:
>> >> >
>> >> > 1. Should we have a chrome/browser/api/prefs/OWNERS file that mirrors
>> >> > chrome/browser/prefs/OWNERS? I think it's important to have people
>> >> > who
>> >> > know
>> >> > the
>> >> > code review API changes.
>> >> >
>> >> > 2. Do you have a preliminary version of the CL extracting
>> >> > PrefServiceBase
>> >> > (or
>> >> > can you share which parts of PrefService you'd like to pull into
>> >> > PrefServiceBase)?
>> >> >
>> >> > <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/10857031/</crxref></crxref>
>
>> >
>> >
>> >
>> > <crxref style="background-image:
>
>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);
> ">http://codereview.chromium.org/10857031/</crxref>
>
>
> http://codereview.chromium.org/10857031/

[Jói, 2012-08-16 13:19:05]

Taking a closer look at PrefService, it seems the interface should in
fact be split up something like this (the naming is provisional),
which is a bit more complicated than what you described and what I
initially started doing:

PrefAccess (everything I originally talked about putting into
PrefServiceBase, except none of the Register... methods).
LocalPrefRegistration (all the Register... methods that don't take a
PrefSyncStatus)
SyncablePrefRegistration (all the Register... methods that take a
PrefSyncStatus)

The three interfaces could be independent (no inheritance between
them) but the current PrefService would inherit from all three.  The
future PrefService implementations after a split-up would inherit from
PrefAccess plus one of the two registration interfaces.

An alternate way to go would be to have each of the registration
interfaces inherit from PrefAccess (then you might call them
PrefServiceBase and SyncablePrefServiceBase) and have the concrete
implementations inherit from one or the other.

I think the split has to be this way rather than as just a base
PrefServiceAPI (with access methods and the non-syncable registration
methods) and a SyncablePrefServiceAPI that inherits from that, since
otherwise you end up with a weird situation in SyncablePrefServiceAPI
where it also has the not-intended-for-sync registration methods.

At this point, since this probably requires a fair bit of discussion
and I'd like to break the concrete dependency ASAP, my suggestion is
to to defer splitting up the interface until you guys have a chance to
talk about how you'd like to split things (both at the interface level
and implementation level).  This means for now I would just extract a
PrefServiceBase that does not deal with the split between syncable and
non-syncable prefs.  I'm going to upload a version of this in a little
bit, so we can continue the discussion on the review thread for that.

Cheers,
Jói



On Thu, Aug 16, 2012 at 1:00 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> Ah, virtual inheritance... Do the style guide and the Chrome elders approve?
>
> On the latter question, they have but only if there is just a single
> concrete class in the multiple inheritance layout.  On the former, I
> believe so; if not it should probably be updated since that is the
> consensus and the convention being followed.
>
> Cheers,
> Jói
>
>
> On Thu, Aug 16, 2012 at 12:58 PM,  <mnissler@chromium.org> wrote:
>> LGTM for chrome/browser/prefs and chrome/browser/policy
>>
>>
>> On 2012/08/16 12:55:03, Jói wrote:
>>>
>>> Cool, thanks Mattias.  I think I can easily split up the syncable vs.
>>> non-syncable in the API, and then splitting up the implementations can
>>> be done later.  In terms of the class hierarchies, from a Browser
>>> Component's point of view there will probably just be a static
>>> accessor method like GetInstance(Profile*) that will get the relevant
>>> service, so the class hierarchies might not make life too complicated
>>> from that standpoint, but that's something that can be dealt with once
>>> the details of the concrete implementation are known.  I'll make a
>>> stab at splitting up the API in the change I'm preparing.
>>
>>
>> Thanks. I meant to refer to the bug but forgot:
>> http://code.google.com/p/chromium/issues/detail?id=85906
>>
>>
>>
>>> As a note on what the concrete split might look like down the road, we
>>> have a couple of parallel interface/concrete class class hierarchies
>>> in content, like RenderView which inherits from RenderWidget, and
>>> RenderViewImpl which inherits from RenderWidgetImpl and from the
>>> RenderView interface (and RenderWidgetImpl which inherits from
>>> RenderWidget, but that case is simple).  This kind of parallel
>>> inheritance is slightly messy because you end up inheriting the top
>>> interface two different ways in the bottom class, so you either need
>>> to go with composition instead, or use virtual inheritance.  Virtual
>>> inheritance seems to work out OK, as long as there is only one
>>> concrete class being inherited.
>>
>>
>> Ah, virtual inheritance... Do the style guide and the Chrome elders approve?
>>
>>
>>> Cheers,
>>> Jói
>>
>>
>>
>>> On Thu, Aug 16, 2012 at 12:42 PM,  <mailto:mnissler@chromium.org> wrote:
>>> > On 2012/08/16 12:04:53, Jói wrote:
>>> >>
>>> >> Thanks Mattias.
>>> >
>>> >
>>> >> Not adding the OWNERS file was an oversight on my part, this is
>>> >> exactly the plan to mirror OWNERS files.  We don't have the capability
>>> >> yet to link files do we?
>>> >
>>> >
>>> > I don't think so, copying is fine.
>>> >
>>> >
>>> >
>>> >> As a nuance regarding OWNERS files:  The tradition we have had in
>>> >> feature directories that we are making into Browser Components, and in
>>> >> the api directory, is that pure refactoring changes that do not change
>>> >> the API in any meaningful way and do not change behavior at all, go to
>>> >> reviewers who are working full-time on Browser Components.  This is so
>>> >> that we can move quickly; many of the changes touch dozens or even
>>> >> hundreds of files and getting them checked in quickly can mean the
>>> >> difference between checking them in the same day they are prepared,
>>> >> vs. multiple days cycling through merge-resolve-try-it-again cycles.
>>> >> Of course, changes that affect behavior or introduce a new API or
>>> >> modify existing APIs in significant ways go to OWNERS for the specific
>>> >> code in question.  Please let me know if you are OK with us continuing
>>> >> this tradition w.r.t. the prefs code.  I should say though that I
>>> >> don't expect major changes after these first couple of changes, at
>>> >> least not until we see further need in the next features we
>>> >> componentize.
>>> >
>>> >
>>> > I'm fine with that, just TBR one of us so we can follow what you're
>>> > doing.
>>> >
>>> >
>>> >
>>> >> I don't have a preview of the PrefServiceBase change yet, but probably
>>> >> later today.  At the moment it looks like the definition of the
>>> >> Preferences class, plus most of the Register/Unregister/Get/Set/Find
>>> >> methods forprefs would be pure virtual methods in PrefServiceBase,
>>> >> with the existing implementations in PrefService unmodified except to
>>> >> make them virtual overrides.
>>> >
>>> >
>>> > Aha, there's an additional caveat here regarding Register/Unregister:
>>> > There
>>> > are
>>> > other plans for splitting up PrefService, namely factoring out all the
>>> > sync-specific bits into a subclass as the basic PrefService
>>> > implementation
>>> > shouldn't have a sync dependency. One unfortunate detail of the sync
>>> > stuff
>>> > in
>>> > the current PrefService implementation is the RegisterXYZPref() calls
>>> > which
>>> > takes an argument that says whether to sync or not. That parameter
>>> > doesn't
>>> > make
>>> > sense for the g_browser_process->local_state() PrefService instance, so
>>> > the
>>> > Register calls would probably have to be different for the basic
>>> > PrefService
>>> > and
>>> > the subclass that supports syncing.
>>> >
>>> > Long story short: There are plans to use the subtype axis for another
>>> > abstraction within PrefService that might collide with your plans. One
>>> > way
>>> > to
>>> > solve this could be to have a PrefServiceAPI that has a basic
>>> > implementation
>>> > in
>>> > PrefService and then a second SyncablePrefServiceAPI that has an
>>> > implementation
>>> > in SyncablePrefService, where SyncablePrefService would ideally derive
>>> > from
>>> > PrefService. The API versions would have to be virtual interfaces then.
>>> >
>>> > I guess you have the situation of browser API exposing class hierarchies
>>> > elsewhere, how do you handle that in other cases?
>>> >
>>> >
>>> >> Cheers,
>>> >> Jói
>>> >
>>> >
>>> >
>>> >
>>> >> On Thu, Aug 16, 2012 at 11:49 AM,  <mailto:mnissler@chromium.org>
>>> >> wrote:
>>> >> > Sounds reasonable, just two questions:
>>> >> >
>>> >> > 1. Should we have a chrome/browser/api/prefs/OWNERS file that mirrors
>>> >> > chrome/browser/prefs/OWNERS? I think it's important to have people
>>> >> > who
>>> >> > know
>>> >> > the
>>> >> > code review API changes.
>>> >> >
>>> >> > 2. Do you have a preliminary version of the CL extracting
>>> >> > PrefServiceBase
>>> >> > (or
>>> >> > can you share which parts of PrefService you'd like to pull into
>>> >> > PrefServiceBase)?
>>> >> >
>>> >> > <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/10857031/</crxref></crxref>
>>
>>> >
>>> >
>>> >
>>> > <crxref style="background-image:
>>
>>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);
>> ">http://codereview.chromium.org/10857031/</crxref>
>>
>>
>> http://codereview.chromium.org/10857031/

[erikwright (departed), 2012-08-16 14:54:44]

LGTM.

[Ben Goodger (Google), 2012-09-26 15:36:55]

The solution here is for prefs to be a toplevel component. not a browser
component. I thought I already discussed this you? I did disucss with prefs
owners several months ago and they were in agreement. The reason is we need to
use prefs from elsewhere outside of src/chrome too.

This seems to make things a bit more complicated.

Can you re-do this as a toplevel component?

[tfarina, 2012-09-26 15:39:23]

On Wed, Sep 26, 2012 at 12:36 PM,  <ben@chromium.org> wrote:
> The solution here is for prefs to be a toplevel component. not a browser
> component. I thought I already discussed this you? I did disucss with prefs
> owners several months ago and they were in agreement. The reason is we need
> to
> use prefs from elsewhere outside of src/chrome too.
>
> This seems to make things a bit more complicated.
>
> Can you re-do this as a toplevel component?
>
There is even a bug filed for this task.

crbug.com/125840

-- 
Thiago

[Jói, 2012-09-26 15:44:20]

I'll split this to a separate thread since the change here is already
committed.

Cheers,
Jói


On Wed, Sep 26, 2012 at 3:36 PM,  <ben@chromium.org> wrote:
> The solution here is for prefs to be a toplevel component. not a browser
> component. I thought I already discussed this you? I did disucss with prefs
> owners several months ago and they were in agreement. The reason is we need
> to
> use prefs from elsewhere outside of src/chrome too.
>
> This seems to make things a bit more complicated.
>
> Can you re-do this as a toplevel component?
>
> http://codereview.chromium.org/10857031/