Interesting!

Marc, do you know how this is handled for other data types? I am surprised this pref needs to be manually handled by the controller? Isn't there some generic code that would enable/disable sync for datatypes based on the prefs?

(I've checked passwords controller and did not find any code checking the value of the pref for passwords.)

Seb, do you know why wallet needs a special treatment here?

This is yet another way in which Wallet is special: This is not the regular "data type preferred" pref - that one can't be toggled by the user directly; it's included under the general "Autofill" toggle.
I don't know why exactly this special treatment is required.

I would have thought that the behavior would be the same as the "autofill" one?

The Autofill one is a super set (both addresses and cards) but in practice I thought they'd do the same thing?

The reason why it's split is that Autofill in this case is for addresses mainly (we don't sync cards). The Google Payments one is specific to the Payments cards, but it can't be on if the Autofill sync is off.

Not sure my explanation was clear, let me know if you want more info.

Does this mean that we have the same issue for AutofillProfile and Autocomplete data?

Are there similar preferences for AutofillProfile and Autocomplete? If so, then probably yes, but looking at AutofillProfileSyncableService that doesn't seem to be the case.

There is kAutofillAddressEnabled.

Pre-USS I think it works, because if I toggle the Autofill addresses pref to off, I stop receiving sync notifications.

D'oh, I was looking in the SyncableService but the handling code is in the Controller: https://cs.chromium.org/chromium/src/components/autofill/core/browser/webdata/autofill_profile_data_type_controller.cc?rcl=c6a42f1e43b9b7da6aa297a0a3bf8ac386ed83e4&l=39

So yes, we probably have the analogous issue for addresses and autocomplete.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/34399e03ced19c57b532a27be8a29b4a2ce63b36

commit 34399e03ced19c57b532a27be8a29b4a2ce63b36
Author: Jan Krcal <jkrcal@chromium.org>
Date: Wed Oct 10 08:08:20 2018

[AF Wallet USS] Disable/enable sync when setting toggles get toggled

This CL fixes an omission for the wallet data USS implementation that
sync status did not react to toggles in settings.

Bug:  890737 
Change-Id: I85206fed2dfff2fa6279a7a9ab220e78da505e15
Reviewed-on: https://chromium-review.googlesource.com/c/1271002
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598252}
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/autofill/core/browser/BUILD.gn
[add] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/autofill/core/browser/autofill_wallet_model_type_controller.cc
[add] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/autofill/core/browser/autofill_wallet_model_type_controller.h
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/browser_sync/profile_sync_components_factory_impl.cc
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/browser_sync/profile_sync_components_factory_impl.h
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/browser_sync/profile_sync_service_unittest.cc
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/sync/driver/data_type_manager.h
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/sync/driver/data_type_manager_impl.cc
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/sync/driver/data_type_manager_impl.h
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/sync/driver/data_type_manager_mock.h
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/sync/driver/fake_sync_service.h
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/sync/driver/model_association_manager.cc
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/sync/driver/model_association_manager.h
[modify] https://crrev.com/34399e03ced19c57b532a27be8a29b4a2ce63b36/components/sync/driver/sync_service.h

Thanks Jan! Can this now be marked fixed?

Reopening this because I found a problem with the current implementation (discovered when trying to write a test for  bug 895824 ): When the pref gets disabled, the controller calls ReadyForStartChanged, and we will stop syncing the corresponding type - so far so good. However, the type will remain in SyncService::GetActiveTypes until we happen to reconfigure.

For Wallet, everything happens to work out for now, because Wallet Metadata still uses the old pre-USS code path which does trigger a reconfigure. But for Autofill Profiles it doesn't work.

I think the fix is probably to also trigger a reconfigure in the new ReadyForStartChanged code path.

I have a fix for this in https://chromium-review.googlesource.com/c/chromium/src/+/1319609

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c9309376a15c980e1e0941cc2b545100ca2781e

commit 1c9309376a15c980e1e0941cc2b545100ca2781e
Author: Marc Treib <treib@chromium.org>
Date: Wed Nov 07 08:08:52 2018

Fix: Properly disable Autofill Sync when Autofill itself is disabled

This is a fix to the SyncService::ReadyForStartChanged code path:
When stopping the data type, set an UNREADY_ERROR (instead of leaving
the error unset). This causes the Sync engine to reconfigure, which
properly marks the data type as no longer active. Before, the data
type did stop syncing, but was still returned as part of
SyncService::GetActiveDataTypes.

It also adds integration tests that AUTOFILL_PROFILE gets disabled on
autofill::prefs::SetProfileAutofillEnabled(.., false).

Bug:  895824 ,  890737 
Change-Id: I3736ef010ad09ecd79a101aeff400d597956569e
Reviewed-on: https://chromium-review.googlesource.com/c/1319609
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605990}
[add] https://crrev.com/1c9309376a15c980e1e0941cc2b545100ca2781e/chrome/browser/sync/test/integration/single_client_autofill_profile_sync_test.cc
[modify] https://crrev.com/1c9309376a15c980e1e0941cc2b545100ca2781e/chrome/test/BUILD.gn
[modify] https://crrev.com/1c9309376a15c980e1e0941cc2b545100ca2781e/components/sync/driver/data_type_manager_impl.cc
[modify] https://crrev.com/1c9309376a15c980e1e0941cc2b545100ca2781e/components/sync/driver/model_association_manager.cc
[modify] https://crrev.com/1c9309376a15c980e1e0941cc2b545100ca2781e/components/sync/driver/model_association_manager.h
[modify] https://crrev.com/1c9309376a15c980e1e0941cc2b545100ca2781e/components/sync/driver/model_association_manager_unittest.cc

Thanks for spotting & fixing this, Marc!