Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2113743002:
Add instrumentation for password autofill and Credential Manager API user flows. (Closed)
|
Created:
4 years, 5 months ago by vasilii Modified:
4 years, 5 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews, darin-cc_chromium.org, gcasto+watchlist_chromium.org, jam, vabr+watchlistpasswordmanager_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd instrumentation for password autofill and Credential Manager API user flows.
The instrumentation is implemented via user actions rather than histograms so
that events can be sequenced, based on their timestamps. This will allow us to
better understand how the Credential Manager API and password autofill interact.
For example, we currently observe that users dismiss the account chooser more
frequently than we expected. We'd like to estimate whether users had the
intention of logging in by measuring whether users log in after dismissing the
account chooser; and if so, whether they used the autofill password manager.
In addition the CL marks kManagerActionBlacklisted as deprecated since it's unused.
BUG=400674
Committed: https://crrev.com/bb7617079a36fb15c450ee0f0aab4b35aefb4ecd
Cr-Commit-Position: refs/heads/master@{#405741}
Patch Set 1 #
Total comments: 4
Patch Set 2 : describe actions better #
Total comments: 44
Patch Set 3 : update the descriptions #
Total comments: 26
Patch Set 4 : more comments #
Total comments: 4
Patch Set 5 : last comments #
Messages
Total messages: 25 (12 generated)
vasilii@chromium.org changed reviewers: + isherman@chromium.org
Hi Ilya, please review the CL.
Hi Vasilii, so far I've just looked at actions.xml. I have a high-level question before looking into the nitty-gritty details: What, specifically, are you trying to measure? And, did you consider using (enumerated) histograms rather than actions? If so, why did you choose actions over the histograms? I ask, because your CL description kind of makes it seem like you're trying to understand something specific. And, it's not clear to me whether the instrumentation that you're adding fully connects to the question that you're trying to answer. https://codereview.chromium.org/2113743002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2113743002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:2265: <action name="CMAPI_AccountChooser_Click"> nit: Please spell out "Credential Manager" -- "CMAPI" is a bit hard to decode otherwise. https://codereview.chromium.org/2113743002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:12111: User chose a different username from the existing credentials This summary is not very clear to me, even as someone who's worked on the password manager (but several years ago). Please write a few sentences here to clearly explain what context the user is in when making this choice. Is a username already filled into the form? Did the user click to get a dropdown of alternate usernames and choose one? What if the user just types over the autofilled username, and matches an existing username? What if the user starts typing, then sees a popup, and chooses from it? (I'm not sure whether I'm even on track, i.e. describing roughly the correct interaction pattern. But hopefully you get my meaning nonetheless.) Please also take a closer look at the descriptions for the other actions you're adding, and clarify them, as needed, as well.
Description was changed from ========== Add user actions to track interoperabilty across password autofill and the Credential Manager API. BUG=400674 ========== to ========== Add user actions to track interoperabilty across password autofill and the Credential Manager API. The user actions are used instead of histograms because they have a timestamp. We'll try to analyse what happens after a site called the CM API. An example: users dismiss the account chooser too often. We want to know if they log in afterwards. If yes, we want to know if they used the autofill password manager. BUG=400674 ==========
I expanded the description to clarify the goals and the approach. https://codereview.chromium.org/2113743002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2113743002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:2265: <action name="CMAPI_AccountChooser_Click"> On 2016/06/30 21:11:35, Ilya Sherman wrote: > nit: Please spell out "Credential Manager" -- "CMAPI" is a bit hard to decode > otherwise. Done. https://codereview.chromium.org/2113743002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:12111: User chose a different username from the existing credentials On 2016/06/30 21:11:35, Ilya Sherman wrote: > This summary is not very clear to me, even as someone who's worked on the > password manager (but several years ago). Please write a few sentences here to > clearly explain what context the user is in when making this choice. Is a > username already filled into the form? Did the user click to get a dropdown of > alternate usernames and choose one? What if the user just types over the > autofilled username, and matches an existing username? What if the user starts > typing, then sees a popup, and chooses from it? (I'm not sure whether I'm even > on track, i.e. describing roughly the correct interaction pattern. But > hopefully you get my meaning nonetheless.) > > Please also take a closer look at the descriptions for the other actions you're > adding, and clarify them, as needed, as well. Done.
Thanks for updating the CL description, Vasilii. Wanting to look at event sequences is a great reason to choose user actions over histograms. Apologies for what might feel like very nit-picky comments inline. I'm really pushing for the greater clarity and precision because metric descriptions are often read by people who are not intimately familiar with a feature's implementation -- and not infrequently, these are decision-makers, who might draw the wrong conclusion based on a misleading metric. I realize that the timezone difference makes this review painfully high-latency, so I'll try my best not to create too many round-trips. Feel free to ask a teammate in your timezone to make some rapid iterations with you, if you feel that would be helpful. Here's a suggested rephrasing of the description -- feel free to use it as is, or to clean it up further: """ Add instrumentation for password autofill and Credential Manager API user flows. The instrumentation is implemented via user actions rather than histograms so that events can be sequenced, based on their timestamps. This will allow us to better understand how the Credential Manager API and password autofill interact. For example, we currently observe that users dismiss the account chooser more frequently than we expected. We'd like to estimate whether users had the intention of logging in by measuring whether users log in after dismissing the account chooser; and if so, whether they used the autofill password manager. """ https://codereview.chromium.org/2113743002/diff/20001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/2113743002/diff/20001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:577: if (!driver) Hmm, I don't understand how this change connects to the rest of the CL. Was it intentional? https://codereview.chromium.org/2113743002/diff/20001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:580: // Allow generation for any non-blacklisted form. Note: If you're going to remove the blacklisted manager action, you'll likely want to update comments that refer to blacklisting as well. https://codereview.chromium.org/2113743002/diff/20001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:666: ProcessLoginPrompt(); Here, too. Please either mention in the CL description why this action is no longer needed; or better yet, if it's not directly related to metrics, please split this change off to a separate CL. https://codereview.chromium.org/2113743002/diff/20001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:1347: } nit: else NOTREACHED? https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2862: <action name="Credential_Manager_AccountChooser_Click"> Optional nit: I'd write "CredentialManager" as a single unit (here and throughout) https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2862: <action name="Credential_Manager_AccountChooser_Click"> Optional nit: I'd replace "Click" with something like "AcceptedCredential" (or "SelectedCredential" -- in the omnibox world, "selected" and "accepted" have very specific meanings, but I'm not sure whether they generalize outside of that specific context) https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2864: <description>User chose a credential via the account chooser</description> nit: Please end the sentence with a period (and please apply this change through the CL, as needed). https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2864: <description>User chose a credential via the account chooser</description> nit: s/User/The user (and please apply this change through the CL, as needed) https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2867: <action name="Credential_Manager_AccountChooser_Dismiss"> Optional nit: I'd name this "Dismissed" rather than "Dismiss" -- I think user action names often sound better when written in the past tense. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2874: <description>Try to automatically sign user in</description> I *think* this description means "The website tried to automatically sign the user in". Or, does it mean that the website tried to do so, and the user had already consented? What does the word "try" mean, i.e. what are the possible failure cases? Would it make more sense to only log an autosignin metric in case of success, or some specific failure case? Also, this doesn't seem like it is really a user action per se. If a website spams the autosignin API, will we just generate an arbitrary number of these user actions? https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2880: A site requested a credential but the store is empty This also doesn't really seem like a user action. What's the context for recording this? https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12105: <description>Username/password were autofilled</description> Please expand this description. Is this just on pageload? Or, if the user selects an alternate username/password pair and autofills that, is that recorded as well? https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12108: <action name="PasswordManager_ChooseNonDefault"> Optional nit: s/Choose/Chose https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12112: logged in with it This description doesn't obviously match the action name. Perhaps a name like "LoggedInWithNonDefaultCredential" would be clearer? You could probably come up with something shorter, but still clear. Additionally, I'd update the action description to more clearly highlight the *user* action. For example, something like "The user logged in with a credential other than the default one. This results in the default credential being updated, to the credential that the user most recently logged in with." By the way, it's not quite clear to me: is this only logged if the user has already saved the alternate credential / only logged if the alternate credential ends up being saved / logged regardless of whether the alternate credential is saved? I think my suggested description is none of those, so it's not quite accurate -- please update it to clarify. By the way, am I right to understand that "non-default credential" really means "non-default username"? If so, I think it might be better to use that wording, as it's clearer. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12116: <action name="PasswordManager_ChoosePSL"> Optional nit: s/Choose/Chose https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12119: User tries to log in with an existing PSL-match credential "PSL" is pretty jargony, i.e. is hard for someone outside of the team to understand. Is there a way to write this description in a way that would be clear to a statistician working primarily on, say, Chrome Translate integration? https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12119: User tries to log in with an existing PSL-match credential What does "tries to" mean here? That is, what are the failure modes? Is this essentially recorded upon a form submission, regardless of whether that form submission succeeds? Metric descriptions are frequently read by developers who are only somewhat familiar with the instrumented feature, so it's very helpful to be extremely clear and precise. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12133: <action name="PasswordManager_OverridePassword"> nit: Perhaps name this "LoggedInWithNewPassword"? (Or something along those lines, but shorter? E.g. "Login" in place of "LoggedIn" would probably be equally clear.) https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12141: <action name="PasswordManager_OverrideUsernameAndPassword"> nit: Perhaps name this "LoggedInWithNewUsername" or "NewCredential"? (I personally prefer "username", since it clarifies what part of the credential is being focused on.) https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12144: User tries to login with a completely new (possibly auto generated) nit: s/login/log in https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12161: <description>Password form submission failed</description> This doesn't seem like a user action, per se. What's the context for recording this? https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12166: <description>Password form submitted successfully</description> This doesn't seem like a user action, per se. What's the context for recording this? If you're intending to use it as a proxy for a successful login, I suspect it will be *very* far from accurate, as many websites will allow the form submit to pass through, and then show a new page with an error message.
Description was changed from ========== Add user actions to track interoperabilty across password autofill and the Credential Manager API. The user actions are used instead of histograms because they have a timestamp. We'll try to analyse what happens after a site called the CM API. An example: users dismiss the account chooser too often. We want to know if they log in afterwards. If yes, we want to know if they used the autofill password manager. BUG=400674 ========== to ========== Add user actions to track interoperabilty across password autofill and the Credential Manager API. The user actions are used instead of histograms because they have a timestamp. We'll try to analyse what happens after a site called the CM API. An example: users dismiss the account chooser too often. We want to know if they log in afterwards. If yes, we want to know if they used the autofill password manager. In addition the CL marks kManagerActionBlacklisted as deprecated since it's unused. BUG=400674 ==========
Description was changed from ========== Add user actions to track interoperabilty across password autofill and the Credential Manager API. The user actions are used instead of histograms because they have a timestamp. We'll try to analyse what happens after a site called the CM API. An example: users dismiss the account chooser too often. We want to know if they log in afterwards. If yes, we want to know if they used the autofill password manager. In addition the CL marks kManagerActionBlacklisted as deprecated since it's unused. BUG=400674 ========== to ========== Add instrumentation for password autofill and Credential Manager API user flows. The instrumentation is implemented via user actions rather than histograms so that events can be sequenced, based on their timestamps. This will allow us to better understand how the Credential Manager API and password autofill interact. For example, we currently observe that users dismiss the account chooser more frequently than we expected. We'd like to estimate whether users had the intention of logging in by measuring whether users log in after dismissing the account chooser; and if so, whether they used the autofill password manager. In addition the CL marks kManagerActionBlacklisted as deprecated since it's unused. BUG=400674 ==========
https://codereview.chromium.org/2113743002/diff/20001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/2113743002/diff/20001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:577: if (!driver) On 2016/07/01 17:59:54, Ilya Sherman wrote: > Hmm, I don't understand how this change connects to the rest of the CL. Was it > intentional? I deprecated this value (it was never assigned to manager_action_). Thus, I don't need to record it. Also I removed the condition which is always false. https://codereview.chromium.org/2113743002/diff/20001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:580: // Allow generation for any non-blacklisted form. On 2016/07/01 17:59:54, Ilya Sherman wrote: > Note: If you're going to remove the blacklisted manager action, you'll likely > want to update comments that refer to blacklisting as well. Done. https://codereview.chromium.org/2113743002/diff/20001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:666: ProcessLoginPrompt(); On 2016/07/01 17:59:54, Ilya Sherman wrote: > Here, too. Please either mention in the CL description why this action is no > longer needed; or better yet, if it's not directly related to metrics, please > split this change off to a separate CL. Done. https://codereview.chromium.org/2113743002/diff/20001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:1347: } On 2016/07/01 17:59:54, Ilya Sherman wrote: > nit: else NOTREACHED? Done. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2862: <action name="Credential_Manager_AccountChooser_Click"> On 2016/07/01 17:59:54, Ilya Sherman wrote: > Optional nit: I'd write "CredentialManager" as a single unit (here and > throughout) Done. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2862: <action name="Credential_Manager_AccountChooser_Click"> On 2016/07/01 17:59:55, Ilya Sherman wrote: > Optional nit: I'd replace "Click" with something like "AcceptedCredential" (or > "SelectedCredential" -- in the omnibox world, "selected" and "accepted" have > very specific meanings, but I'm not sure whether they generalize outside of that > specific context) Done. Shortened, so it fits one line in the code. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2864: <description>User chose a credential via the account chooser</description> On 2016/07/01 17:59:54, Ilya Sherman wrote: > nit: Please end the sentence with a period (and please apply this change through > the CL, as needed). Done. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2864: <description>User chose a credential via the account chooser</description> On 2016/07/01 17:59:54, Ilya Sherman wrote: > nit: s/User/The user (and please apply this change through the CL, as needed) Done. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2867: <action name="Credential_Manager_AccountChooser_Dismiss"> On 2016/07/01 17:59:55, Ilya Sherman wrote: > Optional nit: I'd name this "Dismissed" rather than "Dismiss" -- I think user > action names often sound better when written in the past tense. Done. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2874: <description>Try to automatically sign user in</description> On 2016/07/01 17:59:54, Ilya Sherman wrote: > I *think* this description means "The website tried to automatically sign the > user in". Or, does it mean that the website tried to do so, and the user had > already consented? What does the word "try" mean, i.e. what are the possible > failure cases? Would it make more sense to only log an autosignin metric in > case of success, or some specific failure case? > > Also, this doesn't seem like it is really a user action per se. If a website > spams the autosignin API, will we just generate an arbitrary number of these > user actions? I changed the description. A failure case is the the credential is obsolete. It's super rare as it occurs only when cookies expired and other 100 privacy conditions are satisfied. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2880: A site requested a credential but the store is empty On 2016/07/01 17:59:55, Ilya Sherman wrote: > This also doesn't really seem like a user action. What's the context for > recording this? Removed. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12105: <description>Username/password were autofilled</description> On 2016/07/01 17:59:55, Ilya Sherman wrote: > Please expand this description. Is this just on pageload? Or, if the user > selects an alternate username/password pair and autofills that, is that recorded > as well? Done. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12108: <action name="PasswordManager_ChooseNonDefault"> On 2016/07/01 17:59:54, Ilya Sherman wrote: > Optional nit: s/Choose/Chose Done. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12112: logged in with it On 2016/07/01 17:59:54, Ilya Sherman wrote: > This description doesn't obviously match the action name. Perhaps a name like > "LoggedInWithNonDefaultCredential" would be clearer? You could probably come up > with something shorter, but still clear. > > Additionally, I'd update the action description to more clearly highlight the > *user* action. For example, something like "The user logged in with a > credential other than the default one. This results in the default credential > being updated, to the credential that the user most recently logged in with." > By the way, it's not quite clear to me: is this only logged if the user has > already saved the alternate credential / only logged if the alternate credential > ends up being saved / logged regardless of whether the alternate credential is > saved? I think my suggested description is none of those, so it's not quite > accurate -- please update it to clarify. > > By the way, am I right to understand that "non-default credential" really means > "non-default username"? If so, I think it might be better to use that wording, > as it's clearer. Done. Alternate credential has already been saved. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12116: <action name="PasswordManager_ChoosePSL"> On 2016/07/01 17:59:55, Ilya Sherman wrote: > Optional nit: s/Choose/Chose Done. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12119: User tries to log in with an existing PSL-match credential On 2016/07/01 17:59:54, Ilya Sherman wrote: > "PSL" is pretty jargony, i.e. is hard for someone outside of the team to > understand. Is there a way to write this description in a way that would be > clear to a statistician working primarily on, say, Chrome Translate integration? Done. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12119: User tries to log in with an existing PSL-match credential On 2016/07/01 17:59:55, Ilya Sherman wrote: > What does "tries to" mean here? That is, what are the failure modes? Is this > essentially recorded upon a form submission, regardless of whether that form > submission succeeds? Metric descriptions are frequently read by developers who > are only somewhat familiar with the instrumented feature, so it's very helpful > to be extremely clear and precise. Done. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12133: <action name="PasswordManager_OverridePassword"> On 2016/07/01 17:59:54, Ilya Sherman wrote: > nit: Perhaps name this "LoggedInWithNewPassword"? (Or something along those > lines, but shorter? E.g. "Login" in place of "LoggedIn" would probably be > equally clear.) Done. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12141: <action name="PasswordManager_OverrideUsernameAndPassword"> On 2016/07/01 17:59:54, Ilya Sherman wrote: > nit: Perhaps name this "LoggedInWithNewUsername" or "NewCredential"? (I > personally prefer "username", since it clarifies what part of the credential is > being focused on.) Done. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12144: User tries to login with a completely new (possibly auto generated) On 2016/07/01 17:59:54, Ilya Sherman wrote: > nit: s/login/log in Done. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12161: <description>Password form submission failed</description> On 2016/07/01 17:59:55, Ilya Sherman wrote: > This doesn't seem like a user action, per se. What's the context for recording > this? It means that the login was unsuccessful from the Chrome point of view. https://codereview.chromium.org/2113743002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12166: <description>Password form submitted successfully</description> On 2016/07/01 17:59:55, Ilya Sherman wrote: > This doesn't seem like a user action, per se. What's the context for recording > this? If you're intending to use it as a proxy for a successful login, I > suspect it will be *very* far from accurate, as many websites will allow the > form submit to pass through, and then show a new page with an error message. It means that the password was probably correct.
https://codereview.chromium.org/2113743002/diff/40001/components/password_man... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2113743002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager.h:278: // because it has no match, or it is blacklisted, or it is disabled nit: Should the comment about blacklisting be removed here as well? https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12106: <action name="PasswordManager_ChosePSL"> Please update the action name to also use something clearer/less jargony than "PSL". https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12109: The user submits a form with an existing credential saved for another nit: s/submit/submitted https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12110: subdomain. nit: I think an example user flow might be helpful here, i.e. "For example, the user might have signed in on https://accounts.spotify.com using a credential previously saved for https://play.spotify.com." https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12127: The user tries to log in with an existing (PSL or not) credential but nit: s/tries/tried https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12127: The user tries to log in with an existing (PSL or not) credential but nit: Here as well, please clarify the meaning of "PSL" for those readers not intimately familiar with this feature. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12135: The user tries to log in with a completely new (possibly auto generated) nit: s/tries/tried https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12150: <action name="PasswordManager_SubmitFailed"> nit: I think this metric is really trying to measure "LoginFailed" vs "LoginSucceeded". Is that right? The current name makes me think it's trying to measure something technical about form events, which I'm realizing is probably not your intention. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12153: Password form submitted but the password doesn't seem right. This description is quite vague. Imagine that you read "The user typed a URL into the omnibox, but the URL didn't seem right". What would you think "didn't seem right" meant? Wasn't a URL? Was a commonly typo-ed URL? Was a malicious URL? I'd suggest something like: "The user submitted a password form, and the form submission was cancelled with a failure event. This typically indicates that the provided login details were incorrect." (Note: Only write "The user" if this metric is gated on a user action.) Also, I'm not sure -- does this metric actually cover the case where the form submission succeeds (in terms of the HTTP POST going through), but the user is bounced back to a login page showing a warning? That is, is this metric really measuring what the password manager "thinks" of the form submission? If so, I'd write the description somewhat differently: "The user submitted a password form, but the website appeared to reject the credentials. This action reflects the password manager component's logic for whether or not to offer to save a password. At the time of writing, failure means that either the form submission was canceled with a failure message, or the form submission succeeded, but the user was directed back to a page that still contained a login form." (Or, something along those lines, but clearer/more accurate.) =) https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12157: <action name="PasswordManager_SubmitPassed"> (Ditto) https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12160: Password form submitted successfully and login form disappeared. Please similarly clarify this description. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12182: <action name="PasswordManager_UseNonDefaultUsername"> nit: s/Use/Used https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12185: The user logged in with a credential other than the default one. This Is this, specifically, a previously-saved credential? Or any credential, even one that was not previously saved? (A credential that was not previously saved, presumably, cannot be set as default until it is saved, and it is not saved until the user accepts Chrome's prompt to save it. So, this description doesn't quite make sense in its current form.)
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
https://codereview.chromium.org/2113743002/diff/40001/components/password_man... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2113743002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager.h:278: // because it has no match, or it is blacklisted, or it is disabled On 2016/07/07 05:01:48, Ilya Sherman wrote: > nit: Should the comment about blacklisting be removed here as well? Done. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12106: <action name="PasswordManager_ChosePSL"> On 2016/07/07 05:01:48, Ilya Sherman wrote: > Please update the action name to also use something clearer/less jargony than > "PSL". Done. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12109: The user submits a form with an existing credential saved for another On 2016/07/07 05:01:48, Ilya Sherman wrote: > nit: s/submit/submitted Done. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12110: subdomain. On 2016/07/07 05:01:49, Ilya Sherman wrote: > nit: I think an example user flow might be helpful here, i.e. "For example, the > user might have signed in on https://accounts.spotify.com using a credential > previously saved for https://play.spotify.com.%22 Done. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12127: The user tries to log in with an existing (PSL or not) credential but On 2016/07/07 05:01:48, Ilya Sherman wrote: > nit: Here as well, please clarify the meaning of "PSL" for those readers not > intimately familiar with this feature. Done. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12127: The user tries to log in with an existing (PSL or not) credential but On 2016/07/07 05:01:49, Ilya Sherman wrote: > nit: s/tries/tried Done. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12135: The user tries to log in with a completely new (possibly auto generated) On 2016/07/07 05:01:48, Ilya Sherman wrote: > nit: s/tries/tried Done. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12150: <action name="PasswordManager_SubmitFailed"> On 2016/07/07 05:01:49, Ilya Sherman wrote: > nit: I think this metric is really trying to measure "LoginFailed" vs > "LoginSucceeded". Is that right? The current name makes me think it's trying > to measure something technical about form events, which I'm realizing is > probably not your intention. Done. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12153: Password form submitted but the password doesn't seem right. On 2016/07/07 05:01:48, Ilya Sherman wrote: > This description is quite vague. Imagine that you read "The user typed a URL > into the omnibox, but the URL didn't seem right". What would you think "didn't > seem right" meant? Wasn't a URL? Was a commonly typo-ed URL? Was a malicious > URL? > > I'd suggest something like: "The user submitted a password form, and the form > submission was cancelled with a failure event. This typically indicates that > the provided login details were incorrect." (Note: Only write "The user" if this > metric is gated on a user action.) > > Also, I'm not sure -- does this metric actually cover the case where the form > submission succeeds (in terms of the HTTP POST going through), but the user is > bounced back to a login page showing a warning? That is, is this metric really > measuring what the password manager "thinks" of the form submission? If so, I'd > write the description somewhat differently: "The user submitted a password form, > but the website appeared to reject the credentials. This action reflects the > password manager component's logic for whether or not to offer to save a > password. At the time of writing, failure means that either the form submission > was canceled with a failure message, or the form submission succeeded, but the > user was directed back to a page that still contained a login form." > > (Or, something along those lines, but clearer/more accurate.) =) Done. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12157: <action name="PasswordManager_SubmitPassed"> On 2016/07/07 05:01:48, Ilya Sherman wrote: > (Ditto) Done. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12160: Password form submitted successfully and login form disappeared. On 2016/07/07 05:01:48, Ilya Sherman wrote: > Please similarly clarify this description. Done. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12182: <action name="PasswordManager_UseNonDefaultUsername"> On 2016/07/07 05:01:49, Ilya Sherman wrote: > nit: s/Use/Used Done. https://codereview.chromium.org/2113743002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12185: The user logged in with a credential other than the default one. This On 2016/07/07 05:01:48, Ilya Sherman wrote: > Is this, specifically, a previously-saved credential? Or any credential, even > one that was not previously saved? (A credential that was not previously saved, > presumably, cannot be set as default until it is saved, and it is not saved > until the user accepts Chrome's prompt to save it. So, this description doesn't > quite make sense in its current form.) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. LGTM % a final couple of nits: https://codereview.chromium.org/2113743002/diff/60001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2113743002/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12243: The user submitted a password form, but the website appeared to reject nit: Please remove the extra leading whitespace on this line. https://codereview.chromium.org/2113743002/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12255: The user submitted a password form, the website appeared to accept the nit: s/form, the/form and the
https://codereview.chromium.org/2113743002/diff/60001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2113743002/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12243: The user submitted a password form, but the website appeared to reject On 2016/07/14 23:40:26, Ilya Sherman wrote: > nit: Please remove the extra leading whitespace on this line. Done. https://codereview.chromium.org/2113743002/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:12255: The user submitted a password form, the website appeared to accept the On 2016/07/14 23:40:26, Ilya Sherman wrote: > nit: s/form, the/form and the Done.
The CQ bit was checked by vasilii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2113743002/#ps80001 (title: "last comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add instrumentation for password autofill and Credential Manager API user flows. The instrumentation is implemented via user actions rather than histograms so that events can be sequenced, based on their timestamps. This will allow us to better understand how the Credential Manager API and password autofill interact. For example, we currently observe that users dismiss the account chooser more frequently than we expected. We'd like to estimate whether users had the intention of logging in by measuring whether users log in after dismissing the account chooser; and if so, whether they used the autofill password manager. In addition the CL marks kManagerActionBlacklisted as deprecated since it's unused. BUG=400674 ========== to ========== Add instrumentation for password autofill and Credential Manager API user flows. The instrumentation is implemented via user actions rather than histograms so that events can be sequenced, based on their timestamps. This will allow us to better understand how the Credential Manager API and password autofill interact. For example, we currently observe that users dismiss the account chooser more frequently than we expected. We'd like to estimate whether users had the intention of logging in by measuring whether users log in after dismissing the account chooser; and if so, whether they used the autofill password manager. In addition the CL marks kManagerActionBlacklisted as deprecated since it's unused. BUG=400674 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add instrumentation for password autofill and Credential Manager API user flows. The instrumentation is implemented via user actions rather than histograms so that events can be sequenced, based on their timestamps. This will allow us to better understand how the Credential Manager API and password autofill interact. For example, we currently observe that users dismiss the account chooser more frequently than we expected. We'd like to estimate whether users had the intention of logging in by measuring whether users log in after dismissing the account chooser; and if so, whether they used the autofill password manager. In addition the CL marks kManagerActionBlacklisted as deprecated since it's unused. BUG=400674 ========== to ========== Add instrumentation for password autofill and Credential Manager API user flows. The instrumentation is implemented via user actions rather than histograms so that events can be sequenced, based on their timestamps. This will allow us to better understand how the Credential Manager API and password autofill interact. For example, we currently observe that users dismiss the account chooser more frequently than we expected. We'd like to estimate whether users had the intention of logging in by measuring whether users log in after dismissing the account chooser; and if so, whether they used the autofill password manager. In addition the CL marks kManagerActionBlacklisted as deprecated since it's unused. BUG=400674 Committed: https://crrev.com/bb7617079a36fb15c450ee0f0aab4b35aefb4ecd Cr-Commit-Position: refs/heads/master@{#405741} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bb7617079a36fb15c450ee0f0aab4b35aefb4ecd Cr-Commit-Position: refs/heads/master@{#405741} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
