Histograms - Support histograms for Plugins, GPU

Histograms - Support histograms for Plugins, GPU
and all child processes. Renderer processes also
use this new method to send histograms to browser.
This code is similar to the code that gets profiler
data from all processes.

R=jar@chromium.org,jam@chromium.org
TEST=browser unit tests, interactive UI tests
BUG=114013
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=146394

[ramant (doing other things), 2012-05-31 00:14:41]

Hi Jim and John Abd-El-Malek,
  Did the plumbing to get histograms from all renderer and
  child processes. This code is similar to getting profiler
  data (haven't touched the profiler/tracking_synchronizer's
  code).

  about:histograms does a synchrnous fetch and the data that
  is displayed is the data collected until that point from
  processes (doesn't include the data from latest snapshot
  changes).
  
  Deleted the old renderer's histogram snapshot code.

  Would appreciate your comments.

thanks
raman

[jam, 2012-05-31 16:37:04]

http://codereview.chromium.org/10454086/diff/1/base/metrics/histogram_sender.h
File base/metrics/histogram_sender.h (right):

http://codereview.chromium.org/10454086/diff/1/base/metrics/histogram_sender....
base/metrics/histogram_sender.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
why move this file to base? base shouldn't know anything about multi-process,
and you mention it here in a number of places.

why does this need to be derived from in code in chrome and content? 

perhaps it's quicker if we chat about this in person?

[jam, 2012-06-01 18:23:23]

first round of comments, these will affect the rest of the code so I stopped
there

http://codereview.chromium.org/10454086/diff/3009/base/metrics/histogram_send...
File base/metrics/histogram_sender.h (right):

http://codereview.chromium.org/10454086/diff/3009/base/metrics/histogram_send...
base/metrics/histogram_sender.h:18: // HistogramSender handles the logistics of
gathering up available histograms
it's a little odd that this class is called HistogramSender, but it doesn't send
them. maybe HistogramFlattener?

http://codereview.chromium.org/10454086/diff/3009/chrome/browser/metrics/hist...
File chrome/browser/metrics/histogram_synchronizer.h (right):

http://codereview.chromium.org/10454086/diff/3009/chrome/browser/metrics/hist...
chrome/browser/metrics/histogram_synchronizer.h:22: // This class maintains
state that is used to upload histogram data from the
now that you have the part that sends child process histograms in content,
should this part also be in content?

http://codereview.chromium.org/10454086/diff/3009/chrome/renderer/page_load_h...
File chrome/renderer/page_load_histograms.cc (right):

http://codereview.chromium.org/10454086/diff/3009/chrome/renderer/page_load_h...
chrome/renderer/page_load_histograms.cc:945: histogram_sender_->SendHistograms(
it is a little strange that this class gets a pointer to histogram_sender_ which
has an implementation, only so that it can use the pure virtual function. i.e.
since it doesn't use any of the implementation in it, it seems all it needs is
just a way to tell the histogram code in the child process to send histograms
(i.e. RenderThread::UpdateHistograms()).

[ramant (doing other things), 2012-06-07 02:04:41]

Hi Jim and John Abd-El-Malek,
  Made all the changes John Abd-El-Malek has suggested. Would appreciate if you
could take another look at this CL.

thanks,
raman

http://codereview.chromium.org/10454086/diff/3009/base/metrics/histogram_send...
File base/metrics/histogram_sender.h (right):

http://codereview.chromium.org/10454086/diff/3009/base/metrics/histogram_send...
base/metrics/histogram_sender.h:18: // HistogramSender handles the logistics of
gathering up available histograms
On 2012/06/01 18:23:23, John Abd-El-Malek wrote:
> it's a little odd that this class is called HistogramSender, but it doesn't
send
> them. maybe HistogramFlattener?

Done.

http://codereview.chromium.org/10454086/diff/3009/chrome/browser/metrics/hist...
File chrome/browser/metrics/histogram_synchronizer.h (right):

http://codereview.chromium.org/10454086/diff/3009/chrome/browser/metrics/hist...
chrome/browser/metrics/histogram_synchronizer.h:22: // This class maintains
state that is used to upload histogram data from the
On 2012/06/01 18:23:23, John Abd-El-Malek wrote:
> now that you have the part that sends child process histograms in content,
> should this part also be in content?

Done.

http://codereview.chromium.org/10454086/diff/3009/chrome/renderer/page_load_h...
File chrome/renderer/page_load_histograms.cc (right):

http://codereview.chromium.org/10454086/diff/3009/chrome/renderer/page_load_h...
chrome/renderer/page_load_histograms.cc:945: histogram_sender_->SendHistograms(
On 2012/06/01 18:23:23, John Abd-El-Malek wrote:
> it is a little strange that this class gets a pointer to histogram_sender_
which
> has an implementation, only so that it can use the pure virtual function. i.e.
> since it doesn't use any of the implementation in it, it seems all it needs is
> just a way to tell the histogram code in the child process to send histograms
> (i.e. RenderThread::UpdateHistograms()).

Done.

[jam, 2012-06-07 03:34:32]

(I'll leave the details of the histogram stuff to jar)

http://codereview.chromium.org/10454086/diff/22042/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/10454086/diff/22042/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_service.cc:1086: // renderer/child processes.
Wait time specifies how long to wait before
nit: just "child processes" since renderer is a type of a child process

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
File content/browser/histogram_controller.cc (right):

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_controller.cc:16: using
content::BrowserChildProcessHostIterator;
you don't need using statements since this class is in the content namespace

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
File content/browser/histogram_controller.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_controller.h:14: #include
"content/public/common/process_type.h"
nit: i don't think any of these 4 headers are needed

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
File content/browser/histogram_internals_request_job.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_internals_request_job.h:24: protected:
nit: why protected?

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
File content/browser/histogram_message_filter.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_message_filter.h:28: protected:
why protected instead of just putting this in private?

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
File content/browser/histogram_subscriber.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_subscriber.h:12: #include
"content/public/common/process_type.h"
nit: not used

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_subscriber.h:17: class CONTENT_EXPORT
HistogramSubscriber {
nit: i dont think you need CONTENT_EXPORT

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
File content/browser/histogram_synchronizer_impl.cc (right):

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_synchronizer_impl.cc:34: // all the other threads have
gone away. As a result, it is ok to call it
who destroys this?

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
File content/browser/histogram_synchronizer_impl.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_synchronizer_impl.h:24: // various renderer and child
processes, into the browser process. Such
nit: renderer is a child process. also below

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_synchronizer_impl.h:53: class CONTENT_EXPORT
HistogramSynchronizerImpl
are you sure you need to export this?

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_synchronizer_impl.h:69: friend class
HistogramSynchronizer;
why?

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_synchronizer_impl.h:81: // any/all changes to their
histograms. It also posts a task
in this class, the comments are too verbose. you don't need to describe what the
implementation does internally. just enough for the caller to know when to use
this

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_synchronizer_impl.h:100: // Send histogram_data back
to caller by calling
usually people don't document what the implementation of an overridden function
does in its header

http://codereview.chromium.org/10454086/diff/22042/content/common/child_histo...
File content/common/child_histogram_message_filter.cc (right):

http://codereview.chromium.org/10454086/diff/22042/content/common/child_histo...
content/common/child_histogram_message_filter.cc:62: // IPC, so we can clear
pickled_histograms_ afterwards.
I don't understand this comment. why does this need to be sync IPC? even if you
send async the data will be copied right away

http://codereview.chromium.org/10454086/diff/22042/content/common/child_histo...
File content/common/child_histogram_message_filter.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/common/child_histo...
content/common/child_histogram_message_filter.h:20: class
ChildHistogramMessageFilter : public base::HistogramFlattener,
nit: please put all new code in content in the "content" namespace

http://codereview.chromium.org/10454086/diff/22042/content/common/child_histo...
content/common/child_histogram_message_filter.h:40: virtual
~ChildHistogramMessageFilter();
nit: does any code derive from this? if not, why isn't this just private?

http://codereview.chromium.org/10454086/diff/22042/content/common/child_proce...
File content/common/child_process_messages.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/common/child_proce...
content/common/child_process_messages.h:93: int /* sequence number */)
nit: the convention is to put the parameter name from the dispatching function
here, so sequence_number. also below

http://codereview.chromium.org/10454086/diff/22042/content/public/browser/his...
File content/public/browser/histogram_synchronizer.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/public/browser/his...
content/public/browser/histogram_synchronizer.h:19: // This class is used to
fecth histogram data from the various renderer and
nit: ditto re "renderer and child" -> "child"

http://codereview.chromium.org/10454086/diff/22042/content/public/browser/his...
content/public/browser/histogram_synchronizer.h:29:
base::RefCountedThreadSafe<HistogramSynchronizer> {
do you really need to expose that this is ref counted? i.e. if you have a
GetInstance(), it seems that ownership should lie in content, and whoever wants
a pointer to this in chrome can call GetInstance each time like we usually do
with singletons

http://codereview.chromium.org/10454086/diff/22042/content/public/browser/his...
content/public/browser/histogram_synchronizer.h:35: HistogramSynchronizer() {}
nit: not needed so just take out

http://codereview.chromium.org/10454086/diff/22042/content/public/browser/his...
content/public/browser/histogram_synchronizer.h:39: static void
FetchHistograms();
this function is only called by code in content, so it shouldn't be in the
public api (i.e. it can just be in the _impl.h)

http://codereview.chromium.org/10454086/diff/22042/content/public/browser/his...
content/public/browser/histogram_synchronizer.h:45: static void
FetchHistogramsAsynchronously(MessageLoop* callback_thread,
if all these methods are just static, what's the point of having an interface?

http://codereview.chromium.org/10454086/diff/22042/content/public/renderer/re...
File content/public/renderer/render_thread.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/public/renderer/re...
content/public/renderer/render_thread.h:112: // renderer process when the onload
is complete.
nit: i think you're describing what chrome does here, right? if so, we shouldn't
explain that in the public api of content

also, put this before the platform specific sectoin (so at line 102)

[ramant (doing other things), 2012-06-07 23:39:25]

Hi John Abd-El-Malek,
  Many many thanks for the code review. Made all the changes you have suggested.
Would appreciate if you could take another look at these changes.

thanks very much,
raman

http://codereview.chromium.org/10454086/diff/22042/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/10454086/diff/22042/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_service.cc:1086: // renderer/child processes.
Wait time specifies how long to wait before
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> nit: just "child processes" since renderer is a type of a child process

Done.

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
File content/browser/histogram_controller.cc (right):

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_controller.cc:16: using
content::BrowserChildProcessHostIterator;
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> you don't need using statements since this class is in the content namespace

Done.

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
File content/browser/histogram_controller.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_controller.h:14: #include
"content/public/common/process_type.h"
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> nit: i don't think any of these 4 headers are needed

Deleted all includes except for singleton.h

Done.

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
File content/browser/histogram_internals_request_job.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_internals_request_job.h:24: protected:
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> nit: why protected?

Done.

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
File content/browser/histogram_message_filter.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_message_filter.h:28: protected:
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> why protected instead of just putting this in private?

Done.

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
File content/browser/histogram_subscriber.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_subscriber.h:12: #include
"content/public/common/process_type.h"
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> nit: not used

Done.

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_subscriber.h:17: class CONTENT_EXPORT
HistogramSubscriber {
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> nit: i dont think you need CONTENT_EXPORT

Done.

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
File content/browser/histogram_synchronizer_impl.cc (right):

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_synchronizer_impl.cc:34: // all the other threads have
gone away. As a result, it is ok to call it
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> who destroys this?

Used singleton and deleted g_histogram_synchronizer.

Done.

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
File content/browser/histogram_synchronizer_impl.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_synchronizer_impl.h:24: // various renderer and child
processes, into the browser process. Such
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> nit: renderer is a child process. also below

Done.

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_synchronizer_impl.h:53: class CONTENT_EXPORT
HistogramSynchronizerImpl
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> are you sure you need to export this?

Done.

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_synchronizer_impl.h:69: friend class
HistogramSynchronizer;
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> why?

Added public static method FetchHistogramsAsynchronously and deleted the friend
class.

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_synchronizer_impl.h:81: // any/all changes to their
histograms. It also posts a task
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> in this class, the comments are too verbose. you don't need to describe what
the
> implementation does internally. just enough for the caller to know when to use
> this

Done.

http://codereview.chromium.org/10454086/diff/22042/content/browser/histogram_...
content/browser/histogram_synchronizer_impl.h:100: // Send histogram_data back
to caller by calling
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> usually people don't document what the implementation of an overridden
function
> does in its header

Done.

http://codereview.chromium.org/10454086/diff/22042/content/common/child_histo...
File content/common/child_histogram_message_filter.cc (right):

http://codereview.chromium.org/10454086/diff/22042/content/common/child_histo...
content/common/child_histogram_message_filter.cc:62: // IPC, so we can clear
pickled_histograms_ afterwards.
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> I don't understand this comment. why does this need to be sync IPC? even if
you
> send async the data will be copied right away

Deleted this comment (+1 to your comment).
Done.

http://codereview.chromium.org/10454086/diff/22042/content/common/child_histo...
File content/common/child_histogram_message_filter.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/common/child_histo...
content/common/child_histogram_message_filter.h:20: class
ChildHistogramMessageFilter : public base::HistogramFlattener,
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> nit: please put all new code in content in the "content" namespace

Done.

http://codereview.chromium.org/10454086/diff/22042/content/common/child_histo...
content/common/child_histogram_message_filter.h:40: virtual
~ChildHistogramMessageFilter();
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> nit: does any code derive from this? if not, why isn't this just private?

Done.

http://codereview.chromium.org/10454086/diff/22042/content/common/child_proce...
File content/common/child_process_messages.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/common/child_proce...
content/common/child_process_messages.h:93: int /* sequence number */)
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> nit: the convention is to put the parameter name from the dispatching function
> here, so sequence_number. also below

Done.

http://codereview.chromium.org/10454086/diff/22042/content/public/browser/his...
File content/public/browser/histogram_synchronizer.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/public/browser/his...
content/public/browser/histogram_synchronizer.h:19: // This class is used to
fecth histogram data from the various renderer and
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> nit: ditto re "renderer and child" -> "child"

Done.

http://codereview.chromium.org/10454086/diff/22042/content/public/browser/his...
content/public/browser/histogram_synchronizer.h:29:
base::RefCountedThreadSafe<HistogramSynchronizer> {
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> do you really need to expose that this is ref counted? i.e. if you have a
> GetInstance(), it seems that ownership should lie in content, and whoever
wants
> a pointer to this in chrome can call GetInstance each time like we usually do
> with singletons

Done.

http://codereview.chromium.org/10454086/diff/22042/content/public/browser/his...
content/public/browser/histogram_synchronizer.h:35: HistogramSynchronizer() {}
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> nit: not needed so just take out

Done.

http://codereview.chromium.org/10454086/diff/22042/content/public/browser/his...
content/public/browser/histogram_synchronizer.h:39: static void
FetchHistograms();
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> this function is only called by code in content, so it shouldn't be in the
> public api (i.e. it can just be in the _impl.h)

Done.

http://codereview.chromium.org/10454086/diff/22042/content/public/browser/his...
content/public/browser/histogram_synchronizer.h:45: static void
FetchHistogramsAsynchronously(MessageLoop* callback_thread,
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> if all these methods are just static, what's the point of having an interface?

Done.

http://codereview.chromium.org/10454086/diff/22042/content/public/renderer/re...
File content/public/renderer/render_thread.h (right):

http://codereview.chromium.org/10454086/diff/22042/content/public/renderer/re...
content/public/renderer/render_thread.h:112: // renderer process when the onload
is complete.
On 2012/06/07 03:34:33, John Abd-El-Malek wrote:
> nit: i think you're describing what chrome does here, right? if so, we
shouldn't
> explain that in the public api of content
> 
> also, put this before the platform specific sectoin (so at line 102)

Done.

[jam, 2012-06-09 00:59:57]

Since this patch is large, and I'm not familiar with the details of the moved
code, can jar review this as well and then I can take another look afterewards?

On 2012/06/07 23:39:25, ramant wrote:
> Hi John Abd-El-Malek,
>   Many many thanks for the code review. Made all the changes you have
suggested.
> Would appreciate if you could take another look at these changes.
> 
> thanks very much,
> raman

[jar (doing other things), 2012-06-09 16:32:36]

Yes... I'll do a careful review.. and then you (jam) can have another look.

Thanks,

Jim

On Fri, Jun 8, 2012 at 5:59 PM, <jam@chromium.org> wrote:

> Since this patch is large, and I'm not familiar with the details of the
> moved
> code, can jar review this as well and then I can take another look
> afterewards?
>
>
> On 2012/06/07 23:39:25, ramant wrote:
>
>> Hi John Abd-El-Malek,
>>   Many many thanks for the code review. Made all the changes you have
>>
> suggested.
>
>> Would appreciate if you could take another look at these changes.
>>
>
>  thanks very much,
>> raman
>>
>
>
>
http://codereview.chromium.**org/10454086/<http://codereview.chromium.org/104...
>

[jar (doing other things), 2012-07-09 23:04:11]

LGTM (with nits below)

https://chromiumcodereview.appspot.com/10454086/diff/30155/chrome/browser/chr...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/10454086/diff/30155/chrome/browser/chr...
chrome/browser/chrome_browser_main.cc:1376: // be deleted after the Task is
executed.
This comment could probably be deleted now.

https://chromiumcodereview.appspot.com/10454086/diff/30155/chrome/browser/met...
File chrome/browser/metrics/tracking_synchronizer.cc (right):

https://chromiumcodereview.appspot.com/10454086/diff/30155/chrome/browser/met...
chrome/browser/metrics/tracking_synchronizer.cc:179: LAZY_INSTANCE_INITIALIZER;
nit: It would be nice to get this to use fewer lines via some typedef(s).
maybe:
typedef TrackingSynchronizer::RequestContext RequestContext;

https://chromiumcodereview.appspot.com/10454086/diff/30155/chrome/browser/ui/...
File chrome/browser/ui/webui/about_ui.cc (left):

https://chromiumcodereview.appspot.com/10454086/diff/30155/chrome/browser/ui/...
chrome/browser/ui/webui/about_ui.cc:805:
base::StatisticsRecorder::CollectHistogramStats("Browser");
Please modify the XML file to show these histograms (we forgot to before).

https://chromiumcodereview.appspot.com/10454086/diff/30155/chrome/renderer/pa...
File chrome/renderer/page_load_histograms.cc (right):

https://chromiumcodereview.appspot.com/10454086/diff/30155/chrome/renderer/pa...
chrome/renderer/page_load_histograms.cc:942:
content::RenderThread::Get()->UpdateHistograms(
IF GPU goes away... we'll lose data.  Do we want to expedite uploads
periodically (for instance)?

Should we add a TODO and a bug?

https://chromiumcodereview.appspot.com/10454086/diff/30155/content/browser/hi...
File content/browser/histogram_internals_request_job.cc (right):

https://chromiumcodereview.appspot.com/10454086/diff/30155/content/browser/hi...
content/browser/histogram_internals_request_job.cc:28: #ifndef NDEBUG
Add comment:

We only rush the acquisition of Histogram meta-data (meta-histograms) in Debug
mode, so that developers don't damage our data that we upload to UMA (in
official builds).

https://chromiumcodereview.appspot.com/10454086/diff/30155/content/browser/hi...
File content/browser/histogram_synchronizer.cc (left):

https://chromiumcodereview.appspot.com/10454086/diff/30155/content/browser/hi...
content/browser/histogram_synchronizer.cc:229:
UMA_HISTOGRAM_COUNTS("Histogram.RendersNotRespondingAsynchronous",
nit: consider obsoleting in the histograms.xml file.

https://chromiumcodereview.appspot.com/10454086/diff/30155/content/browser/hi...
File content/browser/histogram_synchronizer.cc (right):

https://chromiumcodereview.appspot.com/10454086/diff/30155/content/browser/hi...
content/browser/histogram_synchronizer.cc:122:
UMA_HISTOGRAM_BOOLEAN("Histogram.ReceivedProcessGroupCount",
Be sure to update histograms.xml

https://chromiumcodereview.appspot.com/10454086/diff/30155/content/browser/hi...
content/browser/histogram_synchronizer.cc:216: return;
nit: lines 214-217 are probably not needed here (unlike the profile... which
*might* be turned off, histogram synchronizer is always(?) turned on via a
singleton.

https://chromiumcodereview.appspot.com/10454086/diff/30155/content/browser/hi...
content/browser/histogram_synchronizer.cc:234: base::Bind(
nit: move to previous line.

https://chromiumcodereview.appspot.com/10454086/diff/30155/content/browser/hi...
content/browser/histogram_synchronizer.cc:350: LAZY_INSTANCE_INITIALIZER;
nit: typedef to reduce length of line.

https://chromiumcodereview.appspot.com/10454086/diff/30155/content/common/chi...
File content/common/child_histogram_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10454086/diff/30155/content/common/chi...
content/common/child_histogram_message_filter.cc:84:
UMA_HISTOGRAM_ENUMERATION("Histogram.InconsistenciesChildProcess",
nit: be sure to update histograms.xml

https://chromiumcodereview.appspot.com/10454086/diff/30155/content/common/chi...
File content/common/child_thread.cc (right):

https://chromiumcodereview.appspot.com/10454086/diff/30155/content/common/chi...
content/common/child_thread.cc:65:
channel_->AddFilter(histogram_message_filter_.get());
Where is the corresponding code for the profiler?

[jam, 2012-07-11 00:42:23]

lgtm

[ramant (doing other things), 2012-07-11 23:52:54]

http://codereview.chromium.org/10454086/diff/30155/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/10454086/diff/30155/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1376: // be deleted after the Task is
executed.
On 2012/07/09 23:04:12, jar wrote:
> This comment could probably be deleted now.

Done.

http://codereview.chromium.org/10454086/diff/30155/chrome/browser/metrics/tra...
File chrome/browser/metrics/tracking_synchronizer.cc (right):

http://codereview.chromium.org/10454086/diff/30155/chrome/browser/metrics/tra...
chrome/browser/metrics/tracking_synchronizer.cc:179: LAZY_INSTANCE_INITIALIZER;
On 2012/07/09 23:04:12, jar wrote:
> nit: It would be nice to get this to use fewer lines via some typedef(s).
> maybe:
> typedef TrackingSynchronizer::RequestContext RequestContext;

jar and I had talked. Defining typedef results in an error.

http://codereview.chromium.org/10454086/diff/30155/chrome/browser/ui/webui/ab...
File chrome/browser/ui/webui/about_ui.cc (left):

http://codereview.chromium.org/10454086/diff/30155/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:805:
base::StatisticsRecorder::CollectHistogramStats("Browser");
On 2012/07/09 23:04:12, jar wrote:
> Please modify the XML file to show these histograms (we forgot to before).

Submitting a separate CL for XML changes.
Done.

http://codereview.chromium.org/10454086/diff/30155/chrome/renderer/page_load_...
File chrome/renderer/page_load_histograms.cc (right):

http://codereview.chromium.org/10454086/diff/30155/chrome/renderer/page_load_...
chrome/renderer/page_load_histograms.cc:942:
content::RenderThread::Get()->UpdateHistograms(
On 2012/07/09 23:04:12, jar wrote:
> IF GPU goes away... we'll lose data.  Do we want to expedite uploads
> periodically (for instance)?
> 
> Should we add a TODO and a bug?
> 

Created issue 136892.
Done.

http://codereview.chromium.org/10454086/diff/30155/content/browser/histogram_...
File content/browser/histogram_internals_request_job.cc (right):

http://codereview.chromium.org/10454086/diff/30155/content/browser/histogram_...
content/browser/histogram_internals_request_job.cc:28: #ifndef NDEBUG
On 2012/07/09 23:04:12, jar wrote:
> Add comment:
> 
> We only rush the acquisition of Histogram meta-data (meta-histograms) in Debug
> mode, so that developers don't damage our data that we upload to UMA (in
> official builds).

Done.

http://codereview.chromium.org/10454086/diff/30155/content/browser/histogram_...
File content/browser/histogram_synchronizer.cc (left):

http://codereview.chromium.org/10454086/diff/30155/content/browser/histogram_...
content/browser/histogram_synchronizer.cc:229:
UMA_HISTOGRAM_COUNTS("Histogram.RendersNotRespondingAsynchronous",
On 2012/07/09 23:04:12, jar wrote:
> nit: consider obsoleting in the histograms.xml file.

Submitting a separate CL for histograms.xml changes.

Done.

http://codereview.chromium.org/10454086/diff/30155/content/browser/histogram_...
File content/browser/histogram_synchronizer.cc (right):

http://codereview.chromium.org/10454086/diff/30155/content/browser/histogram_...
content/browser/histogram_synchronizer.cc:122:
UMA_HISTOGRAM_BOOLEAN("Histogram.ReceivedProcessGroupCount",
On 2012/07/09 23:04:12, jar wrote:
> Be sure to update histograms.xml

Will submit a separate CL for it.
Done.

http://codereview.chromium.org/10454086/diff/30155/content/browser/histogram_...
content/browser/histogram_synchronizer.cc:216: return;
On 2012/07/09 23:04:12, jar wrote:
> nit: lines 214-217 are probably not needed here (unlike the profile... which
> *might* be turned off, histogram synchronizer is always(?) turned on via a
> singleton.

Done.

http://codereview.chromium.org/10454086/diff/30155/content/browser/histogram_...
content/browser/histogram_synchronizer.cc:234: base::Bind(
On 2012/07/09 23:04:12, jar wrote:
> nit: move to previous line.

Done.

http://codereview.chromium.org/10454086/diff/30155/content/browser/histogram_...
content/browser/histogram_synchronizer.cc:350: LAZY_INSTANCE_INITIALIZER;
On 2012/07/09 23:04:12, jar wrote:
> nit: typedef to reduce length of line.

Wasn't able to define a typedef. Will leave it into multiple lines.

http://codereview.chromium.org/10454086/diff/30155/content/common/child_histo...
File content/common/child_histogram_message_filter.cc (right):

http://codereview.chromium.org/10454086/diff/30155/content/common/child_histo...
content/common/child_histogram_message_filter.cc:84:
UMA_HISTOGRAM_ENUMERATION("Histogram.InconsistenciesChildProcess",
On 2012/07/09 23:04:12, jar wrote:
> nit: be sure to update histograms.xml

Done.

http://codereview.chromium.org/10454086/diff/30155/content/common/child_threa...
File content/common/child_thread.cc (right):

http://codereview.chromium.org/10454086/diff/30155/content/common/child_threa...
content/common/child_thread.cc:65:
channel_->AddFilter(histogram_message_filter_.get());
On 2012/07/09 23:04:12, jar wrote:
> Where is the corresponding code for the profiler?

ChildThread::OnMessageReceive implemented GetChildProfilerData message.

For histogram_filter, we needed a separate class (ChildHistogramMessageFilter)
because we wanted to derive from  base::HistogramFlattener.

[jar (doing other things), 2012-07-12 00:00:22]

LGTM

[commit-bot: I haz the power, 2012-07-12 03:14:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10454086/33015

[commit-bot: I haz the power, 2012-07-12 03:14:25]

Can't process patch for file content/public/browser/histogram_fetcher.h.
Unsupported svn property format.

[commit-bot: I haz the power, 2012-07-12 03:44:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10454086/45077

[commit-bot: I haz the power, 2012-07-12 03:44:56]

Can't process patch for file content/public/browser/histogram_fetcher.h.
Unsupported svn property format.

[commit-bot: I haz the power, 2012-07-12 17:09:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10454086/40014

[commit-bot: I haz the power, 2012-07-12 17:09:55]

Can't process patch for file content/public/browser/histogram_fetcher.h.
Unsupported svn property format.

[commit-bot: I haz the power, 2012-07-12 17:48:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10454086/35058

[commit-bot: I haz the power, 2012-07-12 17:48:42]

Failed to apply patch for base/metrics/histogram_flattener.h:
While running patch -p0 --forward --force;
patching file base/metrics/histogram_flattener.h
Hunk #1 FAILED at 2.
Hunk #2 FAILED at 13.
2 out of 2 hunks FAILED -- saving rejects to file
base/metrics/histogram_flattener.h.rej

[M-A Ruel, 2012-07-12 17:51:42]

On 2012/07/12 17:48:42, I haz the power (commit-bot) wrote:
> Failed to apply patch for base/metrics/histogram_flattener.h:
> While running patch -p0 --forward --force;
> patching file base/metrics/histogram_flattener.h
> Hunk #1 FAILED at 2.
> Hunk #2 FAILED at 13.
> 2 out of 2 hunks FAILED -- saving rejects to file
> base/metrics/histogram_flattener.h.rej

svn copy support is broken since the diff generated by rietveld
https://chromiumcodereview.appspot.com/download/issue10454086_35058_38021.diff
is missing data. It works fine on git checkout.

M-A

[jam, 2012-07-12 17:54:22]

On Thu, Jul 12, 2012 at 10:51 AM, <maruel@chromium.org> wrote:

> On 2012/07/12 17:48:42, I haz the power (commit-bot) wrote:
>
>> Failed to apply patch for base/metrics/histogram_**flattener.h:
>> While running patch -p0 --forward --force;
>> patching file base/metrics/histogram_**flattener.h
>> Hunk #1 FAILED at 2.
>> Hunk #2 FAILED at 13.
>> 2 out of 2 hunks FAILED -- saving rejects to file
>> base/metrics/histogram_**flattener.h.rej
>>
>
> svn copy support is broken since the diff generated by rietveld
> https://chromiumcodereview.**appspot.com/download/**
>
issue10454086_35058_38021.diff<https://chromiumcodereview.appspot.com/download/issue10454086_35058_38021.diff>
> is missing data. It works fine on git checkout.
>

Did that used to work before we switched to grabbing the diff from rietveld?


>
> M-A
>
>
https://chromiumcodereview.**appspot.com/10454086/<https://chromiumcodereview...
>

[rtenneti, 2012-07-12 17:57:31]

As jar and you had discussed, using "gcl commit" to commit the patch.

thanks very much for your help,
raman

On Thu, Jul 12, 2012 at 10:51 AM, <maruel@chromium.org> wrote:

> On 2012/07/12 17:48:42, I haz the power (commit-bot) wrote:
>
>> Failed to apply patch for base/metrics/histogram_**flattener.h:
>> While running patch -p0 --forward --force;
>> patching file base/metrics/histogram_**flattener.h
>> Hunk #1 FAILED at 2.
>> Hunk #2 FAILED at 13.
>> 2 out of 2 hunks FAILED -- saving rejects to file
>> base/metrics/histogram_**flattener.h.rej
>>
>
> svn copy support is broken since the diff generated by rietveld
> https://chromiumcodereview.**appspot.com/download/**
>
issue10454086_35058_38021.diff<https://chromiumcodereview.appspot.com/download/issue10454086_35058_38021.diff>
> is missing data. It works fine on git checkout.
>
> M-A
>
>
https://chromiumcodereview.**appspot.com/10454086/<https://chromiumcodereview...
>

[M-A Ruel, 2012-07-12 18:03:45]

2012/7/12 John Abd-El-Malek <jam@chromium.org>

>
>
> On Thu, Jul 12, 2012 at 10:51 AM, <maruel@chromium.org> wrote:
>
>> On 2012/07/12 17:48:42, I haz the power (commit-bot) wrote:
>>
>>> Failed to apply patch for base/metrics/histogram_**flattener.h:
>>> While running patch -p0 --forward --force;
>>> patching file base/metrics/histogram_**flattener.h
>>> Hunk #1 FAILED at 2.
>>> Hunk #2 FAILED at 13.
>>> 2 out of 2 hunks FAILED -- saving rejects to file
>>> base/metrics/histogram_**flattener.h.rej
>>>
>>
>> svn copy support is broken since the diff generated by rietveld
>> https://chromiumcodereview.**appspot.com/download/**
>>
issue10454086_35058_38021.diff<https://chromiumcodereview.appspot.com/download/issue10454086_35058_38021.diff>
>> is missing data. It works fine on git checkout.
>>
>
> Did that used to work before we switched to grabbing the diff from
> rietveld?
>

A long time ago, rietveld.py used to grab the whole diff which *does*
contain the necessary metadata but the CQ doesn't use this link anymore.
This was problematic for patches larger than 1mb, which is more frequent
than one may think, even if each of the individual files are less than 1mb.

Even the API is not that useful,
https://chromiumcodereview.appspot.com/api/10454086/35058 doesn't contain
the necessary metadata either because of the way the diff is split.

M-A

[jam, 2012-07-12 18:10:41]

On Thu, Jul 12, 2012 at 11:03 AM, Marc-Antoine Ruel <maruel@chromium.org>wrote:

> 2012/7/12 John Abd-El-Malek <jam@chromium.org>
>
>>
>>
>> On Thu, Jul 12, 2012 at 10:51 AM, <maruel@chromium.org> wrote:
>>
>>> On 2012/07/12 17:48:42, I haz the power (commit-bot) wrote:
>>>
>>>> Failed to apply patch for base/metrics/histogram_**flattener.h:
>>>> While running patch -p0 --forward --force;
>>>> patching file base/metrics/histogram_**flattener.h
>>>> Hunk #1 FAILED at 2.
>>>> Hunk #2 FAILED at 13.
>>>> 2 out of 2 hunks FAILED -- saving rejects to file
>>>> base/metrics/histogram_**flattener.h.rej
>>>>
>>>
>>> svn copy support is broken since the diff generated by rietveld
>>> https://chromiumcodereview.**appspot.com/download/**
>>>
issue10454086_35058_38021.diff<https://chromiumcodereview.appspot.com/download/issue10454086_35058_38021.diff>
>>> is missing data. It works fine on git checkout.
>>>
>>
>> Did that used to work before we switched to grabbing the diff from
>> rietveld?
>>
>
> A long time ago, rietveld.py used to grab the whole diff which *does*
> contain the necessary metadata but the CQ doesn't use this link anymore.
> This was problematic for patches larger than 1mb, which is more frequent
> than one may think, even if each of the individual files are less than 1mb.
>
> Even the API is not that useful,
> https://chromiumcodereview.appspot.com/api/10454086/35058 doesn't contain
> the necessary metadata either because of the way the diff is split.
>

I'm not familiar with what rietveld.py?

I thought that svn copies used to land with CQ, or am I wrong? If it did
work, did it stop working when the CQ grabbed changed how it grabs the diff
(i.e. because of the bug that rogerta was working on fixing)?


> M-A
>

[M-A Ruel, 2012-07-12 18:17:11]

2012/7/12 John Abd-El-Malek <jam@chromium.org>

>
>
> On Thu, Jul 12, 2012 at 11:03 AM, Marc-Antoine Ruel
<maruel@chromium.org>wrote:
>
>> 2012/7/12 John Abd-El-Malek <jam@chromium.org>
>>
>>>
>>>
>>> On Thu, Jul 12, 2012 at 10:51 AM, <maruel@chromium.org> wrote:
>>>
>>>> On 2012/07/12 17:48:42, I haz the power (commit-bot) wrote:
>>>>
>>>>> Failed to apply patch for base/metrics/histogram_**flattener.h:
>>>>> While running patch -p0 --forward --force;
>>>>> patching file base/metrics/histogram_**flattener.h
>>>>> Hunk #1 FAILED at 2.
>>>>> Hunk #2 FAILED at 13.
>>>>> 2 out of 2 hunks FAILED -- saving rejects to file
>>>>> base/metrics/histogram_**flattener.h.rej
>>>>>
>>>>
>>>> svn copy support is broken since the diff generated by rietveld
>>>> https://chromiumcodereview.**appspot.com/download/**
>>>>
issue10454086_35058_38021.diff<https://chromiumcodereview.appspot.com/download/issue10454086_35058_38021.diff>
>>>> is missing data. It works fine on git checkout.
>>>>
>>>
>>> Did that used to work before we switched to grabbing the diff from
>>> rietveld?
>>>
>>
>> A long time ago, rietveld.py used to grab the whole diff which *does*
>> contain the necessary metadata but the CQ doesn't use this link anymore.
>> This was problematic for patches larger than 1mb, which is more frequent
>> than one may think, even if each of the individual files are less than 1mb.
>>
>> Even the API is not that useful,
>> https://chromiumcodereview.appspot.com/api/10454086/35058 doesn't
>> contain the necessary metadata either because of the way the diff is split.
>>
>
> I'm not familiar with what rietveld.py?
>
> I thought that svn copies used to land with CQ, or am I wrong? If it did
> work, did it stop working when the CQ grabbed changed how it grabs the diff
> (i.e. because of the bug that rogerta was working on fixing)?
>

I'm not sure, I don't smoke these this and even less use svn at all. I
guess this broke with a rietveld upgrade but I'm not sure. In any case, CQ
will refuse patches it can't apply safely. In that case, it has no idea
what the source file is.

M-A