Adds CastMetricsServiceClient to Chromecast shell.

Adds CastMetricsServiceClient to Chromecast shell.

Chromecast shell starts up MetricsService but reporting is largely
controlled in platform-specific ways. On the Chromecast dongle, this
occurs via the API that interfaces with the setup application.

R=lcwu@chromium.org,asvitkine@chromium.org,isherman@chromium.org,bryaneyler@chromium.org
BUG=400925
Committed: https://crrev.com/0ee038dd6ea4cf91ab50f6f4d459e6f2f5dd10d7
Cr-Commit-Position: refs/heads/master@{#291588}

[Alexei Svitkine (slow), 2014-08-20 21:21:47]

Generally, looks fine.

Is there a way to ensure that UMA team gets to review changes to this code in
the future, perhaps via an OWNERS file?

Also, the Chrome version of client class has quite a bit of logic for
synchronizing state from other processes, etc. in it. I'm guessing you will
probably want to have this same logic. Have you given any thought yet about how
you will go about this? Perhaps via a helper that could be added in the
component? (e.g. in components/metrics/content/ that can depend on content/)

https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
File chromecast/metrics/cast_metrics_service_client.cc (right):

https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_service_client.cc:42: return true;
Why not just return false, so that a brand isn't set in the proto?

https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_service_client.cc:83:
!metrics_service_->reporting_active()) {
Doesn't MetricsService do these checks internally already? I think if it's
already started or stopped, calling the method again is a no-op. Please double
check and simplify the code if possible.

https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_service_client.cc:97:
ChromecastConfig::GetInstance()->pref_service(),
Nit: I think it would be cleaner to pass this as a ctor param, first so it's not
repeated here and right below but also it will make testing easier - i.e. lets
you have a unit test with a fake prefs service.

https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_service_client.cc:122: if (IsReportingEnabled())
{
Nit: Chromium style usually prefers no {}'s on single line ifs.

[gunsch, 2014-08-22 16:42:09]

On 2014/08/20 21:21:47, Alexei Svitkine wrote:
> Generally, looks fine.
> 
> Is there a way to ensure that UMA team gets to review changes to this code in
> the future, perhaps via an OWNERS file?
> 
> Also, the Chrome version of client class has quite a bit of logic for
> synchronizing state from other processes, etc. in it. I'm guessing you will
> probably want to have this same logic. Have you given any thought yet about
how
> you will go about this? Perhaps via a helper that could be added in the
> component? (e.g. in components/metrics/content/ that can depend on content/)
> 
>
https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
> File chromecast/metrics/cast_metrics_service_client.cc (right):
> 
>
https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
> chromecast/metrics/cast_metrics_service_client.cc:42: return true;
> Why not just return false, so that a brand isn't set in the proto?
> 
>
https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
> chromecast/metrics/cast_metrics_service_client.cc:83:
> !metrics_service_->reporting_active()) {
> Doesn't MetricsService do these checks internally already? I think if it's
> already started or stopped, calling the method again is a no-op. Please double
> check and simplify the code if possible.
> 
>
https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
> chromecast/metrics/cast_metrics_service_client.cc:97:
> ChromecastConfig::GetInstance()->pref_service(),
> Nit: I think it would be cleaner to pass this as a ctor param, first so it's
not
> repeated here and right below but also it will make testing easier - i.e. lets
> you have a unit test with a fake prefs service.
> 
>
https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
> chromecast/metrics/cast_metrics_service_client.cc:122: if
(IsReportingEnabled())
> {
> Nit: Chromium style usually prefers no {}'s on single line ifs.

Addressed comments and added an OWNERS file. We would definitely appreciate your
input on future metrics code; we have a fair bit of functionality that we intend
to upstream still.

I'm not clear on what you mean by synchronizing state between processes. I read
through the ChromeMetricsServiceClient code and it does have several
asynchronous calls, but most of those seem to be due to memory usage collection.
Is that what you're referring to?

[gunsch, 2014-08-22 16:42:20]

https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
File chromecast/metrics/cast_metrics_service_client.cc (right):

https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_service_client.cc:42: return true;
On 2014/08/20 21:21:47, Alexei Svitkine wrote:
> Why not just return false, so that a brand isn't set in the proto?

Done.

https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_service_client.cc:83:
!metrics_service_->reporting_active()) {
On 2014/08/20 21:21:46, Alexei Svitkine wrote:
> Doesn't MetricsService do these checks internally already? I think if it's
> already started or stopped, calling the method again is a no-op. Please double
> check and simplify the code if possible.

Done.

https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_service_client.cc:97:
ChromecastConfig::GetInstance()->pref_service(),
On 2014/08/20 21:21:46, Alexei Svitkine wrote:
> Nit: I think it would be cleaner to pass this as a ctor param, first so it's
not
> repeated here and right below but also it will make testing easier - i.e. lets
> you have a unit test with a fake prefs service.

Done.

https://codereview.chromium.org/482813004/diff/20001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_service_client.cc:122: if (IsReportingEnabled())
{
On 2014/08/20 21:21:46, Alexei Svitkine wrote:
> Nit: Chromium style usually prefers no {}'s on single line ifs.

Done.

[Alexei Svitkine (slow), 2014-08-22 16:48:02]

Re: Synchronizing state between processes, I mean things like fetching
histograms from sub-processes (done via content::FetchHistogramsAsynchronously
in the chrome client impl), though memory tracking is another such instance
(less important). I assume under Chromecast, you still have separate render
processes right? To log their histograms, they need to be fetched by the client
code.

[gunsch, 2014-08-22 18:08:44]

On 2014/08/22 16:48:02, Alexei Svitkine wrote:
> Re: Synchronizing state between processes, I mean things like fetching
> histograms from sub-processes (done via content::FetchHistogramsAsynchronously
> in the chrome client impl), though memory tracking is another such instance
> (less important). I assume under Chromecast, you still have separate render
> processes right? To log their histograms, they need to be fetched by the
client
> code.

We do have separate render processes, but we don't actually collect the
histograms from it.

Most of the metrics we're concerned about are from our media pipeline; unlike
Linux desktop Chrome, we use an IPC-based media pipeline that does most of the
work in the browser process.

[Alexei Svitkine (slow), 2014-08-22 20:49:44]

LGTM

[lcwu1, 2014-08-22 20:54:50]

lgtm

[gunsch, 2014-08-22 22:10:36]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-08-22 22:12:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gunsch@chromium.org/482813004/40001

[commit-bot: I haz the power, 2014-08-22 23:39:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-23 21:36:49]

Committed patchset #3 (40001) as 4c7f52e63ade5876542b35ac100185d8c22c0086

[commit-bot: I haz the power, 2014-09-10 02:31:17]

Patchset 3 (id:??) landed as
https://crrev.com/0ee038dd6ea4cf91ab50f6f4d459e6f2f5dd10d7
Cr-Commit-Position: refs/heads/master@{#291588}