Introduce MetricsServiceClient and ChromeMetricsServiceClient.

Introduce MetricsServiceClient and ChromeMetricsServiceClient.

In a later CL, MetricsService ctor will take an instance of
MetricsServiceClient.

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

[blundell, 2014-05-19 10:00:23]

LGTM

Is the client effectively a singleton?

https://codereview.chromium.org/292693003/diff/10007/components/metrics/metri...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/292693003/diff/10007/components/metrics/metri...
components/metrics/metrics_service_client.h:15: // An abstraction of operations
that depend on the embedders (e.g. Chrome)
nit: s/embedders/embedder's

https://codereview.chromium.org/292693003/diff/10007/components/metrics/metri...
components/metrics/metrics_service_client.h:19: MetricsServiceClient();
Does this need a defined constructor and .cc file vs. just having the inlined
destructor definition?

[Alexei Svitkine (slow), 2014-05-19 10:55:22]

https://codereview.chromium.org/292693003/diff/10007/components/metrics/metri...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/292693003/diff/10007/components/metrics/metri...
components/metrics/metrics_service_client.h:15: // An abstraction of operations
that depend on the embedders (e.g. Chrome)
On 2014/05/19 10:00:23, blundell wrote:
> nit: s/embedders/embedder's

Done.

https://codereview.chromium.org/292693003/diff/10007/components/metrics/metri...
components/metrics/metrics_service_client.h:19: MetricsServiceClient();
On 2014/05/19 10:00:23, blundell wrote:
> Does this need a defined constructor and .cc file vs. just having the inlined
> destructor definition?

I know that inline ctors and dtors are generally discouraged, but maybe because
this is an interface that rule is relaxed? If we don't define the ctor we also
can't have the DISALLOW_() macro as well, but maybe that's fine.

I don't care either way. Ilya, any preference?

[Ilya Sherman, 2014-05-19 11:02:37]

https://codereview.chromium.org/292693003/diff/30001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/292693003/diff/30001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:29: default:
nit: Please remove the default case, so that the compiler can issue a
compile-time error if a new enum member is ever added.

https://codereview.chromium.org/292693003/diff/30001/components/metrics/metri...
File components/metrics/metrics_service_client.cc (right):

https://codereview.chromium.org/292693003/diff/30001/components/metrics/metri...
components/metrics/metrics_service_client.cc:11: }
nit: namespace metrics

https://codereview.chromium.org/292693003/diff/30001/components/metrics/metri...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/292693003/diff/30001/components/metrics/metri...
components/metrics/metrics_service_client.h:23: virtual bool
IsOffTheRecordSessionActive() = 0;
nit: I think we prefer "Incognito" over "OffTheRecord" these days, to match the
feature name.

https://codereview.chromium.org/292693003/diff/30001/components/metrics/metri...
components/metrics/metrics_service_client.h:27: virtual std::string
GetVersionString() = 0;
Please document all of these methods.  Most will probably have trivial doc
strings, but some have less immediately obvious semantics and ordering
expectations, such as SetClientID().

[Ilya Sherman, 2014-05-19 11:06:23]

https://codereview.chromium.org/292693003/diff/10007/components/metrics/metri...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/292693003/diff/10007/components/metrics/metri...
components/metrics/metrics_service_client.h:19: MetricsServiceClient();
On 2014/05/19 10:55:23, Alexei Svitkine wrote:
> On 2014/05/19 10:00:23, blundell wrote:
> > Does this need a defined constructor and .cc file vs. just having the
inlined
> > destructor definition?
> 
> I know that inline ctors and dtors are generally discouraged, but maybe
because
> this is an interface that rule is relaxed? If we don't define the ctor we also
> can't have the DISALLOW_() macro as well, but maybe that's fine.
> 
> I don't care either way. Ilya, any preference?

No preference.  I think technically inlined constructors and destructors are
allowed as long as the class has only (a small number of) POD members (or none
at all, as in this case).

[Alexei Svitkine (slow), 2014-05-19 11:34:57]

https://codereview.chromium.org/292693003/diff/10007/components/metrics/metri...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/292693003/diff/10007/components/metrics/metri...
components/metrics/metrics_service_client.h:19: MetricsServiceClient();
On 2014/05/19 11:06:24, Ilya Sherman wrote:
> On 2014/05/19 10:55:23, Alexei Svitkine wrote:
> > On 2014/05/19 10:00:23, blundell wrote:
> > > Does this need a defined constructor and .cc file vs. just having the
> inlined
> > > destructor definition?
> > 
> > I know that inline ctors and dtors are generally discouraged, but maybe
> because
> > this is an interface that rule is relaxed? If we don't define the ctor we
also
> > can't have the DISALLOW_() macro as well, but maybe that's fine.
> > 
> > I don't care either way. Ilya, any preference?
> 
> No preference.  I think technically inlined constructors and destructors are
> allowed as long as the class has only (a small number of) POD members (or none
> at all, as in this case).

Okay, in that case - I'll just keep the header file to keep things simpler.

Done.

https://codereview.chromium.org/292693003/diff/30001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/292693003/diff/30001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:29: default:
On 2014/05/19 11:02:37, Ilya Sherman wrote:
> nit: Please remove the default case, so that the compiler can issue a
> compile-time error if a new enum member is ever added.

Done.

[Alexei Svitkine (slow), 2014-05-19 11:43:24]

Forgot to reply to the other comments.

https://codereview.chromium.org/292693003/diff/30001/components/metrics/metri...
File components/metrics/metrics_service_client.cc (right):

https://codereview.chromium.org/292693003/diff/30001/components/metrics/metri...
components/metrics/metrics_service_client.cc:11: }
On 2014/05/19 11:02:37, Ilya Sherman wrote:
> nit: namespace metrics

Oops. File removed now, though.

https://codereview.chromium.org/292693003/diff/30001/components/metrics/metri...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/292693003/diff/30001/components/metrics/metri...
components/metrics/metrics_service_client.h:27: virtual std::string
GetVersionString() = 0;
On 2014/05/19 11:02:37, Ilya Sherman wrote:
> Please document all of these methods.  Most will probably have trivial doc
> strings, but some have less immediately obvious semantics and ordering
> expectations, such as SetClientID().

Done.

[Ilya Sherman, 2014-05-19 11:43:56]

https://codereview.chromium.org/292693003/diff/70001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/292693003/diff/70001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:29: }
You'll still need a NOTREACHED() and default return value outside of the switch
stmt because the Windows compiler is not as clever as clang.

[Ilya Sherman, 2014-05-19 11:44:26]

And meant to say in my previous reply: LGTM with that comment addressed. 
Thanks!

[Alexei Svitkine (slow), 2014-05-19 11:48:59]

https://codereview.chromium.org/292693003/diff/70001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/292693003/diff/70001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:29: }
On 2014/05/19 11:43:57, Ilya Sherman wrote:
> You'll still need a NOTREACHED() and default return value outside of the
switch
> stmt because the Windows compiler is not as clever as clang.

Ah, got it.  Done.

[Alexei Svitkine (slow), 2014-05-19 11:49:04]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-19 11:49:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/292693003/90001

[Ilya Sherman, 2014-05-19 11:51:13]

https://codereview.chromium.org/292693003/diff/90001/components/metrics/metri...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/292693003/diff/90001/components/metrics/metri...
components/metrics/metrics_service_client.h:11: #include
"components/metrics/proto/system_profile.pb.h"
nit: Duplicated line.

[Alexei Svitkine (slow), 2014-05-19 11:52:15]

https://codereview.chromium.org/292693003/diff/90001/components/metrics/metri...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/292693003/diff/90001/components/metrics/metri...
components/metrics/metrics_service_client.h:11: #include
"components/metrics/proto/system_profile.pb.h"
On 2014/05/19 11:51:14, Ilya Sherman wrote:
> nit: Duplicated line.

Oops, fixed.

[Alexei Svitkine (slow), 2014-05-19 11:52:19]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-19 11:52:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/292693003/110001

[Alexei Svitkine (slow), 2014-05-19 11:53:03]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-19 11:53:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/292693003/120001

[commit-bot: I haz the power, 2014-05-19 13:34:15]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-19 14:59:58]

Change committed as 271380