Introduce per-tab preferences service.

Introduce per-tab preferences service.

This enables overriding user preferences on a per-tab rather
than per-profile basis.

Based on this CL: http://codereview.chromium.org/7838030/

BUG=none
TEST=unit_tests pass

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111128

[mnaganov (inactive), 2011-11-15 20:28:02]

Hi Mattias,

In this CL I'm adding per-tab pref service to TabContents. I'm using your WIP CL
as a base. If I'm getting it right, one of your intentions was to simplify
PrefService creation by avoiding pointers re-initialization and moving similar
logic into one place.

No new preferences are added at this step.

[Avi (use Gerrit), 2011-11-15 20:31:32]

We almost certainly do not want to go this route. Please do not add new items to
TabContents that have to do with concepts like "tabs" that are a level above it,
at the Chrome layer.

If you need to keep something in 1:1 correlation with a TabContents, add a
TabHelper to the TabContentsWrapper.

[Avi (use Gerrit), 2011-11-15 20:32:19]

Note that the CL that you based this on does this correctly, and adds the
PrefService to the TabContentsWrapper.

[Mattias Nissler (ping if slow), 2011-11-16 12:08:04]

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/per_tab_us...
File chrome/browser/prefs/per_tab_user_pref_store.cc (right):

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/per_tab_us...
chrome/browser/prefs/per_tab_user_pref_store.cc:6: #include
"chrome/common/pref_names.h"
not needed.

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/per_tab_us...
File chrome/browser/prefs/per_tab_user_pref_store.h (right):

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/per_tab_us...
chrome/browser/prefs/per_tab_user_pref_store.h:9: #include <string>
not needed

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/per_tab_us...
chrome/browser/prefs/per_tab_user_pref_store.h:18: 
remove newline

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/per_tab_us...
chrome/browser/prefs/per_tab_user_pref_store.h:19:
DISALLOW_COPY_AND_ASSIGN(PerTabUserPrefStore);
#include "base/basictypes.h"

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_notif...
File chrome/browser/prefs/pref_notifier_impl.cc (right):

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_notif...
chrome/browser/prefs/pref_notifier_impl.cc:97:
pref_model_associator_->ProcessPrefChange(path);
Seems like this now duplicates the same thing in
PrefValueStore::NotifyPrefChanged(). I'm happy if you move the functionality
here, but then please remove the reference to the associator from
PrefValueStore.

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_servi...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service.cc:189: // notifications. We need to implement
a wrapper that does this.
I originally thought this would be a problem, but it actually isn't since
PrefValueStore will listen to pref changes happening in the PrefStores and
forward the notifications to the correct PrefNotifier. Hence, you can just
remove the comment :)

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service.cc:214: pref_value_store_->pref_notifier_)),
Suggestion: To avoid this cast, maybe it's better to instantiate
PrefNotifierImpl here, and have a set_pref_notifier() call on PrefValueStore?

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service.cc:216:
pref_value_store_->GetPrefStore(PrefValueStore::USER_STORE))),
These casts suck. Can we just pass on the properly-typed pointers to the
constructor?

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service.cc:219:
pref_sync_associator_(pref_value_store_->pref_sync_associator_) {
This seems weird. Pass it in a parameter?

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_value...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:173: friend class PrefService;
Ah, needed for accessing pref_sync_associator_. As mentioned in the other
comment, we should probably just remove the PrefModelAssociator reference from
PrefValueStore.

[mnaganov (inactive), 2011-11-17 16:50:58]

Thanks Avi and Mattias! After addressing your comments the patch has become much
cleaner.

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/per_tab_us...
File chrome/browser/prefs/per_tab_user_pref_store.cc (right):

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/per_tab_us...
chrome/browser/prefs/per_tab_user_pref_store.cc:6: #include
"chrome/common/pref_names.h"
On 2011/11/16 12:08:04, Mattias Nissler wrote:
> not needed.

Done.

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/per_tab_us...
File chrome/browser/prefs/per_tab_user_pref_store.h (right):

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/per_tab_us...
chrome/browser/prefs/per_tab_user_pref_store.h:9: #include <string>
On 2011/11/16 12:08:04, Mattias Nissler wrote:
> not needed

Done.

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/per_tab_us...
chrome/browser/prefs/per_tab_user_pref_store.h:18: 
On 2011/11/16 12:08:04, Mattias Nissler wrote:
> remove newline

Done.

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/per_tab_us...
chrome/browser/prefs/per_tab_user_pref_store.h:19:
DISALLOW_COPY_AND_ASSIGN(PerTabUserPrefStore);
On 2011/11/16 12:08:04, Mattias Nissler wrote:
> #include "base/basictypes.h"

Done.

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_notif...
File chrome/browser/prefs/pref_notifier_impl.cc (right):

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_notif...
chrome/browser/prefs/pref_notifier_impl.cc:97:
pref_model_associator_->ProcessPrefChange(path);
On 2011/11/16 12:08:04, Mattias Nissler wrote:
> Seems like this now duplicates the same thing in
> PrefValueStore::NotifyPrefChanged(). I'm happy if you move the functionality
> here, but then please remove the reference to the associator from
> PrefValueStore.

Right. I haven't understood that from your change. But it looks like removing an
associator from value store requires cascading changes in tests, so let's do it
as a separate change. For now, I'm not adding an associator into notifier.

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_servi...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service.cc:189: // notifications. We need to implement
a wrapper that does this.
On 2011/11/16 12:08:04, Mattias Nissler wrote:
> I originally thought this would be a problem, but it actually isn't since
> PrefValueStore will listen to pref changes happening in the PrefStores and
> forward the notifications to the correct PrefNotifier. Hence, you can just
> remove the comment :)

Done.

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service.cc:214: pref_value_store_->pref_notifier_)),
On 2011/11/16 12:08:04, Mattias Nissler wrote:
> Suggestion: To avoid this cast, maybe it's better to instantiate
> PrefNotifierImpl here, and have a set_pref_notifier() call on PrefValueStore?

I've reconsidered the approach to PrefService initialization, now it looks
prettier.

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service.cc:216:
pref_value_store_->GetPrefStore(PrefValueStore::USER_STORE))),
On 2011/11/16 12:08:04, Mattias Nissler wrote:
> These casts suck. Can we just pass on the properly-typed pointers to the
> constructor?

Fixed.

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service.cc:219:
pref_sync_associator_(pref_value_store_->pref_sync_associator_) {
On 2011/11/16 12:08:04, Mattias Nissler wrote:
> This seems weird. Pass it in a parameter?

Fixed.

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_value...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/8568019/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:173: friend class PrefService;
On 2011/11/16 12:08:04, Mattias Nissler wrote:
> Ah, needed for accessing pref_sync_associator_. As mentioned in the other
> comment, we should probably just remove the PrefModelAssociator reference from
> PrefValueStore.

Removed.

[Avi (use Gerrit), 2011-11-17 18:23:33]

A much better approach. Move things around and you'll have my OK.

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:297:
scoped_ptr<PrefService> per_tab_prefs_;
Since there is no getter, this isn't a tab helper and doesn't belong in this
section (see the comment on line 275). Move this to the observer section, line
328 or wherever it fits alphabetically, and move the allocation in the .cc file
too.

[mnaganov (inactive), 2011-11-17 18:27:00]

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:297:
scoped_ptr<PrefService> per_tab_prefs_;
On 2011/11/17 18:23:33, Avi wrote:
> Since there is no getter, this isn't a tab helper and doesn't belong in this
> section (see the comment on line 275). Move this to the observer section, line
> 328 or wherever it fits alphabetically, and move the allocation in the .cc
file
> too.

I'm not yet sure how to use this. I need to override WebKit prefs using
per_tab_prefs_ when getting WebPreferences from TabContents. It will become more
clear when I'll add plumbing to other places.

[Avi (use Gerrit), 2011-11-17 18:29:09]

My question is: will people say:

tab_contents_wrapper()->per_tab_prefs()->DoSomething()?

If there will be an accessor on TCW to get this, then leave it in this section.
As it is now it silently does its thing with no outside accessibility, and
therefore belongs in the other section.

[mnaganov (inactive), 2011-11-17 18:33:15]

On 2011/11/17 18:29:09, Avi wrote:
> My question is: will people say:
> 
> tab_contents_wrapper()->per_tab_prefs()->DoSomething()?
> 
> If there will be an accessor on TCW to get this, then leave it in this
section.
> As it is now it silently does its thing with no outside accessibility, and
> therefore belongs in the other section.

Yes, I understand what do you mean, but I don't have an answer to this question
yet. I will now move this field to the other section as you are suggesting and
then will move it back if needed.

[Mattias Nissler (ping if slow), 2011-11-17 18:44:39]

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:179: default_store_.get());
You pass in the default store as the async parameter here! Note that the default
store must be passed in for incognito since, it's the default store that
determines which prefs are registered.

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:205: default_store_(new
DefaultPrefStore()),
As I mentioned above, that'll break the incognito pref service.

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:313: bool async);
newline before comment.

[mnaganov (inactive), 2011-11-17 19:53:32]

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:179: default_store_.get());
On 2011/11/17 18:44:39, Mattias Nissler wrote:
> You pass in the default store as the async parameter here! Note that the
default
> store must be passed in for incognito since, it's the default store that
> determines which prefs are registered.

I hope no. Those two constructors differ by the number of arguments. Here the
second version should be selected.

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.cc:205: default_store_(new
DefaultPrefStore()),
On 2011/11/17 18:44:39, Mattias Nissler wrote:
> As I mentioned above, that'll break the incognito pref service.

Ditto. For the incognito mode, the second constructor should be called.

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:313: bool async);
On 2011/11/17 18:44:39, Mattias Nissler wrote:
> newline before comment.

Done.

[mnaganov (inactive), 2011-11-17 19:54:43]

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/8568019/diff/10001/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:297:
scoped_ptr<PrefService> per_tab_prefs_;
On 2011/11/17 18:27:00, Mikhail Naganov (Chromium) wrote:
> On 2011/11/17 18:23:33, Avi wrote:
> > Since there is no getter, this isn't a tab helper and doesn't belong in this
> > section (see the comment on line 275). Move this to the observer section,
line
> > 328 or wherever it fits alphabetically, and move the allocation in the .cc
> file
> > too.
> 
> I'm not yet sure how to use this. I need to override WebKit prefs using
> per_tab_prefs_ when getting WebPreferences from TabContents. It will become
more
> clear when I'll add plumbing to other places.

I have moved this into the "observers" section as you suggested.

[Mattias Nissler (ping if slow), 2011-11-17 20:08:13]

http://codereview.chromium.org/8568019/diff/11012/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/8568019/diff/11012/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:325: DefaultPrefStore* default_store);
Oh, I had totally missed that there's now a second constructor, sorry. Then
again, why do we need it? Looking at the code it seems easy to just have one
that takes all arguments?

[mnaganov (inactive), 2011-11-18 11:20:45]

http://codereview.chromium.org/8568019/diff/11012/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/8568019/diff/11012/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:325: DefaultPrefStore* default_store);
On 2011/11/17 20:08:13, Mattias Nissler wrote:
> Oh, I had totally missed that there's now a second constructor, sorry. Then
> again, why do we need it? Looking at the code it seems easy to just have one
> that takes all arguments?

The difference between them is that the first one creates a new PrefValueStore,
and the other uses CloneAndSpecialize from the existing store. The alternative
solution that was in your original change -- passing a ready PrefValueStore into
a constructor doesn't work well for TestingPrefServiceBase class which derives
from PrefService. We need to create instances of default pref store, pref
notifier and sync associator and to pass them both to PrefService itself and to
PrefValueStore (as extracting them back from PrefValueStore requires casts that
you were against of).

In your CL, you dealt only with factory functions where you can introduce local
variables:

PrefService* CreatePrefService(...) {
  PrefNotifier* pref_notifier = new PrefNotifierImpl(...);
  return new PrefService(new PrefValueStore(..., pref_notifier, ...),
pref_notifier)
}

But I don't know any elegant way to reuse the same pref notifier in a derived
constructor. I can't even use derived class fields for binding an instance
reference because the base class constructor must be called first. What I need
to be able is to write something like this:

TestingPrefServiceBase(...)
  : PrefService(new PrefValueStore(..., pref_notifier = new PrefNotifierImpl(),
...), ..., pref_notifier, ...) {

Which isn't possible.

[mnaganov (inactive), 2011-11-18 11:24:48]

http://codereview.chromium.org/8568019/diff/11012/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/8568019/diff/11012/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:325: DefaultPrefStore* default_store);
On 2011/11/18 11:20:45, Mikhail Naganov (Chromium) wrote:
> On 2011/11/17 20:08:13, Mattias Nissler wrote:
> > Oh, I had totally missed that there's now a second constructor, sorry. Then
> > again, why do we need it? Looking at the code it seems easy to just have one
> > that takes all arguments?
> 
> The difference between them is that the first one creates a new
PrefValueStore,
> and the other uses CloneAndSpecialize from the existing store. The alternative
> solution that was in your original change -- passing a ready PrefValueStore
into
> a constructor doesn't work well for TestingPrefServiceBase class which derives
> from PrefService. We need to create instances of default pref store, pref
> notifier and sync associator and to pass them both to PrefService itself and
to
> PrefValueStore (as extracting them back from PrefValueStore requires casts
that
> you were against of).
> 
> In your CL, you dealt only with factory functions where you can introduce
local
> variables:
> 
> PrefService* CreatePrefService(...) {
>   PrefNotifier* pref_notifier = new PrefNotifierImpl(...);
>   return new PrefService(new PrefValueStore(..., pref_notifier, ...),
> pref_notifier)
> }
> 
> But I don't know any elegant way to reuse the same pref notifier in a derived
> constructor. I can't even use derived class fields for binding an instance
> reference because the base class constructor must be called first. What I need
> to be able is to write something like this:
> 
> TestingPrefServiceBase(...)
>   : PrefService(new PrefValueStore(..., pref_notifier = new
PrefNotifierImpl(),
> ...), ..., pref_notifier, ...) {
> 
> Which isn't possible.

In theory, we can exploit multiple inheritance here, by using a helper class
which will stand the first in the list of base classes for
TestingPrefServiceBase and will have fields for objects that need to be passed
both to PrefValueStore and PrefService. But this seems to be ugly as well, isn't
it?

[Mattias Nissler (ping if slow), 2011-11-18 12:01:37]

http://codereview.chromium.org/8568019/diff/11012/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/8568019/diff/11012/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:325: DefaultPrefStore* default_store);
On 2011/11/18 11:20:45, Mikhail Naganov (Chromium) wrote:
> On 2011/11/17 20:08:13, Mattias Nissler wrote:
> > Oh, I had totally missed that there's now a second constructor, sorry. Then
> > again, why do we need it? Looking at the code it seems easy to just have one
> > that takes all arguments?
> 
> The difference between them is that the first one creates a new
PrefValueStore,
> and the other uses CloneAndSpecialize from the existing store. The alternative
> solution that was in your original change -- passing a ready PrefValueStore
into
> a constructor doesn't work well for TestingPrefServiceBase class which derives
> from PrefService. We need to create instances of default pref store, pref
> notifier and sync associator and to pass them both to PrefService itself and
to
> PrefValueStore (as extracting them back from PrefValueStore requires casts
that
> you were against of).
> 
> In your CL, you dealt only with factory functions where you can introduce
local
> variables:
> 
> PrefService* CreatePrefService(...) {
>   PrefNotifier* pref_notifier = new PrefNotifierImpl(...);
>   return new PrefService(new PrefValueStore(..., pref_notifier, ...),
> pref_notifier)
> }
> 
> But I don't know any elegant way to reuse the same pref notifier in a derived
> constructor. I can't even use derived class fields for binding an instance
> reference because the base class constructor must be called first. What I need
> to be able is to write something like this:
> 
> TestingPrefServiceBase(...)
>   : PrefService(new PrefValueStore(..., pref_notifier = new
PrefNotifierImpl(),
> ...), ..., pref_notifier, ...) {
> 
> Which isn't possible.

Ah, I remember :) TestingPrefServiceBase was originally created to work around
this exact problem: Passing the same newly-allocated object to two different
initializers in the constructor. TestingPrefService instantiates the PrefStores
and then passes them on to the TestingPrefServiceBase ctor which saves local
pointers as well as using them for the PrefService ctor call. So I believe you
could do exactly the same for the pointers you need multiple times? Just pass in
the PrefValueStorePointer that has PrefNotifierImpl already set and then again
in a separate ctor parameter? Maybe that doesn't work nicely though since
PrefNotifierImpl currently requires the PrefService pointer on construction.
Alternatively, maybe we can just make a PrefValueStore::set_notifier() call, so
the PrefService ctor can instantiate PrefNotifierImpl and install it on
PrefValueStore? I hope I'm not missing something again.

[mnaganov (inactive), 2011-11-18 14:35:04]

http://codereview.chromium.org/8568019/diff/11012/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/8568019/diff/11012/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:325: DefaultPrefStore* default_store);
On 2011/11/18 12:01:37, Mattias Nissler wrote:
> On 2011/11/18 11:20:45, Mikhail Naganov (Chromium) wrote:
> > On 2011/11/17 20:08:13, Mattias Nissler wrote:
> > > Oh, I had totally missed that there's now a second constructor, sorry.
Then
> > > again, why do we need it? Looking at the code it seems easy to just have
one
> > > that takes all arguments?
> > 
> > The difference between them is that the first one creates a new
> PrefValueStore,
> > and the other uses CloneAndSpecialize from the existing store. The
alternative
> > solution that was in your original change -- passing a ready PrefValueStore
> into
> > a constructor doesn't work well for TestingPrefServiceBase class which
derives
> > from PrefService. We need to create instances of default pref store, pref
> > notifier and sync associator and to pass them both to PrefService itself and
> to
> > PrefValueStore (as extracting them back from PrefValueStore requires casts
> that
> > you were against of).
> > 
> > In your CL, you dealt only with factory functions where you can introduce
> local
> > variables:
> > 
> > PrefService* CreatePrefService(...) {
> >   PrefNotifier* pref_notifier = new PrefNotifierImpl(...);
> >   return new PrefService(new PrefValueStore(..., pref_notifier, ...),
> > pref_notifier)
> > }
> > 
> > But I don't know any elegant way to reuse the same pref notifier in a
derived
> > constructor. I can't even use derived class fields for binding an instance
> > reference because the base class constructor must be called first. What I
need
> > to be able is to write something like this:
> > 
> > TestingPrefServiceBase(...)
> >   : PrefService(new PrefValueStore(..., pref_notifier = new
> PrefNotifierImpl(),
> > ...), ..., pref_notifier, ...) {
> > 
> > Which isn't possible.
> 
> Ah, I remember :) TestingPrefServiceBase was originally created to work around
> this exact problem: Passing the same newly-allocated object to two different
> initializers in the constructor. TestingPrefService instantiates the
PrefStores
> and then passes them on to the TestingPrefServiceBase ctor which saves local
> pointers as well as using them for the PrefService ctor call. So I believe you
> could do exactly the same for the pointers you need multiple times? Just pass
in
> the PrefValueStorePointer that has PrefNotifierImpl already set and then again
> in a separate ctor parameter? Maybe that doesn't work nicely though since
> PrefNotifierImpl currently requires the PrefService pointer on construction.
> Alternatively, maybe we can just make a PrefValueStore::set_notifier() call,
so
> the PrefService ctor can instantiate PrefNotifierImpl and install it on
> PrefValueStore? I hope I'm not missing something again.

I have discovered a nice trick that in constructor initializers of a derived
class you can reference already initialized fields of base class, which made
possible writing TestingPrefServiceBase constructor that passes same instances
both to PrefService and PrefValueStore. Hope you will enjoy it!

[Mattias Nissler (ping if slow), 2011-11-18 14:57:07]

http://codereview.chromium.org/8568019/diff/17001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/8568019/diff/17001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:394: // and owned by this PrefService.
Subclasses may access it for unit testing.
nit: It's not necessarily created by the PrefService any longer.

http://codereview.chromium.org/8568019/diff/17001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_mock_builder.cc (right):

http://codereview.chromium.org/8568019/diff/17001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_mock_builder.cc:117: PrefModelAssociator*
pref_sync_associator = new PrefModelAssociator();
Any reason not to make the associator NULL in this case?

http://codereview.chromium.org/8568019/diff/17001/chrome/test/base/testing_pr...
File chrome/test/base/testing_pref_service.cc (right):

http://codereview.chromium.org/8568019/diff/17001/chrome/test/base/testing_pr...
chrome/test/base/testing_pref_service.cc:25: new PrefModelAssociator(),
same here: Do we really need a non-null associator?

http://codereview.chromium.org/8568019/diff/17001/chrome/test/base/testing_pr...
chrome/test/base/testing_pref_service.cc:31:
PrefService::user_pref_store_.get(),
How can this work? The PrefValueStore ctor() run before the PrefService ctor(),
so I'd expect these references to all return NULL. We'll not segfault, but this
won't pass tests.

[mnaganov (inactive), 2011-11-21 14:52:20]

http://codereview.chromium.org/8568019/diff/17001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/8568019/diff/17001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:394: // and owned by this PrefService.
Subclasses may access it for unit testing.
On 2011/11/18 14:57:07, Mattias Nissler wrote:
> nit: It's not necessarily created by the PrefService any longer.

Done.

http://codereview.chromium.org/8568019/diff/17001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_mock_builder.cc (right):

http://codereview.chromium.org/8568019/diff/17001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_mock_builder.cc:117: PrefModelAssociator*
pref_sync_associator = new PrefModelAssociator();
On 2011/11/18 14:57:07, Mattias Nissler wrote:
> Any reason not to make the associator NULL in this case?

In the old code, only the Incognito mode constructor was passing a NULL
associator. I was trying to preserve this. Now removing.

http://codereview.chromium.org/8568019/diff/17001/chrome/test/base/testing_pr...
File chrome/test/base/testing_pref_service.cc (right):

http://codereview.chromium.org/8568019/diff/17001/chrome/test/base/testing_pr...
chrome/test/base/testing_pref_service.cc:25: new PrefModelAssociator(),
On 2011/11/18 14:57:07, Mattias Nissler wrote:
> same here: Do we really need a non-null associator?

Not sure, but I trust you. Set to NULL.

http://codereview.chromium.org/8568019/diff/17001/chrome/test/base/testing_pr...
chrome/test/base/testing_pref_service.cc:31:
PrefService::user_pref_store_.get(),
On 2011/11/18 14:57:07, Mattias Nissler wrote:
> How can this work? The PrefValueStore ctor() run before the PrefService
ctor(),
> so I'd expect these references to all return NULL. We'll not segfault, but
this
> won't pass tests.

Oh, right. Of course, call semantics requires arguments to be evaluated first. I
have added more parameters to TestingPrefServiceBase to deal with this
correctly.

[Mattias Nissler (ping if slow), 2011-11-21 15:12:37]

Prefs LGTM, please also get Avi's thumbs up for the TabContents-related changes.

http://codereview.chromium.org/8568019/diff/17001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_mock_builder.cc (right):

http://codereview.chromium.org/8568019/diff/17001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_mock_builder.cc:117: PrefModelAssociator*
pref_sync_associator = new PrefModelAssociator();
On 2011/11/21 14:52:20, Mikhail Naganov (Chromium) wrote:
> On 2011/11/18 14:57:07, Mattias Nissler wrote:
> > Any reason not to make the associator NULL in this case?
> 
> In the old code, only the Incognito mode constructor was passing a NULL
> associator. I was trying to preserve this. Now removing.

You only need the associator for syncing stuff to the cloud. Actually, having
more than one associator on the same profile (as would be the case for the
per-tab PrefService) would probably thrash values in the best case and crash the
browser in the worst case :)

[mnaganov (inactive), 2011-11-21 16:36:38]

Avi, are you OK with this change?

http://codereview.chromium.org/8568019/diff/17001/chrome/test/base/testing_pr...
File chrome/test/base/testing_pref_service.cc (right):

http://codereview.chromium.org/8568019/diff/17001/chrome/test/base/testing_pr...
chrome/test/base/testing_pref_service.cc:25: new PrefModelAssociator(),
On 2011/11/18 14:57:07, Mattias Nissler wrote:
> same here: Do we really need a non-null associator?

I have found the reason: ProfileSyncServicePreferenceTest tests require this so
I have added it back.

[Avi (use Gerrit), 2011-11-21 16:40:53]

I'm happy with the TCW changes (with the alphabetization as noted below). If you
need to eventually turn it into a helper with an accessor off the TCW, I'd be
happy with that too.

Do the sorting; you have my LGTM.

http://codereview.chromium.org/8568019/diff/27002/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/8568019/diff/27002/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:309:
profile()->GetPrefs()->CreatePrefServiceWithPerTabPrefStore());
Keep in the same order as the declarations.

http://codereview.chromium.org/8568019/diff/27002/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/8568019/diff/27002/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:327:
scoped_ptr<PluginObserver> plugin_observer_;
Alphabetize, plz.

[mnaganov (inactive), 2011-11-21 17:02:10]

Thanks!

http://codereview.chromium.org/8568019/diff/27002/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/8568019/diff/27002/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:309:
profile()->GetPrefs()->CreatePrefServiceWithPerTabPrefStore());
On 2011/11/21 16:40:54, Avi wrote:
> Keep in the same order as the declarations.

Done.

http://codereview.chromium.org/8568019/diff/27002/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/8568019/diff/27002/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:327:
scoped_ptr<PluginObserver> plugin_observer_;
On 2011/11/21 16:40:54, Avi wrote:
> Alphabetize, plz.

Sorry, I didn't realize they are sorted by type name, not by field name. Fixed.

[Peter Kasting, 2011-11-21 17:55:28]

Drive-by: This is a serious functional change, so please add a valid BUG= line,
and if manual testing is possible yet, a TEST= line as well.