Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(261)

Issue 949323002: Collecting UMA metrics for the UnmaskPrompt. (Closed)

Created:
5 years, 10 months ago by Walter Cacau
Modified:
5 years, 9 months ago
CC:
chromium-reviews, estade+watch_chromium.org, browser-components-watch_chromium.org, rouslan+autofillwatch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Collecting UMA metrics for the UnmaskPrompt. BUG=454140 Committed: https://crrev.com/1b447a9e6add5a07a3f2357388aeedf08ef53f27 Cr-Commit-Position: refs/heads/master@{#321319}

Patch Set 1 #

Patch Set 2 : Moving collection to CardUnmaskPromptControllerImpl. #

Total comments: 2

Patch Set 3 : Adding first test #

Patch Set 4 : #

Total comments: 16

Patch Set 5 : review comments #

Total comments: 8

Patch Set 6 : review comments 2 #

Total comments: 3

Patch Set 7 : isherman comments #

Total comments: 7

Patch Set 8 : estade comments #

Total comments: 4

Patch Set 9 : #

Patch Set 10 : rebasing #

Patch Set 11 : fixing build #

Patch Set 12 : fix android build #

Unified diffs Side-by-side diffs Delta from patch set Stats (+542 lines, -6 lines) Patch
M chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.h View 1 2 3 4 5 6 7 1 chunk +12 lines, -1 line 0 comments Download
M chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc View 1 2 3 4 5 6 7 6 chunks +74 lines, -5 lines 0 comments Download
A chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +329 lines, -0 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M components/autofill/core/browser/autofill_metrics.h View 1 2 3 4 5 2 chunks +52 lines, -0 lines 0 comments Download
M components/autofill/core/browser/autofill_metrics.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +32 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 3 chunks +42 lines, -0 lines 0 comments Download

Messages

Total messages: 30 (10 generated)
Walter Cacau
5 years, 9 months ago (2015-03-17 01:45:07 UTC) #2
Evan Stade
approach looks fine. ping me when ready for full review https://codereview.chromium.org/949323002/diff/20001/components/autofill/core/browser/autofill_metrics.h File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/949323002/diff/20001/components/autofill/core/browser/autofill_metrics.h#newcode305 ...
5 years, 9 months ago (2015-03-17 01:49:15 UTC) #3
Walter Cacau
Ready for review https://codereview.chromium.org/949323002/diff/20001/components/autofill/core/browser/autofill_metrics.h File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/949323002/diff/20001/components/autofill/core/browser/autofill_metrics.h#newcode305 components/autofill/core/browser/autofill_metrics.h:305: UNMASK_PROMPT_LOCAL_SAVE_KEPT_OPT_IN, On 2015/03/17 01:49:15, Evan Stade ...
5 years, 9 months ago (2015-03-18 01:27:43 UTC) #5
Evan Stade
good stuff. I don't understand why histograms.xml appears to have so many unrelated changes. https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc ...
5 years, 9 months ago (2015-03-18 01:47:59 UTC) #6
Walter Cacau
I think I was able to fix histograms.xml. Apparently running pretty_print.py caused the unrelated changes. ...
5 years, 9 months ago (2015-03-18 17:04:54 UTC) #7
Evan Stade
https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc#newcode70 chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:70: AutofillMetrics::LogUnmaskPromptEvent( you can move this logging down to OnUnmaskDialogClosed ...
5 years, 9 months ago (2015-03-18 21:29:07 UTC) #8
Ilya Sherman
https://codereview.chromium.org/949323002/diff/80001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/949323002/diff/80001/tools/metrics/histograms/histograms.xml#newcode2060 tools/metrics/histograms/histograms.xml:2060: + <summary>Events tracking the usage of the unmasking prompt.</summary> ...
5 years, 9 months ago (2015-03-18 22:48:55 UTC) #9
Walter Cacau
https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc#newcode70 chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:70: AutofillMetrics::LogUnmaskPromptEvent( On 2015/03/18 21:29:07, Evan Stade wrote: > you ...
5 years, 9 months ago (2015-03-18 23:50:05 UTC) #10
Ilya Sherman
Histograms LGTM. Thanks. https://codereview.chromium.org/949323002/diff/100001/components/autofill/core/browser/autofill_metrics.cc File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/949323002/diff/100001/components/autofill/core/browser/autofill_metrics.cc#newcode352 components/autofill/core/browser/autofill_metrics.cc:352: GetRealPanResult metricResult; nit: hacker_case https://codereview.chromium.org/949323002/diff/100001/components/autofill/core/browser/autofill_metrics.cc#newcode357 components/autofill/core/browser/autofill_metrics.cc:357: ...
5 years, 9 months ago (2015-03-19 00:16:01 UTC) #11
Evan Stade
https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc#newcode107 chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:107: LogEvents(); nit: I'd call this LogOnCloseEvents or something https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc#newcode137 ...
5 years, 9 months ago (2015-03-19 00:30:34 UTC) #12
Walter Cacau
https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc#newcode107 chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:107: LogEvents(); On 2015/03/19 00:30:34, Evan Stade wrote: > nit: ...
5 years, 9 months ago (2015-03-19 00:39:25 UTC) #13
Evan Stade
lgtm https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc#newcode137 chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:137: ::UNMASK_PROMPT_CLOSED_FAILED_TO_UNMASK_NON_RETRIABLE_FAILURE); On 2015/03/19 00:39:25, Walter Cacau wrote: > ...
5 years, 9 months ago (2015-03-19 01:06:57 UTC) #14
Walter Cacau
https://codereview.chromium.org/949323002/diff/140001/chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc (right): https://codereview.chromium.org/949323002/diff/140001/chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc#newcode23 chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc:23: CreditCard CreateCard() { On 2015/03/19 01:06:57, Evan Stade wrote: ...
5 years, 9 months ago (2015-03-19 01:20:41 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/949323002/180001
5 years, 9 months ago (2015-03-19 01:32:11 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_rel/builds/70565)
5 years, 9 months ago (2015-03-19 01:49:22 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/949323002/200001
5 years, 9 months ago (2015-03-19 05:36:51 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: android_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile_rel/builds/26155)
5 years, 9 months ago (2015-03-19 05:58:35 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/949323002/220001
5 years, 9 months ago (2015-03-19 06:06:42 UTC) #28
commit-bot: I haz the power
Committed patchset #12 (id:220001)
5 years, 9 months ago (2015-03-19 08:58:33 UTC) #29
commit-bot: I haz the power
5 years, 9 months ago (2015-03-19 08:59:11 UTC) #30
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/1b447a9e6add5a07a3f2357388aeedf08ef53f27
Cr-Commit-Position: refs/heads/master@{#321319}

Powered by Google App Engine
This is Rietveld 408576698