Using ShouldBeUploaded is inconsistent across Autofill and Password Manager. |
|||||||
Issue descriptionFor checking whether to upload votes for a form, function FormStructure::ShouldBeUploaded used both from Autofill and Password Manager code. Because Password Manager uploads votes rarely it's ok to upload Password Manager votes for any form, so call of this function might be skipped in Password Manager code. On other hand, ShouldBeUploaded has check on |has_password_field_| and as result uploads for any password form are treated as Password Manager uploads, while they may come from Autofill. So the solution: 1.Remove call of ShouldBeUploaded in Password Manager code. 2.Remove checks on has_password_field_ in ShouldBeUploaded.
,
Oct 1
This is a small and non-risky CL. It is required in order to support password generation (which is launched in M-69) on sign-up forms with 2 fields and to improve Password Manager filling.
,
Oct 1
This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 1
Let's target this for M71. Seems like this isn't absolutely critical and that M69 currently has this issue?
,
Oct 2
Hi Abdul, generally, you're right of course. But in this specific case, this small CL is a complete blocker for some of our most important annual goals. I'm happy to provide more context offline. But it would be really good to get approval on this merge. Thanks Patrick
,
Oct 2
How safe is this merge overall?
,
Oct 2
Approving merge for M70. branch:3538. Already confirmed in #2 that this is safe cl.
,
Oct 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/194e89dde24c20c5fe48f0b0b4a140a127d4ae76 commit 194e89dde24c20c5fe48f0b0b4a140a127d4ae76 Author: Vadym Doroshenko <dvadym@chromium.org> Date: Tue Oct 02 18:34:03 2018 [MERGE] Make function ShouldBeUploaded more consistent. For checking whether to upload votes for a form, FormStructure::ShouldBeUploaded is called both from Autofill and Password Manager code. Because Password Manager uploads votes rarely it's ok to upload Password Manager votes for any form, so call of this function might be skipped in Password Manager code. On other hand, ShouldBeUploaded has a check on |has_password_field_| and as result uploads for any password form are treated as Password Manager uploads, while they may come from Autofill. This CL implements: 1.Removing call of ShouldBeUploaded in Password Manager code. 2.Removing checks on has_password_field_ in ShouldBeUploaded. Bug:882941 Change-Id: Iba9ab1f74451520f0a636e0748f795d73e922ce7 Reviewed-on: https://chromium-review.googlesource.com/1219710 Commit-Queue: Vadym Doroshenko <dvadym@chromium.org> Reviewed-by: Roger McFarlane <rogerm@chromium.org> Reviewed-by: Dominic Battré <battre@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#594335}(cherry picked from commit 1567a35a494568bcb8ee68d855e7f6176c182878) Reviewed-on: https://chromium-review.googlesource.com/c/1257551 Cr-Commit-Position: refs/branch-heads/3538@{#826} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/194e89dde24c20c5fe48f0b0b4a140a127d4ae76/components/autofill/core/browser/autofill_manager_unittest.cc [modify] https://crrev.com/194e89dde24c20c5fe48f0b0b4a140a127d4ae76/components/autofill/core/browser/form_structure.cc [modify] https://crrev.com/194e89dde24c20c5fe48f0b0b4a140a127d4ae76/components/autofill/core/browser/form_structure.h [modify] https://crrev.com/194e89dde24c20c5fe48f0b0b4a140a127d4ae76/components/autofill/core/browser/form_structure_unittest.cc [modify] https://crrev.com/194e89dde24c20c5fe48f0b0b4a140a127d4ae76/components/password_manager/core/browser/password_form_manager_unittest.cc [modify] https://crrev.com/194e89dde24c20c5fe48f0b0b4a140a127d4ae76/components/password_manager/core/browser/votes_uploader.cc [modify] https://crrev.com/194e89dde24c20c5fe48f0b0b4a140a127d4ae76/components/password_manager/core/browser/votes_uploader.h
,
Oct 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/194e89dde24c20c5fe48f0b0b4a140a127d4ae76 Commit: 194e89dde24c20c5fe48f0b0b4a140a127d4ae76 Author: dvadym@chromium.org Commiter: rogerm@chromium.org Date: 2018-10-02 18:34:03 +0000 UTC [MERGE] Make function ShouldBeUploaded more consistent. For checking whether to upload votes for a form, FormStructure::ShouldBeUploaded is called both from Autofill and Password Manager code. Because Password Manager uploads votes rarely it's ok to upload Password Manager votes for any form, so call of this function might be skipped in Password Manager code. On other hand, ShouldBeUploaded has a check on |has_password_field_| and as result uploads for any password form are treated as Password Manager uploads, while they may come from Autofill. This CL implements: 1.Removing call of ShouldBeUploaded in Password Manager code. 2.Removing checks on has_password_field_ in ShouldBeUploaded. Bug:882941 Change-Id: Iba9ab1f74451520f0a636e0748f795d73e922ce7 Reviewed-on: https://chromium-review.googlesource.com/1219710 Commit-Queue: Vadym Doroshenko <dvadym@chromium.org> Reviewed-by: Roger McFarlane <rogerm@chromium.org> Reviewed-by: Dominic Battré <battre@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#594335}(cherry picked from commit 1567a35a494568bcb8ee68d855e7f6176c182878) Reviewed-on: https://chromium-review.googlesource.com/c/1257551 Cr-Commit-Position: refs/branch-heads/3538@{#826} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
,
Nov 20
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by bugdroid1@chromium.org
, Sep 26