Pass MetricsServiceClient into MetricsService, and create a simple stubbed TestMetricsServiceClient…

Pass MetricsServiceClient into MetricsService, and create a simple stubbed TestMetricsServiceClient class.

BUG=374237, 374235
TEST=compiles
R=asvitkine@chromium.org, blundell@chromium.org

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

[Alexei Svitkine (slow), 2014-05-20 07:47:10]

LGTM % nit

Sorry I didn't get a chance to look at this yesterday.

https://codereview.chromium.org/290343005/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/290343005/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:481: DCHECK(state_manager_);
Nit: Add DCHECK(client_);

[blundell, 2014-05-20 08:08:56]

not LGTM

You didn't add the test client :).

https://codereview.chromium.org/290343005/diff/20001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/290343005/diff/20001/components/metrics.gypi#...
components/metrics.gypi:59: # TODO(isherman): Remove all //chrome dependencies
on this target, and
I don't understand this TODO. It's fine for //chrome code to use e.g.
TestMetricsServiceClient.

[Ilya Sherman, 2014-05-20 08:28:26]

> You didn't add the test client :).

Oops!  Now added :)

https://codereview.chromium.org/290343005/diff/20001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/290343005/diff/20001/components/metrics.gypi#...
components/metrics.gypi:59: # TODO(isherman): Remove all //chrome dependencies
on this target, and
On 2014/05/20 08:08:56, blundell wrote:
> I don't understand this TODO. It's fine for //chrome code to use e.g.
> TestMetricsServiceClient.

I agree that there's no DEPS issues if //chrome code uses the
TestMetricsServiceClient.  However, in practice, I think all of the //chrome
code that does depend on the TestMetricsServiceClient will end up in the
component.  Hence, this target will eventually become obsolete.  The TODO is a
reminder to clean that up once the componentization is complete.

[blundell, 2014-05-20 09:07:07]

LGTM

https://codereview.chromium.org/290343005/diff/20001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/290343005/diff/20001/components/metrics.gypi#...
components/metrics.gypi:59: # TODO(isherman): Remove all //chrome dependencies
on this target, and
OK. Personally I think it would be fine to have the separate test_support
target, but I don't feel strongly about it.

On 2014/05/20 08:28:26, Ilya Sherman wrote:
> On 2014/05/20 08:08:56, blundell wrote:
> > I don't understand this TODO. It's fine for //chrome code to use e.g.
> > TestMetricsServiceClient.
> 
> I agree that there's no DEPS issues if //chrome code uses the
> TestMetricsServiceClient.  However, in practice, I think all of the //chrome
> code that does depend on the TestMetricsServiceClient will end up in the
> component.  Hence, this target will eventually become obsolete.  The TODO is a
> reminder to clean that up once the componentization is complete.

[Ilya Sherman, 2014-05-20 09:08:51]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-05-20 09:09:23]

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

[Ilya Sherman, 2014-05-20 09:17:23]

https://codereview.chromium.org/290343005/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/290343005/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:481: DCHECK(state_manager_);
On 2014/05/20 07:47:10, Alexei Svitkine wrote:
> Nit: Add DCHECK(client_);

Done.

[Ilya Sherman, 2014-05-20 09:17:28]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-05-20 09:18:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/290343005/60001

[commit-bot: I haz the power, 2014-05-20 15:45:51]

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

[commit-bot: I haz the power, 2014-05-20 15:45:51]

Failed to apply patch for chrome/browser/metrics/metrics_service_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/metrics/metrics_service_unittest.cc
  Hunk #1 FAILED at 14.
  Hunk #4 succeeded at 209 (offset 1 line).
  Hunk #5 succeeded at 238 (offset 1 line).
  Hunk #6 succeeded at 337 (offset 1 line).
  1 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/metrics/metrics_service_unittest.cc.rej

Patch:       chrome/browser/metrics/metrics_service_unittest.cc
Index: chrome/browser/metrics/metrics_service_unittest.cc
diff --git a/chrome/browser/metrics/metrics_service_unittest.cc
b/chrome/browser/metrics/metrics_service_unittest.cc
index
11ac829668c50c494cca10db61615a2c70357672..66bcedecfa63238f62923ae02d6b373634d178e5
100644
--- a/chrome/browser/metrics/metrics_service_unittest.cc
+++ b/chrome/browser/metrics/metrics_service_unittest.cc
@@ -14,6 +14,7 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/scoped_testing_local_state.h"
 #include "chrome/test/base/testing_browser_process.h"
+#include "components/metrics/test_metrics_service_client.h"
 #include "components/variations/metrics_util.h"
 #include "content/public/common/process_type.h"
 #include "content/public/common/webplugininfo.h"
@@ -31,8 +32,9 @@ using metrics::MetricsLogManager;
 
 class TestMetricsService : public MetricsService {
  public:
-  explicit TestMetricsService(metrics::MetricsStateManager* state_manager)
-      : MetricsService(state_manager) {
+  TestMetricsService(metrics::MetricsStateManager* state_manager,
+                     metrics::MetricsServiceClient* client)
+      : MetricsService(state_manager, client) {
   }
   virtual ~TestMetricsService() {}
 
@@ -177,7 +179,8 @@ TEST_F(MetricsServiceTest,
InitialStabilityLogAfterCleanShutDown) {
   EnableMetricsReporting();
   GetLocalState()->SetBoolean(prefs::kStabilityExitedCleanly, true);
 
-  TestMetricsService service(GetMetricsStateManager());
+  metrics::TestMetricsServiceClient client;
+  TestMetricsService service(GetMetricsStateManager(), &client);
   service.InitializeMetricsRecordingState();
   // No initial stability log should be generated.
   EXPECT_FALSE(service.log_manager()->has_unsent_logs());
@@ -206,7 +209,8 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAfterCrash) {
 
   GetLocalState()->SetBoolean(prefs::kStabilityExitedCleanly, false);
 
-  TestMetricsService service(GetMetricsStateManager());
+  metrics::TestMetricsServiceClient client;
+  TestMetricsService service(GetMetricsStateManager(), &client);
   service.InitializeMetricsRecordingState();
 
   // The initial stability log should be generated and persisted in unsent
logs.
@@ -234,7 +238,8 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAfterCrash) {
 }
 
 TEST_F(MetricsServiceTest, RegisterSyntheticTrial) {
-  MetricsService service(GetMetricsStateManager());
+  metrics::TestMetricsServiceClient client;
+  MetricsService service(GetMetricsStateManager(), &client);
 
   // Add two synthetic trials and confirm that they show up in the list.
   SyntheticTrialGroup trial1(metrics::HashName("TestTrial1"),
@@ -332,7 +337,8 @@ TEST_F(MetricsServiceTest, CrashReportingEnabled) {
 }
 
 TEST_F(MetricsServiceTest, MetricsServiceObserver) {
-  MetricsService service(GetMetricsStateManager());
+  metrics::TestMetricsServiceClient client;
+  MetricsService service(GetMetricsStateManager(), &client);
   TestMetricsServiceObserver observer1;
   TestMetricsServiceObserver observer2;

[Ilya Sherman, 2014-05-20 15:54:11]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-05-20 15:56:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/290343005/80001

[commit-bot: I haz the power, 2014-05-20 19:34:33]

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:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-20 19:47:38]

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

[commit-bot: I haz the power, 2014-05-20 19:47:38]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Ilya Sherman, 2014-05-20 22:27:29]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-05-20 22:28:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/290343005/100001

[Ilya Sherman, 2014-05-21 08:34:36]

The CQ bit was unchecked by isherman@chromium.org

[Ilya Sherman, 2014-05-21 08:34:42]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-05-21 08:35:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/290343005/100001

[Ilya Sherman, 2014-05-21 09:20:37]

Committed patchset #6 manually as r271873 (presubmit successful).