Double-check credit card upload logic for Butter |
||||||||
Issue descriptionLooking at the code, I'm fairly sure that uploading credit cards currently doesn't work for Butter, and only ever worked accidentally :) Details: autofill::IsCreditCardUploadEnabled [1] checks a bunch of things on the SyncService: 1) CanSyncFeatureStart(): This used to be true in Butter mode, even though it wasn't true. I recently fixed that by adding an is-account-primary check [2], and thus probably broke upload for Butter. 2) GetPreferredDataTypes().Has(AUTOFILL_PROFILE): This is true in Butter mode, but only accidentally (all types are preferred by default, even if they'll never become active in Butter mode). 3) GetUploadToGoogleState(AUTOFILL_WALLET_DATA) == ACTIVE: This is currently true in Butter mode, but IMO that's wrong: GetUploadToGoogleState is for consent, which we don't have, so I'm planning to change this [3]. [1] https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofill_experiments.cc?rcl=58e0403558cd8e9dcf797e257ca9a29b1dbe32e9&l=68 [2] https://chromium-review.googlesource.com/c/chromium/src/+/1339939 [3] https://chromium-review.googlesource.com/c/chromium/src/+/1341916
,
Nov 19
Thanks for finding this! Do you have a list of things that we should check to see if we should offer upload?
,
Nov 20
I'm not exactly sure what the logic should be. Maybe the function should actually return a tri-state to differentiate Butter from full Sync? Something like DONT_OFFER, OFFER_WITH_EXPLICIT_CONSENT, OFFER (Not sure if that is useful to callers, or if the names make sense.) Anyway, for OFFER, the existing logic should be fine (but it can maybe be more unified with the below case). For OFFER_WITH_EXPLICIT_CONSENT, we should drop the GetUploadToGoogleState() and CanSyncFeatureStart() checks, and maybe instead check GetActiveDataTypes().Has(WALLET)?
,
Nov 20
Just chatted with Marc offline: We shouldn't need a tri-state, because the UI for the Butter prompt and full sync will look and behave the same. Seb: Assigning to myself, because I think it probably makes sense for someone in the Sync team to take this up, since most of the complexity here comes from which APIs to call, and because you have plenty on your plate for M72. Do you agree? I'd like to make sure that we have an integration test covering this flow, to make sure this keeps working. It looks like those could be added reasonably easily to https://cs.chromium.org/chromium/src/chrome/browser/ui/views/autofill/save_card_bubble_views_browsertest.cc.
,
Nov 20
,
Nov 20
Makes sense! Please let me know when you have updates, as one of my current CLs will need to use the new "state". Thanks!
,
Nov 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/995f8099303a3eb47660661b5093538d08d544d3 commit 995f8099303a3eb47660661b5093538d08d544d3 Author: Florian Uunk <feuunk@chromium.org> Date: Mon Nov 26 17:20:49 2018 Make SaveCardBubble browsertest use the sync state The browsertests used to rely on the save manager being hardcoded to upload to Google if the observer_for_testing was set. This makes it impossible to test that the logic for showing the upload prompt works properly. BUG= 906630 , 859761 Change-Id: Id7352a869388aac4ff36e1284750e22ac68c91b3 Reviewed-on: https://chromium-review.googlesource.com/c/1348335 Commit-Queue: Florian Uunk <feuunk@chromium.org> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Cr-Commit-Position: refs/heads/master@{#610866} [modify] https://crrev.com/995f8099303a3eb47660661b5093538d08d544d3/chrome/browser/ui/views/autofill/save_card_bubble_views_browsertest.cc [modify] https://crrev.com/995f8099303a3eb47660661b5093538d08d544d3/chrome/browser/ui/views/autofill/save_card_bubble_views_browsertest_base.cc [modify] https://crrev.com/995f8099303a3eb47660661b5093538d08d544d3/chrome/browser/ui/views/autofill/save_card_bubble_views_browsertest_base.h [modify] https://crrev.com/995f8099303a3eb47660661b5093538d08d544d3/components/autofill/core/browser/autofill_experiments.cc [modify] https://crrev.com/995f8099303a3eb47660661b5093538d08d544d3/components/autofill/core/browser/credit_card_save_manager.cc
,
Nov 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b54154ace3a231159f9abe78c134dfd62f353ef6 commit b54154ace3a231159f9abe78c134dfd62f353ef6 Author: Sebastien Seguin-Gagnon <sebsg@chromium.org> Date: Wed Nov 28 20:15:20 2018 Revert "Make SaveCardBubble browsertest use the sync state" This reverts commit 995f8099303a3eb47660661b5093538d08d544d3. Reason for revert: Breaks browser tests on continues builders (consistent failures) See crbug.com/908787 Original change's description: > Make SaveCardBubble browsertest use the sync state > > The browsertests used to rely on the save manager being hardcoded to > upload to Google if the observer_for_testing was set. This makes it > impossible to test that the logic for showing the upload prompt works > properly. > > BUG= 906630 , 859761 > > Change-Id: Id7352a869388aac4ff36e1284750e22ac68c91b3 > Reviewed-on: https://chromium-review.googlesource.com/c/1348335 > Commit-Queue: Florian Uunk <feuunk@chromium.org> > Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> > Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> > Cr-Commit-Position: refs/heads/master@{#610866} TBR=vasilii@chromium.org,sebsg@chromium.org,feuunk@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 906630 , 859761 Change-Id: I145d2b56295af6014b6a94c44beb38b1bf6b9e01 Reviewed-on: https://chromium-review.googlesource.com/c/1352680 Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org> Cr-Commit-Position: refs/heads/master@{#611844} [modify] https://crrev.com/b54154ace3a231159f9abe78c134dfd62f353ef6/chrome/browser/ui/views/autofill/save_card_bubble_views_browsertest.cc [modify] https://crrev.com/b54154ace3a231159f9abe78c134dfd62f353ef6/chrome/browser/ui/views/autofill/save_card_bubble_views_browsertest_base.cc [modify] https://crrev.com/b54154ace3a231159f9abe78c134dfd62f353ef6/chrome/browser/ui/views/autofill/save_card_bubble_views_browsertest_base.h [modify] https://crrev.com/b54154ace3a231159f9abe78c134dfd62f353ef6/components/autofill/core/browser/autofill_experiments.cc [modify] https://crrev.com/b54154ace3a231159f9abe78c134dfd62f353ef6/components/autofill/core/browser/credit_card_save_manager.cc
,
Nov 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/727e802b4f1704b1370ec5b23ec738c227976adf commit 727e802b4f1704b1370ec5b23ec738c227976adf Author: Florian Uunk <feuunk@chromium.org> Date: Fri Nov 30 14:17:52 2018 Re-enable the card up- and downstream in Butter. The previous logic depended on GetUploadToGoogleState, which we changed to return false in Butter mode. BUG= 906630 R=treib,sebsg Change-Id: I42c40c20d6d997d571b427b512fe1afc9fd50329 Reviewed-on: https://chromium-review.googlesource.com/c/1354448 Commit-Queue: Florian Uunk <feuunk@chromium.org> Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#612638} [modify] https://crrev.com/727e802b4f1704b1370ec5b23ec738c227976adf/components/autofill/core/browser/autofill_experiments.cc [modify] https://crrev.com/727e802b4f1704b1370ec5b23ec738c227976adf/components/autofill/core/browser/autofill_experiments_unittest.cc [modify] https://crrev.com/727e802b4f1704b1370ec5b23ec738c227976adf/components/autofill/core/browser/personal_data_manager.cc [modify] https://crrev.com/727e802b4f1704b1370ec5b23ec738c227976adf/components/autofill/core/browser/personal_data_manager_unittest.cc
,
Dec 3
Requesting a merge for the fix in #9. I tested this on Canary today, and works as expected.
,
Dec 3
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 4
Thanks for the fix. Can you please clarify why this is needed for M72 vs waiting until 73? Is Butter launching in 72? How safe is this fix and is it behind a flag?
,
Dec 5
Thanks Abdul. Yes, we're planning to do a 1% stable experiment for Butter, so it would be great to get this into M72. The fix should be safe, I tested it multiple times, and there is good test coverage. Butter itself is also behind a Finch flag.
,
Dec 6
Approving merge for M72. 3626
,
Dec 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a19cdcabab74131abfea8d0ee603a7c5e1c3b6bd commit a19cdcabab74131abfea8d0ee603a7c5e1c3b6bd Author: Florian Uunk <feuunk@chromium.org> Date: Thu Dec 06 09:22:11 2018 Re-enable the card up- and downstream in Butter. The previous logic depended on GetUploadToGoogleState, which we changed to return false in Butter mode. BUG= 906630 R=treib,sebsg Change-Id: I42c40c20d6d997d571b427b512fe1afc9fd50329 Reviewed-on: https://chromium-review.googlesource.com/c/1354448 Commit-Queue: Florian Uunk <feuunk@chromium.org> Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#612638}(cherry picked from commit 727e802b4f1704b1370ec5b23ec738c227976adf) Reviewed-on: https://chromium-review.googlesource.com/c/1365433 Reviewed-by: Florian Uunk <feuunk@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#100} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/a19cdcabab74131abfea8d0ee603a7c5e1c3b6bd/components/autofill/core/browser/autofill_experiments.cc [modify] https://crrev.com/a19cdcabab74131abfea8d0ee603a7c5e1c3b6bd/components/autofill/core/browser/autofill_experiments_unittest.cc [modify] https://crrev.com/a19cdcabab74131abfea8d0ee603a7c5e1c3b6bd/components/autofill/core/browser/personal_data_manager.cc [modify] https://crrev.com/a19cdcabab74131abfea8d0ee603a7c5e1c3b6bd/components/autofill/core/browser/personal_data_manager_unittest.cc
,
Dec 6
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a19cdcabab74131abfea8d0ee603a7c5e1c3b6bd Commit: a19cdcabab74131abfea8d0ee603a7c5e1c3b6bd Author: feuunk@chromium.org Commiter: feuunk@chromium.org Date: 2018-12-06 09:22:11 +0000 UTC Re-enable the card up- and downstream in Butter. The previous logic depended on GetUploadToGoogleState, which we changed to return false in Butter mode. BUG= 906630 R=treib,sebsg Change-Id: I42c40c20d6d997d571b427b512fe1afc9fd50329 Reviewed-on: https://chromium-review.googlesource.com/c/1354448 Commit-Queue: Florian Uunk <feuunk@chromium.org> Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#612638}(cherry picked from commit 727e802b4f1704b1370ec5b23ec738c227976adf) Reviewed-on: https://chromium-review.googlesource.com/c/1365433 Reviewed-by: Florian Uunk <feuunk@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#100} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by rouslan@google.com
, Nov 19