Remove PasswordForm::layout? |
|||||
Issue descriptionPasswordForm::layout has been added in the past to help Chrome distinguish a particular type of squashed sign-in+sign-up forms from change-password forms. The original bug is issue 449714 . It is rather narrow in what kind of forms it recognises, and there has never been perceived any need to extend it. Nevertheless, it adds complexity in form parsing, and increases the size of PasswordForm. We should either: (1) remove PasswordForm::layout, or (2) land https://crrev.com/c/1099240 to add layout support in the new parser The original issue mentions godaddy.com and nordstrom.com as two instances of the squashed forms. AFAICT, those sites no longer squash the forms (godaddy seems to have just the sign-up form on one page, and nordstrom seems to have the sign-in fields without a <form>, and the sign-up fields in a form called "signin" :)).
,
Jun 27 2018
The NextAction date has arrived: 2018-06-27
,
Jun 28 2018
Hm, the work on the parser is still ongoing, so I'd like to punt deciding this a bit more.
,
Aug 27
The NextAction date has arrived: 2018-08-27
,
Aug 29
Looking at the histograms PasswordManager.SubmittedFormType and PasswordManager.SubmittedNonSecureFormType, the non-standard layout is reported in 0.26% and 0.4% cases, respectively. The non-standard layout is also used to prevent reporting such forms as change password forms (PasswordForm::IsPossibleChangePasswordForm). This, in turn, influences whether such forms are autofilled. No layout info might thus mean no autofill for some forms (may be both improvement or regression in particular cases). Overall, it seems like getting rid of the layout info won't cause any disturbance, but I will check with battre@ and dvadym@ whether we want some UKM on which forms are affected by no autofill because of no layout info.
,
Aug 29
I'm ok with removing layout. Probably the best solution is to use server-side for classification of sign-in + sign-up forms.
,
Aug 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0394317d12275e0bf8b8dfdbd0dd95149e3b9087 commit 0394317d12275e0bf8b8dfdbd0dd95149e3b9087 Author: Vaclav Brozek <vabr@chromium.org> Date: Wed Aug 29 18:37:15 2018 Remove dead code from PasswordFormFillData Pieces of dead code removed: * is_possible_change_password_form -- this data member is never read, only assigned. * UsernamesCollection data type, never used * name data member -- only used on iOS, so removed from the Mojo bindings (which are only used on platforms using //content) Bug: 852772 Change-Id: I29337fbfc632dbce38d6b85cd00e885e28f738c7 Reviewed-on: https://chromium-review.googlesource.com/1195504 Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Commit-Queue: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#587211} [modify] https://crrev.com/0394317d12275e0bf8b8dfdbd0dd95149e3b9087/chrome/renderer/autofill/password_autofill_agent_browsertest.cc [modify] https://crrev.com/0394317d12275e0bf8b8dfdbd0dd95149e3b9087/components/autofill/content/common/autofill_types.mojom [modify] https://crrev.com/0394317d12275e0bf8b8dfdbd0dd95149e3b9087/components/autofill/content/common/autofill_types_struct_traits.cc [modify] https://crrev.com/0394317d12275e0bf8b8dfdbd0dd95149e3b9087/components/autofill/content/common/autofill_types_struct_traits.h [modify] https://crrev.com/0394317d12275e0bf8b8dfdbd0dd95149e3b9087/components/autofill/content/common/autofill_types_struct_traits_unittest.cc [modify] https://crrev.com/0394317d12275e0bf8b8dfdbd0dd95149e3b9087/components/autofill/core/common/password_form_fill_data.cc [modify] https://crrev.com/0394317d12275e0bf8b8dfdbd0dd95149e3b9087/components/autofill/core/common/password_form_fill_data.h [modify] https://crrev.com/0394317d12275e0bf8b8dfdbd0dd95149e3b9087/components/password_manager/core/browser/password_autofill_manager_unittest.cc
,
Aug 31
Looking at a random sample of 3k+ sites with password forms did not improve the guess from #5 much: in 0.1% a regression was confirmed, but enough sites could not be checked for unrelated reasons that the ceiling is at 0.4%. However, in that sample, the non-standard layout was found in 1.5% of cases, which is one order of magnitude more than what UMA reports for the whole population.
,
Sep 6
On a team meeting it was decided that PF::layout should go. I will upload a CL for that soon.
,
Sep 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/313187fcdacd9d14f4279e05993ea4987f036f98 commit 313187fcdacd9d14f4279e05993ea4987f036f98 Author: Vaclav Brozek <vabr@chromium.org> Date: Thu Sep 06 20:56:35 2018 Deprecate two empty-username-related metrics This CL stops logging into, and deprecates: PasswordManager.EmptyUsernames.TextAndPasswordFieldCount and PasswordManager.EmptyUsernames.PasswordFieldCount. Those metrics were used for an investigation, which has concluded back in 2016. Bug: 852772 Change-Id: Ib15807b8384d9053d8df7e09465bbef16f9145fa Reviewed-on: https://chromium-review.googlesource.com/1209503 Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Reviewed-by: Robert Kaplow <rkaplow@chromium.org> Commit-Queue: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#589293} [modify] https://crrev.com/313187fcdacd9d14f4279e05993ea4987f036f98/components/autofill/content/renderer/password_form_conversion_utils.cc [modify] https://crrev.com/313187fcdacd9d14f4279e05993ea4987f036f98/tools/metrics/histograms/histograms.xml
,
Sep 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b1b8eb459ad1d34ffe35818f1229c14dc0018628 commit b1b8eb459ad1d34ffe35818f1229c14dc0018628 Author: Vaclav Brozek <vabr@chromium.org> Date: Fri Sep 07 13:55:00 2018 Remove PasswordForm::layout As described in the associated bug, the |layout| data member of |PasswordForm| contributes less benefit than it causes complexity. It is not used in the new FormData -> PasswordForm parser, and this CL also remvoes it from the old one, as well as removing all related code. Bug: 852772 Change-Id: Icdc6979a9c0a57093a459f2dfc2e27766f9e9142 Reviewed-on: https://chromium-review.googlesource.com/1209582 Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Reviewed-by: Mike West <mkwst@chromium.org> Commit-Queue: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#589507} [modify] https://crrev.com/b1b8eb459ad1d34ffe35818f1229c14dc0018628/components/autofill/content/common/autofill_types.mojom [modify] https://crrev.com/b1b8eb459ad1d34ffe35818f1229c14dc0018628/components/autofill/content/common/autofill_types_struct_traits.cc [modify] https://crrev.com/b1b8eb459ad1d34ffe35818f1229c14dc0018628/components/autofill/content/common/autofill_types_struct_traits.h [modify] https://crrev.com/b1b8eb459ad1d34ffe35818f1229c14dc0018628/components/autofill/content/common/autofill_types_struct_traits_unittest.cc [modify] https://crrev.com/b1b8eb459ad1d34ffe35818f1229c14dc0018628/components/autofill/content/renderer/password_form_conversion_utils.cc [modify] https://crrev.com/b1b8eb459ad1d34ffe35818f1229c14dc0018628/components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc [modify] https://crrev.com/b1b8eb459ad1d34ffe35818f1229c14dc0018628/components/autofill/core/common/password_form.cc [modify] https://crrev.com/b1b8eb459ad1d34ffe35818f1229c14dc0018628/components/autofill/core/common/password_form.h [modify] https://crrev.com/b1b8eb459ad1d34ffe35818f1229c14dc0018628/components/password_manager/core/browser/password_form_manager.cc [modify] https://crrev.com/b1b8eb459ad1d34ffe35818f1229c14dc0018628/tools/metrics/histograms/enums.xml
,
Sep 7
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by vabr@chromium.org
, Jun 14 2018