Merge histograms from providers into StatisticsRecorder for display.

Merge histograms from providers into StatisticsRecorder for display.

BUG=602295
Review-Url: https://codereview.chromium.org/2658163002
Cr-Commit-Position: refs/heads/master@{#447407}
Committed: https://chromium.googlesource.com/chromium/src/+/9075a09693ec32f975a12dd8207eab03a5cf48f9

[bcwhite, 2017-01-26 23:53:02]

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

[commit-bot: I haz the power, 2017-01-26 23:54:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 01:05:20]

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

[commit-bot: I haz the power, 2017-01-27 01:05:21]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bcwhite, 2017-01-27 01:13:25]

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

[commit-bot: I haz the power, 2017-01-27 01:14:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-01-27 01:21:49]

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

[bcwhite, 2017-01-27 01:21:55]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-01-27 01:21:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 04:03:16]

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

[commit-bot: I haz the power, 2017-01-27 04:03:16]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2017-01-27 14:34:25]

bcwhite@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2017-01-27 16:22:03]

Looks great!

LGTM % comments

Just to double check:

Does this work even if UMA reporting is disabled? i.e. I think we still create
all the providers in that case (good) - but do we make sure they still receive
all the subprocess stuff?

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
File base/metrics/statistics_recorder.cc (right):

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
base/metrics/statistics_recorder.cc:317: void
StatisticsRecorder::ImportProvidedHistograms() {
Can you add a thread check here?

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
base/metrics/statistics_recorder.cc:323: if (provider)
Nit: Maybe add a comment here to mention this is check is because the weak
pointer may have been invalidated.

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
File base/metrics/statistics_recorder.h (right):

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
base/metrics/statistics_recorder.h:115: // the global StatisticsRecorder.
Nit: Mention ImportProvidedHistograms() here.

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
File base/metrics/statistics_recorder_unittest.cc (right):

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
base/metrics/statistics_recorder_unittest.cc:680: };
Nit: DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/2658163002/diff/40001/content/browser/histogr...
File content/browser/histogram_internals_request_job.h (right):

https://codereview.chromium.org/2658163002/diff/40001/content/browser/histogr...
content/browser/histogram_internals_request_job.h:21: // URLRequestSimpleJob:
Nit: net::

[bcwhite, 2017-01-27 18:03:50]

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

[commit-bot: I haz the power, 2017-01-27 18:04:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-01-27 18:05:31]

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
File base/metrics/statistics_recorder.cc (right):

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
base/metrics/statistics_recorder.cc:317: void
StatisticsRecorder::ImportProvidedHistograms() {
On 2017/01/27 16:22:03, Alexei Svitkine (slow) wrote:
> Can you add a thread check here?

Not sure if I can because base/ doesn't generally know about specific threads.

The providers, though, have thread-checkers that will break if not called on the
UI thread.

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
base/metrics/statistics_recorder.cc:323: if (provider)
On 2017/01/27 16:22:03, Alexei Svitkine (slow) wrote:
> Nit: Maybe add a comment here to mention this is check is because the weak
> pointer may have been invalidated.

Done.

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
File base/metrics/statistics_recorder.h (right):

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
base/metrics/statistics_recorder.h:115: // the global StatisticsRecorder.
On 2017/01/27 16:22:03, Alexei Svitkine (slow) wrote:
> Nit: Mention ImportProvidedHistograms() here.

Done.

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
File base/metrics/statistics_recorder_unittest.cc (right):

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
base/metrics/statistics_recorder_unittest.cc:680: };
On 2017/01/27 16:22:03, Alexei Svitkine (slow) wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN()

Done.

https://codereview.chromium.org/2658163002/diff/40001/content/browser/histogr...
File content/browser/histogram_internals_request_job.h (right):

https://codereview.chromium.org/2658163002/diff/40001/content/browser/histogr...
content/browser/histogram_internals_request_job.h:21: // URLRequestSimpleJob:
On 2017/01/27 16:22:03, Alexei Svitkine (slow) wrote:
> Nit: net::

Done.

[Alexei Svitkine (slow), 2017-01-27 18:06:57]

lgtm

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
File base/metrics/statistics_recorder.cc (right):

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
base/metrics/statistics_recorder.cc:317: void
StatisticsRecorder::ImportProvidedHistograms() {
On 2017/01/27 18:05:31, bcwhite wrote:
> On 2017/01/27 16:22:03, Alexei Svitkine (slow) wrote:
> > Can you add a thread check here?
> 
> Not sure if I can because base/ doesn't generally know about specific threads.
> 
> The providers, though, have thread-checkers that will break if not called on
the
> UI thread.

Can you add base::ThreadChecker as a member and use
DCHECK(thread_checker_.CalledOnValidThread()) ?

[bcwhite, 2017-01-27 18:08:10]

bcwhite@chromium.org changed reviewers:
+ piman@chromium.org

[bcwhite, 2017-01-27 18:08:10]

piman@chromium.org: Please review changes in 

content/browser/histogram_internals_request_job.{h,cc}

[bcwhite, 2017-01-27 18:12:57]

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
File base/metrics/statistics_recorder.cc (right):

https://codereview.chromium.org/2658163002/diff/40001/base/metrics/statistics...
base/metrics/statistics_recorder.cc:317: void
StatisticsRecorder::ImportProvidedHistograms() {
On 2017/01/27 18:06:57, Alexei Svitkine (slow) wrote:
> On 2017/01/27 18:05:31, bcwhite wrote:
> > On 2017/01/27 16:22:03, Alexei Svitkine (slow) wrote:
> > > Can you add a thread check here?
> > 
> > Not sure if I can because base/ doesn't generally know about specific
threads.
> > 
> > The providers, though, have thread-checkers that will break if not called on
> the
> > UI thread.
> 
> Can you add base::ThreadChecker as a member and use
> DCHECK(thread_checker_.CalledOnValidThread()) ?

These are static methods so there's no object to hold the thread_checker_.  A
static one could be created like the other static objects are, but I don't think
it's worth the complication.

[Alexei Svitkine (slow), 2017-01-27 18:14:58]

Ah right - fair enough!

On Fri, Jan 27, 2017 at 1:12 PM, <bcwhite@chromium.org> wrote:

>
> https://codereview.chromium.org/2658163002/diff/40001/
> base/metrics/statistics_recorder.cc
> File base/metrics/statistics_recorder.cc (right):
>
> https://codereview.chromium.org/2658163002/diff/40001/
> base/metrics/statistics_recorder.cc#newcode317
> base/metrics/statistics_recorder.cc:317: void
> StatisticsRecorder::ImportProvidedHistograms() {
> On 2017/01/27 18:06:57, Alexei Svitkine (slow) wrote:
> > On 2017/01/27 18:05:31, bcwhite wrote:
> > > On 2017/01/27 16:22:03, Alexei Svitkine (slow) wrote:
> > > > Can you add a thread check here?
> > >
> > > Not sure if I can because base/ doesn't generally know about
> specific threads.
> > >
> > > The providers, though, have thread-checkers that will break if not
> called on
> > the
> > > UI thread.
> >
> > Can you add base::ThreadChecker as a member and use
> > DCHECK(thread_checker_.CalledOnValidThread()) ?
>
> These are static methods so there's no object to hold the
> thread_checker_. A static one could be created like the other static
> objects are, but I don't think it's worth the complication.
>
> https://codereview.chromium.org/2658163002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-01-27 19:07:25]

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

[commit-bot: I haz the power, 2017-01-27 19:07:26]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2017-01-27 20:23:43]

Forgot to comment on this:

> Does this work even if UMA reporting is disabled? i.e. I think we still create
> all the providers in that case (good) - but do we make sure they still receive
> all the subprocess stuff?

Yes, it does.  The provider objects are always created and always collect
information via persistent memory.  The "reporting" flag only affects whether it
gets uploaded or not.  Similarly, the "Enabled" experiment only affects whether
the persistent memory segments are created, not whether they are collected from
any that exist.

[bcwhite, 2017-01-31 14:31:59]

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

[commit-bot: I haz the power, 2017-01-31 14:32:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 15:46:40]

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

[commit-bot: I haz the power, 2017-01-31 15:46:41]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2017-01-31 18:16:00]

bcwhite@chromium.org changed reviewers:
+ jam@chromium.org
- piman@chromium.org

[bcwhite, 2017-01-31 18:16:01]

-piman
+jam

Please review changes in 

content/browser/histogram_internals_request_job.{h,cc}

[jam, 2017-02-01 01:20:02]

lgtm

[bcwhite, 2017-02-01 01:37:07]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2017-02-01 01:37:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2658163002/#ps80001
(title: "rebased")

[commit-bot: I haz the power, 2017-02-01 01:37:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 01:59:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485913027433210,
"parent_rev": "23b671d023dd981f7d7672f8ffaaa160cffbfe12", "commit_rev":
"9075a09693ec32f975a12dd8207eab03a5cf48f9"}

[commit-bot: I haz the power, 2017-02-01 02:02:58]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485913027433210,
"parent_rev": "23b671d023dd981f7d7672f8ffaaa160cffbfe12", "commit_rev":
"9075a09693ec32f975a12dd8207eab03a5cf48f9"}

[commit-bot: I haz the power, 2017-02-01 02:36:36]

Description was changed from

==========
Merge histograms from providers into StatisticsRecorder for display.

BUG=602295
==========

to

==========
Merge histograms from providers into StatisticsRecorder for display.

BUG=602295

Review-Url: https://codereview.chromium.org/2658163002
Cr-Commit-Position: refs/heads/master@{#447407}
Committed:
https://chromium.googlesource.com/chromium/src/+/9075a09693ec32f975a12dd8207e...
==========

[commit-bot: I haz the power, 2017-02-01 02:36:37]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9075a09693ec32f975a12dd8207e...