Remove dependency of PrefSyncableService on PrefsTabHelper.

Remove dependency of PrefSyncableService on PrefsTabHelper.

In preparation of the componentization of PrefSyncableService (with the
goal of sharing the code on iOS), remove the dependency on PrefsTabHelper
by passing a list of preferences to register as overlaid.

Add a helper method in chrome/browser/prefspref_service_syncable_util.cc
to pass the list of preferences previously registered in PrefsTabHelper.

Remove the method PrefsTabHelper::InitIncognitoUserPrefStore().

BUG=522536
TBR=jochen@chromium.org

Committed: https://crrev.com/0b1722f004e4424b18585a29234408b7112815cf
Cr-Commit-Position: refs/heads/master@{#348656}

[sdefresne, 2015-09-11 10:01:54]

sdefresne@chromium.org changed reviewers:
+ bauerb@chromium.org, gab@chromium.org

[sdefresne, 2015-09-11 10:01:55]

Second CL in preparation of componentization of PrefSyncableService (split from
https://codereview.chromium.org/1326353002/).

gab/bauerb: can one of you review this?

[gab, 2015-09-11 12:58:32]

Overall I find that having this API take a callback which is the same for all
callers is fairly unfriendly.

Overall looks like InitIncognitoUserPrefStore() doesn't really need to be on
PrefsTabsHelper (it doesn't do much and doesn't really tie into other logic in
that class).

Perhaps PrefServiceSyncable::CreateIncognitoPrefService() should simply take a
vector of overlayed pref names? And then we could have
PrefsTabHelper::CreateIncognitoPrefService(PrefService*) for users that want to
get PrefsTabHelper's version of incognito prefs? (this also probably doesn't
belong on PrefsTabHelper overall as it has nothing to do with per-tab state...).

@bauerb: I'll leave it up to you to make a call here as I'm not versed in
PrefServiceSyncable.

Thanks,
Gab

https://codereview.chromium.org/1332283003/diff/20001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/1332283003/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable.cc:74: if
(!incognito_pref_created_cb.is_null())
This wasn't previously optional. Enforce it by contract and DCHECK that it's
set?

https://codereview.chromium.org/1332283003/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable.cc:79: ->ForkForIncognito();
Weird formatting... preferred old one: same number of lines, more readable IMO..

https://codereview.chromium.org/1332283003/diff/20001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/1332283003/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable.h:15: class
PrefModelAssociatorClient;
This fwd-decl doesn't belong in this CL yet.

[Bernhard Bauer, 2015-09-11 16:17:43]

On 2015/09/11 12:58:32, gab wrote:
> Overall I find that having this API take a callback which is the same for all
> callers is fairly unfriendly.
> 
> Overall looks like InitIncognitoUserPrefStore() doesn't really need to be on
> PrefsTabsHelper (it doesn't do much and doesn't really tie into other logic in
> that class).
> 
> Perhaps PrefServiceSyncable::CreateIncognitoPrefService() should simply take a
> vector of overlayed pref names? And then we could have
> PrefsTabHelper::CreateIncognitoPrefService(PrefService*) for users that want
to
> get PrefsTabHelper's version of incognito prefs? (this also probably doesn't
> belong on PrefsTabHelper overall as it has nothing to do with per-tab
state...).
> 
> @bauerb: I'll leave it up to you to make a call here as I'm not versed in
> PrefServiceSyncable.

Yes, I like that suggestion.

[sdefresne, 2015-09-14 13:17:26]

bauerb: please take another look.
gab: for your information.

I've updated the CreateIncognitoPrefService() to take a list of preference name
to register as overlaid as suggested. Thank you, this makes the code much
cleaner.

https://codereview.chromium.org/1332283003/diff/20001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/1332283003/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable.cc:74: if
(!incognito_pref_created_cb.is_null())
On 2015/09/11 at 12:58:32, gab wrote:
> This wasn't previously optional. Enforce it by contract and DCHECK that it's
set?

This is commented out in the downstream iOS fork (as we do not build
PrefsTabHelper on that platform), and this is why I made it optional. I've
changed to use a std::vector<const char*> of preference names to overlay. Since
we may want to pass an empty vector, I've not added a DCHECK() that the vector
is empty.

https://codereview.chromium.org/1332283003/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable.cc:79: ->ForkForIncognito();
On 2015/09/11 at 12:58:32, gab wrote:
> Weird formatting... preferred old one: same number of lines, more readable
IMO..

Reverted.

https://codereview.chromium.org/1332283003/diff/20001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/1332283003/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable.h:15: class
PrefModelAssociatorClient;
On 2015/09/11 at 12:58:32, gab wrote:
> This fwd-decl doesn't belong in this CL yet.

Ack.

[Bernhard Bauer, 2015-09-14 14:33:39]

LGTM, thanks!

[gab, 2015-09-14 16:20:10]

lgtm % comment and naming nits.

Thanks!
Gab

https://codereview.chromium.org/1332283003/diff/40001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/1332283003/diff/40001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable.h:42: // cannot be changed in the
incognito preferences.
s/that cannot be changed in the incognito preferences/whose changes will not be
persisted by the returned incognito pref service/

https://codereview.chromium.org/1332283003/diff/40001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_syncable_util.h (right):

https://codereview.chromium.org/1332283003/diff/40001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable_util.h:24: // Creates an incognito
copy of the pref service that shares most pref stores
s/shares most pref stores/shares most prefs/

https://codereview.chromium.org/1332283003/diff/40001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable_util.h:24: // Creates an incognito
copy of the pref service that shares most pref stores
s/the pref service/|pref_service|/

https://codereview.chromium.org/1332283003/diff/40001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable_util.h:30: PrefStore*
incognito_extension_prefs);
I know this was there prior to your change, but could we
s/incognito_extensions_prefs/incognito_extension_pref_store/ here and elsewhere?

It took me some time to realize what this was otherwise.

[sdefresne, 2015-09-14 17:28:29]

sdefresne@chromium.org changed reviewers:
+ jochen@chromium.org

[sdefresne, 2015-09-14 17:28:29]

TBR-ing jochen for mechanical change to client code after API change approved by
OWNERS
  chrome/browser/extensions/test_extension_prefs.cc
  chrome/browser/profiles/profile_impl.cc
  chrome/test/base/testing_profile.cc

https://codereview.chromium.org/1332283003/diff/40001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/1332283003/diff/40001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable.h:42: // cannot be changed in the
incognito preferences.
On 2015/09/14 at 16:20:10, gab wrote:
> s/that cannot be changed in the incognito preferences/whose changes will not
be persisted by the returned incognito pref service/

Done.

https://codereview.chromium.org/1332283003/diff/40001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_syncable_util.h (right):

https://codereview.chromium.org/1332283003/diff/40001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable_util.h:24: // Creates an incognito
copy of the pref service that shares most pref stores
On 2015/09/14 at 16:20:10, gab wrote:
> s/shares most pref stores/shares most prefs/

Done and done.

https://codereview.chromium.org/1332283003/diff/40001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable_util.h:30: PrefStore*
incognito_extension_prefs);
On 2015/09/14 at 16:20:10, gab wrote:
> I know this was there prior to your change, but could we
s/incognito_extensions_prefs/incognito_extension_pref_store/ here and elsewhere?
> 
> It took me some time to realize what this was otherwise.

Done.

[sdefresne, 2015-09-14 17:28:41]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-09-14 17:28:42]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
gab@chromium.org
Link to the patchset: https://codereview.chromium.org/1332283003/#ps60001
(title: "Fix grammar error and rename incognito_extensions_prefs to
incognito_extensions_prefs_store")

[commit-bot: I haz the power, 2015-09-14 17:30:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1332283003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1332283003/60001

[commit-bot: I haz the power, 2015-09-14 18:12:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-14 18:13:01]

Patchset 4 (id:??) landed as
https://crrev.com/0b1722f004e4424b18585a29234408b7112815cf
Cr-Commit-Position: refs/heads/master@{#348656}

[commit-bot: I haz the power, 2015-09-23 12:34:24]

Patchset 4 (id:??) landed as
https://crrev.com/0b1722f004e4424b18585a29234408b7112815cf
Cr-Commit-Position: refs/heads/master@{#348656}