[Media Router] Add P1 UMA metrics for Media Router.

[Media Router] Add P1 UMA metrics for Media Router.

This change adds the following metrics:
- Count of times the Media Router icon is clicked.
- Where the user clicked to open the Media Router dialog.
- Count of times a session is successful/unsuccessful in starting.
- Number of devices displayed after 3s of the Media Router dialog loading.

BUG=541372
Committed: https://crrev.com/9f153fafa1f2f58376308d41732c50ca906c6bad
Cr-Commit-Position: refs/heads/master@{#356150}

[apacible, 2015-10-22 23:49:40]

Patchset #1 (id:1) has been deleted

[apacible, 2015-10-23 00:58:00]

The CQ bit was checked by apacible@chromium.org to run a CQ dry run

[apacible, 2015-10-23 00:58:06]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2015-10-23 00:58:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423683002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423683002/40001

[apacible, 2015-10-23 00:59:30]

Description was changed from

==========
p1 uma

BUG=
==========

to

==========
[Media Router] Add P1 UMA metrics for Media Router.

This change adds the following metrics:
- Count of times the Media Router icon is clicked.
- Where the user clicked to open the Media Router dialog.
- Count of times a session is successful/unsuccessful in starting.
- Number of devices displayed after 3s of the Media Router dialog loading.

BUG=541372
==========

[apacible, 2015-10-23 01:04:18]

Patchset #1 (id:40001) has been deleted

[apacible, 2015-10-23 01:04:23]

Patchset #1 (id:60001) has been deleted

[apacible, 2015-10-23 01:04:27]

The CQ bit was checked by apacible@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-23 01:04:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423683002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423683002/80001

[commit-bot: I haz the power, 2015-10-23 03:40:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-23 03:40:03]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[apacible, 2015-10-23 05:34:55]

Patchset #1 (id:80001) has been deleted

[apacible, 2015-10-23 05:38:31]

apacible@chromium.org changed reviewers:
+ cpu@chromium.org, imcheng@chromium.org, isherman@chromium.org,
pkasting@chromium.org

[apacible, 2015-10-23 05:38:31]

PTAL, thanks!

cpu for chrome/browser/renderer_context_menu/*
imcheng for everything
isherman for tools/metrics/*
pkasting for c/b/ui/*

[Ilya Sherman, 2015-10-23 20:40:22]

https://codereview.chromium.org/1423683002/diff/100001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1423683002/diff/100001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:7: #include
"base/metrics/histogram.h"
nit: Please include histogram_macros instead

https://codereview.chromium.org/1423683002/diff/100001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:147:
media_router::TOTAL_COUNT);
nit: Please create a wrapper function for logging to this one histogram, that
you'd call as something like
RecordMediaRouterDialogOrigin(media_router::TOOLBAR);

https://codereview.chromium.org/1423683002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/1423683002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:11:
#include "base/metrics/histogram_base.h"
Please replace these with a histogram_macros include.

https://codereview.chromium.org/1423683002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:217:
"MediaRouter.Ui.Action.StartLocalSessionSuccessful", false);
Please record this as UMA_HISTOGRAM_BOOLEAN(..., route);, so that the macro is
only written once for this histogram.

https://codereview.chromium.org/1423683002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1423683002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:18748: +<histogram
name="MediaRouter.Ui.Action.StartLocalSessionSuccessful">
Please associate an enum with this histogram.

[cpu_(ooo_6.6-7.5), 2015-10-23 20:56:03]

lgtm for render_view_context_menu.cc

[apacible, 2015-10-23 22:42:32]

Patchset #2 (id:120001) has been deleted

[apacible, 2015-10-23 22:52:55]

https://codereview.chromium.org/1423683002/diff/100001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1423683002/diff/100001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:7: #include
"base/metrics/histogram.h"
On 2015/10/23 20:40:22, Ilya Sherman wrote:
> nit: Please include histogram_macros instead

Done (moved to media_router_metrics).

https://codereview.chromium.org/1423683002/diff/100001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:147:
media_router::TOTAL_COUNT);
On 2015/10/23 20:40:22, Ilya Sherman wrote:
> nit: Please create a wrapper function for logging to this one histogram, that
> you'd call as something like
> RecordMediaRouterDialogOrigin(media_router::TOOLBAR);

Done.

https://codereview.chromium.org/1423683002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/1423683002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:11:
#include "base/metrics/histogram_base.h"
On 2015/10/23 20:40:22, Ilya Sherman wrote:
> Please replace these with a histogram_macros include.

Done.

https://codereview.chromium.org/1423683002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:217:
"MediaRouter.Ui.Action.StartLocalSessionSuccessful", false);
On 2015/10/23 20:40:22, Ilya Sherman wrote:
> Please record this as UMA_HISTOGRAM_BOOLEAN(..., route);, so that the macro is
> only written once for this histogram.

Done.

https://codereview.chromium.org/1423683002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1423683002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:18748: +<histogram
name="MediaRouter.Ui.Action.StartLocalSessionSuccessful">
On 2015/10/23 20:40:22, Ilya Sherman wrote:
> Please associate an enum with this histogram.

Done.

[Peter Kasting, 2015-10-23 23:48:51]

LGTM

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/toolbar/media_router_action_platform_delegate_cocoa.mm
(right):

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/toolbar/media_router_action_platform_delegate_cocoa.mm:39:
if ([wrenchMenuController isMenuOpen]) {
Nit: If you reverse this conditional you can avoid the {}.

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:145: }
Nit: Shorter:

    media_router::MediaRouterMetrics::RecordMediaRouterDialogOrigin(
        GetPlatformDelegate()->CloseOverflowMenuIfOpen() ?
            media_router::OVERFLOW_MENU : media_router::TOOLBAR);

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/view...
File
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc
(right):

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc:33:
if (app_menu_button->IsMenuShowing()) {
Nit: Same comment

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:387:
DVLOG(1) << "OnReportSinkCount";
Do you need these?  We try hard to avoid logging and it seems like this is the
sort of thing we shouldn't check in?

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:389:
int sink_count = -1;
Do you get a warning if you omit the initializers here?  If not, I'd omit them.

[apacible, 2015-10-24 22:06:28]

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/toolbar/media_router_action_platform_delegate_cocoa.mm
(right):

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/toolbar/media_router_action_platform_delegate_cocoa.mm:39:
if ([wrenchMenuController isMenuOpen]) {
On 2015/10/23 23:48:51, Peter Kasting wrote:
> Nit: If you reverse this conditional you can avoid the {}.

Done.

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:145: }
On 2015/10/23 23:48:51, Peter Kasting wrote:
> Nit: Shorter:
> 
>     media_router::MediaRouterMetrics::RecordMediaRouterDialogOrigin(
>         GetPlatformDelegate()->CloseOverflowMenuIfOpen() ?
>             media_router::OVERFLOW_MENU : media_router::TOOLBAR);

Done.

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/view...
File
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc
(right):

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc:33:
if (app_menu_button->IsMenuShowing()) {
On 2015/10/23 23:48:51, Peter Kasting wrote:
> Nit: Same comment

Done.

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:387:
DVLOG(1) << "OnReportSinkCount";
On 2015/10/23 23:48:51, Peter Kasting wrote:
> Do you need these?  We try hard to avoid logging and it seems like this is the
> sort of thing we shouldn't check in?

+imcheng: Do you remember our plans with the per-function logging here (and
elsewhere)?

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:389:
int sink_count = -1;
On 2015/10/23 23:48:51, Peter Kasting wrote:
> Do you get a warning if you omit the initializers here?  If not, I'd omit
them.

Done.

[Ilya Sherman, 2015-10-26 02:34:03]

LGTM

[imcheng, 2015-10-26 17:18:14]

lgtm

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/1423683002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:387:
DVLOG(1) << "OnReportSinkCount";
On 2015/10/24 22:06:28, apacible wrote:
> On 2015/10/23 23:48:51, Peter Kasting wrote:
> > Do you need these?  We try hard to avoid logging and it seems like this is
the
> > sort of thing we shouldn't check in?
> 
> +imcheng: Do you remember our plans with the per-function logging here (and
> elsewhere)?

It helps with our debugging. Since it is only a DVLOG I feel it is fine to check
in for now, but once Media Router is in a more stable state then we can do a
cleanup to remove excessive logging.

https://codereview.chromium.org/1423683002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/media_router/media_router_ui_interface.js (right):

https://codereview.chromium.org/1423683002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/media_router/media_router_ui_interface.js:152:
chrome.send('reportSinkCount', [{sinkCount: sinkCount}]);
Is it possible to just return the sinkCount in the list without using object?

chrome.send('reportSinkCount', [sinkCount]);

[apacible, 2015-10-26 17:50:56]

Patchset #4 (id:180001) has been deleted

[apacible, 2015-10-26 17:52:32]

https://codereview.chromium.org/1423683002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/1423683002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:217:
"MediaRouter.Ui.Action.StartLocalSessionSuccessful", false);
On 2015/10/23 22:52:55, apacible wrote:
> On 2015/10/23 20:40:22, Ilya Sherman wrote:
> > Please record this as UMA_HISTOGRAM_BOOLEAN(..., route);, so that the macro
is
> > only written once for this histogram.
> 
> Done.

Switched back because of these warnings:

e:\b\build\slave\win\build\src\chrome\browser\ui\webui\media_router\media_router_webui_message_handler.cc(214)
: error C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win\build\src\chrome\browser\ui\webui\media_router\media_router_webui_message_handler.cc(214)
: warning C4800: 'const media_router::MediaRoute *' : forcing value to bool
'true' or 'false' (performance warning)

https://codereview.chromium.org/1423683002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/media_router/media_router_ui_interface.js (right):

https://codereview.chromium.org/1423683002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/media_router/media_router_ui_interface.js:152:
chrome.send('reportSinkCount', [{sinkCount: sinkCount}]);
On 2015/10/26 17:18:14, imcheng1 wrote:
> Is it possible to just return the sinkCount in the list without using object?
> 
> chrome.send('reportSinkCount', [sinkCount]);

Done.

[apacible, 2015-10-26 21:00:00]

The CQ bit was checked by apacible@chromium.org

[apacible, 2015-10-26 21:00:01]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
pkasting@chromium.org, isherman@chromium.org, imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1423683002/#ps220001
(title: "Fix JS.")

[commit-bot: I haz the power, 2015-10-26 21:00:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423683002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423683002/220001

[commit-bot: I haz the power, 2015-10-26 22:18:30]

Committed patchset #5 (id:220001)

[commit-bot: I haz the power, 2015-10-26 22:19:42]

Patchset 5 (id:??) landed as
https://crrev.com/9f153fafa1f2f58376308d41732c50ca906c6bad
Cr-Commit-Position: refs/heads/master@{#356150}