Get rid of trivial IncognitoUserPrefStore and PerTabUserPrefStore.

Get rid of trivial IncognitoUserPrefStore and PerTabUserPrefStore.

As suggested here:
http://codereview.chromium.org/8879016/diff/9025/chrome/browser/prefs/per_tab_user_pref_store.cc#newcode10

R=bauerb@chromium.org
BUG=none
TEST=none

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

[Bernhard Bauer, 2011-12-30 17:54:14]

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

http://codereview.chromium.org/9015012/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service.cc:114: static void
InitIncognitoUserPrefStore(OverlayUserPrefStore* pref_store) {
You could move this into browser_prefs.cc, where the other pref registrations
are.

[mnaganov (inactive), 2011-12-30 21:32:33]

On 2011/12/30 17:54:14, Bernhard Bauer wrote:
>
http://codereview.chromium.org/9015012/diff/1/chrome/browser/prefs/pref_servi...
> File chrome/browser/prefs/pref_service.cc (right):
> 
>
http://codereview.chromium.org/9015012/diff/1/chrome/browser/prefs/pref_servi...
> chrome/browser/prefs/pref_service.cc:114: static void
> InitIncognitoUserPrefStore(OverlayUserPrefStore* pref_store) {
> You could move this into browser_prefs.cc, where the other pref registrations
> are.

This is a bit different from prefs registration. We configure the overlay
storage to map preferences. And this configuration is specific to the kind of of
PrefService we are creating.

I can agree that prefs_tab_helper might be a good place for per-tab preferences
registration, thus all per-tab preferences will be mentioned in a single file.
But I'm not sure if it's possible to make code from browser/prefs to depend on
browser/ui. I think that UI is usually on the top of component layers.

[Bernhard Bauer, 2011-12-30 23:40:28]

On 2011/12/30 21:32:33, Mikhail Naganov (Chromium) wrote:
> On 2011/12/30 17:54:14, Bernhard Bauer wrote:
> >
>
http://codereview.chromium.org/9015012/diff/1/chrome/browser/prefs/pref_servi...
> > File chrome/browser/prefs/pref_service.cc (right):
> > 
> >
>
http://codereview.chromium.org/9015012/diff/1/chrome/browser/prefs/pref_servi...
> > chrome/browser/prefs/pref_service.cc:114: static void
> > InitIncognitoUserPrefStore(OverlayUserPrefStore* pref_store) {
> > You could move this into browser_prefs.cc, where the other pref
registrations
> > are.
> 
> This is a bit different from prefs registration. We configure the overlay
> storage to map preferences. And this configuration is specific to the kind of
of
> PrefService we are creating.
> 
> I can agree that prefs_tab_helper might be a good place for per-tab
preferences
> registration, thus all per-tab preferences will be mentioned in a single file.
> But I'm not sure if it's possible to make code from browser/prefs to depend on
> browser/ui. I think that UI is usually on the top of component layers.

browser_prefs.cc already depends on lots of different components, including
browser/ui/. I'd rather keep the dependencies in a that file than adding them to
PrefService. I agree that the actual set of per-tab pref keys is best kept in
PrefsTabHelper, so maybe browser_prefs.cc could dispatch to a static method on
that class?

[mnaganov (inactive), 2012-01-05 13:33:20]

On 2011/12/30 23:40:28, Bernhard Bauer wrote:
> On 2011/12/30 21:32:33, Mikhail Naganov (Chromium) wrote:
> > On 2011/12/30 17:54:14, Bernhard Bauer wrote:
> > >
> >
>
http://codereview.chromium.org/9015012/diff/1/chrome/browser/prefs/pref_servi...
> > > File chrome/browser/prefs/pref_service.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9015012/diff/1/chrome/browser/prefs/pref_servi...
> > > chrome/browser/prefs/pref_service.cc:114: static void
> > > InitIncognitoUserPrefStore(OverlayUserPrefStore* pref_store) {
> > > You could move this into browser_prefs.cc, where the other pref
> registrations
> > > are.
> > 
> > This is a bit different from prefs registration. We configure the overlay
> > storage to map preferences. And this configuration is specific to the kind
of
> of
> > PrefService we are creating.
> > 
> > I can agree that prefs_tab_helper might be a good place for per-tab
> preferences
> > registration, thus all per-tab preferences will be mentioned in a single
file.
> > But I'm not sure if it's possible to make code from browser/prefs to depend
on
> > browser/ui. I think that UI is usually on the top of component layers.
> 
> browser_prefs.cc already depends on lots of different components, including
> browser/ui/. I'd rather keep the dependencies in a that file than adding them
to
> PrefService. I agree that the actual set of per-tab pref keys is best kept in
> PrefsTabHelper, so maybe browser_prefs.cc could dispatch to a static method on
> that class?

I have moved overlay prefs registration into prefs_tab_helper. I still can't get
why are you referring to browser_prefs.cc. Looking inside
chrome/browser/prefs/browser_prefs.cc, I only see PrefService-level
registrations there, while overlay prefs are registered on PrefStore level.
PrefService doesn't expose its PrefStores for direct manipulation, so it looks
natural to have overlay prefs initialization inside PrefService methods, not
inside browser_prefs.cc. Am I missing something?

[Bernhard Bauer, 2012-01-05 13:47:26]

LGTM

On 2012/01/05 13:33:20, Mikhail Naganov (Chromium) wrote:
> On 2011/12/30 23:40:28, Bernhard Bauer wrote:
> > On 2011/12/30 21:32:33, Mikhail Naganov (Chromium) wrote:
> > > On 2011/12/30 17:54:14, Bernhard Bauer wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/9015012/diff/1/chrome/browser/prefs/pref_servi...
> > > > File chrome/browser/prefs/pref_service.cc (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/9015012/diff/1/chrome/browser/prefs/pref_servi...
> > > > chrome/browser/prefs/pref_service.cc:114: static void
> > > > InitIncognitoUserPrefStore(OverlayUserPrefStore* pref_store) {
> > > > You could move this into browser_prefs.cc, where the other pref
> > registrations
> > > > are.
> > > 
> > > This is a bit different from prefs registration. We configure the overlay
> > > storage to map preferences. And this configuration is specific to the kind
> of
> > of
> > > PrefService we are creating.
> > > 
> > > I can agree that prefs_tab_helper might be a good place for per-tab
> > preferences
> > > registration, thus all per-tab preferences will be mentioned in a single
> file.
> > > But I'm not sure if it's possible to make code from browser/prefs to
depend
> on
> > > browser/ui. I think that UI is usually on the top of component layers.
> > 
> > browser_prefs.cc already depends on lots of different components, including
> > browser/ui/. I'd rather keep the dependencies in a that file than adding
them
> to
> > PrefService. I agree that the actual set of per-tab pref keys is best kept
in
> > PrefsTabHelper, so maybe browser_prefs.cc could dispatch to a static method
on
> > that class?
> 
> I have moved overlay prefs registration into prefs_tab_helper. 

OK, that's fine as well.

> I still can't get
> why are you referring to browser_prefs.cc. Looking inside
> chrome/browser/prefs/browser_prefs.cc, I only see PrefService-level
> registrations there, while overlay prefs are registered on PrefStore level.
> PrefService doesn't expose its PrefStores for direct manipulation, so it looks
> natural to have overlay prefs initialization inside PrefService methods, not
> inside browser_prefs.cc. Am I missing something?

I only suggested browser_prefs.cc because it already knows about other pref
names, but I just wanted to keep them out of PrefService itself, as I would like
to keep PrefService as general as possible.

[mnaganov (inactive), 2012-01-05 16:45:25]

Peter, an OWNER's approval is required for files in chrome/browser/ui. Please
take a look.

On 2012/01/05 13:47:26, Bernhard Bauer wrote:
> LGTM
> 
> On 2012/01/05 13:33:20, Mikhail Naganov (Chromium) wrote:
> > On 2011/12/30 23:40:28, Bernhard Bauer wrote:
> > > On 2011/12/30 21:32:33, Mikhail Naganov (Chromium) wrote:
> > > > On 2011/12/30 17:54:14, Bernhard Bauer wrote:
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/9015012/diff/1/chrome/browser/prefs/pref_servi...
> > > > > File chrome/browser/prefs/pref_service.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/9015012/diff/1/chrome/browser/prefs/pref_servi...
> > > > > chrome/browser/prefs/pref_service.cc:114: static void
> > > > > InitIncognitoUserPrefStore(OverlayUserPrefStore* pref_store) {
> > > > > You could move this into browser_prefs.cc, where the other pref
> > > registrations
> > > > > are.
> > > > 
> > > > This is a bit different from prefs registration. We configure the
overlay
> > > > storage to map preferences. And this configuration is specific to the
kind
> > of
> > > of
> > > > PrefService we are creating.
> > > > 
> > > > I can agree that prefs_tab_helper might be a good place for per-tab
> > > preferences
> > > > registration, thus all per-tab preferences will be mentioned in a single
> > file.
> > > > But I'm not sure if it's possible to make code from browser/prefs to
> depend
> > on
> > > > browser/ui. I think that UI is usually on the top of component layers.
> > > 
> > > browser_prefs.cc already depends on lots of different components,
including
> > > browser/ui/. I'd rather keep the dependencies in a that file than adding
> them
> > to
> > > PrefService. I agree that the actual set of per-tab pref keys is best kept
> in
> > > PrefsTabHelper, so maybe browser_prefs.cc could dispatch to a static
method
> on
> > > that class?
> > 
> > I have moved overlay prefs registration into prefs_tab_helper. 
> 
> OK, that's fine as well.
> 
> > I still can't get
> > why are you referring to browser_prefs.cc. Looking inside
> > chrome/browser/prefs/browser_prefs.cc, I only see PrefService-level
> > registrations there, while overlay prefs are registered on PrefStore level.
> > PrefService doesn't expose its PrefStores for direct manipulation, so it
looks
> > natural to have overlay prefs initialization inside PrefService methods, not
> > inside browser_prefs.cc. Am I missing something?
> 
> I only suggested browser_prefs.cc because it already knows about other pref
> names, but I just wanted to keep them out of PrefService itself, as I would
like
> to keep PrefService as general as possible.

[Peter Kasting, 2012-01-05 17:23:42]

chrome/browser/ui LGTM

http://codereview.chromium.org/9015012/diff/6001/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

http://codereview.chromium.org/9015012/diff/6001/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:384: // per-profile preference on a
per-tab pref store delegates to underlay
Nit: "underlay" -> "the underlying"

[mnaganov (inactive), 2012-01-05 17:31:30]

http://codereview.chromium.org/9015012/diff/6001/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

http://codereview.chromium.org/9015012/diff/6001/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:384: // per-profile preference on a
per-tab pref store delegates to underlay
On 2012/01/05 17:23:43, Peter Kasting wrote:
> Nit: "underlay" -> "the underlying"

Done.