Remove dependency of PrefSyncableService on Profile.

Remove dependency of PrefSyncableService on Profile.

In preparation of componentization of PrefSyncableService (with the goal
of sharing the code with iOS embedder), remove the dependency on Profile
by converting the two helper static method to free functions and moving
them to chrome/browser/prefs/pref_service_syncable_util.{cc,h} that will
stay in chrome/browser/prefs.

BUG=522536
TBR=thakis@chromium.org

Committed: https://crrev.com/ed27d86b3de3a37060a8887475fd019bdb66bbd8
Cr-Commit-Position: refs/heads/master@{#348600}

[sdefresne, 2015-09-08 20:31:33]

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

[sdefresne, 2015-09-08 20:31:34]

blundell: FYI

gab/mnissler: could one of you review this CL? It componentize all the
pref_syncable code from //chrome/browser/prefs into //components/syncable_prefs
(with their unittests).

This depends on removing the code to support the migration of
"session.urls_to_restore_on_startup" but depending on the discussion offline, I
may change PrefModelAssociatorRegistry to also expose the pair old/new pref name
so that I don't block the componentization of that code on the removal of that
preference (as we have a goal to unfork iOS code by EOY).

Thank you.

[sdefresne, 2015-09-08 20:32:48]

On 2015/09/08 at 20:31:34, sdefresne wrote:
> blundell: FYI
> 
> gab/mnissler: could one of you review this CL? It componentize all the
pref_syncable code from //chrome/browser/prefs into //components/syncable_prefs
(with their unittests).
> 
> This depends on removing the code to support the migration of
"session.urls_to_restore_on_startup" but depending on the discussion offline, I
may change PrefModelAssociatorRegistry to also expose the pair old/new pref name
so that I don't block the componentization of that code on the removal of that
preference (as we have a goal to unfork iOS code by EOY).
> 
> Thank you.

Forgot to say that you should concentrate on:
- chrome/browser/prefs
- components/syncable_prefs
- chrome/browser/profiles
- chrome/test/base

as the rest is mostly just mechanical change to adapt client code to the API
changes.

[gab, 2015-09-08 21:53:48]

On 2015/09/08 20:32:48, sdefresne wrote:
> On 2015/09/08 at 20:31:34, sdefresne wrote:
> > blundell: FYI
> > 
> > gab/mnissler: could one of you review this CL? It componentize all the
> pref_syncable code from //chrome/browser/prefs into
//components/syncable_prefs
> (with their unittests).
> > 
> > This depends on removing the code to support the migration of
> "session.urls_to_restore_on_startup" but depending on the discussion offline,
I
> may change PrefModelAssociatorRegistry to also expose the pair old/new pref
name
> so that I don't block the componentization of that code on the removal of that
> preference (as we have a goal to unfork iOS code by EOY).
> > 
> > Thank you.
> 
> Forgot to say that you should concentrate on:
> - chrome/browser/prefs
> - components/syncable_prefs
> - chrome/browser/profiles
> - chrome/test/base
> 
> as the rest is mostly just mechanical change to adapt client code to the API
> changes.

[gab, 2015-09-08 21:55:55]

On 2015/09/08 21:53:48, gab -- OOO sick child wrote:
> On 2015/09/08 20:32:48, sdefresne wrote:
> > On 2015/09/08 at 20:31:34, sdefresne wrote:
> > > blundell: FYI
> > > 
> > > gab/mnissler: could one of you review this CL? It componentize all the
> > pref_syncable code from //chrome/browser/prefs into
> //components/syncable_prefs
> > (with their unittests).
> > > 
> > > This depends on removing the code to support the migration of
> > "session.urls_to_restore_on_startup" but depending on the discussion
offline,
> I
> > may change PrefModelAssociatorRegistry to also expose the pair old/new pref
> name
> > so that I don't block the componentization of that code on the removal of
that
> > preference (as we have a goal to unfork iOS code by EOY).
> > > 
> > > Thank you.
> > 
> > Forgot to say that you should concentrate on:
> > - chrome/browser/prefs
> > - components/syncable_prefs
> > - chrome/browser/profiles
> > - chrome/test/base
> > 
> > as the rest is mostly just mechanical change to adapt client code to the API
> > changes.

Is there any way we can move things in smaller chunks?

Can we avoid having the client code side-effects in the same CL (e.g., move
headers first?)

Something like a CL per header being moved (with a reference in description to
the final CL with the .cc files showing the actual move without all the cruft)

Not sure if there's precedent for this in componentization land?

Thanks,
Gab

[sdefresne, 2015-09-09 09:58:51]

Patchset #2 (id:20001) has been deleted

[sdefresne, 2015-09-09 09:58:56]

Patchset #2 (id:40001) has been deleted

[sdefresne, 2015-09-09 11:26:40]

Patchset #2 (id:60001) has been deleted

[sdefresne, 2015-09-09 11:46:41]

Patchset #2 (id:80001) has been deleted

[sdefresne, 2015-09-09 11:50:52]

I've split the CL in two.

This first CL just clean the dependency on PrefModelAssociator,
PrefServiceSyncable, PrefServiceSyncableFactory and test classes on //chrome but
introducing a client for PrefModelAssociator and refactoring how some of the
calls are made (i.e. using injection for policy and a callback for
TabsPrefHelper).

The second CL (https://codereview.chromium.org/1305213009) create the components
and move all the files without introducing any code change (there is some
circular dependencies between PrefModelAssociator and PrefServiceSyncable that
make it hard to componentize the files individually).

WDYT?

[gab, 2015-09-10 14:15:08]

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

[gab, 2015-09-10 14:15:10]

lg overall, easier to follow now that refactoring is first done in place prior
to the actual file move.

This is still a bit too big for my liking though. I'd say this CL is really 4
CLs:
 1) Introduce pref_service_syncable_util.h and side-effects.
 2) Update PrefServiceFactorySyncable's interface and side-effects.
 3) PrefModelAssociatorClient
 4) CreateIncognitoPrefService interface change.

I dropped some comments below but will do a deeper review of each separate
change post split.

Reviewing this I've also come to realize that while I'm familiar with pref code
from PrefService to prefs/tracked/ through factories/etc., I'm not very familiar
with PrefServiceSyncable et al (which is the main focus here..).

@Bernhard: would you be a better reviewer for the PrefServiceSyncable code?

Thanks!
Gab

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/c...
File chrome/browser/prefs/chrome_pref_model_associator_client.cc (right):

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/c...
chrome/browser/prefs/chrome_pref_model_associator_client.cc:23:
content_settings::WebsiteSettingsRegistry* registry =
const& registry = *...GetInstance();

and then use it as a const& without needing to deref below?

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_model_associator.cc:225: PrefModelAssociatorClient*
pref_model_associator_client) {
Can we make this a member method and have it use |pref_model_associator_client_|
directly? Looks like the only callers are this class and its unittest.

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_model_associator.h (right):

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_model_associator.h:189: PrefModelAssociatorClient*
pref_model_associator_client_;
Should this be a const*?

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_model_associator_client.h (right):

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_model_associator_client.h:14: enum
PrefModelAssociatorPreferenceType {
Make this an enum class (or move it inside the PrefModelAssociatorClient's
class) and then make the value's names much shorter.

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_model_associator_client.h:27: // synchronisation
instead of being merged.
"Should be merged instead of being merged"..? Feels like this comment is missing
something?

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_model_associator_client.h:28: void
RegisterPreference(const std::string& pref_name,
Pref registration is already a separate concept in prefs and using it here will
be confusing to readers familiar with pref code.

Suggest something more specific like MarkMergedPreference()?

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_model_associator_client.h:32: // service value during
synchronisation instead of being merged, and its
s/service/server/

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_model_associator_client.h:32: // service value during
synchronisation instead of being merged, and its
Once again I don't see what you mean by "instead of being merged" here?

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_model_associator_client.h:32: // service value during
synchronisation instead of being merged, and its
s/, and/as well as/

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_model_associator_client.h:38: std::map<std::string,
PrefModelAssociatorPreferenceType> pref_registry_;
PrefRegistry is already a separate concept in prefs and using it here will be
confusing to readers familiar with pref code.

Suggest a more specific name like |merged_preferences_types_|?

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_model_associator_unittest.cc (right):

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_model_associator_unittest.cc:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
Not sure why this file is marked 'D' by reitveld?!

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_model_associator_unittest.cc:38: pref_service_ =
factory.CreateSyncable(pref_registry.get());
We're trying to move towards a world where all registrations happen first and
then ownership of the PrefRegistry is handed to |PrefService| (i.e. no dynamic
registration).

In that light, please make all registrations before creating the PrefService.

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_model_associator_unittest.cc:468:
TEST_F(IndividualPreferenceMergeTest, URLsToRestoreOnStartup) {
Change name of this test to reflect its new generic nature.

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_service_syncable.h:42: PrefModelAssociatorClient*
pref_model_associator_client);
Feels this should be a constructor input (since it's (1) not optional, (2) not
set multiple times, and (3) there is no cyclic dependency forcing it to be
created after).

This would also allow other two classes depending on it to take it in there
constructor (since this class builds them).

It would also prevent programming mistakes (i.e. forgetting to set it on a
subclass, etc.).

I'd expect it to be set on PrefServiceSyncableFactory (just like other set_foo()
calls on PrefServiceFactory) and then passed to the PrefServiceSyncable()
constructor in CreateSyncable().

ref:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre...

(reviewing further I see you already added it to the factory, so it's only a
made of changing from a setter to a constructor argument IMO unless I'm missing
a use case?)

https://codereview.chromium.org/1326353002/diff/100001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_service_syncable.h:47: // incognito windows).
Update comment.

[sdefresne, 2015-09-10 17:09:20]

I've started the split. Can you review this first CL that only introduce
pref_service_syncable_util.{cc,h}?

[gab, 2015-09-11 11:31:16]

lgtm, thanks

[sdefresne, 2015-09-14 09:57:17]

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

[sdefresne, 2015-09-14 09:57:18]

TBR-ing thakis for mechanical changes to client after API change was approved by
OWNERS for
  chrome/browser/extensions/external_pref_loader.cc
  chrome/browser/notifications/extension_welcome_notification.cc
  chrome/browser/notifications/extension_welcome_notification_unittest.cc
  chrome/browser/profiles/off_the_record_profile_impl.cc
  chrome/browser/sync/chrome_sync_client.cc

[sdefresne, 2015-09-14 09:57:22]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-09-14 09:57:23]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/1326353002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-14 09:57:25]

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

[commit-bot: I haz the power, 2015-09-14 10:08:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-14 10:08:29]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sdefresne, 2015-09-14 10:12:41]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-09-14 10:12:41]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/1326353002/#ps180001
(title: "Fixing ChromeOS and Linux compilation")

[commit-bot: I haz the power, 2015-09-14 10:12:48]

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

[commit-bot: I haz the power, 2015-09-14 11:02:47]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2015-09-14 11:03:33]

Patchset 6 (id:??) landed as
https://crrev.com/ed27d86b3de3a37060a8887475fd019bdb66bbd8
Cr-Commit-Position: refs/heads/master@{#348600}

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

Patchset 6 (id:??) landed as
https://crrev.com/ed27d86b3de3a37060a8887475fd019bdb66bbd8
Cr-Commit-Position: refs/heads/master@{#348600}