Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1182423004:
Adding UMA stat when generated password set for newly created PasswordFormManager. (Closed)
|
Created:
5 years, 6 months ago by Deepak Modified:
5 years, 5 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. |
DescriptionAdding UMA stat when generated password set for newly created PasswordFormManager.
UMA added for tracking setting of generated password in Password Manager.
BUG=501176
Committed: https://crrev.com/1ac7d6953e17f85410b1a4e35ea96106aac5b23a
Cr-Commit-Position: refs/heads/master@{#336529}
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : Changes as per review comments. #
Total comments: 2
Patch Set 4 : Addressing nit. #
Total comments: 4
Patch Set 5 : Changes as per review comments. #
Messages
Total messages: 31 (8 generated)
deepak.m1@samsung.com changed reviewers: + ajith.v@chromium.org, gcasto@chromium.org, vabr@chromium.org
PTAL
I'm explicitly pushing this review to Garrett, to make sure this interpretation of his TODO is in line with his expectation. Cheers, Vaclav
vabr@chromium.org changed reviewers: - vabr@chromium.org
On 2015/06/17 08:23:32, vabr (Chromium) wrote: > I'm explicitly pushing this review to Garrett, to make sure this interpretation > of his TODO is in line with his expectation. > > Cheers, > Vaclav @vabr I agree with you.
peer review looks good to me!
On 2015/06/17 12:20:15, AKV wrote: > peer review looks good to me! @Garrett PTAL
Friendly ping!
Friendly ping!
https://codereview.chromium.org/1182423004/diff/20001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/1182423004/diff/20001/components/password_man... components/password_manager/core/browser/password_manager.cc:159: } For completeness, this loop here should actually be the same as the loop in ProvisionallySavePassword(). It's unlikely that this function will be triggered where there isn't a perfect match, but it's possible. https://codereview.chromium.org/1182423004/diff/20001/components/password_man... components/password_manager/core/browser/password_manager.cc:173: UMA_HISTOGRAM_BOOLEAN("PasswordManager.SetGeneratedPasswordForForm", The name should be something like "PasswordManager.GeneratedFormHasNoFormManager". It also should be called before line 161, as |password_is_generated| will always be true at this point. https://codereview.chromium.org/1182423004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1182423004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27636: + Indicates setting of generated password for newly created Something like "Indicates if the generation state of a password was changed but existing form was found."
@Garrett Changes done as suggested. PTAL https://codereview.chromium.org/1182423004/diff/20001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/1182423004/diff/20001/components/password_man... components/password_manager/core/browser/password_manager.cc:159: } On 2015/06/22 23:23:57, Garrett Casto wrote: > For completeness, this loop here should actually be the same as the loop in > ProvisionallySavePassword(). It's unlikely that this function will be triggered > where there isn't a perfect match, but it's possible. Done. https://codereview.chromium.org/1182423004/diff/20001/components/password_man... components/password_manager/core/browser/password_manager.cc:173: UMA_HISTOGRAM_BOOLEAN("PasswordManager.SetGeneratedPasswordForForm", On 2015/06/22 23:23:57, Garrett Casto wrote: > The name should be something like > "PasswordManager.GeneratedFormHasNoFormManager". It also should be called before > line 161, as |password_is_generated| will always be true at this point. Done. https://codereview.chromium.org/1182423004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1182423004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27636: + Indicates setting of generated password for newly created On 2015/06/22 23:23:57, Garrett Casto wrote: > Something like "Indicates if the generation state of a password was changed but > existing form was found." Done.
@Garrett Friendly Ping..
@Garrett Please review and let me know if anything more required. Thanks
lgtm https://codereview.chromium.org/1182423004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1182423004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27665: + form was found. was not found.
Thanks for review. nit Addressed. https://codereview.chromium.org/1182423004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1182423004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27665: + form was found. On 2015/06/26 21:49:43, Garrett Casto wrote: > was not found. Done.
The CQ bit was checked by deepak.m1@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from gcasto@chromium.org Link to the patchset: https://codereview.chromium.org/1182423004/#ps60001 (title: "Addressing nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1182423004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
deepak.m1@samsung.com changed reviewers: + isherman@chromium.org
adding @isherman for tools/metrics/histograms/histograms.xml rubber stamp approval. Please approve.
https://codereview.chromium.org/1182423004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1182423004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27711: +<histogram name="PasswordManager.GeneratedFormHasNoFormManager" enum="Boolean"> Please define and use a custom boolean here, with labels like "Has form manager" and "Lacks for manager". https://codereview.chromium.org/1182423004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27717: + form was not found. nit: Suggested rephrasing (feel free to refine further): "When the generation state of a password was changed, records whether an existing form corresponding to the password was found."
Thanks @Ilya for review. Changes done as suggested. PTAL https://codereview.chromium.org/1182423004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1182423004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27711: +<histogram name="PasswordManager.GeneratedFormHasNoFormManager" enum="Boolean"> On 2015/06/28 02:19:11, Ilya Sherman wrote: > Please define and use a custom boolean here, with labels like "Has form manager" > and "Lacks for manager". Done. https://codereview.chromium.org/1182423004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27717: + form was not found. On 2015/06/28 02:19:11, Ilya Sherman wrote: > nit: Suggested rephrasing (feel free to refine further): "When the generation > state of a password was changed, records whether an existing form corresponding > to the password was found." Done.
histograms lgtm, thanks
The CQ bit was checked by isherman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gcasto@chromium.org Link to the patchset: https://codereview.chromium.org/1182423004/#ps80001 (title: "Changes as per review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1182423004/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/1ac7d6953e17f85410b1a4e35ea96106aac5b23a Cr-Commit-Position: refs/heads/master@{#336529}
Message was sent while issue was closed.
On 2015/06/29 04:10:10, Ilya Sherman wrote: > histograms lgtm, thanks Thank you Ilya for quick review :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
