New password form parser doesn't set |other_possible_usernames| and |all_possible_passwords| |
|||
Issue descriptionPasswordForm::other_possible_usernames and PasswordForm::all_possible_passwords are used now for sending votes when the user changes a username or a password in the bubble. In order to support these votes form parser needs to set these variables. |all_possible_passwords| is used also for allowing the user to choose another password to save. Now |other_possible_usernames| corresponds to all text fields (values and id) other than username field. For simplicity (if we like) we can also include a username field. |all_possible_passwords| corresponds to all password fields.
,
Sep 6
Thanks for filing this, Vadym! For the record: Vadym and I agreed to explicitly include the "chosen" username among the |other_possible_usernames| and the "chosen" password among |all_possible_passwords|. This won't affect the old parser and the old PasswordFormManager. Also, |other_possible_usernames| should be renamed to |all_possible_usernames|, to reflect this change. That will make the name incompatible with the usage in the old parser, but that's just a temporary glitch until the old parser is removed.
,
Sep 6
So actually, all_possible_passwords have been produced by the new parser for some time now: https://cs.chromium.org/chromium/src/components/password_manager/core/browser/form_parsing/form_parser.cc?type=cs&sq=package:chromium&g=0&l=662 But other_possible_usernames have not, so I will add them. Also, we agreed with Vadym that renaming other_possible_usernames is low priority, so I will create a special bug to track that, mark it as being blocked by issue 831123 and include a TODO linking to it to the code.
,
Sep 6
The TODO bug is https://crbug.com/881346.
,
Sep 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9c4e31f43f11d7a34577b5f07265d7644f848ac0 commit 9c4e31f43f11d7a34577b5f07265d7644f848ac0 Author: Vaclav Brozek <vabr@chromium.org> Date: Thu Sep 06 21:21:10 2018 Teach form_parser.cc other_possible_usernames When Chrome tries to figure out which text field of a password form is the username, it picks one and stores the rest in PasswordForm::other_possible_usernames. This field has so far only been populated by the old form parser (password_form_conversion_utils.cc). This CL also teaches the new parser to populate that field. However, there is a slight change: the new parser will include also the username which Chrome picked, for simplicity of the processing of the information later. The change in meaning is not breaking, because the output of the old and the new parser is consumed by different versions of PasswordFormManager. https://crbug.com/881346 tracks renaming of the other_possible_usernames field once there is only one meaning to it. Bug: 880721 Change-Id: I114472dc8d1b37a8f77bdb79689008e7795d81a4 Reviewed-on: https://chromium-review.googlesource.com/1210503 Commit-Queue: Vaclav Brozek <vabr@chromium.org> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Cr-Commit-Position: refs/heads/master@{#589301} [modify] https://crrev.com/9c4e31f43f11d7a34577b5f07265d7644f848ac0/components/password_manager/core/browser/form_parsing/form_parser.cc [modify] https://crrev.com/9c4e31f43f11d7a34577b5f07265d7644f848ac0/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc
,
Sep 7
|
|||
►
Sign in to add a comment |
|||
Comment 1 by dvadym@chromium.org
, Sep 5