Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1182913004:
Add a metric for autofilled Android credentials. (Closed)
|
Created:
5 years, 6 months ago by msramek Modified:
5 years, 6 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlist_chromium.org, asvitkine+watch_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 metric for autofilled Android credentials.
BUG=437865
Committed: https://crrev.com/00b54761877196c979f5f03102112b311234b439
Cr-Commit-Position: refs/heads/master@{#335247}
Patch Set 1 #Patch Set 2 : Rename to "Offered to fill" #Patch Set 3 : Forgot about enum. #
Total comments: 9
Patch Set 4 : Addressed comments. #
Total comments: 4
Patch Set 5 : Changed the histogram name. #
Messages
Total messages: 13 (3 generated)
msramek@chromium.org changed reviewers: + isherman@chromium.org, vabr@chromium.org
Hi Václav and Ilya, could you have a look? Thanks, Martin
Actually, since we're only reporting that one of the options is Android credentials, this is a bit of a misnomer. Let me change this to "offered to fill".
LGTM with some comments. Thanks! Vaclav https://codereview.chromium.org/1182913004/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/1182913004/diff/40001/components/password_man... components/password_manager/core/browser/password_manager.cc:115: const autofill::PasswordFormFillData& fill_data) { Note that the declaration comment for PasswordFormFillData says: " Note that the realms in this struct are only set when the password's realm differs from the realm of the form that we are filling." Should you be passing the filled form realm as well? https://codereview.chromium.org/1182913004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1182913004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27507: + credentials in the dropdown come from an Android app. Please double-check if it should be "come" or "comes". It is 3rd person singular, but perhaps not indicative. I give up. :) https://codereview.chromium.org/1182913004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62135: + <int value="0" label="Credentials NOT from Android"/> Would "No credentials from Android" "At least one credential from Android" be more precise?
https://codereview.chromium.org/1182913004/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/1182913004/diff/40001/components/password_man... components/password_manager/core/browser/password_manager.cc:115: const autofill::PasswordFormFillData& fill_data) { On 2015/06/18 13:21:47, vabr (Chromium) wrote: > Note that the declaration comment for PasswordFormFillData says: " Note that the > realms in this struct are only set when the password's realm differs from the > realm of the form that we are filling." > > Should you be passing the filled form realm as well? My understanding is that no. We're looking for the original signon realm "android://...", but we're filling into an HTML form in the browser, so the domain is necessarily not "android://...", so they must differ. Or is it possible for this code to run in an "android://..." context via something like Android Vebview? https://codereview.chromium.org/1182913004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1182913004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27507: + credentials in the dropdown come from an Android app. On 2015/06/18 13:21:47, vabr (Chromium) wrote: > Please double-check if it should be "come" or "comes". It is 3rd person > singular, but perhaps not indicative. I give up. :) Sorry, I just messed up. It's not conjunctive or anything. I was referring to "one pair of username and password" as "one credentials", which itself doesn't make much sense. After reading http://english.stackexchange.com/questions/252073/which-is-correct-credential..., I decided to just use the singular "credential" for everyone's sanity. https://codereview.chromium.org/1182913004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62135: + <int value="0" label="Credentials NOT from Android"/> On 2015/06/18 13:21:47, vabr (Chromium) wrote: > Would > "No credentials from Android" > "At least one credential from Android" > be more precise? Ah, thanks. I wanted to use very general wording, so I can reuse this enum for more metrics, and didn't realize that it no longer works for this one. Since this enum is now tied specifically to this particular metrics, I again changed the naming to contain the word "offer".
lgtm https://codereview.chromium.org/1182913004/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/1182913004/diff/40001/components/password_man... components/password_manager/core/browser/password_manager.cc:115: const autofill::PasswordFormFillData& fill_data) { On 2015/06/18 14:22:27, msramek wrote: > On 2015/06/18 13:21:47, vabr (Chromium) wrote: > > Note that the declaration comment for PasswordFormFillData says: " Note that > the > > realms in this struct are only set when the password's realm differs from the > > realm of the form that we are filling." > > > > Should you be passing the filled form realm as well? > > My understanding is that no. We're looking for the original signon realm > "android://...", but we're filling into an HTML form in the browser, so the > domain is necessarily not "android://...", so they must differ. > > Or is it possible for this code to run in an "android://..." context via > something like Android Vebview? Acknowledged. The Webview should not fill passwords, it should be isolated from the user's profile. https://codereview.chromium.org/1182913004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1182913004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27507: + credentials in the dropdown come from an Android app. On 2015/06/18 14:22:27, msramek wrote: > On 2015/06/18 13:21:47, vabr (Chromium) wrote: > > Please double-check if it should be "come" or "comes". It is 3rd person > > singular, but perhaps not indicative. I give up. :) > > Sorry, I just messed up. It's not conjunctive or anything. > > I was referring to "one pair of username and password" as "one credentials", > which itself doesn't make much sense. After reading > http://english.stackexchange.com/questions/252073/which-is-correct-credential..., > I decided to just use the singular "credential" for everyone's sanity. Acknowledged. https://codereview.chromium.org/1182913004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62135: + <int value="0" label="Credentials NOT from Android"/> On 2015/06/18 14:22:27, msramek wrote: > On 2015/06/18 13:21:47, vabr (Chromium) wrote: > > Would > > "No credentials from Android" > > "At least one credential from Android" > > be more precise? > > Ah, thanks. I wanted to use very general wording, so I can reuse this enum for > more metrics, and didn't realize that it no longer works for this one. > > Since this enum is now tied specifically to this particular metrics, I again > changed the naming to contain the word "offer". Acknowledged.
Histograms LGTM % nits. https://codereview.chromium.org/1182913004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1182913004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27501: +<histogram name="PasswordManager.OfferedToFillAndroidCredentials" nit: I'd suggest something like "SuggestionsIncludeAndroidAppCredentials" for clarity -- IMO that name makes it clearer what the baseline is. https://codereview.chromium.org/1182913004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62136: + <int value="1" label="At least one credential is from Android"/> nit: These names are somewhat long, given that they'll be shown as the left column of a table-like display in the dashboard. It's fine if you prefer to keep these, but I'd suggest something more like "None from Android" and "1+ from Android".
https://codereview.chromium.org/1182913004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1182913004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27501: +<histogram name="PasswordManager.OfferedToFillAndroidCredentials" On 2015/06/18 21:00:13, Ilya Sherman wrote: > nit: I'd suggest something like "SuggestionsIncludeAndroidAppCredentials" for > clarity -- IMO that name makes it clearer what the baseline is. Done. Added "Fill" as prefix to clarify what is meant by suggestions. https://codereview.chromium.org/1182913004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62136: + <int value="1" label="At least one credential is from Android"/> On 2015/06/18 21:00:13, Ilya Sherman wrote: > nit: These names are somewhat long, given that they'll be shown as the left > column of a table-like display in the dashboard. It's fine if you prefer to > keep these, but I'd suggest something more like "None from Android" and "1+ from > Android". Makes sense. Done.
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1182913004/#ps80001 (title: "Changed the histogram name.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1182913004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/00b54761877196c979f5f03102112b311234b439 Cr-Commit-Position: refs/heads/master@{#335247} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
