Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2481453002:
Add "PasswordManager.SignInPromoDismissalCount" histogram. (Closed)
DescriptionAdd "PasswordManager.SignInPromoDismissalCount" histogram.
It measures number of times the user dismissed the sign-in promo in the password bubble.
BUG=662432
Committed: https://crrev.com/96dc6b4f71706f04a41e2abe3af204529c535c14
Cr-Commit-Position: refs/heads/master@{#430575}
Patch Set 1 #
Total comments: 12
Patch Set 2 : comments #
Total comments: 2
Patch Set 3 : comments #
Messages
Total messages: 22 (13 generated)
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vasilii@chromium.org changed reviewers: + isherman@chromium.org
Hi Ilya, please review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2481453002/diff/1/chrome/browser/ui/passwords... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/2481453002/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:133: int sign_in_promo_count; nit: I'd add "shown" before "count" https://codereview.chromium.org/2481453002/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:184: UMA_HISTOGRAM_COUNTS_100("PasswordManager.SignInPromoDismissalCount", Does CHROME_SIGNIN_CANCEL not correspond to a dismissal? https://codereview.chromium.org/2481453002/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:457: show_count++; nit: Please use pre-increment. https://codereview.chromium.org/2481453002/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:460: interaction_keeper_->set_sign_in_promo_count(show_count); What's the purpose of tracking this in two locations? That is, why not just track it exclusively via the pref that's being set above? https://codereview.chromium.org/2481453002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2481453002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:42453: + The number of times the Sign In promo in the password bubble was dismissed. Please document precisely when this is recorded.
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2481453002/diff/1/chrome/browser/ui/passwords... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/2481453002/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:133: int sign_in_promo_count; On 2016/11/05 01:31:39, Ilya Sherman wrote: > nit: I'd add "shown" before "count" Done. https://codereview.chromium.org/2481453002/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:184: UMA_HISTOGRAM_COUNTS_100("PasswordManager.SignInPromoDismissalCount", On 2016/11/05 01:31:40, Ilya Sherman wrote: > Does CHROME_SIGNIN_CANCEL not correspond to a dismissal? No. The request was to count implicit dismissals. CHROME_SIGNIN_CANCEL and CHROME_SIGNIN_OK both contribute to click-through rate. The dialog won't show up again. https://codereview.chromium.org/2481453002/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:457: show_count++; On 2016/11/05 01:31:39, Ilya Sherman wrote: > nit: Please use pre-increment. Why? https://codereview.chromium.org/2481453002/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:460: interaction_keeper_->set_sign_in_promo_count(show_count); On 2016/11/05 01:31:39, Ilya Sherman wrote: > What's the purpose of tracking this in two locations? That is, why not just > track it exclusively via the pref that's being set above? Because if one closes the tab then GetWebContents() returns 0. PasswordsModelDelegate may also be null. Their destruction order can't be predicted. I can't find a profile then. https://codereview.chromium.org/2481453002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2481453002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:42453: + The number of times the Sign In promo in the password bubble was dismissed. On 2016/11/05 01:31:40, Ilya Sherman wrote: > Please document precisely when this is recorded. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % a nit: https://codereview.chromium.org/2481453002/diff/1/chrome/browser/ui/passwords... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/2481453002/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:457: show_count++; On 2016/11/07 10:40:24, vasilii wrote: > On 2016/11/05 01:31:39, Ilya Sherman wrote: > > nit: Please use pre-increment. > > Why? Hmm, I think the style guide used to recommend pre-incrmenet universally. I guess that's not the case anymore, so nevermind. https://codereview.chromium.org/2481453002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2481453002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42454: + dismissed. It's recorded when the event happens. nit: Suggested rephrasing for the second sentence: "Recorded each time the promo is implicitly dismissed".
vabr@chromium.org changed reviewers: + vabr@chromium.org
https://codereview.chromium.org/2481453002/diff/1/chrome/browser/ui/passwords... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/2481453002/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:457: show_count++; On 2016/11/07 19:43:20, Ilya Sherman wrote: > On 2016/11/07 10:40:24, vasilii wrote: > > On 2016/11/05 01:31:39, Ilya Sherman wrote: > > > nit: Please use pre-increment. > > > > Why? > > Hmm, I think the style guide used to recommend pre-incrmenet universally. I > guess that's not the case anymore, so nevermind. The style guide still requires it for iterators, but explicitly says it does not matter for non-object values: https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement
https://codereview.chromium.org/2481453002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2481453002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42454: + dismissed. It's recorded when the event happens. On 2016/11/07 19:43:20, Ilya Sherman wrote: > nit: Suggested rephrasing for the second sentence: "Recorded each time the promo > is implicitly dismissed". 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/2481453002/#ps40001 (title: "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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add "PasswordManager.SignInPromoDismissalCount" histogram. It measures number of times the user dismissed the sign-in promo in the password bubble. BUG=662432 ========== to ========== Add "PasswordManager.SignInPromoDismissalCount" histogram. It measures number of times the user dismissed the sign-in promo in the password bubble. BUG=662432 Committed: https://crrev.com/96dc6b4f71706f04a41e2abe3af204529c535c14 Cr-Commit-Position: refs/heads/master@{#430575} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/96dc6b4f71706f04a41e2abe3af204529c535c14 Cr-Commit-Position: refs/heads/master@{#430575} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
