Move MetricsService final log collection tasks to client.

Move MetricsService final log collection tasks to client.

BUG=374232
R=isherman@chromium.org

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

[Ilya Sherman, 2014-05-21 13:37:06]

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

https://codereview.chromium.org/293023005/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:70: :
waiting_for_asynchronous_reporting_step_(false),
nit: Perhaps we could give this a more descriptive name, like
waiting_for_final_log_collection_step_?

https://codereview.chromium.org/293023005/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:163: // child processes.
Wait time specifies how long to wait before absolutely
nit: "Wait time" -> "timeout"

https://codereview.chromium.org/293023005/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.h (right):

https://codereview.chromium.org/293023005/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.h:11: #include
"base/bind.h"
nit: base/callback.h

https://codereview.chromium.org/293023005/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.h:30: OVERRIDE;
nit: Hmm, this name is not super clear as to what the embedder should do. 
Perhaps something like "OnLogWillBeCut()" or "StartFinalLogCollection()" or
"OnWillCloseLog()" or "CollectFinalMetrics()" or something like one of these. 
(I realize that I interleaved two different types of names -- feel free to go
with either style, whichever you prefer.)

https://codereview.chromium.org/293023005/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.cc (left):

https://codereview.chromium.org/293023005/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:1276: DCHECK(IsSingleThreaded());
Hmm, why did you drop this DCHECK?

https://codereview.chromium.org/293023005/diff/40001/components/metrics/test_...
File components/metrics/test_metrics_service_client.cc (right):

https://codereview.chromium.org/293023005/diff/40001/components/metrics/test_...
components/metrics/test_metrics_service_client.cc:7: #include "base/bind.h"
nit: base/callback.h

[Alexei Svitkine (slow), 2014-05-21 14:20:14]

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

https://codereview.chromium.org/293023005/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:70: :
waiting_for_asynchronous_reporting_step_(false),
On 2014/05/21 13:37:07, Ilya Sherman wrote:
> nit: Perhaps we could give this a more descriptive name, like
> waiting_for_final_log_collection_step_?

Done.

https://codereview.chromium.org/293023005/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:163: // child processes.
Wait time specifies how long to wait before absolutely
On 2014/05/21 13:37:07, Ilya Sherman wrote:
> nit: "Wait time" -> "timeout"

Done.

https://codereview.chromium.org/293023005/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.h (right):

https://codereview.chromium.org/293023005/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.h:11: #include
"base/bind.h"
On 2014/05/21 13:37:07, Ilya Sherman wrote:
> nit: base/callback.h

Done.

https://codereview.chromium.org/293023005/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.h:30: OVERRIDE;
On 2014/05/21 13:37:07, Ilya Sherman wrote:
> nit: Hmm, this name is not super clear as to what the embedder should do. 
> Perhaps something like "OnLogWillBeCut()" or "StartFinalLogCollection()" or
> "OnWillCloseLog()" or "CollectFinalMetrics()" or something like one of these. 
> (I realize that I interleaved two different types of names -- feel free to go
> with either style, whichever you prefer.)

Done. Went with CollectFinalMetrics().

https://codereview.chromium.org/293023005/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.cc (left):

https://codereview.chromium.org/293023005/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:1276: DCHECK(IsSingleThreaded());
On 2014/05/21 13:37:07, Ilya Sherman wrote:
> Hmm, why did you drop this DCHECK?

I didn't drop it in so much as I didn't re-create in ChromeMetricsServiceClient.
Added it now.

https://codereview.chromium.org/293023005/diff/40001/components/metrics/test_...
File components/metrics/test_metrics_service_client.cc (right):

https://codereview.chromium.org/293023005/diff/40001/components/metrics/test_...
components/metrics/test_metrics_service_client.cc:7: #include "base/bind.h"
On 2014/05/21 13:37:07, Ilya Sherman wrote:
> nit: base/callback.h

Done.

[Ilya Sherman, 2014-05-21 14:22:54]

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

https://codereview.chromium.org/293023005/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:55: }
I hadn't realized what this method does.  Seems like we could use a
base::ThreadChecker instead?  (There's another concept in the MetricsService
that has a similar name, that has to do with running during startup or
shutdown.)

https://codereview.chromium.org/293023005/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.h (right):

https://codereview.chromium.org/293023005/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.h:42: bool
waiting_for_final_log_collection_step_;
nit: Now that the method is named "CollectFinalMetrics", it probably doesn't
make sense to use "log" in this name.

[Alexei Svitkine (slow), 2014-05-21 14:32:47]

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

https://codereview.chromium.org/293023005/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:55: }
On 2014/05/21 14:22:55, Ilya Sherman wrote:
> I hadn't realized what this method does.  Seems like we could use a
> base::ThreadChecker instead?  (There's another concept in the MetricsService
> that has a similar name, that has to do with running during startup or
> shutdown.)

Right you are! Somehow I totally forgot about base::ThreadChecker... Done.

https://codereview.chromium.org/293023005/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.h (right):

https://codereview.chromium.org/293023005/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.h:42: bool
waiting_for_final_log_collection_step_;
On 2014/05/21 14:22:55, Ilya Sherman wrote:
> nit: Now that the method is named "CollectFinalMetrics", it probably doesn't
> make sense to use "log" in this name.

Done. Also renamed final_log_collection_done_callback_.

[Ilya Sherman, 2014-05-21 14:34:01]

LGTM, thanks :)

[Alexei Svitkine (slow), 2014-05-21 14:34:13]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-05-21 15:03:13]

The CQ bit was unchecked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-05-21 15:03:13]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:59:13]

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

[commit-bot: I haz the power, 2014-05-22 03:09:29]

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...)

[commit-bot: I haz the power, 2014-05-22 04:56:57]

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

[commit-bot: I haz the power, 2014-05-22 04:56:57]

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

[Alexei Svitkine (slow), 2014-05-22 07:12:39]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-22 07:15:37]

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

[Alexei Svitkine (slow), 2014-05-22 08:31:35]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-22 08:34:07]

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

[Alexei Svitkine (slow), 2014-05-22 14:39:46]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-22 14:40:26]

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

[Alexei Svitkine (slow), 2014-05-22 15:06:18]

Committed patchset #7 manually as r272184 (presubmit successful).