Over to Seb for triage or reassignment.  It looks like this regressed somewhere between M66 and M68.  In a dicey, buttery world, I have a feeling this bug will have a lot more impact than we'd like.

The enterprise team added a policy option around cc autofill. Perhaps this is a side-effect of that code?

I can repro on my side, I started taking a look but my brain is too fried on this Friday pm to understand it :) I'll take a look Monday.

In the meantime, +feuunk@: Do you have an idea of what might be causing this on top of your head? 

Looks related to some of the changes scottchen made to the settings for DICe. (Perhaps crbug.com/815018?)

Thanks for the link, I cannot repro the issue when I revert the CL:
https://chromium-review.googlesource.com/c/chromium/src/+/935836

scottchen@ could you please take a look?

I see what's going on, but not sure what the correct solution would be.

So the CL in question does the following:
- When sync-everything toggle is turned on, we used to actually overwrite every individual sync pref to be true as well. This means that we would not be able to revert it to their previous settings once the user turns sync-everything off.
- As part of the unified-consent project requirement, restoring to previous setting is needed.
- My CL made it stop overwriting every pref's value to "true", but instead just respect the result from GetPreferredDataTypes which is masked by the sync_everything pref[1] (This logic already existed, not something I added). This means that the true value each pref is still preserved on disk and when sync_everything is turned off, we can access them easily.
- I now see that "autofill::prefs::kAutofillWalletImportEnabled" for some reason is not part of the registered_type for sync service, which is why the on-disk value is still being individually retrieved [2] as opposed to respecting the sync_everything settings like other sync data-types.

So, that is the problem, and the reason I don't have a good solution is:
- I don't know why kAutofillWalletImportEnabled isn't one of the registered_types in sync-service. Can we make it so (and this bug would go away)? or are there good reasons for it to be an exception?
- If it NEEDS to stay as an exception, I guess we'll have no choice but to keep overwriting its on-disk value. But this would be confusing since it could go out-of-sync with the "autofillSynced" pref's on-disk value, in the following scenario:
1) turn sync-everything off
2) turn "autofill" off
3) turn sync-everything on
   - at this point, "autofillSynced" pref is still "off" on disk, even though the toggle shows "on" (which is intended)
   - now, should we turn the "paymentsIntegrationEnabled" pref to "on"?
   - If so, what happens when we turn "sync-everything" off again? "autofillSynced" will be "off" and "paymentsIntegrationEnabled" would be "on", which should never be possible. Where in the code do we reconcile this?

Open to suggestions - I personally think if we can make "paymentsIntegrationEnabled" part of registered_types in the sync-service and  be returned as part of GetPreferredDataTypes() when "sync_everything" is true, that would be most ideal and get us to where we want to be, but I'm not sure how much work would be involved in this.

[1] https://cs.chromium.org/chromium/src/components/sync/base/sync_prefs.cc?type=cs&q=getpreferredDataTypes&l=227
[2] https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/people_handler.cc?type=cs&q=autofill::prefs::kAutofillWalletImportEnabled&l=969

Thanks for the detailed reply!

ewald@ do you know if someone on the team would know why kAutofillWalletImportEnabled isn't one of the registered_types?

I'll ask the Payments team.

I'm not sure what registered_types means, and I think at the time that this toggle switch was created, all client-side code was likely done by a dedicated Chrome team.  My only guess is because it's gated by the Autofill switch (when sync-everything is off and you turn Autofill on, the Payments switch turns on too regardless of what value it previously was) but it's clear from #c7 that you're already well aware of that.

Interesting! I have no idea why kAutofillWalletImportEnabled isn't one of the registered types...that's probably a question for a sync engineer (maybe +Nicolas can weigh in).

Either way, this is quite topical. Based on recent discussions we've been having with privacy, legal, and UX for our Unity design, +Zach and I decided that we should go back to the old behavior for these subtoggles. Specifically, when the user turns "Sync everything" ON (after it had previously been OFF), we should change the *actual* values of the subtoggles to ON (so when the user switches "Sync everything" back OFF, everything will still be ON). In other words, the state of the subtoggles doesn't need to be "sticky" across turning "Sync everything" off/on; turning "Sync everything" ON is in essence saying "I want to turn all of these toggles back to ON."

I was planning to just make this behavioral update with Unity. But if it would solve this issue to simply go back to the old behavior (where turning off "Sync everything" always results in all the subtoggles starting ON, regardless of previous values), we could just make that behavioral change now. Seb - could you help with that? Would it be as simple as reverting the CL in question which created the current behavior?

Note we're also tracking implementing this behavior in Issue 814973, so if we land a CL here we can reference that bug as well.

P.S. to Scott - very sorry for the back and forth with this >.< It's been tough to get everyone (product, UX, legal, privacy) to agree on the desired behavior here...but ultimately I think we're now landing in the right spot.

Yes, reverting the CL would fix the bug for us :)

I'm also fine with the idea of fixing it in another CL, but it would need to land before the M68 branch.

Let know what you think!

From a product behavior perspective, the desired behavior is outlined in #10. I defer to you on what the best way to go about achieving that is :) I don't know if there were other parts of Scott's CL that we want to keep (in which case landing a new CL might be the right path).

Scott what do you think? I'm fine with either as long as the bug is fixed :)

Scott, do you plan on making a follow up CL for this, or should we revert crrev.com/c/935836? Let me know what you prefer :)

Thanks!

I re-looked at that CL, and I *THINK* you'll be able to simply revert the CL and not get any major conflicts. 

I won't be able to work on this soon, so please give it a try and ping me if you find that it's conflicting with the UI changes that's landed since that CL.

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

commit f2bcdd989cec695add8d1b0414005136d62dbc06
Author: Sebastien SG <sebsg@chromium.org>
Date: Wed May 16 20:35:30 2018

Revert "Settings[DICE]: changing sync-everything doesn't impact on-disk prefs"

This reverts commit 46a6d04b50d8f4d7a03089a065a3066d36c37b32.

Reason for revert: Introduced and inconsistency in how the Sync Payments cards toggle. See  crbug.com/839643 

Original change's description:
> Settings[DICE]: changing sync-everything doesn't impact on-disk prefs
>
> This CL stops toggling "sync everything" from overriding all individual
> prefs' on-disk value with "true" (see reasoning in bug report).
>
> Bug: 815018
> Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
> Change-Id: If6287376009b200355426113618933af9ad78243
> Reviewed-on: https://chromium-review.googlesource.com/935836
> Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> Reviewed-by: Tommy Li <tommycli@chromium.org>
> Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
> Commit-Queue: Scott Chen <scottchen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#542929}

TBR=tommycli@chromium.org,dpapad@chromium.org,mastiz@chromium.org,scottchen@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 815018, 839643 
Change-Id: Iba41940bdfde775048553dd4e0abd6c52e85f7bf
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Reviewed-on: https://chromium-review.googlesource.com/1061893
Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Scott Chen <scottchen@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559272}
[modify] https://crrev.com/f2bcdd989cec695add8d1b0414005136d62dbc06/chrome/browser/resources/settings/people_page/sync_browser_proxy.js
[modify] https://crrev.com/f2bcdd989cec695add8d1b0414005136d62dbc06/chrome/browser/resources/settings/people_page/sync_page.js
[modify] https://crrev.com/f2bcdd989cec695add8d1b0414005136d62dbc06/chrome/browser/ui/webui/settings/people_handler.cc
[modify] https://crrev.com/f2bcdd989cec695add8d1b0414005136d62dbc06/chrome/browser/ui/webui/settings/people_handler.h
[modify] https://crrev.com/f2bcdd989cec695add8d1b0414005136d62dbc06/chrome/test/data/webui/settings/people_page_sync_page_test.js
[modify] https://crrev.com/f2bcdd989cec695add8d1b0414005136d62dbc06/chrome/test/data/webui/settings/test_sync_browser_proxy.js
[modify] https://crrev.com/f2bcdd989cec695add8d1b0414005136d62dbc06/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/f2bcdd989cec695add8d1b0414005136d62dbc06/components/browser_sync/profile_sync_service.h

I tested it today and it works like it used to. Thanks!