Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1099033004:
Add a text/password field count metric for empty username forms. (Closed)
|
Created:
5 years, 8 months ago by msramek Modified:
5 years, 7 months ago CC:
chromium-reviews, vabr+watchlist_chromium.org, browser-components-watch_chromium.org, asvitkine+watch_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, rouslan+autofillwatch_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a text/password field count metric for empty username forms.
Another step in the investigation of no-username forms. Are we dealing with small forms that only contain one password field, or with larger but somehow malformed forms?
BUG=456728
Committed: https://crrev.com/9088052d6f7767519f2f4f57bddf73f9871cb8fb
Cr-Commit-Position: refs/heads/master@{#330084}
Patch Set 1 #
Total comments: 2
Patch Set 2 : std::count_if #
Total comments: 3
Patch Set 3 : Rebase. #Patch Set 4 : Moved to renderer. #Patch Set 5 : Superfluous include #
Total comments: 10
Patch Set 6 : Removed counter. #
Total comments: 3
Patch Set 7 : Checking for no passwords. #
Total comments: 4
Patch Set 8 : Metric description, inclusion. #
Total comments: 2
Patch Set 9 : Updated the description. #
Messages
Total messages: 28 (5 generated)
msramek@chromium.org changed reviewers: + dvadym@chromium.org, isherman@chromium.org, vasilii@chromium.org
Hi guys, I'm adding another metric for no-username forms. Can you PTAL? Thanks, Martin
lgtm https://codereview.chromium.org/1099033004/diff/1/components/password_manager... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/1099033004/diff/1/components/password_manager... components/password_manager/core/browser/password_manager.cc:423: for (const autofill::FormFieldData& field : iter->form_data.fields) { optional: I think using std::count_if + lambda here improves readability.
https://codereview.chromium.org/1099033004/diff/1/components/password_manager... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/1099033004/diff/1/components/password_manager... components/password_manager/core/browser/password_manager.cc:423: for (const autofill::FormFieldData& field : iter->form_data.fields) { On 2015/04/21 13:45:24, vasilii wrote: > optional: I think using std::count_if + lambda here improves readability. Done.
Might it be easier to simply add a RAPPOR metric for the specific sites that have such forms? https://codereview.chromium.org/1099033004/diff/20001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/1099033004/diff/20001/components/password_man... components/password_manager/core/browser/password_manager.cc:426: return field.IsTextField() || field.IsPasswordField(); Hmm, do you really want to count search and url fields, but exclude date fields, as text fields here? I'm not sure that IsTextField(), in the way that the autocomplete_manager code uses it, is such a well-defined concept that all clients will want the same implementation. I'm a little hesitant to declare it as a FormFieldData method. (FWIW, the form_control_type should be stored as an enum rather than a string. The only reason that it's still stored as a string is that I never got around to this cleanup task.)
https://codereview.chromium.org/1099033004/diff/20001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/1099033004/diff/20001/components/password_man... components/password_manager/core/browser/password_manager.cc:426: return field.IsTextField() || field.IsPasswordField(); On 2015/04/21 22:35:50, Ilya Sherman wrote: > Hmm, do you really want to count search and url fields, but exclude date fields, > as text fields here? I'm not sure that IsTextField(), in the way that the > autocomplete_manager code uses it, is such a well-defined concept that all > clients will want the same implementation. I'm a little hesitant to declare it > as a FormFieldData method. > > (FWIW, the form_control_type should be stored as an enum rather than a string. > The only reason that it's still stored as a string is that I never got around to > this cleanup task.) Well, I used the IsTextField() method, because I needed an approximation. The password manager is interested in blink::WebInputElement::isTextField(), but at this point the form is parsed and I no longer have access to that. In that sense, including "url" and "search" and not including "date" is probably correct, because that really matches <input type="text" /> ("date" is, I assume, <input type="date" /> ). What might make more sense though is to move this metric to GetPasswordForm() in renderer, so it can work with WebInputElements.
https://codereview.chromium.org/1099033004/diff/20001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/1099033004/diff/20001/components/password_man... components/password_manager/core/browser/password_manager.cc:426: return field.IsTextField() || field.IsPasswordField(); On 2015/04/22 09:36:01, msramek wrote: > On 2015/04/21 22:35:50, Ilya Sherman wrote: > > Hmm, do you really want to count search and url fields, but exclude date > fields, > > as text fields here? I'm not sure that IsTextField(), in the way that the > > autocomplete_manager code uses it, is such a well-defined concept that all > > clients will want the same implementation. I'm a little hesitant to declare > it > > as a FormFieldData method. > > > > (FWIW, the form_control_type should be stored as an enum rather than a string. > > > The only reason that it's still stored as a string is that I never got around > to > > this cleanup task.) > > Well, I used the IsTextField() method, because I needed an approximation. The > password manager is interested in blink::WebInputElement::isTextField(), but at > this point the form is parsed and I no longer have access to that. > > In that sense, including "url" and "search" and not including "date" is probably > correct, because that really matches <input type="text" /> ("date" is, I assume, > <input type="date" /> ). > > What might make more sense though is to move this metric to GetPasswordForm() in > renderer, so it can work with WebInputElements. Yes, that might make sense. Alternately, did you see my suggestion about using a RAPPOR metric? It seems to me like you're really looking for examples of sites where the feature fails, and RAPPOR will likely be better for that purpose.
On 2015/04/22 21:36:22, Ilya Sherman wrote: > https://codereview.chromium.org/1099033004/diff/20001/components/password_man... > File components/password_manager/core/browser/password_manager.cc (right): > > https://codereview.chromium.org/1099033004/diff/20001/components/password_man... > components/password_manager/core/browser/password_manager.cc:426: return > field.IsTextField() || field.IsPasswordField(); > On 2015/04/22 09:36:01, msramek wrote: > > On 2015/04/21 22:35:50, Ilya Sherman wrote: > > > Hmm, do you really want to count search and url fields, but exclude date > > fields, > > > as text fields here? I'm not sure that IsTextField(), in the way that the > > > autocomplete_manager code uses it, is such a well-defined concept that all > > > clients will want the same implementation. I'm a little hesitant to declare > > it > > > as a FormFieldData method. > > > > > > (FWIW, the form_control_type should be stored as an enum rather than a > string. > > > > > The only reason that it's still stored as a string is that I never got > around > > to > > > this cleanup task.) > > > > Well, I used the IsTextField() method, because I needed an approximation. The > > password manager is interested in blink::WebInputElement::isTextField(), but > at > > this point the form is parsed and I no longer have access to that. > > > > In that sense, including "url" and "search" and not including "date" is > probably > > correct, because that really matches <input type="text" /> ("date" is, I > assume, > > <input type="date" /> ). > > > > What might make more sense though is to move this metric to GetPasswordForm() > in > > renderer, so it can work with WebInputElements. > > Yes, that might make sense. Alternately, did you see my suggestion about using > a RAPPOR metric? It seems to me like you're really looking for examples of > sites where the feature fails, and RAPPOR will likely be better for that > purpose. At this stage we are trying to understand the impact of different use cases on the issue. Once we have a clear idea of what's going on, we'll start fixing it.
Hello again Ilya, although I followed up on your suggestion about RAPPOR, I didn't land that CL yet, not only because of technical issues, but also because in the meantime I learned that RAPPOR works a bit differently than I expected. It might be difficult for us to collect information on obscure sites; We'll need to think this through. In the meantime, I would like to proceed with this simpler UMA metric. As discussed, I removed the IsTextField() method and moved the calculation to renderer. PTAL :) Thanks, Martin
msramek@chromium.org changed reviewers: + vabr@chromium.org
Adding Václav on our team's side, since both Vasilii and Vadym are OOO. PTAL. Thanks, Martin
components/autofill/content/renderer/password_form_conversion_utils.cc LGTM with comments and a question. Cheers, Vaclav https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:265: // Count the total number of text and password fields. Note that nit: The comment is clear and correct, but probably not needed. The name of the variable is good enough to convey the needed meaning. https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:379: } else { Do you want to report this even if there are not passwords found? https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:384: num_text_and_password_fields); Alternatively, you can use layout_sequence.size(). But if you prefer the dedicated counter for readability, that's fine.
Thanks, Václav! https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:265: // Count the total number of text and password fields. Note that On 2015/05/12 10:26:02, vabr (Chromium) wrote: > nit: The comment is clear and correct, but probably not needed. The name of the > variable is good enough to convey the needed meaning. Acknowledged. https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:379: } else { On 2015/05/12 10:26:02, vabr (Chromium) wrote: > Do you want to report this even if there are not passwords found? Do you mean no password fields found? No. But if I understand correctly, this method is only called for forms that we already know to contain a password field, right? (at least my quick testing confirms that; I didn't check for all call chains) If you mean for forms where we have no password saved, then yes. Currently, if you encounter a form with no username and successfully submit it, we will always offer to save. Therefore, any such form could have resulted in saved credentials with no username, and thus I want to get a statistic on all the forms in the wild. https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:384: num_text_and_password_fields); On 2015/05/12 10:26:02, vabr (Chromium) wrote: > Alternatively, you can use layout_sequence.size(). > But if you prefer the dedicated counter for readability, that's fine. No, let's use layout_sequence.size(). I just really didn't notice. The advantage here is that I'll avoid the confusion with semantics of "text field".
Still LGTM, with 2 more comments. Thanks! Vaclav https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:379: } else { On 2015/05/12 10:50:44, msramek wrote: > On 2015/05/12 10:26:02, vabr (Chromium) wrote: > > Do you want to report this even if there are not passwords found? > > Do you mean no password fields found? Yes, sorry for the confusion, I meant password fields, not values. > No. But if I understand correctly, this > method is only called for forms that we already know to contain a password > field, right? I don't think so, at least I could not find any evidence for it in the code. If you look just below the block you added, there is a specific function to locate the password fields, which also handles the case when |passwords| is empty. It might be a good idea to check for that here as well. > > (at least my quick testing confirms that; I didn't check for all call chains) > > If you mean for forms where we have no password saved, then yes. Currently, if > you encounter a form with no username and successfully submit it, we will always > offer to save. Therefore, any such form could have resulted in saved credentials > with no username, and thus I want to get a statistic on all the forms in the > wild. https://codereview.chromium.org/1099033004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1099033004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:25708: + username field. Please also say that only forms with at least one password field are included in the count.
https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:379: } else { On 2015/05/12 11:06:11, vabr (Chromium) wrote: > On 2015/05/12 10:50:44, msramek wrote: > > On 2015/05/12 10:26:02, vabr (Chromium) wrote: > > > Do you want to report this even if there are not passwords found? > > > > Do you mean no password fields found? > Yes, sorry for the confusion, I meant password fields, not values. > > > No. But if I understand correctly, this > > method is only called for forms that we already know to contain a password > > field, right? > I don't think so, at least I could not find any evidence for it in the code. > > If you look just below the block you added, there is a specific function to > locate the password fields, which also handles the case when |passwords| is > empty. It might be a good idea to check for that here as well. > > > > > (at least my quick testing confirms that; I didn't check for all call chains) > > > > If you mean for forms where we have no password saved, then yes. Currently, if > > you encounter a form with no username and successfully submit it, we will > always > > offer to save. Therefore, any such form could have resulted in saved > credentials > > with no username, and thus I want to get a statistic on all the forms in the > > wild. > Interesting. I would expect none of this code to trigger on such fields, as confirmed by my quick test. Nevertheless, you're right that LocateSpecificPasswords() checks this case. I moved this block to the end of the method. LocateSpecificPasswords() only returns true if either |password| or |new_password| has been found, so no additional conditions are needed. https://codereview.chromium.org/1099033004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1099033004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:25708: + username field. On 2015/05/12 11:06:11, vabr (Chromium) wrote: > Please also say that only forms with at least one password field are included in > the count. I changed "forms" to "password forms", is that clear enough?
Still LGTM, still 2 more comments :). One is in code, the other one is a question: What about the bias of this histogram towards submitted forms? GetPasswordForm is called for form parsing, and also for form submission. So submitted forms will get some additionals data points compared to non-submitted forms. Is that OK? Cheers, Vaclav https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:379: } else { On 2015/05/12 11:27:03, msramek wrote: > On 2015/05/12 11:06:11, vabr (Chromium) wrote: > > On 2015/05/12 10:50:44, msramek wrote: > > > On 2015/05/12 10:26:02, vabr (Chromium) wrote: > > > > Do you want to report this even if there are not passwords found? > > > > > > Do you mean no password fields found? > > Yes, sorry for the confusion, I meant password fields, not values. > > > > > No. But if I understand correctly, this > > > method is only called for forms that we already know to contain a password > > > field, right? > > I don't think so, at least I could not find any evidence for it in the code. > > > > If you look just below the block you added, there is a specific function to > > locate the password fields, which also handles the case when |passwords| is > > empty. It might be a good idea to check for that here as well. > > > > > > > > (at least my quick testing confirms that; I didn't check for all call > chains) > > > > > > If you mean for forms where we have no password saved, then yes. Currently, > if > > > you encounter a form with no username and successfully submit it, we will > > always > > > offer to save. Therefore, any such form could have resulted in saved > > credentials > > > with no username, and thus I want to get a statistic on all the forms in the > > > wild. > > > > Interesting. I would expect none of this code to trigger on such fields, as > confirmed by my quick test. Nevertheless, you're right that > LocateSpecificPasswords() checks this case. > > I moved this block to the end of the method. LocateSpecificPasswords() only > returns true if either |password| or |new_password| has been found, so no > additional conditions are needed. This has the caveat that you won't get this count for password forms with 3+ password fields, where the first 3 are non-empty and equal, or all different, etc. (see when LocateSpecificPasswords returns false). https://codereview.chromium.org/1099033004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1099033004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:25708: + username field. On 2015/05/12 11:27:03, msramek wrote: > On 2015/05/12 11:06:11, vabr (Chromium) wrote: > > Please also say that only forms with at least one password field are included > in > > the count. > > I changed "forms" to "password forms", is that clear enough? Acknowledged.
Thanks, Václav. I realized that as well. I would say that it is OK at least for the first iteration. The way how UMA works, we are not going to get close to anything as clear as "the ratio of forms on the internet that have X fields" anyway. https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/1099033004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:379: } else { On 2015/05/12 11:33:58, vabr (Chromium) wrote: > On 2015/05/12 11:27:03, msramek wrote: > > On 2015/05/12 11:06:11, vabr (Chromium) wrote: > > > On 2015/05/12 10:50:44, msramek wrote: > > > > On 2015/05/12 10:26:02, vabr (Chromium) wrote: > > > > > Do you want to report this even if there are not passwords found? > > > > > > > > Do you mean no password fields found? > > > Yes, sorry for the confusion, I meant password fields, not values. > > > > > > > No. But if I understand correctly, this > > > > method is only called for forms that we already know to contain a password > > > > field, right? > > > I don't think so, at least I could not find any evidence for it in the code. > > > > > > If you look just below the block you added, there is a specific function to > > > locate the password fields, which also handles the case when |passwords| is > > > empty. It might be a good idea to check for that here as well. > > > > > > > > > > > (at least my quick testing confirms that; I didn't check for all call > > chains) > > > > > > > > If you mean for forms where we have no password saved, then yes. > Currently, > > if > > > > you encounter a form with no username and successfully submit it, we will > > > always > > > > offer to save. Therefore, any such form could have resulted in saved > > > credentials > > > > with no username, and thus I want to get a statistic on all the forms in > the > > > > wild. > > > > > > > Interesting. I would expect none of this code to trigger on such fields, as > > confirmed by my quick test. Nevertheless, you're right that > > LocateSpecificPasswords() checks this case. > > > > I moved this block to the end of the method. LocateSpecificPasswords() only > > returns true if either |password| or |new_password| has been found, so no > > additional conditions are needed. > > This has the caveat that you won't get this count for password forms with 3+ > password fields, where the first 3 are non-empty and equal, or all different, > etc. (see when LocateSpecificPasswords returns false). I know. But in this case the password manager will also bail, not offering to save the form. So for the investigation of saved empty usernames, that is ok.
LGTM! Thanks for the discussion. Vaclav
Thanks. This looks better to me now; just a couple of minor comments: https://codereview.chromium.org/1099033004/diff/120001/components/autofill/co... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/1099033004/diff/120001/components/autofill/co... components/autofill/content/renderer/password_form_conversion_utils.cc:11: #include "base/metrics/histogram.h" nit: Please include histogram_macros instead. https://codereview.chromium.org/1099033004/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1099033004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:25708: + have a username field. Please document when this is recorded.
Patchset #8 (id:140001) has been deleted
https://codereview.chromium.org/1099033004/diff/120001/components/autofill/co... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/1099033004/diff/120001/components/autofill/co... components/autofill/content/renderer/password_form_conversion_utils.cc:11: #include "base/metrics/histogram.h" On 2015/05/13 00:17:19, Ilya Sherman wrote: > nit: Please include histogram_macros instead. Done. https://codereview.chromium.org/1099033004/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1099033004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:25708: + have a username field. On 2015/05/13 00:17:19, Ilya Sherman wrote: > Please document when this is recorded. Done.
LGTM % a final comment. Thanks. https://codereview.chromium.org/1099033004/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1099033004/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:25709: + successfully parsed. Please clarify when such forms are parsed. Is it once per page load?
https://codereview.chromium.org/1099033004/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1099033004/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:25709: + successfully parsed. On 2015/05/13 17:30:40, Ilya Sherman wrote: > Please clarify when such forms are parsed. Is it once per page load? Done. It is done on page load and also on form submission. As was noted in the previous discussion with Václav, this can create a bias towards submitted forms, but this is not worth solving right now, as there are other biases already due to how UMA works. We won't be able to get the exact ratio of N-field forms, but we will at least see if there are a lot of forms for particular N.
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org, vabr@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1099033004/#ps180001 (title: "Updated the description.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1099033004/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/9088052d6f7767519f2f4f57bddf73f9871cb8fb Cr-Commit-Position: refs/heads/master@{#330084} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
