[Chromecast] Moves persistent UMA client ID handling to common code.

[Chromecast] Moves metrics client ID/providers to CastContentBrowserClient.

This removes platform_metrics_providers.h and its link-time implementations.

R=byungchul@chromium.org,gfhuang@chromium.org,halliwell@chromium.org
BUG=internal b/20121400

Committed: https://crrev.com/24edba4890dd86ffda6223048211520267a130a0
Cr-Commit-Position: refs/heads/master@{#344371}

[halliwell, 2015-08-19 13:25:17]

https://codereview.chromium.org/1298473004/diff/20001/chromecast/browser/metr...
File chromecast/browser/metrics/cast_metrics_service_client.h (right):

https://codereview.chromium.org/1298473004/diff/20001/chromecast/browser/metr...
chromecast/browser/metrics/cast_metrics_service_client.h:51: void
SetPersistedClientId(const std::string& client_id);
nit: persistent

[gunsch, 2015-08-19 15:34:46]

https://codereview.chromium.org/1298473004/diff/20001/chromecast/browser/metr...
File chromecast/browser/metrics/cast_metrics_service_client.h (right):

https://codereview.chromium.org/1298473004/diff/20001/chromecast/browser/metr...
chromecast/browser/metrics/cast_metrics_service_client.h:51: void
SetPersistedClientId(const std::string& client_id);
On 2015/08/19 13:25:17, halliwell wrote:
> nit: persistent

I'm not sure I agree, actually. "SetPersistentClientId" to me implies that it
should be stored/persisted. This method is for providing a previously-persisted
client ID to CastMetricsServiceClient.

But I'm open to other reasons or other name suggestions, too. I wasn't the
happiest with this name.

[gfhuang, 2015-08-19 15:48:27]

https://codereview.chromium.org/1298473004/diff/20001/chromecast/browser/metr...
File chromecast/browser/metrics/cast_metrics_service_client.cc (right):

https://codereview.chromium.org/1298473004/diff/20001/chromecast/browser/metr...
chromecast/browser/metrics/cast_metrics_service_client.cc:99:
pref_service_->SetString(::metrics::prefs::kMetricsClientID, client_id);
why this is needed?

we don't want to write to ::metrics::prefs::kMetricsClientID directory.
that's the whole idea of Load/SaveClientInfo

https://codereview.chromium.org/1298473004/diff/20001/chromecast/browser/metr...
File chromecast/browser/metrics/cast_metrics_service_client.h (right):

https://codereview.chromium.org/1298473004/diff/20001/chromecast/browser/metr...
chromecast/browser/metrics/cast_metrics_service_client.h:91: std::string
platform_client_id_;
persisted_client_id_

[gunsch, 2015-08-19 15:52:50]

https://codereview.chromium.org/1298473004/diff/20001/chromecast/browser/metr...
File chromecast/browser/metrics/cast_metrics_service_client.cc (right):

https://codereview.chromium.org/1298473004/diff/20001/chromecast/browser/metr...
chromecast/browser/metrics/cast_metrics_service_client.cc:99:
pref_service_->SetString(::metrics::prefs::kMetricsClientID, client_id);
On 2015/08/19 15:48:27, gfhuang wrote:
> why this is needed?
> 
> we don't want to write to ::metrics::prefs::kMetricsClientID directory.
> that's the whole idea of Load/SaveClientInfo

Sort of a safety mechanism to replace PlatformSetClientID, but I think it
_should_ already be safely set by metrics code. I can remove it.

https://codereview.chromium.org/1298473004/diff/20001/chromecast/browser/metr...
File chromecast/browser/metrics/cast_metrics_service_client.h (right):

https://codereview.chromium.org/1298473004/diff/20001/chromecast/browser/metr...
chromecast/browser/metrics/cast_metrics_service_client.h:91: std::string
platform_client_id_;
On 2015/08/19 15:48:27, gfhuang wrote:
> persisted_client_id_

Done.

[gunsch, 2015-08-19 19:55:11]

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

[commit-bot: I haz the power, 2015-08-19 19:55:36]

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

[commit-bot: I haz the power, 2015-08-19 20:02:30]

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

[commit-bot: I haz the power, 2015-08-19 20:02:31]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[halliwell, 2015-08-19 20:56:18]

https://codereview.chromium.org/1298473004/diff/20001/chromecast/browser/metr...
File chromecast/browser/metrics/cast_metrics_service_client.h (right):

https://codereview.chromium.org/1298473004/diff/20001/chromecast/browser/metr...
chromecast/browser/metrics/cast_metrics_service_client.h:51: void
SetPersistedClientId(const std::string& client_id);
On 2015/08/19 15:34:46, gunsch wrote:
> On 2015/08/19 13:25:17, halliwell wrote:
> > nit: persistent
> 
> I'm not sure I agree, actually. "SetPersistentClientId" to me implies that it
> should be stored/persisted. This method is for providing a
previously-persisted
> client ID to CastMetricsServiceClient.
> 
> But I'm open to other reasons or other name suggestions, too. I wasn't the
> happiest with this name.

I see, thanks for explanation.

https://codereview.chromium.org/1298473004/diff/80001/chromecast/browser/cast...
File chromecast/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/1298473004/diff/80001/chromecast/browser/cast...
chromecast/browser/cast_content_browser_client.cc:103:
CastBrowserProcess::GetInstance()->SetCastContentBrowserClient(this);
why not pass 'this' through CastBrowserMainParts ctor, which creates
CastBrowserProcess?  then no need for a setter.

[byungchul, 2015-08-19 21:11:20]

https://codereview.chromium.org/1298473004/diff/80001/chromecast/browser/metr...
File chromecast/browser/metrics/cast_metrics_service_client.cc (right):

https://codereview.chromium.org/1298473004/diff/80001/chromecast/browser/metr...
chromecast/browser/metrics/cast_metrics_service_client.cc:135: } else {
no else after return.

https://codereview.chromium.org/1298473004/diff/80001/chromecast/browser/metr...
File chromecast/browser/metrics/cast_metrics_service_client.h (right):

https://codereview.chromium.org/1298473004/diff/80001/chromecast/browser/metr...
chromecast/browser/metrics/cast_metrics_service_client.h:51: void
SetPersistedClientId(const std::string& client_id);
SetPersistedClientId() is confusing with SetMetricsClientId() while both are
somewhat opposite. What about SetMetricsClientIdHint() or so?

[gunsch, 2015-08-19 21:23:12]

https://codereview.chromium.org/1298473004/diff/80001/chromecast/browser/cast...
File chromecast/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/1298473004/diff/80001/chromecast/browser/cast...
chromecast/browser/cast_content_browser_client.cc:103:
CastBrowserProcess::GetInstance()->SetCastContentBrowserClient(this);
On 2015/08/19 20:56:18, halliwell wrote:
> why not pass 'this' through CastBrowserMainParts ctor, which creates
> CastBrowserProcess?  then no need for a setter.

Discussed offline, this is in keeping with CastBrowserProcess's existing
pattern. Will leave as-is.

https://codereview.chromium.org/1298473004/diff/80001/chromecast/browser/metr...
File chromecast/browser/metrics/cast_metrics_service_client.cc (right):

https://codereview.chromium.org/1298473004/diff/80001/chromecast/browser/metr...
chromecast/browser/metrics/cast_metrics_service_client.cc:135: } else {
On 2015/08/19 21:11:20, byungchul wrote:
> no else after return.

Done.

https://codereview.chromium.org/1298473004/diff/80001/chromecast/browser/metr...
File chromecast/browser/metrics/cast_metrics_service_client.h (right):

https://codereview.chromium.org/1298473004/diff/80001/chromecast/browser/metr...
chromecast/browser/metrics/cast_metrics_service_client.h:51: void
SetPersistedClientId(const std::string& client_id);
On 2015/08/19 21:11:20, byungchul wrote:
> SetPersistedClientId() is confusing with SetMetricsClientId() while both are
> somewhat opposite. What about SetMetricsClientIdHint() or so?

How about SetForceClientId? (added some docs to explain)

[byungchul, 2015-08-19 21:38:19]

lgtm

https://codereview.chromium.org/1298473004/diff/100001/chromecast/browser/met...
File chromecast/browser/metrics/cast_metrics_service_client.h (right):

https://codereview.chromium.org/1298473004/diff/100001/chromecast/browser/met...
chromecast/browser/metrics/cast_metrics_service_client.h:52: // client ID. Must
be called before Initialize.
comment that it is optional.

[gunsch, 2015-08-19 21:46:51]

https://codereview.chromium.org/1298473004/diff/100001/chromecast/browser/met...
File chromecast/browser/metrics/cast_metrics_service_client.h (right):

https://codereview.chromium.org/1298473004/diff/100001/chromecast/browser/met...
chromecast/browser/metrics/cast_metrics_service_client.h:52: // client ID. Must
be called before Initialize.
On 2015/08/19 21:38:19, byungchul wrote:
> comment that it is optional.

Done.

[halliwell, 2015-08-19 22:02:17]

On 2015/08/19 21:46:51, gunsch wrote:
>
https://codereview.chromium.org/1298473004/diff/100001/chromecast/browser/met...
> File chromecast/browser/metrics/cast_metrics_service_client.h (right):
> 
>
https://codereview.chromium.org/1298473004/diff/100001/chromecast/browser/met...
> chromecast/browser/metrics/cast_metrics_service_client.h:52: // client ID.
Must
> be called before Initialize.
> On 2015/08/19 21:38:19, byungchul wrote:
> > comment that it is optional.
> 
> Done.

lgtm

[gunsch, 2015-08-19 23:52:45]

The CQ bit was checked by gunsch@chromium.org

[gunsch, 2015-08-19 23:52:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
byungchul@chromium.org, halliwell@chromium.org
Link to the patchset: https://codereview.chromium.org/1298473004/#ps160001
(title: "GN updates, initialize pointer to nullptr")

[commit-bot: I haz the power, 2015-08-20 00:01:14]

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

[commit-bot: I haz the power, 2015-08-20 00:09:47]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-08-20 00:10:36]

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