Move histogram_manager from components/metrics to components/cronet

Move histogram_manager from components/metrics to components/cronet on android.

BUG=441441
TEST=build/android/test_runner.py gtest -s components_unittests --gtest_filter=HistogramManager*

Committed: https://crrev.com/c2bd1aa68e7c0dc7e2db8d24c42fba69966c8cdc
Cr-Commit-Position: refs/heads/master@{#324352}

[mef, 2015-04-08 17:47:46]

mef@chromium.org changed reviewers:
+ asvitkine@chromium.org, xunjieli@chromium.org

[mef, 2015-04-08 17:47:47]

PTAL. Alexei, do you know how to run components unit tests on Android?

[Alexei Svitkine (slow), 2015-04-08 17:56:37]

No idea how to run components unit tests on android - I assume the bots do the
right thing?

If not, I'd ask components owners / Chrome android team.

https://codereview.chromium.org/1075503002/diff/20001/components/cronet/histo...
File components/cronet/histogram_manager_unittest.cc (left):

https://codereview.chromium.org/1075503002/diff/20001/components/cronet/histo...
components/cronet/histogram_manager_unittest.cc:17: TEST(HistogramManager,
DISABLED_HistogramBucketFields) {
Any idea why this is still disabled/flaky? Are you planning to take a look at
that after this?

Otherwise, this test wouldn't actually be running.

[mef, 2015-04-08 18:06:04]

mef@chromium.org changed reviewers:
+ rtenneti@chromium.org

[mef, 2015-04-08 18:06:04]

+rtenneti

https://codereview.chromium.org/1075503002/diff/20001/components/cronet/histo...
File components/cronet/histogram_manager_unittest.cc (left):

https://codereview.chromium.org/1075503002/diff/20001/components/cronet/histo...
components/cronet/histogram_manager_unittest.cc:17: TEST(HistogramManager,
DISABLED_HistogramBucketFields) {
On 2015/04/08 17:56:37, Alexei Svitkine wrote:
> Any idea why this is still disabled/flaky? Are you planning to take a look at
> that after this?
Good question, I probably should, although I'm not sure how to test this. 
I could drop this unittest and test through java instrumentation, but I suppose
that we may want to use HistogramManager in Cronet on iOS in foreseeable future.


> Otherwise, this test wouldn't actually be running.

Raman, do you remember, what was causing test flakiness?

https://codereview.chromium.org/1075503002/diff/20001/components/cronet/histo...
File components/cronet/histogram_manager_unittest.cc (right):

https://codereview.chromium.org/1075503002/diff/20001/components/cronet/histo...
components/cronet/histogram_manager_unittest.cc:22:
UMA_HISTOGRAM_ENUMERATION("UmaHistogramManager", 1, 2);
I've got presubmit warning that "UmaHistogramManager" is not found in
histograms.xml. Is there a better name for testing?

[Alexei Svitkine (slow), 2015-04-08 18:11:38]

https://codereview.chromium.org/1075503002/diff/20001/components/cronet/histo...
File components/cronet/histogram_manager_unittest.cc (right):

https://codereview.chromium.org/1075503002/diff/20001/components/cronet/histo...
components/cronet/histogram_manager_unittest.cc:22:
UMA_HISTOGRAM_ENUMERATION("UmaHistogramManager", 1, 2);
On 2015/04/08 18:06:04, mef wrote:
> I've got presubmit warning that "UmaHistogramManager" is not found in
> histograms.xml. Is there a better name for testing?

Hmm, the presubmit should probably be ignoring _unittest.cc files.

You can probably just fix that pretty easily here:

https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py

[Alexei Svitkine (slow), 2015-04-08 18:12:07]

https://codereview.chromium.org/1075503002/diff/20001/components/cronet/histo...
File components/cronet/histogram_manager_unittest.cc (right):

https://codereview.chromium.org/1075503002/diff/20001/components/cronet/histo...
components/cronet/histogram_manager_unittest.cc:22:
UMA_HISTOGRAM_ENUMERATION("UmaHistogramManager", 1, 2);
On 2015/04/08 18:11:38, Alexei Svitkine wrote:
> On 2015/04/08 18:06:04, mef wrote:
> > I've got presubmit warning that "UmaHistogramManager" is not found in
> > histograms.xml. Is there a better name for testing?
> 
> Hmm, the presubmit should probably be ignoring _unittest.cc files.
> 
> You can probably just fix that pretty easily here:
> 
> https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py

Correct link:
https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&l=387

[xunjieli, 2015-04-08 18:12:12]

>
https://codereview.chromium.org/1075503002/diff/20001/components/cronet/histo...
> components/cronet/histogram_manager_unittest.cc:17: TEST(HistogramManager,
> DISABLED_HistogramBucketFields) {
> On 2015/04/08 17:56:37, Alexei Svitkine wrote:
> > Any idea why this is still disabled/flaky? Are you planning to take a look
at
> > that after this?
> Good question, I probably should, although I'm not sure how to test this. 
> I could drop this unittest and test through java instrumentation, but I
suppose
> that we may want to use HistogramManager in Cronet on iOS in foreseeable
future.
> 

I might be wrong, but I believe you can do the following to run components
unittests on android:

ninja -C out/Debug components_unittests_apk
build/android/test_runner.py gtest -s components_unittests --verbose

[ramant (doing other things), 2015-04-08 18:22:13]

https://codereview.chromium.org/1075503002/diff/20001/components/cronet/histo...
File components/cronet/histogram_manager_unittest.cc (left):

https://codereview.chromium.org/1075503002/diff/20001/components/cronet/histo...
components/cronet/histogram_manager_unittest.cc:17: TEST(HistogramManager,
DISABLED_HistogramBucketFields) {
On 2015/04/08 18:06:04, mef wrote:
> On 2015/04/08 17:56:37, Alexei Svitkine wrote:
> > Any idea why this is still disabled/flaky? Are you planning to take a look
at
> > that after this?
> Good question, I probably should, although I'm not sure how to test this. 
> I could drop this unittest and test through java instrumentation, but I
suppose
> that we may want to use HistogramManager in Cronet on iOS in foreseeable
future.
> 
> 
> > Otherwise, this test wouldn't actually be running.
> 
> Raman, do you remember, what was causing test flakiness?

I don't remember. Is it possible try LOCAL_HISTOGRAM_ENUMERATION and see if it
works or not. The difference between this UMA_... whether the histogram is used
for upload or not.

[mef, 2015-04-08 19:51:28]

https://codereview.chromium.org/1075503002/diff/40001/components/cronet/histo...
File components/cronet/histogram_manager_unittest.cc (right):

https://codereview.chromium.org/1075503002/diff/40001/components/cronet/histo...
components/cronet/histogram_manager_unittest.cc:17:
base::StatisticsRecorder::Initialize();
I've added StatisticsRecorder::Initialize() and re-enabled the test.
It works on my machine. :)

https://codereview.chromium.org/1075503002/diff/40001/components/cronet/histo...
components/cronet/histogram_manager_unittest.cc:24:
UMA_HISTOGRAM_ENUMERATION("UmaHistogramManager", 1, 2);
using LOCAL_HISTOGRAM_ENUMERATION results in empty deltas :( , but this is just
a presubmit WARNING. :)

[Alexei Svitkine (slow), 2015-04-08 20:21:49]

lgtm

[xunjieli, 2015-04-08 21:05:53]

https://codereview.chromium.org/1075503002/diff/40001/components/cronet/histo...
File components/cronet/histogram_manager.h (right):

https://codereview.chromium.org/1075503002/diff/40001/components/cronet/histo...
components/cronet/histogram_manager.h:19: namespace metrics {
Should the namespace be changed? Also, I wonder whether this file should be
placed in cronet/android/, since I am assuming it will only be applicable to
android (not iOS).

[mef, 2015-04-08 21:24:24]

https://codereview.chromium.org/1075503002/diff/40001/components/cronet/histo...
File components/cronet/histogram_manager.h (right):

https://codereview.chromium.org/1075503002/diff/40001/components/cronet/histo...
components/cronet/histogram_manager.h:19: namespace metrics {
On 2015/04/08 21:05:53, xunjieli wrote:
> Should the namespace be changed? 
Good point, done.

> Also, I wonder whether this file should be
> placed in cronet/android/, since I am assuming it will only be applicable to
> android (not iOS).
I think we may want to use in on iOS.

[xunjieli, 2015-04-08 21:30:51]

lgtm mod nits below.

https://codereview.chromium.org/1075503002/diff/60001/components/cronet/histo...
File components/cronet/histogram_manager.h (right):

https://codereview.chromium.org/1075503002/diff/60001/components/cronet/histo...
components/cronet/histogram_manager.h:21: // A HistogramManager instance is
created by the app.  It is the central
nit: there are two spaces after .

https://codereview.chromium.org/1075503002/diff/60001/components/cronet/histo...
File components/cronet/histogram_manager_unittest.cc (right):

https://codereview.chromium.org/1075503002/diff/60001/components/cronet/histo...
components/cronet/histogram_manager_unittest.cc:48:
metrics::ChromeUserMetricsExtension uma_proto2;
nit: You already have "using" for this on line 15.

[mef, 2015-04-08 21:41:21]

https://codereview.chromium.org/1075503002/diff/60001/components/cronet/histo...
File components/cronet/histogram_manager_unittest.cc (right):

https://codereview.chromium.org/1075503002/diff/60001/components/cronet/histo...
components/cronet/histogram_manager_unittest.cc:48:
metrics::ChromeUserMetricsExtension uma_proto2;
On 2015/04/08 21:30:51, xunjieli wrote:
> nit: You already have "using" for this on line 15.

Oops, I've started adding those, but decided to go with using. Done.

[ramant (doing other things), 2015-04-09 00:10:22]

lgtm

https://codereview.chromium.org/1075503002/diff/80001/components/cronet/histo...
File components/cronet/histogram_manager.h (right):

https://codereview.chromium.org/1075503002/diff/80001/components/cronet/histo...
components/cronet/histogram_manager.h:22: // controller for the acquisition of
log data, and recoding deltas for
nit: recoding -> recording

https://codereview.chromium.org/1075503002/diff/80001/components/cronet/histo...
File components/cronet/histogram_manager_unittest.cc (right):

https://codereview.chromium.org/1075503002/diff/80001/components/cronet/histo...
components/cronet/histogram_manager_unittest.cc:18: // TODO(rtenneti): enable
flaky HistogramBucketFields unit test.
nit: Would appreciate if you could delete the TODO. Many thanks for fixing this.

[mef, 2015-04-09 00:41:40]

Thanks, everybody!

https://codereview.chromium.org/1075503002/diff/80001/components/cronet/histo...
File components/cronet/histogram_manager.h (right):

https://codereview.chromium.org/1075503002/diff/80001/components/cronet/histo...
components/cronet/histogram_manager.h:22: // controller for the acquisition of
log data, and recoding deltas for
On 2015/04/09 00:10:22, ramant wrote:
> nit: recoding -> recording

Done.

https://codereview.chromium.org/1075503002/diff/80001/components/cronet/histo...
File components/cronet/histogram_manager_unittest.cc (right):

https://codereview.chromium.org/1075503002/diff/80001/components/cronet/histo...
components/cronet/histogram_manager_unittest.cc:18: // TODO(rtenneti): enable
flaky HistogramBucketFields unit test.
On 2015/04/09 00:10:22, ramant wrote:
> nit: Would appreciate if you could delete the TODO. Many thanks for fixing
this.

Done.

[mef, 2015-04-09 00:41:55]

The CQ bit was checked by mef@chromium.org

[mef, 2015-04-09 00:41:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, xunjieli@chromium.org, rtenneti@chromium.org
Link to the patchset: https://codereview.chromium.org/1075503002/#ps100001
(title: "Address Raman's comments")

[commit-bot: I haz the power, 2015-04-09 00:43:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1075503002/100001

[commit-bot: I haz the power, 2015-04-09 03:27:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-09 03:27:52]

Patchset 6 (id:??) landed as
https://crrev.com/c2bd1aa68e7c0dc7e2db8d24c42fba69966c8cdc
Cr-Commit-Position: refs/heads/master@{#324352}