WIP: Introduce per-TabContents PrefService.

WIP: Introduce per-TabContents PrefService.

BUG=
TEST=

[Mattias Nissler (ping if slow), 2011-09-07 17:54:24]

Hi guys,

here is some code that outlines my proposal. I've commented the important bits.
Highlights include:

- The per-TabContents PrefService has its own pref namespace, so prefs are
separately registered with it, resulting in people incorrectly using the main
PrefService getting DCHECKS.
- Other pref sources like command line, extensions, and policy also work with
per-TabContents prefs.
- No user-visible changes to the PrefService interface.

The patch is larger than it should be just because our current PrefService
construction code is very convoluted. We can fix that up in a first CL before we
introduce the per-TabContents prefs (I'm happy to donate cycles for that).

http://codereview.chromium.org/7838030/diff/1/chrome/browser/prefs/tab_conten...
File chrome/browser/prefs/tab_contents_user_pref_store.h (right):

http://codereview.chromium.org/7838030/diff/1/chrome/browser/prefs/tab_conten...
chrome/browser/prefs/tab_contents_user_pref_store.h:11: class
TabContentsUserPrefStore : public PersistentPrefStore {
This would hold the pref values set in the per-TabContents PrefStore. You
implemented this already in your CL.

http://codereview.chromium.org/7838030/diff/1/chrome/browser/profiles/profile.h
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7838030/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile.h:356: virtual PrefService*
GetPrefsForTabContents(TabContents* contents) = 0;
This is the accessor for getting the per-TabContents PrefService. There's no
need to put it here, since the TabContents pointer gives us all we need. In
fact, it may be better to implement this as a static function somewhere?

[Avi (use Gerrit), 2011-09-07 18:00:56]

http://codereview.chromium.org/7838030/diff/1/chrome/browser/profiles/profile.h
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7838030/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile.h:352: // Ideally, this would live in a
per-TabContest context object, but since on
What is a "per-TabContent (typo) context object"? You mean a TabContentsWrapper?

Perhaps a tab helper there? I'm not sure I understand the problem.

[Mattias Nissler (ping if slow), 2011-09-07 18:24:46]

Thanks Avi for your helpful comment!

http://codereview.chromium.org/7838030/diff/1/chrome/browser/profiles/profile.h
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7838030/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile.h:352: // Ideally, this would live in a
per-TabContest context object, but since on
On 2011/09/07 18:00:56, Avi wrote:
> What is a "per-TabContent (typo) context object"? You mean a
TabContentsWrapper?
> 
> Perhaps a tab helper there? I'm not sure I understand the problem.

Ah, I wasn't aware of TabContentsWrapper :) I'll move this to TabContentsWrapper
then.

Out of curiosity: Why isn't the TabContentsWrapper implementing BrowserContext
then, but Profile? Seems like inverted dependencies to uninitiated me.

[Torne, 2011-09-08 14:27:16]

On 2011/09/07 17:54:24, Mattias Nissler wrote:
> - The per-TabContents PrefService has its own pref namespace, so prefs are
> separately registered with it, resulting in people incorrectly using the main
> PrefService getting DCHECKS.

There seem to be some problems with this, now that I've seen it implemented:

First, currently in this CL it looks like *every* PrefService for each
TabContents is separate and thus you actually have a separate namespace for
every individual tab, which means the prefs need to get registered on tab
creation.. unless I'm missing something that would handle this (perhaps
something not currently implemented?)

Second, there still needs to *be* a global value for these prefs, since the
settings UI will use them, and that global value needs to be persisted..

How are you planning on having the values set initially? How are you planning on
making global changes in prefs (e.g. user ticking/unticking things in the
settings UI) actually set off all the relevant notifications?

[Avi (use Gerrit), 2011-09-08 14:33:37]

>Why isn't the TabContentsWrapper implementing BrowserContext
then, but Profile? Seems like inverted dependencies to uninitiated me.

Not sure what you mean.

At the content/ layer, we have TabContents and BrowserContext. At the chrome/
layer we have TabContentsWrapper, which wraps and extends (in a manner of
speaking) the TabContents, and we have the Profile, which implements and extends
(literally) the BrowserContext.

http://codereview.chromium.org/7838030/diff/13/chrome/browser/ui/tab_contents...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/7838030/diff/13/chrome/browser/ui/tab_contents...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:253:
per_tab_prefs_.reset(profile()->GetPrefs()->CreateTabContentsPrefService());
These items are initialized in the order of their declaration; move this
initialization to match the new location.

http://codereview.chromium.org/7838030/diff/13/chrome/browser/ui/tab_contents...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/7838030/diff/13/chrome/browser/ui/tab_contents...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:214: PrefService*
per_tab_prefs() {
When you notice a pattern of alphabetical items, please respect it and insert
your new item in the correct position.

http://codereview.chromium.org/7838030/diff/13/chrome/browser/ui/tab_contents...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:316:
scoped_ptr<PrefService> per_tab_prefs_;
ditto re alphabetizing

[Mattias Nissler (ping if slow), 2011-09-08 14:42:22]

On 2011/09/08 14:27:16, Torne wrote:
> On 2011/09/07 17:54:24, Mattias Nissler wrote:
> > - The per-TabContents PrefService has its own pref namespace, so prefs are
> > separately registered with it, resulting in people incorrectly using the
main
> > PrefService getting DCHECKS.
> 
> There seem to be some problems with this, now that I've seen it implemented:
> 
> First, currently in this CL it looks like *every* PrefService for each
> TabContents is separate and thus you actually have a separate namespace for
> every individual tab, which means the prefs need to get registered on tab
> creation.. unless I'm missing something that would handle this (perhaps
> something not currently implemented?)

Nope, that's correct. You'd just register the prefs when you create the per-tab
PrefService. Is there anything wrong with that?

> Second, there still needs to *be* a global value for these prefs, since the
> settings UI will use them, and that global value needs to be persisted..

I assume by "global" you mean profile-scoped. Then, these values should probably
come from the Profile prefs. The way we handle this for incognito is making the
custom user pref store implementation have a pointer to the user pref store of
the main PrefService, delegating GetValue() calls if there is no override
present. I didn't include that in the code, but it's easy to add.

> How are you planning on having the values set initially? How are you planning
on
> making global changes in prefs (e.g. user ticking/unticking things in the
> settings UI) actually set off all the relevant notifications?

This one is also solved by having the TabContentsUserContents delegate to the
main user pref store. For notifications we'd install a tweaked PrefNotifier to
forward notifications to the derived PrefServices (I put a TODO in the code
regarding the same thing for incognito, where it's currently broken).

Note that the drawback of keeping per-Profile pref values is that you don't get
the DCHECK on reading from the wrong PrefService, which indicated desirable in
our conversation yesterday? However, if you want profile-scoped prefs, you'll
also have to allow reading them, even if it's only for displaying the values in
the pref UI.

[Mattias Nissler (ping if slow), 2011-09-08 14:49:56]

On Thu, Sep 8, 2011 at 4:33 PM, <avi@chromium.org> wrote:

>  Why isn't the TabContentsWrapper implementing BrowserContext
>>
> then, but Profile? Seems like inverted dependencies to uninitiated me.
>
> Not sure what you mean.
>
> At the content/ layer, we have TabContents and BrowserContext. At the
> chrome/
> layer we have TabContentsWrapper, which wraps and extends (in a manner of
> speaking) the TabContents, and we have the Profile, which implements and
> extends
> (literally) the BrowserContext.
>

Yes, that's what I was referring to. So we have TabContents and it has a
pointer to a BrowserContext implementation for interacting with consumer
code. Since TabContentsWrapper is 1:1 with a TabContents, I had expected
that TabContentsWrapper implements (literal) BrowserContext and keeps a
pointer to the Profile. But what we have is the other way round, i.e.
Profile implements BrowserContext() and we have to use a TabContents
property to get access to the TabContentsWrapper for a given TabContents.
That's what came unexpected to me. I'm sure there's a good explanation to
that, and that's what I intended to ask for.


>
>
> http://codereview.chromium.**org/7838030/diff/13/chrome/**
>
browser/ui/tab_contents/tab_**contents_wrapper.cc<http://codereview.chromium.org/7838030/diff/13/chrome/browser/ui/tab_contents/tab_contents_wrapper.cc>
> File chrome/browser/ui/tab_**contents/tab_contents_wrapper.**cc (right):
>
> http://codereview.chromium.**org/7838030/diff/13/chrome/**
>
browser/ui/tab_contents/tab_**contents_wrapper.cc#newcode253<http://codereview.chromium.org/7838030/diff/13/chrome/browser/ui/tab_contents/tab_contents_wrapper.cc#newcode253>
> chrome/browser/ui/tab_**contents/tab_contents_wrapper.**cc:253:
> per_tab_prefs_.reset(profile()**->GetPrefs()->**
> CreateTabContentsPrefService()**);
> These items are initialized in the order of their declaration; move this
> initialization to match the new location.
>
> http://codereview.chromium.**org/7838030/diff/13/chrome/**
>
browser/ui/tab_contents/tab_**contents_wrapper.h<http://codereview.chromium.org/7838030/diff/13/chrome/browser/ui/tab_contents/tab_contents_wrapper.h>
> File chrome/browser/ui/tab_**contents/tab_contents_wrapper.**h (right):
>
> http://codereview.chromium.**org/7838030/diff/13/chrome/**
>
browser/ui/tab_contents/tab_**contents_wrapper.h#newcode214<http://codereview.chromium.org/7838030/diff/13/chrome/browser/ui/tab_contents/tab_contents_wrapper.h#newcode214>
> chrome/browser/ui/tab_**contents/tab_contents_wrapper.**h:214:
> PrefService*
> per_tab_prefs() {
> When you notice a pattern of alphabetical items, please respect it and
> insert your new item in the correct position.
>
> http://codereview.chromium.**org/7838030/diff/13/chrome/**
>
browser/ui/tab_contents/tab_**contents_wrapper.h#newcode316<http://codereview.chromium.org/7838030/diff/13/chrome/browser/ui/tab_contents/tab_contents_wrapper.h#newcode316>
> chrome/browser/ui/tab_**contents/tab_contents_wrapper.**h:316:
> scoped_ptr<PrefService> per_tab_prefs_;
> ditto re alphabetizing
>
>
>
http://codereview.chromium.**org/7838030/<http://codereview.chromium.org/7838...
>

[Avi (use Gerrit), 2011-09-08 15:03:55]

On 2011/09/08 14:49:56, Mattias Nissler wrote:
> I'm sure there's a good explanation to
> that, and that's what I intended to ask for.

The simple explanation is that TCW was invented to wrap TC long before Profile
had a subset called BrowserContext abstracted to serve as something for content.

Whether your scheme makes more sense in the long run is something I need to
think about. I'm still in the old-school camp of thinking about TCs and
Profiles, and having chrome correlates for each works for my mental model.

[Mattias Nissler (ping if slow), 2011-09-08 15:08:20]

On Thu, Sep 8, 2011 at 5:03 PM, <avi@chromium.org> wrote:

> On 2011/09/08 14:49:56, Mattias Nissler wrote:
>
>> I'm sure there's a good explanation to
>> that, and that's what I intended to ask for.
>>
>
> The simple explanation is that TCW was invented to wrap TC long before
> Profile
> had a subset called BrowserContext abstracted to serve as something for
> content.
>
> Whether your scheme makes more sense in the long run is something I need to
> think about. I'm still in the old-school camp of thinking about TCs and
> Profiles, and having chrome correlates for each works for my mental model.


Same for me :) I only see the content part of the code occasionally and
every time I take a look it's different :)


>
>
>
http://codereview.chromium.**org/7838030/<http://codereview.chromium.org/7838...
>