Remove kAutofillWalletSyncExperimentEnabled.

Remove kAutofillWalletSyncExperimentEnabled.

This sync experiment has been at 100% of all users for a while.

Also, add the autofill_wallet type to sync/tools/testserver/chromiumsync.py. Previously, AutofillWalletDataTypeController::IsEnabled was returning false in the test environment because the sync setting was false. Now that we enable by default, the sync integration tests are covering this type. Which I assume is a good thing.

BUG=594182
Committed: https://crrev.com/04c8fd5e7c57f75262f477e6ac94e18ca76b6371
Cr-Commit-Position: refs/heads/master@{#381304}

[Justin Donnelly, 2016-03-11 22:38:32]

Patchset #1 (id:1) has been deleted

[Justin Donnelly, 2016-03-14 17:09:48]

Patchset #2 (id:40001) has been deleted

[Justin Donnelly, 2016-03-14 19:45:02]

Description was changed from

==========
Remove kAutofillWalletSyncExperimentEnabled.

This sync experiment has been at 100% of all users for a while.

BUG=594182
==========

to

==========
Remove kAutofillWalletSyncExperimentEnabled.

This sync experiment has been at 100% of all users for a while.

Also, add the autofill_wallet type to sync/tools/testserver/chromiumsync.py.
Previously, AutofillWalletDataTypeController::IsEnabled was returning false in
the test environment because the sync setting was false. Now that we enable by
default, the sync integration tests are covering this type. Which I assume is a
good thing.

BUG=594182
==========

[Justin Donnelly, 2016-03-14 20:02:38]

Patchset #1 (id:20001) has been deleted

[Justin Donnelly, 2016-03-14 20:02:58]

Patchset #1 (id:60001) has been deleted

[Justin Donnelly, 2016-03-14 20:03:09]

Patchset #1 (id:80001) has been deleted

[Justin Donnelly, 2016-03-14 20:20:22]

jdonnelly@chromium.org changed reviewers:
+ estade@chromium.org, zea@chromium.org

[Nicolas Zea, 2016-03-14 22:44:51]

Could you also update experiment_specifics.proto to note that the field is now
obsolete as of m51?

[Nicolas Zea, 2016-03-14 22:45:47]

Also src/sync/internal_api/public/util/sync_experiments.h

[Evan Stade, 2016-03-14 23:01:58]

https://codereview.chromium.org/1785923010/diff/100001/chrome/browser/autofil...
File chrome/browser/autofill/options_util.cc (left):

https://codereview.chromium.org/1785923010/diff/100001/chrome/browser/autofil...
chrome/browser/autofill/options_util.cc:15: bool
WalletIntegrationAvailableForProfile(Profile* profile) {
it doesn't seem like this is actually used anywhere

https://codereview.chromium.org/1785923010/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_experiments.cc (right):

https://codereview.chromium.org/1785923010/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_experiments.cc:80: }
nit: no curlies

https://codereview.chromium.org/1785923010/diff/100001/components/autofill/co...
File components/autofill/core/browser/options_util.cc (right):

https://codereview.chromium.org/1785923010/diff/100001/components/autofill/co...
components/autofill/core/browser/options_util.cc:13:
sync_service->GetPreferredDataTypes().Has(syncer::AUTOFILL_PROFILE);
so why would it not have this now? users with a google account but no wallet?
can you add a comment perhaps?

[Justin Donnelly, 2016-03-15 17:08:30]

On 2016/03/14 22:44:51, Nicolas Zea wrote:
> Could you also update experiment_specifics.proto to note that the field is now
> obsolete as of m51?

Done

[Justin Donnelly, 2016-03-15 17:09:23]

On 2016/03/14 22:45:47, Nicolas Zea wrote:
> Also src/sync/internal_api/public/util/sync_experiments.h

Done

[Justin Donnelly, 2016-03-15 17:09:45]

https://codereview.chromium.org/1785923010/diff/100001/chrome/browser/autofil...
File chrome/browser/autofill/options_util.cc (left):

https://codereview.chromium.org/1785923010/diff/100001/chrome/browser/autofil...
chrome/browser/autofill/options_util.cc:15: bool
WalletIntegrationAvailableForProfile(Profile* profile) {
On 2016/03/14 23:01:58, Evan Stade wrote:
> it doesn't seem like this is actually used anywhere

Woohoo! More code to delete. :)

https://codereview.chromium.org/1785923010/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_experiments.cc (right):

https://codereview.chromium.org/1785923010/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_experiments.cc:80: }
On 2016/03/14 23:01:58, Evan Stade wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/1785923010/diff/100001/components/autofill/co...
File components/autofill/core/browser/options_util.cc (right):

https://codereview.chromium.org/1785923010/diff/100001/components/autofill/co...
components/autofill/core/browser/options_util.cc:13:
sync_service->GetPreferredDataTypes().Has(syncer::AUTOFILL_PROFILE);
On 2016/03/14 23:01:58, Evan Stade wrote:
> so why would it not have this now? users with a google account but no wallet?
> can you add a comment perhaps?

This indicates whether the user has the autofill sync type enabled. But it turns
out we don't use this, either. (Ever since we changed the settings UI to always
show the payments integration option.)

More deleted code!

[Nicolas Zea, 2016-03-15 18:19:27]

https://codereview.chromium.org/1785923010/diff/120001/sync/internal_api/publ...
File sync/internal_api/public/util/experiments.h (right):

https://codereview.chromium.org/1785923010/diff/120001/sync/internal_api/publ...
sync/internal_api/public/util/experiments.h:40: bool wallet_sync_enabled;
We can actually just delete this (and kWalletSyncTag above)

https://codereview.chromium.org/1785923010/diff/120001/sync/protocol/experime...
File sync/protocol/experiments_specifics.proto (right):

https://codereview.chromium.org/1785923010/diff/120001/sync/protocol/experime...
sync/protocol/experiments_specifics.proto:77: optional WalletSyncFlags
wallet_sync = 9;
nit: prefix with obsolete_ to ensure no one uses it accidentally (I think we
actually read from it in sync_manager_impl.cc, which we don't need to do any
more for example).

[Evan Stade, 2016-03-15 19:15:05]

(+24 lines, -424 lines)

lgtm!

[Nicolas Zea, 2016-03-15 19:43:07]

LGTM++

[Justin Donnelly, 2016-03-15 20:02:26]

https://codereview.chromium.org/1785923010/diff/120001/sync/internal_api/publ...
File sync/internal_api/public/util/experiments.h (right):

https://codereview.chromium.org/1785923010/diff/120001/sync/internal_api/publ...
sync/internal_api/public/util/experiments.h:40: bool wallet_sync_enabled;
On 2016/03/15 18:19:27, Nicolas Zea wrote:
> We can actually just delete this (and kWalletSyncTag above)

Done.

https://codereview.chromium.org/1785923010/diff/120001/sync/protocol/experime...
File sync/protocol/experiments_specifics.proto (right):

https://codereview.chromium.org/1785923010/diff/120001/sync/protocol/experime...
sync/protocol/experiments_specifics.proto:77: optional WalletSyncFlags
wallet_sync = 9;
On 2016/03/15 18:19:27, Nicolas Zea wrote:
> nit: prefix with obsolete_ to ensure no one uses it accidentally

Done. Sorry, should have done this earlier.

> (I think we
> actually read from it in sync_manager_impl.cc, which we don't need to do any
> more for example).

Removed the wallet section from sync_manager_impl.cc. Let me know if that looks
right. There was also a reference in sync/protocol/proto_value_conversions.cc,
which I removed.

And just to double check, nothing I'm doing here will stop us from still syncing
the wallet type, right?

[Nicolas Zea, 2016-03-15 20:05:55]

On 2016/03/15 20:02:26, Justin Donnelly wrote:
>
https://codereview.chromium.org/1785923010/diff/120001/sync/internal_api/publ...
> File sync/internal_api/public/util/experiments.h (right):
> 
>
https://codereview.chromium.org/1785923010/diff/120001/sync/internal_api/publ...
> sync/internal_api/public/util/experiments.h:40: bool wallet_sync_enabled;
> On 2016/03/15 18:19:27, Nicolas Zea wrote:
> > We can actually just delete this (and kWalletSyncTag above)
> 
> Done.
> 
>
https://codereview.chromium.org/1785923010/diff/120001/sync/protocol/experime...
> File sync/protocol/experiments_specifics.proto (right):
> 
>
https://codereview.chromium.org/1785923010/diff/120001/sync/protocol/experime...
> sync/protocol/experiments_specifics.proto:77: optional WalletSyncFlags
> wallet_sync = 9;
> On 2016/03/15 18:19:27, Nicolas Zea wrote:
> > nit: prefix with obsolete_ to ensure no one uses it accidentally
> 
> Done. Sorry, should have done this earlier.
> 
> > (I think we
> > actually read from it in sync_manager_impl.cc, which we don't need to do any
> > more for example).
> 
> Removed the wallet section from sync_manager_impl.cc. Let me know if that
looks
> right. There was also a reference in sync/protocol/proto_value_conversions.cc,
> which I removed.
> 
> And just to double check, nothing I'm doing here will stop us from still
syncing
> the wallet type, right?

Right. Since you updated the data type controller to just check
kAutofillWalletImportEnabled, this should be fine (and the integration tests you
modified should verify that too).

[Justin Donnelly, 2016-03-15 20:07:38]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2016-03-15 20:07:56]

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

[commit-bot: I haz the power, 2016-03-15 20:48:32]

Description was changed from

==========
Remove kAutofillWalletSyncExperimentEnabled.

This sync experiment has been at 100% of all users for a while.

Also, add the autofill_wallet type to sync/tools/testserver/chromiumsync.py.
Previously, AutofillWalletDataTypeController::IsEnabled was returning false in
the test environment because the sync setting was false. Now that we enable by
default, the sync integration tests are covering this type. Which I assume is a
good thing.

BUG=594182
==========

to

==========
Remove kAutofillWalletSyncExperimentEnabled.

This sync experiment has been at 100% of all users for a while.

Also, add the autofill_wallet type to sync/tools/testserver/chromiumsync.py.
Previously, AutofillWalletDataTypeController::IsEnabled was returning false in
the test environment because the sync setting was false. Now that we enable by
default, the sync integration tests are covering this type. Which I assume is a
good thing.

BUG=594182
==========

[commit-bot: I haz the power, 2016-03-15 20:48:34]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2016-03-15 20:50:31]

Description was changed from

==========
Remove kAutofillWalletSyncExperimentEnabled.

This sync experiment has been at 100% of all users for a while.

Also, add the autofill_wallet type to sync/tools/testserver/chromiumsync.py.
Previously, AutofillWalletDataTypeController::IsEnabled was returning false in
the test environment because the sync setting was false. Now that we enable by
default, the sync integration tests are covering this type. Which I assume is a
good thing.

BUG=594182
==========

to

==========
Remove kAutofillWalletSyncExperimentEnabled.

This sync experiment has been at 100% of all users for a while.

Also, add the autofill_wallet type to sync/tools/testserver/chromiumsync.py.
Previously, AutofillWalletDataTypeController::IsEnabled was returning false in
the test environment because the sync setting was false. Now that we enable by
default, the sync integration tests are covering this type. Which I assume is a
good thing.

BUG=594182

Committed: https://crrev.com/04c8fd5e7c57f75262f477e6ac94e18ca76b6371
Cr-Commit-Position: refs/heads/master@{#381304}
==========

[commit-bot: I haz the power, 2016-03-15 20:50:33]

Patchset 3 (id:??) landed as
https://crrev.com/04c8fd5e7c57f75262f477e6ac94e18ca76b6371
Cr-Commit-Position: refs/heads/master@{#381304}