[Sync] Add support for immediately enabling wallet datatype

[Sync] Add support for immediately enabling/disabling wallet datatype

This patch modifies the wallet datatype to always have a datatype controller,
and give that datatype controller the responsibility of enabling and disabling
the datatype as necessary. If the sync experiment pref is disabled, the datatype
will not start up, and if it's already started will turn itself off. Similarly, if the
experiment is enabled, the datatype is capable of starting itself up.

BUG=470362
Committed: https://crrev.com/e62811208dcf694cb28c24b87eda1570fb45a06e
Cr-Commit-Position: refs/heads/master@{#322274}

[Nicolas Zea, 2015-03-25 00:02:33]

zea@chromium.org changed reviewers:
+ brettw@chromium.org, maniscalco@chromium.org, pvalenzuela@chromium.org

[Nicolas Zea, 2015-03-25 00:02:34]

+Patrick for UniqueClientEntity change
+Nick for everything else

+Brett FYI, as this overlaps with what we discussed w.r.t. disabling the sync
datatype on the fly. I decided if we're enabling automatically we should also be
able to disable immediately via the experiment too.

[pval...(no longer on Chromium), 2015-03-25 00:09:10]

test/FakeServer change LGTM

[maniscalco, 2015-03-25 00:31:48]

https://codereview.chromium.org/1024553010/diff/60001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/autofill_wallet_data_type_controller.cc (right):

https://codereview.chromium.org/1024553010/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/autofill_wallet_data_type_controller.cc:34:
pref_registrar_.Init(profile->GetPrefs());
We're on the UI thread here, right?  How about adding a...

DCHECK(content::BrowserThread::CurrentlyOn(BrowserThread::UI));

https://codereview.chromium.org/1024553010/diff/60001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (left):

https://codereview.chromium.org/1024553010/diff/60001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:1183: DVLOG(2) << "Enabling types: "
<< syncer::ModelTypeSetToString(to_register);
Was autofill wallet the only type for which we wanted the ability to enable via
experiment?

https://codereview.chromium.org/1024553010/diff/60001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/1024553010/diff/60001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:1157: //
profile()->GetPrefs()->SetBoolean(
Is this intentionally commented out or should it be remove entirely?

https://codereview.chromium.org/1024553010/diff/60001/components/sync_driver/...
File components/sync_driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/1024553010/diff/60001/components/sync_driver/...
components/sync_driver/data_type_manager_impl.cc:263: State old_state = state_;
Opportunity for const,

const State old_state = state_;

https://codereview.chromium.org/1024553010/diff/60001/components/sync_driver/...
components/sync_driver/data_type_manager_impl.cc:264: state_ = DOWNLOAD_PENDING;
So is it important that we set state_ to DOWNLOAD_PENDING *before* we call
NotifyStart() ?

[Nicolas Zea, 2015-03-25 00:41:17]

PTAL

https://codereview.chromium.org/1024553010/diff/60001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/autofill_wallet_data_type_controller.cc (right):

https://codereview.chromium.org/1024553010/diff/60001/chrome/browser/sync/glu...
chrome/browser/sync/glue/autofill_wallet_data_type_controller.cc:34:
pref_registrar_.Init(profile->GetPrefs());
On 2015/03/25 00:31:47, maniscalco wrote:
> We're on the UI thread here, right?  How about adding a...
> 
> DCHECK(content::BrowserThread::CurrentlyOn(BrowserThread::UI));

Done.

https://codereview.chromium.org/1024553010/diff/60001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (left):

https://codereview.chromium.org/1024553010/diff/60001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:1183: DVLOG(2) << "Enabling types: "
<< syncer::ModelTypeSetToString(to_register);
On 2015/03/25 00:31:47, maniscalco wrote:
> Was autofill wallet the only type for which we wanted the ability to enable
via
> experiment?

Yes. This code was effectively dead code previously, being kept around in case
we wanted to bring this logic back. In the meantime though we've greatly
improved the ability of datatype controllers to control their own destiny, so
this can be removed.

https://codereview.chromium.org/1024553010/diff/60001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/1024553010/diff/60001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:1157: //
profile()->GetPrefs()->SetBoolean(
On 2015/03/25 00:31:48, maniscalco wrote:
> Is this intentionally commented out or should it be remove entirely?

Blarg, this was from manual testing before uploading. Brought back.

https://codereview.chromium.org/1024553010/diff/60001/components/sync_driver/...
File components/sync_driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/1024553010/diff/60001/components/sync_driver/...
components/sync_driver/data_type_manager_impl.cc:263: State old_state = state_;
On 2015/03/25 00:31:48, maniscalco wrote:
> Opportunity for const,
> 
> const State old_state = state_;

Done.

https://codereview.chromium.org/1024553010/diff/60001/components/sync_driver/...
components/sync_driver/data_type_manager_impl.cc:264: state_ = DOWNLOAD_PENDING;
On 2015/03/25 00:31:48, maniscalco wrote:
> So is it important that we set state_ to DOWNLOAD_PENDING *before* we call
> NotifyStart() ?

Yes, as NotifyStart() can trigger ProfileSyncServiceObservers, which might be
checking GetActiveDataTypes, which looks at the configuration state (the tests
in this patch uncovered this issue). Added a comment explaining that further.

[maniscalco, 2015-03-25 00:47:54]

LGTM

https://codereview.chromium.org/1024553010/diff/60001/components/sync_driver/...
File components/sync_driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/1024553010/diff/60001/components/sync_driver/...
components/sync_driver/data_type_manager_impl.cc:264: state_ = DOWNLOAD_PENDING;
On 2015/03/25 00:41:17, Nicolas Zea wrote:
> On 2015/03/25 00:31:48, maniscalco wrote:
> > So is it important that we set state_ to DOWNLOAD_PENDING *before* we call
> > NotifyStart() ?
> 
> Yes, as NotifyStart() can trigger ProfileSyncServiceObservers, which might be
> checking GetActiveDataTypes, which looks at the configuration state (the tests
> in this patch uncovered this issue). Added a comment explaining that further.

Got it.  That's a helpful comment.  Thanks for adding it.

[Nicolas Zea, 2015-03-25 04:24:33]

Patchset #7 (id:120001) has been deleted

[Nicolas Zea, 2015-03-25 04:24:42]

Patchset #6 (id:100001) has been deleted

[Nicolas Zea, 2015-03-25 19:12:35]

zea@chromium.org changed reviewers:
+ nyquist@chromium.org

[Nicolas Zea, 2015-03-25 19:12:36]

+Tommy for PSSAndroid changes

[brettw, 2015-03-25 19:34:37]

I don't think you need my review for anything, but I looked briefly and LGTM.
Thanks for getting this so quickly!

[nyquist, 2015-03-26 00:07:02]

lgtm

https://codereview.chromium.org/1024553010/diff/160001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/1024553010/diff/160001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_android.cc:67: AUTOFILL_WALLET = 1 <<
15,
Any reason why this isn't called AUTOFILL_WALLET_DATA?

[Nicolas Zea, 2015-03-26 00:09:34]

https://codereview.chromium.org/1024553010/diff/160001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/1024553010/diff/160001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_android.cc:67: AUTOFILL_WALLET = 1 <<
15,
On 2015/03/26 00:07:01, nyquist wrote:
> Any reason why this isn't called AUTOFILL_WALLET_DATA?

I think it needs to match the notification type, which is "autofill_wallet". The
sync C++ model type name should probably just be autofill_wallet too, but that's
a separate issue.

[Nicolas Zea, 2015-03-26 00:09:38]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2015-03-26 00:09:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
pvalenzuela@chromium.org, maniscalco@chromium.org
Link to the patchset: https://codereview.chromium.org/1024553010/#ps160001
(title: "Fix migration and android tests")

[commit-bot: I haz the power, 2015-03-26 00:10:46]

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

[commit-bot: I haz the power, 2015-03-26 00:30:40]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-03-26 00:31:23]

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