Introduce //components/user_prefs, use to eliminate c/b/prefs dependency in Autofill.

Introduce //components/user_prefs.

The user_prefs component provides:

a) The UserPrefs class, used to map PrefService objects to BrowserContext. This addresses a TODO to get rid of PrefServiceFromBrowserContext from base/prefs/pref_service.h, where clearly a mention of a content class did not belong.

b) A place for PrefRegistrySyncable to live, where it can be used by components that need to register prefs.

We also use (b) in this change to eliminate Autofill's dependency on chrome/browser/prefs. Work is ongoing to move Autofill to //components/autofill.

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

[Jói, 2013-02-27 15:05:57]

Hi Mattias,

Outside of the A and A+ files, base/prefs/pref_service.(h|cc) and DEPS files,
there are two different trivial changes:
a) Update include path of pref_registry_simple.h
b) Switch PrefServiceFromBrowserContext(context) to
components::UserPrefs::GetInstance()->Lookup(context)

Cheers,
Jói

[tfarina, 2013-02-27 15:08:01]

Wow, this is cool Joi!

Do you mind adding a README file to //components/user_prefs describing what it
is, what is for, etc? Just an overview for people not familiar with it.

[Jói, 2013-02-27 15:11:53]

> Do you mind adding a README file to //components/user_prefs describing what
> it
> is, what is for, etc? Just an overview for people not familiar with it.

Done.

On Wed, Feb 27, 2013 at 3:08 PM,  <tfarina@chromium.org> wrote:
> Wow, this is cool Joi!
>
> Do you mind adding a README file to //components/user_prefs describing what
> it
> is, what is for, etc? Just an overview for people not familiar with it.
>
> https://codereview.chromium.org/12340111/

[tfarina, 2013-02-27 15:17:48]

https://codereview.chromium.org/12340111/diff/1/components/user_prefs.gypi
File components/user_prefs.gypi (right):

https://codereview.chromium.org/12340111/diff/1/components/user_prefs.gypi#ne...
components/user_prefs.gypi:1: # Copyright (c) 2013 The Chromium Authors. All
rights reserved.
no (c) ?

https://codereview.chromium.org/12340111/diff/1/components/user_prefs.gypi#ne...
components/user_prefs.gypi:13: '../base/base.gyp:base_prefs',
duplicated

https://codereview.chromium.org/12340111/diff/1/components/user_prefs/user_pr...
File components/user_prefs/user_prefs.h (right):

https://codereview.chromium.org/12340111/diff/1/components/user_prefs/user_pr...
components/user_prefs/user_prefs.h:10: #include
"components/user_prefs/user_prefs_delegate.h"
does scoped_ptr allow you to forward declare this?

https://codereview.chromium.org/12340111/diff/1/components/user_prefs/user_pr...
components/user_prefs/user_prefs.h:18: };
no ; here

https://codereview.chromium.org/12340111/diff/1/components/user_prefs/user_pr...
components/user_prefs/user_prefs.h:31: ~UserPrefs() {}
can you avoid inlining?

https://codereview.chromium.org/12340111/diff/1/components/user_prefs/user_pr...
components/user_prefs/user_prefs.h:43: UserPrefs() {}
can you avoid inlining here?

https://codereview.chromium.org/12340111/diff/1/components/user_prefs/user_pr...
components/user_prefs/user_prefs.h:44: friend struct
DefaultSingletonTraits<UserPrefs>;
can you put this first? right after private: ?

https://codereview.chromium.org/12340111/diff/1/components/user_prefs/user_pr...
File components/user_prefs/user_prefs_delegate.h (right):

https://codereview.chromium.org/12340111/diff/1/components/user_prefs/user_pr...
components/user_prefs/user_prefs_delegate.h:12: };
no need of ; here

https://codereview.chromium.org/12340111/diff/1/components/user_prefs/user_pr...
components/user_prefs/user_prefs_delegate.h:18: virtual ~UserPrefsDelegate() {}
this can't be protected, right?

[Mattias Nissler (ping if slow), 2013-02-27 15:29:27]

Before I do full pass through this and and stamp it, I'd like to understand
better what the plan is for PrefRegistrySyncable. How would a component access
it, and how would we make sure that happens at the right time (i.e. before
creating PrefService)?

https://codereview.chromium.org/12340111/diff/1/components/user_prefs/OWNERS
File components/user_prefs/OWNERS (right):

https://codereview.chromium.org/12340111/diff/1/components/user_prefs/OWNERS#...
components/user_prefs/OWNERS:4: pam@chromium.org
Feel free to add your name here, you know this stuff well enough meanwhile :)

[Jói, 2013-02-27 15:34:13]

> Before I do full pass through this and and stamp it, I'd like to understand
> better what the plan is for PrefRegistrySyncable. How would a component
> access
> it, and how would we make sure that happens at the right time (i.e. before
> creating PrefService)?

Components that need it would #include the class directly and
implement a ComponentName::RegisterUserPrefs function.  It would be
the embedder's responsibility to call each of these functions to
register prefs, just as is done today within chrome/browser.

> Feel free to add your name here, you know this stuff well enough
> meanwhile :)

Thanks, I'm already in components/OWNERS so no need to list me again.

Cheers,
Jói



On Wed, Feb 27, 2013 at 3:29 PM,  <mnissler@chromium.org> wrote:
> Before I do full pass through this and and stamp it, I'd like to understand
> better what the plan is for PrefRegistrySyncable. How would a component
> access
> it, and how would we make sure that happens at the right time (i.e. before
> creating PrefService)?
>
>
> https://codereview.chromium.org/12340111/diff/1/components/user_prefs/OWNERS
> File components/user_prefs/OWNERS (right):
>
>
https://codereview.chromium.org/12340111/diff/1/components/user_prefs/OWNERS#...
> components/user_prefs/OWNERS:4: pam@chromium.org
> Feel free to add your name here, you know this stuff well enough
> meanwhile :)
>
> https://codereview.chromium.org/12340111/

[Mattias Nissler (ping if slow), 2013-02-27 15:48:58]

On Wed, Feb 27, 2013 at 4:33 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> > Before I do full pass through this and and stamp it, I'd like to
> understand
> > better what the plan is for PrefRegistrySyncable. How would a component
> > access
> > it, and how would we make sure that happens at the right time (i.e.
> before
> > creating PrefService)?
>
> Components that need it would #include the class directly and
> implement a ComponentName::RegisterUserPrefs function.  It would be
> the embedder's responsibility to call each of these functions to
> register prefs, just as is done today within chrome/browser.
>

OK, that makes sense. Maybe worthwhile to document somewhere?


>
> > Feel free to add your name here, you know this stuff well enough
> > meanwhile :)
>
> Thanks, I'm already in components/OWNERS so no need to list me again.
>

Wow, if that project succeeds, you're going to be owning All The Things :)

Will do a full review pass then.


> Cheers,
> Jói
>
>
>
> On Wed, Feb 27, 2013 at 3:29 PM,  <mnissler@chromium.org> wrote:
> > Before I do full pass through this and and stamp it, I'd like to
> understand
> > better what the plan is for PrefRegistrySyncable. How would a component
> > access
> > it, and how would we make sure that happens at the right time (i.e.
> before
> > creating PrefService)?
> >
> >
> >
> https://codereview.chromium.org/12340111/diff/1/components/user_prefs/OWNERS
> > File components/user_prefs/OWNERS (right):
> >
> >
>
https://codereview.chromium.org/12340111/diff/1/components/user_prefs/OWNERS#...
> > components/user_prefs/OWNERS:4: pam@chromium.org
> > Feel free to add your name here, you know this stuff well enough
> > meanwhile :)
> >
> > https://codereview.chromium.org/12340111/
>

[Mattias Nissler (ping if slow), 2013-02-27 16:16:42]

LGTM. One suggestion that you may choose to ignore.

https://codereview.chromium.org/12340111/diff/5001/components/user_prefs/user...
File components/user_prefs/user_prefs.h (right):

https://codereview.chromium.org/12340111/diff/5001/components/user_prefs/user...
components/user_prefs/user_prefs.h:40: PrefService*
Lookup(content::BrowserContext* context);
I wonder whether you want to make a static helper for shortcutting this? Maybe:

components::UserPrefs::Get(context)

instead of:

components::UserPrefs::GetInstance()->Lookup(context)

I don't feel strong though. Your call.

[Jói, 2013-02-27 16:55:41]

>> Components that need it would #include the class directly and
>> implement a ComponentName::RegisterUserPrefs function.  It would be
>> the embedder's responsibility to call each of these functions to
>> register prefs, just as is done today within chrome/browser.
>
>
> OK, that makes sense. Maybe worthwhile to document somewhere?

Added to class documentation for PrefRegistrySyncable.

> Wow, if that project succeeds, you're going to be owning All The Things :)

Oh no, you're on to my secret plan! ;-)

>
https://codereview.chromium.org/12340111/diff/5001/components/user_prefs/user...
> components/user_prefs/user_prefs.h:40: PrefService*
> Lookup(content::BrowserContext* context);
> I wonder whether you want to make a static helper for shortcutting this?
> Maybe:
>
> components::UserPrefs::Get(context)

Done, good idea.

[Jói, 2013-02-28 14:43:37]

Elliot: Could you take a look at chrome/browser/profiles/* ?

That (specifically the one-time initialization of UserPrefs in the Profile
constructor) is the only non-trivial part of this change outside Prefs changes
which Mattias is an owner for.

Cheers,
Jói

[Elliot Glaysher, 2013-02-28 21:13:18]

https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
File components/user_prefs/user_prefs.h (right):

https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
components/user_prefs/user_prefs.h:18: };
The dependency on content isn't reflected in the gyp file.

https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
components/user_prefs/user_prefs.h:42: void
Initialize(scoped_ptr<UserPrefsDelegate> delegate);
I'd be a lot more comfortable with the UserPrefs if the delegate weren't a
singleton, but was instead a mapping from BrowserContext to delegate or
BrowserContext to PrefService.

[tfarina, 2013-02-28 21:40:49]

https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
File components/user_prefs/user_prefs.h (right):

https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
components/user_prefs/user_prefs.h:18: };
On 2013/02/28 21:13:18, Elliot Glaysher wrote:
> The dependency on content isn't reflected in the gyp file.
it isn't necessary to list content.gyp:content_browser because only a pointer is
passed but no use is made actually. i.e., we don't have any context->Foo() that
would require us to link with the content library.

[Jói, 2013-02-28 21:45:59]

Thanks Elliot.

I would like to know more about what you're proposing, see below.

Cheers,
Jói

https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
File components/user_prefs/user_prefs.h (right):

https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
components/user_prefs/user_prefs.h:18: };
On 2013/02/28 21:40:49, tfarina wrote:
> On 2013/02/28 21:13:18, Elliot Glaysher wrote:
> > The dependency on content isn't reflected in the gyp file.
> it isn't necessary to list content.gyp:content_browser because only a pointer
is
> passed but no use is made actually. i.e., we don't have any context->Foo()
that
> would require us to link with the content library.

What Thiago said :)

https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
components/user_prefs/user_prefs.h:42: void
Initialize(scoped_ptr<UserPrefsDelegate> delegate);
On 2013/02/28 21:13:18, Elliot Glaysher wrote:
> I'd be a lot more comfortable with the UserPrefs if the delegate weren't a
> singleton, but was instead a mapping from BrowserContext to delegate or
> BrowserContext to PrefService.

I'm not exactly sure what you mean.  UserPrefs is a way to map from
BrowserContext to PrefService, in an embedder-specific way.  The singleton is
not exactly stateful; it just has a pointer to the single, embedder-provided
implementation of the delegate.  This is analogous to how ContentClient and its
ilk are done.

[Elliot Glaysher, 2013-02-28 21:47:23]

https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
File components/user_prefs/user_prefs.h (right):

https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
components/user_prefs/user_prefs.h:18: };
On 2013/02/28 21:40:49, tfarina wrote:
> On 2013/02/28 21:13:18, Elliot Glaysher wrote:
> > The dependency on content isn't reflected in the gyp file.
> it isn't necessary to list content.gyp:content_browser because only a pointer
is
> passed but no use is made actually. i.e., we don't have any context->Foo()
that
> would require us to link with the content library.

I'm aware that we aren't dereferencing the type, however, we're still
referencing a type from a deeper layer here.

[Jói, 2013-02-28 21:58:45]

Do you feel that the gyp file should state a dependency, when the
linker will in fact not link anything from content into user_prefs?
The linker would complain if and when a true dependency is added.  For
now, the name of the type is all that is used, and the actual
dependency on content is in the delegate implementation.

Cheers,
Jói


On Thu, Feb 28, 2013 at 9:47 PM,  <erg@chromium.org> wrote:
>
>
https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
> File components/user_prefs/user_prefs.h (right):
>
>
https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
> components/user_prefs/user_prefs.h:18: };
> On 2013/02/28 21:40:49, tfarina wrote:
>>
>> On 2013/02/28 21:13:18, Elliot Glaysher wrote:
>> > The dependency on content isn't reflected in the gyp file.
>> it isn't necessary to list content.gyp:content_browser because only a
>
> pointer is
>>
>> passed but no use is made actually. i.e., we don't have any
>
> context->Foo() that
>>
>> would require us to link with the content library.
>
>
> I'm aware that we aren't dereferencing the type, however, we're still
> referencing a type from a deeper layer here.
>
> https://codereview.chromium.org/12340111/

[Elliot Glaysher, 2013-02-28 21:58:55]

https://codereview.chromium.org/12340111/diff/16001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/12340111/diff/16001/chrome/browser/profiles/p...
chrome/browser/profiles/profile.cc:57: components::UserPrefs* user_prefs =
components::UserPrefs::GetInstance();
You mentioned ContetnClient. If this is supposed to be a single, global
singleton, putting this in the startup code (ChromeMainDelegate or one of its
ExtraParts) would make more sense.

Putting it here doesn't make it feel like this is startup code, but something
user configurable since Profile isn't the only consumer of BrowserContexts.

https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
File components/user_prefs/user_prefs.h (right):

https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
components/user_prefs/user_prefs.h:42: void
Initialize(scoped_ptr<UserPrefsDelegate> delegate);
On 2013/02/28 21:46:00, Jói wrote:
> On 2013/02/28 21:13:18, Elliot Glaysher wrote:
> > I'd be a lot more comfortable with the UserPrefs if the delegate weren't a
> > singleton, but was instead a mapping from BrowserContext to delegate or
> > BrowserContext to PrefService.
> 
> I'm not exactly sure what you mean.  UserPrefs is a way to map from
> BrowserContext to PrefService, in an embedder-specific way.  The singleton is
> not exactly stateful; it just has a pointer to the single, embedder-provided
> implementation of the delegate.  This is analogous to how ContentClient and
its
> ilk are done.

What I'm asking is how likely is it that non-chrome/ things like apps will want
to inject their own logic for mapping a BrowserContext to a PrefService.

[Elliot Glaysher, 2013-02-28 22:02:59]

+jam to settle a question of dependencies. The new components/user_prefs
component forward declares a type in content/ but doesn't depend on content/. I
thought that wasn't ok. I may be wrong.

On 2013/02/28 21:58:45, Jói wrote:
> Do you feel that the gyp file should state a dependency, when the
> linker will in fact not link anything from content into user_prefs?

I do. This is still exposing information about one component to another.
content/ has these data structures, and whether or not components/ dereferences
them, it depends on them.

[Jói, 2013-02-28 22:05:14]

> Putting it here doesn't make it feel like this is startup code, but
> something user configurable since Profile isn't the only consumer of
> BrowserContexts.

True.  The reason to put it here is that Profile is the only owner of
a PrefService keyed by BrowserContext.

I could hunt for a good place to put it in e.g. ChromeMainDelegate but
my worry was that this wouldn't catch edge cases such as unit tests.
My previous attempt at doing this in the ProfileManager ctor failed in
such a way.

> What I'm asking is how likely is it that non-chrome/ things like apps
> will want to inject their own logic for mapping a BrowserContext to a
> PrefService.

I'd say not very likely.  If you're asking whether this should hang
off of a BrowserContext in a similar way as our tab helpers now hang
off of WebContents, then I'd say that's a good idea but the current
approach solves a fairly immediate need, so could act at least as an
interim step.

Cheers,
Jói


On Thu, Feb 28, 2013 at 9:58 PM,  <erg@chromium.org> wrote:
>
>
https://codereview.chromium.org/12340111/diff/16001/chrome/browser/profiles/p...
> File chrome/browser/profiles/profile.cc (right):
>
>
https://codereview.chromium.org/12340111/diff/16001/chrome/browser/profiles/p...
> chrome/browser/profiles/profile.cc:57: components::UserPrefs* user_prefs
> = components::UserPrefs::GetInstance();
> You mentioned ContetnClient. If this is supposed to be a single, global
> singleton, putting this in the startup code (ChromeMainDelegate or one
> of its ExtraParts) would make more sense.
>
> Putting it here doesn't make it feel like this is startup code, but
> something user configurable since Profile isn't the only consumer of
> BrowserContexts.
>
>
>
https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
> File components/user_prefs/user_prefs.h (right):
>
>
https://codereview.chromium.org/12340111/diff/16001/components/user_prefs/use...
> components/user_prefs/user_prefs.h:42: void
> Initialize(scoped_ptr<UserPrefsDelegate> delegate);
> On 2013/02/28 21:46:00, Jói wrote:
>>
>> On 2013/02/28 21:13:18, Elliot Glaysher wrote:
>> > I'd be a lot more comfortable with the UserPrefs if the delegate
>
> weren't a
>>
>> > singleton, but was instead a mapping from BrowserContext to delegate
>
> or
>>
>> > BrowserContext to PrefService.
>
>
>> I'm not exactly sure what you mean.  UserPrefs is a way to map from
>> BrowserContext to PrefService, in an embedder-specific way.  The
>
> singleton is
>>
>> not exactly stateful; it just has a pointer to the single,
>
> embedder-provided
>>
>> implementation of the delegate.  This is analogous to how
>
> ContentClient and its
>>
>> ilk are done.
>
>
> What I'm asking is how likely is it that non-chrome/ things like apps
> will want to inject their own logic for mapping a BrowserContext to a
> PrefService.
>
> https://codereview.chromium.org/12340111/

[Jói, 2013-02-28 22:10:04]

> I do. This is still exposing information about one component to another.
> content/ has these data structures, and whether or not components/
> dereferences
> them, it depends on them.

As a thought experiment, consider a hypothetical embedder that doesn't
use the |context| pointer to implement the delegate method.  IMHO
there is no dependency on content unless you actually use the type.
Stating its name does not make you dependent on its implementation.

Cheers,
Jói


On Thu, Feb 28, 2013 at 10:03 PM,  <erg@chromium.org> wrote:
> +jam to settle a question of dependencies. The new components/user_prefs
> component forward declares a type in content/ but doesn't depend on
> content/. I
> thought that wasn't ok. I may be wrong.
>
>
> On 2013/02/28 21:58:45, Jói wrote:
>>
>> Do you feel that the gyp file should state a dependency, when the
>> linker will in fact not link anything from content into user_prefs?
>
>
> I do. This is still exposing information about one component to another.
> content/ has these data structures, and whether or not components/
> dereferences
> them, it depends on them.
>
>
> https://codereview.chromium.org/12340111/
>
> --
> You received this message because you are subscribed to the Google Groups
> "browser-components-watch" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to browser-components-watch+unsubscribe@chromium.org.
> To post to this group, send email to browser-components-watch@chromium.org.
> For more options, visit
> https://groups.google.com/a/chromium.org/groups/opt_out.
>
>

[Jói, 2013-02-28 22:16:17]

Hey Elliot,

I just realized that BrowserContext is already a SupportsUserData.  I
think hanging the PrefService off of that, instead of leaving it a
member variable of Profile, is a considerably better solution.  I will
switch to that tomorrow.  Thanks for the push-back :)

Cheers,
Jói


On Thu, Feb 28, 2013 at 10:09 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> I do. This is still exposing information about one component to another.
>> content/ has these data structures, and whether or not components/
>> dereferences
>> them, it depends on them.
>
> As a thought experiment, consider a hypothetical embedder that doesn't
> use the |context| pointer to implement the delegate method.  IMHO
> there is no dependency on content unless you actually use the type.
> Stating its name does not make you dependent on its implementation.
>
> Cheers,
> Jói
>
>
> On Thu, Feb 28, 2013 at 10:03 PM,  <erg@chromium.org> wrote:
>> +jam to settle a question of dependencies. The new components/user_prefs
>> component forward declares a type in content/ but doesn't depend on
>> content/. I
>> thought that wasn't ok. I may be wrong.
>>
>>
>> On 2013/02/28 21:58:45, Jói wrote:
>>>
>>> Do you feel that the gyp file should state a dependency, when the
>>> linker will in fact not link anything from content into user_prefs?
>>
>>
>> I do. This is still exposing information about one component to another.
>> content/ has these data structures, and whether or not components/
>> dereferences
>> them, it depends on them.
>>
>>
>> https://codereview.chromium.org/12340111/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "browser-components-watch" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to browser-components-watch+unsubscribe@chromium.org.
>> To post to this group, send email to browser-components-watch@chromium.org.
>> For more options, visit
>> https://groups.google.com/a/chromium.org/groups/opt_out.
>>
>>

[Jói, 2013-02-28 22:25:34]

Mattias:  This will mean //components/user_prefs shrinks down to just
PrefRegistrySyncable, at which point I'll just make it a static
library in all configurations, rather than a component with the
inherent exports and such.  Hopefully you are still OK with this
location for PrefRegistrySyncable; if not I'm not sure where to put it
for components to use.

Cheers,
Jói


On Thu, Feb 28, 2013 at 10:15 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Hey Elliot,
>
> I just realized that BrowserContext is already a SupportsUserData.  I
> think hanging the PrefService off of that, instead of leaving it a
> member variable of Profile, is a considerably better solution.  I will
> switch to that tomorrow.  Thanks for the push-back :)
>
> Cheers,
> Jói
>
>
> On Thu, Feb 28, 2013 at 10:09 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>> I do. This is still exposing information about one component to another.
>>> content/ has these data structures, and whether or not components/
>>> dereferences
>>> them, it depends on them.
>>
>> As a thought experiment, consider a hypothetical embedder that doesn't
>> use the |context| pointer to implement the delegate method.  IMHO
>> there is no dependency on content unless you actually use the type.
>> Stating its name does not make you dependent on its implementation.
>>
>> Cheers,
>> Jói
>>
>>
>> On Thu, Feb 28, 2013 at 10:03 PM,  <erg@chromium.org> wrote:
>>> +jam to settle a question of dependencies. The new components/user_prefs
>>> component forward declares a type in content/ but doesn't depend on
>>> content/. I
>>> thought that wasn't ok. I may be wrong.
>>>
>>>
>>> On 2013/02/28 21:58:45, Jói wrote:
>>>>
>>>> Do you feel that the gyp file should state a dependency, when the
>>>> linker will in fact not link anything from content into user_prefs?
>>>
>>>
>>> I do. This is still exposing information about one component to another.
>>> content/ has these data structures, and whether or not components/
>>> dereferences
>>> them, it depends on them.
>>>
>>>
>>> https://codereview.chromium.org/12340111/
>>>
>>> --
>>> You received this message because you are subscribed to the Google Groups
>>> "browser-components-watch" group.
>>> To unsubscribe from this group and stop receiving emails from it, send an
>>> email to browser-components-watch+unsubscribe@chromium.org.
>>> To post to this group, send email to browser-components-watch@chromium.org.
>>> For more options, visit
>>> https://groups.google.com/a/chromium.org/groups/opt_out.
>>>
>>>

[Elliot Glaysher, 2013-02-28 22:33:54]

-jam: question solved another way.

On 2013/02/28 22:16:17, Jói wrote:
> I just realized that BrowserContext is already a SupportsUserData.  I
> think hanging the PrefService off of that, instead of leaving it a
> member variable of Profile, is a considerably better solution.

I concur. That's a much better solution.

[Mattias Nissler (ping if slow), 2013-03-01 09:59:54]

On Thu, Feb 28, 2013 at 11:24 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> Mattias:  This will mean //components/user_prefs shrinks down to just
> PrefRegistrySyncable, at which point I'll just make it a static
> library in all configurations, rather than a component with the
> inherent exports and such.  Hopefully you are still OK with this
> location for PrefRegistrySyncable; if not I'm not sure where to put it
> for components to use.
>

I'm not really familiar with the components architecture, but is there some
place where generic component support stuff lives that components can call
into? Anyhow, I don't care much where PrefRegistrySyncable ends up, given
that it's no longer an integral a part of core PrefService.


>
> Cheers,
> Jói
>
>
> On Thu, Feb 28, 2013 at 10:15 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> > Hey Elliot,
> >
> > I just realized that BrowserContext is already a SupportsUserData.  I
> > think hanging the PrefService off of that, instead of leaving it a
> > member variable of Profile, is a considerably better solution.  I will
> > switch to that tomorrow.  Thanks for the push-back :)
> >
> > Cheers,
> > Jói
> >
> >
> > On Thu, Feb 28, 2013 at 10:09 PM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >>> I do. This is still exposing information about one component to
> another.
> >>> content/ has these data structures, and whether or not components/
> >>> dereferences
> >>> them, it depends on them.
> >>
> >> As a thought experiment, consider a hypothetical embedder that doesn't
> >> use the |context| pointer to implement the delegate method.  IMHO
> >> there is no dependency on content unless you actually use the type.
> >> Stating its name does not make you dependent on its implementation.
> >>
> >> Cheers,
> >> Jói
> >>
> >>
> >> On Thu, Feb 28, 2013 at 10:03 PM,  <erg@chromium.org> wrote:
> >>> +jam to settle a question of dependencies. The new
> components/user_prefs
> >>> component forward declares a type in content/ but doesn't depend on
> >>> content/. I
> >>> thought that wasn't ok. I may be wrong.
> >>>
> >>>
> >>> On 2013/02/28 21:58:45, Jói wrote:
> >>>>
> >>>> Do you feel that the gyp file should state a dependency, when the
> >>>> linker will in fact not link anything from content into user_prefs?
> >>>
> >>>
> >>> I do. This is still exposing information about one component to
> another.
> >>> content/ has these data structures, and whether or not components/
> >>> dereferences
> >>> them, it depends on them.
> >>>
> >>>
> >>> https://codereview.chromium.org/12340111/
> >>>
> >>> --
> >>> You received this message because you are subscribed to the Google
> Groups
> >>> "browser-components-watch" group.
> >>> To unsubscribe from this group and stop receiving emails from it, send
> an
> >>> email to browser-components-watch+unsubscribe@chromium.org.
> >>> To post to this group, send email to
> browser-components-watch@chromium.org.
> >>> For more options, visit
> >>> https://groups.google.com/a/chromium.org/groups/opt_out.
> >>>
> >>>
>

[Jói, 2013-03-01 14:53:52]

PTAL, I've switched to a SupportsUserData approach.

The real changes from the last patch set reviewed are in profile_impl.h|cc,
profile.cc (now virtually unchanged) and user_data.h|cc, and deletion of
user_data_delegate.h, the rest is just merging LKGR.

Cheers,
Jói

[Jói, 2013-03-01 15:19:15]

> The real changes from the last patch set reviewed are in profile_impl.h|cc,
> profile.cc (now virtually unchanged) and user_data.h|cc, and deletion of
> user_data_delegate.h, the rest is just merging LKGR.

Also testing_profile.cc|h

On 2013/03/01 14:53:52, Jói wrote:
> PTAL, I've switched to a SupportsUserData approach.
> 
> The real changes from the last patch set reviewed are in profile_impl.h|cc,
> profile.cc (now virtually unchanged) and user_data.h|cc, and deletion of
> user_data_delegate.h, the rest is just merging LKGR.
> 
> Cheers,
> Jói

[Mattias Nissler (ping if slow), 2013-03-01 18:59:40]

LGTM

[Elliot Glaysher, 2013-03-01 20:36:35]

ty for the fix. lgtm.

[Jói, 2013-03-04 14:52:11]

Tests revealed that moving ownership to the user data object is
premature, that will need to be a larger change that resolves
dependencies between different things that ProfileImpl currently owns.
 Moved ownership back to ProfileImpl, so that the UserPrefs object
just stores a non-owning pointer.  Please see diff from patch 9 to 10.

Cheers,
Jói


On Fri, Mar 1, 2013 at 8:36 PM,  <erg@chromium.org> wrote:
> ty for the fix. lgtm.
>
> https://codereview.chromium.org/12340111/

[tfarina, 2013-03-04 16:30:51]

https://codereview.chromium.org/12340111/diff/15163/components/user_prefs.gypi
File components/user_prefs.gypi (right):

https://codereview.chromium.org/12340111/diff/15163/components/user_prefs.gyp...
components/user_prefs.gypi:13: '../base/base.gyp:base_prefs',
duplicated

https://codereview.chromium.org/12340111/diff/15163/components/user_prefs/use...
File components/user_prefs/user_prefs.h (right):

https://codereview.chromium.org/12340111/diff/15163/components/user_prefs/use...
components/user_prefs/user_prefs.h:17: };
rm ;

[Elliot Glaysher, 2013-03-04 18:56:54]

slgtm

[Jói, 2013-03-05 20:54:19]

https://codereview.chromium.org/12340111/diff/15163/components/user_prefs.gypi
File components/user_prefs.gypi (right):

https://codereview.chromium.org/12340111/diff/15163/components/user_prefs.gyp...
components/user_prefs.gypi:13: '../base/base.gyp:base_prefs',
On 2013/03/04 16:30:52, tfarina wrote:
> duplicated

Done.

https://codereview.chromium.org/12340111/diff/15163/components/user_prefs/use...
File components/user_prefs/user_prefs.h (right):

https://codereview.chromium.org/12340111/diff/15163/components/user_prefs/use...
components/user_prefs/user_prefs.h:17: };
On 2013/03/04 16:30:52, tfarina wrote:
> rm ;

Done.

[Jói, 2013-03-05 21:02:50]

TBR=ben@chromium.org for trivial updates in many places.

mnissler@ has reviewed the entire change, and is an owner for Prefs.  erg@ is an
owner for and has reviewed the parts related to Profile.  Other parts are
trivial.

Cheers,
Jói

[commit-bot: I haz the power, 2013-03-05 21:03:57]

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

[commit-bot: I haz the power, 2013-03-05 21:13:04]

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

[commit-bot: I haz the power, 2013-03-05 21:26:31]

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

[commit-bot: I haz the power, 2013-03-06 00:42:51]

Change committed as 186301

[tfarina, 2013-03-06 02:59:22]

https://chromiumcodereview.appspot.com/12340111/diff/40001/components/user_pr...
File components/user_prefs/pref_registry_syncable.h (right):

https://chromiumcodereview.appspot.com/12340111/diff/40001/components/user_pr...
components/user_prefs/pref_registry_syncable.h:31: class USER_PREFS_EXPORT
PrefRegistrySyncable : public PrefRegistry {
just noticed that UserPrefs was put in components namespace. Was this intended
for PrefRegistrySyncable too, but not done, given the number of usages in
//chrome?

[Jói, 2013-03-06 03:06:51]

> USER_PREFS_EXPORT PrefRegistrySyncable : public PrefRegistry {
> just noticed that UserPrefs was put in components namespace. Was this
> intended for PrefRegistrySyncable too, but not done, given the number of
> usages in //chrome?

It should be done for PrefRegistrySyncable as well. I meant to add a
TODO but it seems that I forgot.

On Tue, Mar 5, 2013 at 6:59 PM,  <tfarina@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/12340111/diff/40001/components/user_pr...
> File components/user_prefs/pref_registry_syncable.h (right):
>
>
https://chromiumcodereview.appspot.com/12340111/diff/40001/components/user_pr...
> components/user_prefs/pref_registry_syncable.h:31: class
> USER_PREFS_EXPORT PrefRegistrySyncable : public PrefRegistry {
> just noticed that UserPrefs was put in components namespace. Was this
> intended for PrefRegistrySyncable too, but not done, given the number of
> usages in //chrome?
>
> https://chromiumcodereview.appspot.com/12340111/