Add metrics for interactions with the Apps Launcher promo dialog.

Add metrics for interactions with the Apps Launcher promo dialog.

BUG=252702
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209300

[MAD, 2013-06-21 19:13:29]

Let me know if you prefer I ask someone else to review this...

Thanks!

BYE
MAD

[MAD, 2013-06-25 11:05:50]

ping?

Should I ask someone else?

Thanks!

BYE
MAD

[miket_OOO, 2013-06-25 15:57:25]

lgtm

https://codereview.chromium.org/17226003/diff/1/chrome/browser/ui/webui/app_l...
File chrome/browser/ui/webui/app_launcher_page_ui.cc (right):

https://codereview.chromium.org/17226003/diff/1/chrome/browser/ui/webui/app_l...
chrome/browser/ui/webui/app_launcher_page_ui.cc:126: 
Extra whitespace not needed.

https://codereview.chromium.org/17226003/diff/1/chrome/browser/ui/webui/ntp/a...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

https://codereview.chromium.org/17226003/diff/1/chrome/browser/ui/webui/ntp/a...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:107: void OnLearnMore(const
base::ListValue* args);
I know you were just following the pattern, but these comments don't tell the
reader anything, and they're subject to copy/paste errors.

[Ilya Sherman, 2013-06-25 21:35:15]

https://codereview.chromium.org/17226003/diff/1/chrome/browser/ui/webui/ntp/a...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/17226003/diff/1/chrome/browser/ui/webui/ntp/a...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:703:
apps::APP_LAUNCHER_PROMO_MAX);
I'd strongly recommend creating a method like
apps::LogAppLauncherPromoHistogram(), and just passing in
apps::APP_LAUNCHER_PROMO_DISMISSED from here.  That both reduces the total
number of lines of code, and makes it harder for a single callsite to have a
typo or other easy-to-miss error.

https://codereview.chromium.org/17226003/diff/1/tools/metrics/histograms/hist...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/17226003/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:12166: <int value="3"
label="LEARN_MORE"/>
Optional nit: I'd recommend making these be more human-readable/friendly labels,
e.g. "Learn more" instead of "LEARN_MORE".

[MAD, 2013-06-26 03:33:40]

Thanks! All addressed. How about this now?

Addin dbeam@ for webui owner review...

BYE
MAD

https://codereview.chromium.org/17226003/diff/1/chrome/browser/ui/webui/app_l...
File chrome/browser/ui/webui/app_launcher_page_ui.cc (right):

https://codereview.chromium.org/17226003/diff/1/chrome/browser/ui/webui/app_l...
chrome/browser/ui/webui/app_launcher_page_ui.cc:126: 
On 2013/06/25 15:57:25, miket wrote:
> Extra whitespace not needed.

Done.

https://codereview.chromium.org/17226003/diff/1/chrome/browser/ui/webui/ntp/a...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/17226003/diff/1/chrome/browser/ui/webui/ntp/a...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:703:
apps::APP_LAUNCHER_PROMO_MAX);
On 2013/06/25 21:35:15, Ilya Sherman wrote:
> I'd strongly recommend creating a method like
> apps::LogAppLauncherPromoHistogram(), and just passing in
> apps::APP_LAUNCHER_PROMO_DISMISSED from here.  That both reduces the total
> number of lines of code, and makes it harder for a single callsite to have a
> typo or other easy-to-miss error.

Done.

https://codereview.chromium.org/17226003/diff/1/chrome/browser/ui/webui/ntp/a...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

https://codereview.chromium.org/17226003/diff/1/chrome/browser/ui/webui/ntp/a...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:107: void OnLearnMore(const
base::ListValue* args);
On 2013/06/25 15:57:25, miket wrote:
> I know you were just following the pattern, but these comments don't tell the
> reader anything, and they're subject to copy/paste errors.

Done.

https://codereview.chromium.org/17226003/diff/1/tools/metrics/histograms/hist...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/17226003/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:12166: <int value="3"
label="LEARN_MORE"/>
On 2013/06/25 21:35:15, Ilya Sherman wrote:
> Optional nit: I'd recommend making these be more human-readable/friendly
labels,
> e.g. "Learn more" instead of "LEARN_MORE".

Done.
I was following the style of the others around...

[MAD, 2013-06-26 20:44:21]

Ping?

- Ilya for how I addressed your comments
- dbeam for webui owner review

Thanks!

BYE
MAD

[Ilya Sherman, 2013-06-26 22:19:49]

https://codereview.chromium.org/17226003/diff/12001/apps/apps.gypi
File apps/apps.gypi (right):

https://codereview.chromium.org/17226003/diff/12001/apps/apps.gypi#newcode52
apps/apps.gypi:52: 'metrics_names.h',
Did you mean to include these files in this CL?  I don't see them included in
Patch Set 2.

https://codereview.chromium.org/17226003/diff/12001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/17226003/diff/12001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:803:
apps::AppLauncherPromoHistogramValues histogram) {
nit: Please add a DCHECK() verifying that the argument is less than
APP_LAUNCHER_PROMO_MAX.

https://codereview.chromium.org/17226003/diff/12001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

https://codereview.chromium.org/17226003/diff/12001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:106:
apps::AppLauncherPromoHistogramValues histogram);
nit: Docs?

https://codereview.chromium.org/17226003/diff/12001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:106:
apps::AppLauncherPromoHistogramValues histogram);
nit: "histogram" is a misleading variable name here; what you're recording is a
metric or a sample or a value, not a whole histogram.

[Dan Beam, 2013-06-27 00:59:31]

webui lgtm

https://codereview.chromium.org/17226003/diff/12001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

https://codereview.chromium.org/17226003/diff/12001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:90: void
HandleGenerateAppForLink(const base::ListValue* args);
nit: in theory these should mention which args will be passed to them (which is
why they're traditionally spread apart more like they were before)

[MAD, 2013-06-27 21:43:35]

How about this now?

Thanks!

BYE
MAD...

https://codereview.chromium.org/17226003/diff/12001/apps/apps.gypi
File apps/apps.gypi (right):

https://codereview.chromium.org/17226003/diff/12001/apps/apps.gypi#newcode52
apps/apps.gypi:52: 'metrics_names.h',
On 2013/06/26 22:19:49, Ilya Sherman wrote:
> Did you mean to include these files in this CL?  I don't see them included in
> Patch Set 2.

Done.

https://codereview.chromium.org/17226003/diff/12001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/17226003/diff/12001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:803:
apps::AppLauncherPromoHistogramValues histogram) {
On 2013/06/26 22:19:49, Ilya Sherman wrote:
> nit: Please add a DCHECK() verifying that the argument is less than
> APP_LAUNCHER_PROMO_MAX.

Done.

https://codereview.chromium.org/17226003/diff/12001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

https://codereview.chromium.org/17226003/diff/12001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:90: void
HandleGenerateAppForLink(const base::ListValue* args);
On 2013/06/27 00:59:31, Dan Beam wrote:
> nit: in theory these should mention which args will be passed to them (which
is
> why they're traditionally spread apart more like they were before)

Done.

https://codereview.chromium.org/17226003/diff/12001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:106:
apps::AppLauncherPromoHistogramValues histogram);
On 2013/06/26 22:19:49, Ilya Sherman wrote:
> nit: "histogram" is a misleading variable name here; what you're recording is
a
> metric or a sample or a value, not a whole histogram.

Done.

https://codereview.chromium.org/17226003/diff/12001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:106:
apps::AppLauncherPromoHistogramValues histogram);
On 2013/06/26 22:19:49, Ilya Sherman wrote:
> nit: Docs?

Done.

https://codereview.chromium.org/17226003/diff/44001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (left):

https://codereview.chromium.org/17226003/diff/44001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:110: void
HandleSetNotificationsDisabled(const base::ListValue* args);
Those last two were not in the implementation anymore.

[Ilya Sherman, 2013-06-27 23:15:55]

LGTM, thanks.

https://codereview.chromium.org/17226003/diff/44001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/17226003/diff/44001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:770: DCHECK(bucket <
extension_misc::APP_LAUNCH_BUCKET_BOUNDARY);
nit: DCHECK_LT

https://codereview.chromium.org/17226003/diff/44001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:805: DCHECK(value <
apps::APP_LAUNCHER_PROMO_MAX);
nit: DCHECK_LT

https://codereview.chromium.org/17226003/diff/44001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:806:
UMA_HISTOGRAM_ENUMERATION(apps::kAppLauncherPromoHistogram,
Optional nit: I'd just inline the histogram name at this point, but up to you.

[MAD, 2013-06-27 23:21:05]

Thanks! CQ'ing...

BYE
MAD

https://codereview.chromium.org/17226003/diff/44001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/17226003/diff/44001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:770: DCHECK(bucket <
extension_misc::APP_LAUNCH_BUCKET_BOUNDARY);
On 2013/06/27 23:15:56, Ilya Sherman wrote:
> nit: DCHECK_LT

Done.

https://codereview.chromium.org/17226003/diff/44001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:805: DCHECK(value <
apps::APP_LAUNCHER_PROMO_MAX);
On 2013/06/27 23:15:56, Ilya Sherman wrote:
> nit: DCHECK_LT

Done.

https://codereview.chromium.org/17226003/diff/44001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:806:
UMA_HISTOGRAM_ENUMERATION(apps::kAppLauncherPromoHistogram,
On 2013/06/27 23:15:56, Ilya Sherman wrote:
> Optional nit: I'd just inline the histogram name at this point, but up to you.

We also use it in another file, so I prefer to keep it that way...

[commit-bot: I haz the power, 2013-06-27 23:22:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/17226003/55001

[commit-bot: I haz the power, 2013-06-28 00:02:01]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Ilya Sherman, 2013-06-28 00:37:32]

https://codereview.chromium.org/17226003/diff/44001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/17226003/diff/44001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:806:
UMA_HISTOGRAM_ENUMERATION(apps::kAppLauncherPromoHistogram,
On 2013/06/27 23:21:05, MAD wrote:
> On 2013/06/27 23:15:56, Ilya Sherman wrote:
> > Optional nit: I'd just inline the histogram name at this point, but up to
you.
> 
> We also use it in another file, so I prefer to keep it that way...

Which other file?

[MAD, 2013-06-28 03:31:35]

On 2013/06/28 00:37:32, Ilya Sherman wrote:
>
https://codereview.chromium.org/17226003/diff/44001/chrome/browser/ui/webui/n...
> File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):
> 
>
https://codereview.chromium.org/17226003/diff/44001/chrome/browser/ui/webui/n...
> chrome/browser/ui/webui/ntp/app_launcher_handler.cc:806:
> UMA_HISTOGRAM_ENUMERATION(apps::kAppLauncherPromoHistogram,
> On 2013/06/27 23:21:05, MAD wrote:
> > On 2013/06/27 23:15:56, Ilya Sherman wrote:
> > > Optional nit: I'd just inline the histogram name at this point, but up to
> you.
> > 
> > We also use it in another file, so I prefer to keep it that way...
> 
> Which other file?

D'Ho! I forgot that I also call this static method method in the other file...
Stoopid MAD... ;-)

[commit-bot: I haz the power, 2013-06-28 17:36:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/17226003/71001

[commit-bot: I haz the power, 2013-06-28 18:05:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-28 18:16:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/17226003/89001

[commit-bot: I haz the power, 2013-06-28 23:01:23]

Retried try job too often on win_rel for step(s) chrome_frame_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-29 03:34:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/17226003/89001

[commit-bot: I haz the power, 2013-06-29 13:42:52]

Change committed as 209300