Collect profiler stats from browser child processes.

Collect profiler stats from all browser child processes.

R=jam
Bug=104770
TEST=about:profiler after lauching a plug-in or GPU should show
     tracking data for plugin-gpu processes

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112571

[jam, 2011-11-19 23:36:25]

(didn't look at tracking_synchronizer.cc closely)

http://codereview.chromium.org/8588023/diff/3024/chrome/browser/metrics/track...
File chrome/browser/metrics/tracking_synchronizer.cc (right):

http://codereview.chromium.org/8588023/diff/3024/chrome/browser/metrics/track...
chrome/browser/metrics/tracking_synchronizer.cc:227: // Default constr
references a singleton.
nit: this comment, and below, seem unnecessary. i.e. they're explaining what
someone can see when reading the code, which is not what we should be using
comments for (explaining tricky parts)

http://codereview.chromium.org/8588023/diff/3024/chrome/renderer/renderer_tra...
File chrome/renderer/renderer_tracking.cc (right):

http://codereview.chromium.org/8588023/diff/3024/chrome/renderer/renderer_tra...
chrome/renderer/renderer_tracking.cc:30:
IPC_MESSAGE_HANDLER(ChromeViewMsg_GetRendererProfilerData,
this whole file, and these chrome specific messages, aren't necessary anymore.
since the code for all the other child processes is in ChildThread, it also gets
used by the renderer process.

in the browser side, do a "AddFilter(new ProfilerMessageFilter);" in
RenderProcessHostImpl

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_con...
File content/browser/profiler_controller.h (right):

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_con...
content/browser/profiler_controller.h:21: class CONTENT_EXPORT
ProfilerSubscriber {
nit: please put this in a separate file

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_con...
content/browser/profiler_controller.h:34: // ProfilerController is used on the
browser processes to collect profiler data.
nit: process not processes, since there's only one browser process

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_con...
content/browser/profiler_controller.h:37: class CONTENT_EXPORT
ProfilerController {
for this and above, since we're currently working on the Content API
(http://www.chromium.org/developers/content-module/content-api), it would be
helpful if new code gets checked in with the API in mind. i.e. there should be a
content::ProfilerController  interface in content/public/browser, and a
content::ProfilerControllerImpl in content/browser that implements that
interface

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_con...
content/browser/profiler_controller.h:49: void
CancelSubscriber(ProfilerSubscriber* subscriber);
nit: the convention is register/unregister

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_mes...
File content/browser/profiler_message_filter.cc (right):

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_mes...
content/browser/profiler_message_filter.cc:21: // Always on IO thread
(BrowserMessageFilter guarantee).
nit: this comment is unnecessary

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_mes...
File content/browser/profiler_message_filter.h (right):

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_mes...
content/browser/profiler_message_filter.h:14: // This class sends and receives
profiler messages on the browser process.
nit: in the browser process

http://codereview.chromium.org/8588023/diff/3024/content/common/child_process...
File content/common/child_process_messages.h (right):

http://codereview.chromium.org/8588023/diff/3024/content/common/child_process...
content/common/child_process_messages.h:84: std::string /* pickled Value
containing profiler data. */)
just send the DictionaryValue directly here, and let the IPC code automatically
serialize it to a string

[ramant (doing other things), 2011-11-25 23:59:48]

Hi Jim, John Abd-El-Malek,
  Made all the changes jam has suggested. Deleted some of the unused code
(content::SetProfilerStatus could be used to set the enable/disable profiler
status).

  Not 100% sure if I named the method names correctly (will work with Jim to
come up with correct names). Will work with eroman to add UI specific nuance in
a separate CL.

  Would appreciate your comments.

thanks much,
raman

http://codereview.chromium.org/8588023/diff/3024/chrome/browser/metrics/track...
File chrome/browser/metrics/tracking_synchronizer.cc (right):

http://codereview.chromium.org/8588023/diff/3024/chrome/browser/metrics/track...
chrome/browser/metrics/tracking_synchronizer.cc:227: // Default constr
references a singleton.
On 2011/11/19 23:36:25, John Abd-El-Malek wrote:
> nit: this comment, and below, seem unnecessary. i.e. they're explaining what
> someone can see when reading the code, which is not what we should be using
> comments for (explaining tricky parts)

Done.

http://codereview.chromium.org/8588023/diff/3024/chrome/renderer/renderer_tra...
File chrome/renderer/renderer_tracking.cc (right):

http://codereview.chromium.org/8588023/diff/3024/chrome/renderer/renderer_tra...
chrome/renderer/renderer_tracking.cc:30:
IPC_MESSAGE_HANDLER(ChromeViewMsg_GetRendererProfilerData,
On 2011/11/19 23:36:25, John Abd-El-Malek wrote:
> this whole file, and these chrome specific messages, aren't necessary anymore.
> since the code for all the other child processes is in ChildThread, it also
gets
> used by the renderer process.
> 
> in the browser side, do a "AddFilter(new ProfilerMessageFilter);" in
> RenderProcessHostImpl

Done.

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_con...
File content/browser/profiler_controller.h (right):

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_con...
content/browser/profiler_controller.h:21: class CONTENT_EXPORT
ProfilerSubscriber {
On 2011/11/19 23:36:25, John Abd-El-Malek wrote:
> nit: please put this in a separate file

Done.

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_con...
content/browser/profiler_controller.h:34: // ProfilerController is used on the
browser processes to collect profiler data.
On 2011/11/19 23:36:25, John Abd-El-Malek wrote:
> nit: process not processes, since there's only one browser process

Done.

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_con...
content/browser/profiler_controller.h:37: class CONTENT_EXPORT
ProfilerController {
On 2011/11/19 23:36:25, John Abd-El-Malek wrote:
> for this and above, since we're currently working on the Content API
> (http://www.chromium.org/developers/content-module/content-api), it would be
> helpful if new code gets checked in with the API in mind. i.e. there should be
a
> content::ProfilerController  interface in content/public/browser, and a
> content::ProfilerControllerImpl in content/browser that implements that
> interface

Done.

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_con...
content/browser/profiler_controller.h:49: void
CancelSubscriber(ProfilerSubscriber* subscriber);
On 2011/11/19 23:36:25, John Abd-El-Malek wrote:
> nit: the convention is register/unregister

Done.

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_mes...
File content/browser/profiler_message_filter.cc (right):

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_mes...
content/browser/profiler_message_filter.cc:21: // Always on IO thread
(BrowserMessageFilter guarantee).
On 2011/11/19 23:36:25, John Abd-El-Malek wrote:
> nit: this comment is unnecessary

Done.

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_mes...
File content/browser/profiler_message_filter.h (right):

http://codereview.chromium.org/8588023/diff/3024/content/browser/profiler_mes...
content/browser/profiler_message_filter.h:14: // This class sends and receives
profiler messages on the browser process.
On 2011/11/19 23:36:25, John Abd-El-Malek wrote:
> nit: in the browser process

Done.

http://codereview.chromium.org/8588023/diff/3024/content/common/child_process...
File content/common/child_process_messages.h (right):

http://codereview.chromium.org/8588023/diff/3024/content/common/child_process...
content/common/child_process_messages.h:84: std::string /* pickled Value
containing profiler data. */)
On 2011/11/19 23:36:25, John Abd-El-Malek wrote:
> just send the DictionaryValue directly here, and let the IPC code
automatically
> serialize it to a string

Done.

[jam, 2011-11-28 15:17:34]

http://codereview.chromium.org/8588023/diff/61009/chrome/browser/metrics/trac...
File chrome/browser/metrics/tracking_synchronizer.h (right):

http://codereview.chromium.org/8588023/diff/61009/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:132: class RequestContext {
is this class used by other files, or only tracking_synchronizer.cc? if the
former, then please put this in a separate header. Note that the name
RequestContext is a little confusing since there's already a
content::RequestContext class (I realize they're in different namespaces, but
this is a core class that's used all over). If the later, then please move this
to the cc file.

http://codereview.chromium.org/8588023/diff/61009/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/8588023/diff/61009/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_message_filter.cc:21: #include
"chrome/browser/metrics/tracking_synchronizer.h"
nit: not needed anymore

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_co...
File content/browser/profiler_controller_impl.cc (right):

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_co...
content/browser/profiler_controller_impl.cc:23: content::ProfilerController*
content::ProfilerController::GetInstance() {
by convention, put statics at top of the funcitons. also, content api statics go
first

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_co...
content/browser/profiler_controller_impl.cc:52:
subscriber_->OnProfilerDataCollected(sequence_number, profiler_data);
why call the subscriber on the IO thread? the interface should hide internal
thread implementations and make this interface all one thread (i.e.
register/unregister/get notified on the same thread (UI) ). 

it seems that there's a bug (which the above would fix) since a subscriber can
unregister on the UI thread, but as subscriber_ is being nulled in
ProfilerControllerImpl on the UI thread, it can be used on the IO thread.

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_co...
File content/browser/profiler_controller_impl.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_co...
content/browser/profiler_controller_impl.h:13: class DictionaryValue;
here and below, no need to duplicate the forward declarations from the parent
interface

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_co...
content/browser/profiler_controller_impl.h:21: class CONTENT_EXPORT
ProfilerControllerImpl
nit: eventually, all the code in content will be in the content namespace. for
new code, please put it in the namespace to save us the work of doing that later

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_co...
content/browser/profiler_controller_impl.h:31: // Register the subscriber so
that it will be called when for example
here and below, don't duplicate the comments from the interface in the
implmentation.

// ProfilerController implementation:

should be enough

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_me...
File content/browser/profiler_message_filter.cc (right):

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_me...
content/browser/profiler_message_filter.cc:7: #include "base/tracked_objects.h"
not needed?

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_me...
content/browser/profiler_message_filter.cc:8: #include "base/values.h"
ditto

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_me...
File content/browser/profiler_message_filter.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_me...
content/browser/profiler_message_filter.h:10: #include "base/process.h"
not needed

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_me...
content/browser/profiler_message_filter.h:11: #include
"content/common/child_process_info.h"
ditto

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_sy...
File content/browser/profiler_synchronizer.cc (right):

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_sy...
content/browser/profiler_synchronizer.cc:37:
base::Bind(&ProfilerController::OnPendingProcesses,
nit: per other comments, all this stuff should be internal details and not
exposed to subscribers

http://codereview.chromium.org/8588023/diff/61009/content/common/child_proces...
File content/common/child_process_host.cc (right):

http://codereview.chromium.org/8588023/diff/61009/content/common/child_proces...
content/common/child_process_host.cc:289: bool enable =
tracked_objects::ThreadData::tracking_status();
instead of having this code here and in RenderProcessImpl, can't you just have
it in one place (ProfilerMessageFilter::OnChannelConnected)?

http://codereview.chromium.org/8588023/diff/61009/content/common/child_proces...
File content/common/child_process_messages.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/common/child_proces...
content/common/child_process_messages.h:8: #include "base/process.h"
not needed

http://codereview.chromium.org/8588023/diff/61009/content/common/child_proces...
content/common/child_process_messages.h:9: #include "base/values.h"
nit: order

http://codereview.chromium.org/8588023/diff/61009/content/common/child_thread.cc
File content/common/child_thread.cc (right):

http://codereview.chromium.org/8588023/diff/61009/content/common/child_thread...
content/common/child_thread.cc:7: #include "base/bind.h"
nit: not needed?

http://codereview.chromium.org/8588023/diff/61009/content/common/child_thread...
content/common/child_thread.cc:9: #include "base/json/json_writer.h"
nit: not needed

http://codereview.chromium.org/8588023/diff/61009/content/common/child_thread...
content/common/child_thread.cc:236: base::DictionaryValue* value =
tracked_objects::ThreadData::ToValue();
memory leak of value here

http://codereview.chromium.org/8588023/diff/61009/content/common/child_thread.h
File content/common/child_thread.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/common/child_thread...
content/common/child_thread.h:12: #include "content/common/child_process_info.h"
not needed

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
File content/public/browser/profiler_controller.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_controller.h:30: ProfilerController() {}
nit: not needed

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_controller.h:44: // This is called on UI thread.
nit: this is the sort of stuff we should hide from subscribers (see my other
comments)

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_controller.h:49: // This is called on IO thread.
this function is not used by the embedder (chrome), but only as an internal
function by other content code. so it shouldn't be part of the public api that
we expose to embedders. instead just have this function be on
ProfilerControllerImpl and have the the filter call the impl class directly

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_controller.h:55:
DISALLOW_COPY_AND_ASSIGN(ProfilerController);
DISALLOW_COPY_AND_ASSIGN isn't needed on interfaces since you can't instantiate
them

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
File content/public/browser/profiler_subscriber.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_subscriber.h:16: // See also:
child_profiler_message_filter.h
nit: these two comments aren't necessary, someone can search quickly to get the
same information

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_subscriber.h:17: class CONTENT_EXPORT
ProfilerSubscriber {
all classes in content/public have to be in the content namespace

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_subscriber.h:21: virtual void
OnPendingProcesses(int sequence_number,
nit: here and below, please add comments for the methods

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
File content/public/browser/profiler_synchronizer.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_synchronizer.h:16: CONTENT_EXPORT int
GetProfilerData(int sequence_number);
just a few general notes about the api:
-it seems that a return value which is always known is not that useful
-it seems easier for the profiler subscribers, and a little cleaner from an API
point of view, if we hide the fact that we have process hosts on different
threads from them (and them having to worry about incrementing/decrementing this
counter). so why not have ProfilerControllerImpl keep the stats from all the
child processes and give it at one time?
-why are these static methods in a separate file, and not part of
ProfilerController? it seems a little confusing that a ProfilerSubscriber has to
register with ProfilerController to receive the updates, but it actually has to
call another method in a different file to get this started

[jam, 2011-11-28 18:48:16]

http://codereview.chromium.org/8588023/diff/61009/chrome/browser/metrics/trac...
File chrome/browser/metrics/tracking_synchronizer.cc (right):

http://codereview.chromium.org/8588023/diff/61009/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:224: if (processes_pending_ <= 0
&& process_group_count_ <= 0) {
one more thing: if one of the processes crash, will this ever get to 0? i dont
think so. that's another reason to not do the process counter stuff

[ramant (doing other things), 2011-11-29 01:32:20]

Hi John Abd-El-Malek,
  Many many thanks for the thorough/awesome code review. Would appreciate if you
could take a look at the changes one more time.

thanks very much,
raman

http://codereview.chromium.org/8588023/diff/61009/chrome/browser/metrics/trac...
File chrome/browser/metrics/tracking_synchronizer.cc (right):

http://codereview.chromium.org/8588023/diff/61009/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:224: if (processes_pending_ <= 0
&& process_group_count_ <= 0) {
On 2011/11/28 18:48:16, John Abd-El-Malek wrote:
> one more thing: if one of the processes crash, will this ever get to 0? i dont
> think so. that's another reason to not do the process counter stuff

In FetchProfilerDataAsynchronously, we post a delayed task to unregister
(cleanup) after 60 secs. 

We upload an UMA stat on how many processes were unresponsive (and may be
collect stats on how long it took to send a response from all processes).

Wondered if it is worth book keeping of how many  processes have responded etc,
but jar and I had thought it is good to do it.

many many thanks for your comments.

http://codereview.chromium.org/8588023/diff/61009/chrome/browser/metrics/trac...
File chrome/browser/metrics/tracking_synchronizer.h (right):

http://codereview.chromium.org/8588023/diff/61009/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:132: class RequestContext {
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> is this class used by other files, or only tracking_synchronizer.cc? if the
> former, then please put this in a separate header. Note that the name
> RequestContext is a little confusing since there's already a
> content::RequestContext class (I realize they're in different namespaces, but
> this is a core class that's used all over). If the later, then please move
this
> to the cc file.

Done.

http://codereview.chromium.org/8588023/diff/61009/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/8588023/diff/61009/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_message_filter.cc:21: #include
"chrome/browser/metrics/tracking_synchronizer.h"
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> nit: not needed anymore

Done.

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_co...
File content/browser/profiler_controller_impl.cc (right):

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_co...
content/browser/profiler_controller_impl.cc:23: content::ProfilerController*
content::ProfilerController::GetInstance() {
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> by convention, put statics at top of the funcitons. also, content api statics
go
> first

Done.

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_co...
content/browser/profiler_controller_impl.cc:52:
subscriber_->OnProfilerDataCollected(sequence_number, profiler_data);
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> why call the subscriber on the IO thread? the interface should hide internal
> thread implementations and make this interface all one thread (i.e.
> register/unregister/get notified on the same thread (UI) ). 
> 
> it seems that there's a bug (which the above would fix) since a subscriber can
> unregister on the UI thread, but as subscriber_ is being nulled in
> ProfilerControllerImpl on the UI thread, it can be used on the IO thread.

Done.

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_co...
File content/browser/profiler_controller_impl.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_co...
content/browser/profiler_controller_impl.h:13: class DictionaryValue;
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> here and below, no need to duplicate the forward declarations from the parent
> interface

Done.

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_co...
content/browser/profiler_controller_impl.h:21: class CONTENT_EXPORT
ProfilerControllerImpl
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> nit: eventually, all the code in content will be in the content namespace. for
> new code, please put it in the namespace to save us the work of doing that
later

Done.

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_co...
content/browser/profiler_controller_impl.h:31: // Register the subscriber so
that it will be called when for example
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> here and below, don't duplicate the comments from the interface in the
> implmentation.
> 
> // ProfilerController implementation:
> 
> should be enough

Done.

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_me...
File content/browser/profiler_message_filter.cc (right):

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_me...
content/browser/profiler_message_filter.cc:7: #include "base/tracked_objects.h"
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> not needed?

Done.

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_me...
content/browser/profiler_message_filter.cc:8: #include "base/values.h"
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> ditto

Done.

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_me...
File content/browser/profiler_message_filter.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_me...
content/browser/profiler_message_filter.h:10: #include "base/process.h"
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> not needed

Done.

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_me...
content/browser/profiler_message_filter.h:11: #include
"content/common/child_process_info.h"
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> ditto

Done.

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_sy...
File content/browser/profiler_synchronizer.cc (right):

http://codereview.chromium.org/8588023/diff/61009/content/browser/profiler_sy...
content/browser/profiler_synchronizer.cc:37:
base::Bind(&ProfilerController::OnPendingProcesses,
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> nit: per other comments, all this stuff should be internal details and not
> exposed to subscribers

Done.

http://codereview.chromium.org/8588023/diff/61009/content/common/child_proces...
File content/common/child_process_host.cc (right):

http://codereview.chromium.org/8588023/diff/61009/content/common/child_proces...
content/common/child_process_host.cc:289: bool enable =
tracked_objects::ThreadData::tracking_status();
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> instead of having this code here and in RenderProcessImpl, can't you just have
> it in one place (ProfilerMessageFilter::OnChannelConnected)?

Done.

http://codereview.chromium.org/8588023/diff/61009/content/common/child_proces...
File content/common/child_process_messages.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/common/child_proces...
content/common/child_process_messages.h:8: #include "base/process.h"
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> not needed

Done.

http://codereview.chromium.org/8588023/diff/61009/content/common/child_proces...
content/common/child_process_messages.h:9: #include "base/values.h"
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> nit: order

Done.

http://codereview.chromium.org/8588023/diff/61009/content/common/child_thread.cc
File content/common/child_thread.cc (right):

http://codereview.chromium.org/8588023/diff/61009/content/common/child_thread...
content/common/child_thread.cc:7: #include "base/bind.h"
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> nit: not needed?

Done.

http://codereview.chromium.org/8588023/diff/61009/content/common/child_thread...
content/common/child_thread.cc:9: #include "base/json/json_writer.h"
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> nit: not needed

Done.

http://codereview.chromium.org/8588023/diff/61009/content/common/child_thread...
content/common/child_thread.cc:236: base::DictionaryValue* value =
tracked_objects::ThreadData::ToValue();
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> memory leak of value here 

many many thanks for catching this (made the mistake when I converted to not use
json string).

Done.

http://codereview.chromium.org/8588023/diff/61009/content/common/child_thread.h
File content/common/child_thread.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/common/child_thread...
content/common/child_thread.h:12: #include "content/common/child_process_info.h"
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> not needed

Done.

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
File content/public/browser/profiler_controller.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_controller.h:30: ProfilerController() {}
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> nit: not needed

Done.

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_controller.h:44: // This is called on UI thread.
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> nit: this is the sort of stuff we should hide from subscribers (see my other
> comments)

Done.

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_controller.h:49: // This is called on IO thread.
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> this function is not used by the embedder (chrome), but only as an internal
> function by other content code. so it shouldn't be part of the public api that
> we expose to embedders. instead just have this function be on
> ProfilerControllerImpl and have the the filter call the impl class directly

Done.

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_controller.h:55:
DISALLOW_COPY_AND_ASSIGN(ProfilerController);
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> DISALLOW_COPY_AND_ASSIGN isn't needed on interfaces since you can't
instantiate
> them

Done.

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
File content/public/browser/profiler_subscriber.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_subscriber.h:16: // See also:
child_profiler_message_filter.h
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> nit: these two comments aren't necessary, someone can search quickly to get
the
> same information

Done.

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_subscriber.h:17: class CONTENT_EXPORT
ProfilerSubscriber {
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> all classes in content/public have to be in the content namespace

Done.

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_subscriber.h:21: virtual void
OnPendingProcesses(int sequence_number,
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> nit: here and below, please add comments for the methods

Done.

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
File content/public/browser/profiler_synchronizer.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_synchronizer.h:16: CONTENT_EXPORT int
GetProfilerData(int sequence_number);
On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> just a few general notes about the api:
> -it seems that a return value which is always known is not that useful

Didn't want chrome code to know how many type process type iterators there are.
In future if we were to add another process type, thought it is better to hide
those details here.

> -it seems easier for the profiler subscribers, and a little cleaner from an
API
> point of view, if we hide the fact that we have process hosts on different
> threads from them (and them having to worry about incrementing/decrementing
this
> counter). so why not have ProfilerControllerImpl keep the stats from all the
> child processes and give it at one time?

We wanted to send the data back to UI as soon as we have received it. UI could
display how many processes it is waiting for. Currently UI updates the profiler
page with all the data that is collected every 0.5 secs.
As you had pointed the asynchronous nature doesn't stop the UI from updating
even if one process hasn't responded with the data.

> -why are these static methods in a separate file, and not part of
> ProfilerController? it seems a little confusing that a ProfilerSubscriber has
to
> register with ProfilerController to receive the updates, but it actually has
to
> call another method in a different file to get this started

Thanks for the above idea. Deleted this file and moved all the code into
ProfilerController.
Done.

[jam, 2011-11-30 00:22:25]

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
File content/public/browser/profiler_synchronizer.h (right):

http://codereview.chromium.org/8588023/diff/61009/content/public/browser/prof...
content/public/browser/profiler_synchronizer.h:16: CONTENT_EXPORT int
GetProfilerData(int sequence_number);
On 2011/11/29 01:32:20, ramant wrote:
> On 2011/11/28 15:17:34, John Abd-El-Malek wrote:
> > just a few general notes about the api:
> > -it seems that a return value which is always known is not that useful
> 
> Didn't want chrome code to know how many type process type iterators there
are.
> In future if we were to add another process type, thought it is better to hide
> those details here.

but it's not hiding it if we tell the embedder this count.

that's why i think it's still much easier, and simpler from an API point of
view, to just call method once with all the data. another possibility is to make
the callback have another parameter, i.e. "bool end", which is set to true the
last time. that way it doesnt have to do the increment business, and we dont
have to give it this magic number.

> 
> > -it seems easier for the profiler subscribers, and a little cleaner from an
> API
> > point of view, if we hide the fact that we have process hosts on different
> > threads from them (and them having to worry about incrementing/decrementing
> this
> > counter). so why not have ProfilerControllerImpl keep the stats from all the
> > child processes and give it at one time?
> 
> We wanted to send the data back to UI as soon as we have received it. UI could
> display how many processes it is waiting for. Currently UI updates the
profiler
> page with all the data that is collected every 0.5 secs.
> As you had pointed the asynchronous nature doesn't stop the UI from updating
> even if one process hasn't responded with the data.
> 
> > -why are these static methods in a separate file, and not part of
> > ProfilerController? it seems a little confusing that a ProfilerSubscriber
has
> to
> > register with ProfilerController to receive the updates, but it actually has
> to
> > call another method in a different file to get this started
> 
> Thanks for the above idea. Deleted this file and moved all the code into
> ProfilerController.
> Done.

http://codereview.chromium.org/8588023/diff/65035/chrome/browser/metrics/trac...
File chrome/browser/metrics/tracking_synchronizer.cc (right):

http://codereview.chromium.org/8588023/diff/65035/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:28: ~RequestContext();
nit: up to you, but since this class is in the cc file I'd inline the methods to
make it quicker to see what it does

http://codereview.chromium.org/8588023/diff/65035/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:31: friend class
TrackingSynchronizer;
nit: you can get rid fo the private constructor and friend now, since this class
is in the cc file

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
File content/browser/profiler_controller.cc (right):

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
content/browser/profiler_controller.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
this stuff should really be in profiler_controller_impl.cc, and they should be
member functions since they're all related

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
File content/browser/profiler_controller_impl.cc (right):

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
content/browser/profiler_controller_impl.cc:51: if
(BrowserThread::CurrentlyOn(BrowserThread::IO)) {
nit: usually this is written as

(!BrowserThread::CurrentlyOn(BrowserThread::UI))

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
content/browser/profiler_controller_impl.cc:55: base::Unretained(this),
using base::Unretained, you're assuming that this object will be alive. are you
100% sure? why not just use base::WeakPtrFactory to make sure this is safe?

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
File content/browser/profiler_controller_impl.h (right):

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
content/browser/profiler_controller_impl.h:24: //
----------------------------------
nit: why add the "// ----------------------------------"? i dont see that in
other headers

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_me...
File content/browser/profiler_message_filter.h (right):

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_me...
content/browser/profiler_message_filter.h:30: // Message handlers. It calls
ProfilerControllerImpl's OnProfilerDataCollected
don't comment on what the function is doing. anyone can see that when they look
at it. see

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...

"Note that you should never describe the code itself. Assume that the person
reading the code knows C++ better than you do, even though he or she does not
know what you are trying to do"

http://codereview.chromium.org/8588023/diff/65035/content/public/browser/prof...
File content/public/browser/profiler_controller.h (right):

http://codereview.chromium.org/8588023/diff/65035/content/public/browser/prof...
content/public/browser/profiler_controller.h:21: // Contact all processes and
ask them to profiler data. It returns 2 to indicate
it seems that these methods belong on ProfileController, instead of being static
functions.

http://codereview.chromium.org/8588023/diff/65035/content/public/browser/prof...
content/public/browser/profiler_controller.h:22: // that
ProfilerControllerImpl's OnPendingProcesses will be called once for
nit: the comment in the interface shouldn't describe what happens internally. it
should just say that ProfilerSubscriber::OnPendingProcesses will be called

[ramant (doing other things), 2011-11-30 19:37:21]

Hi John Abd-El-Malek,
  Many many thanks for your comments. Made all the changes you had suggested.
Implemented sending "bool end" after sending messages to child processes.

  Added weak_ptr_factory to access ProfilerControllerImpl on UI and IO threads.
Initialized the factories on the correct threads to get the weak_ptr. Moved the
creation of TrackingSynchronizer after threads are created. Would appreciate if
you could suggest if there is a simpler way to do this.

thanks much,
raman

http://codereview.chromium.org/8588023/diff/65035/chrome/browser/metrics/trac...
File chrome/browser/metrics/tracking_synchronizer.cc (right):

http://codereview.chromium.org/8588023/diff/65035/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:28: ~RequestContext();
On 2011/11/30 00:22:25, John Abd-El-Malek wrote:
> nit: up to you, but since this class is in the cc file I'd inline the methods
to
> make it quicker to see what it does

Done.

http://codereview.chromium.org/8588023/diff/65035/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:31: friend class
TrackingSynchronizer;
On 2011/11/30 00:22:25, John Abd-El-Malek wrote:
> nit: you can get rid fo the private constructor and friend now, since this
class
> is in the cc file

Done.

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
File content/browser/profiler_controller.cc (right):

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
content/browser/profiler_controller.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/11/30 00:22:25, John Abd-El-Malek wrote:
> this stuff should really be in profiler_controller_impl.cc, and they should be
> member functions since they're all related

Done.

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
File content/browser/profiler_controller_impl.cc (right):

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
content/browser/profiler_controller_impl.cc:51: if
(BrowserThread::CurrentlyOn(BrowserThread::IO)) {
On 2011/11/30 00:22:25, John Abd-El-Malek wrote:
> nit: usually this is written as
> 
> (!BrowserThread::CurrentlyOn(BrowserThread::UI)) 

Done.

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
content/browser/profiler_controller_impl.cc:55: base::Unretained(this),
On 2011/11/30 00:22:25, John Abd-El-Malek wrote:
> using base::Unretained, you're assuming that this object will be alive. are
you
> 100% sure? why not just use base::WeakPtrFactory to make sure this is safe?

Done.

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
File content/browser/profiler_controller_impl.h (right):

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
content/browser/profiler_controller_impl.h:24: //
----------------------------------
On 2011/11/30 00:22:25, John Abd-El-Malek wrote:
> nit: why add the "// ----------------------------------"? i dont see that in
> other headers

Done.

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_me...
File content/browser/profiler_message_filter.h (right):

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_me...
content/browser/profiler_message_filter.h:30: // Message handlers. It calls
ProfilerControllerImpl's OnProfilerDataCollected
On 2011/11/30 00:22:25, John Abd-El-Malek wrote:
> don't comment on what the function is doing. anyone can see that when they
look
> at it. see
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...
> 
> "Note that you should never describe the code itself. Assume that the person
> reading the code knows C++ better than you do, even though he or she does not
> know what you are trying to do"

Done.

http://codereview.chromium.org/8588023/diff/65035/content/public/browser/prof...
File content/public/browser/profiler_controller.h (right):

http://codereview.chromium.org/8588023/diff/65035/content/public/browser/prof...
content/public/browser/profiler_controller.h:21: // Contact all processes and
ask them to profiler data. It returns 2 to indicate
On 2011/11/30 00:22:25, John Abd-El-Malek wrote:
> it seems that these methods belong on ProfileController, instead of being
static
> functions.

Done.

http://codereview.chromium.org/8588023/diff/65035/content/public/browser/prof...
content/public/browser/profiler_controller.h:22: // that
ProfilerControllerImpl's OnPendingProcesses will be called once for
On 2011/11/30 00:22:25, John Abd-El-Malek wrote:
> nit: the comment in the interface shouldn't describe what happens internally.
it
> should just say that ProfilerSubscriber::OnPendingProcesses will be called

Done.

[jam, 2011-12-01 03:43:16]

lgtm with switching back to base::Unretained, sorry about missing that earlier

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
File content/browser/profiler_controller_impl.cc (right):

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
content/browser/profiler_controller_impl.cc:55: base::Unretained(this),
On 2011/11/30 19:37:21, ramant wrote:
> On 2011/11/30 00:22:25, John Abd-El-Malek wrote:
> > using base::Unretained, you're assuming that this object will be alive. are
> you
> > 100% sure? why not just use base::WeakPtrFactory to make sure this is safe?
> 
> Done.

oops, i'm really sorry.  I forgot that this object is a singelton when I said my
last comment about using weakfactory. since it's a singelton, it will outlive
these threads and base::Unretained is fine as before. so all the weakptr changes
aren't necessary.

[ramant (doing other things), 2011-12-01 21:33:35]

Hi John Abd-El-Malek,
  Used base::Unretained and reverted the weak_ptr changes. Integrated with the
latest changes (as of couple hours ago) with the trunk.

thanks,
raman

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
File content/browser/profiler_controller_impl.cc (right):

http://codereview.chromium.org/8588023/diff/65035/content/browser/profiler_co...
content/browser/profiler_controller_impl.cc:55: base::Unretained(this),
On 2011/12/01 03:43:17, John Abd-El-Malek wrote:
> On 2011/11/30 19:37:21, ramant wrote:
> > On 2011/11/30 00:22:25, John Abd-El-Malek wrote:
> > > using base::Unretained, you're assuming that this object will be alive.
are
> > you
> > > 100% sure? why not just use base::WeakPtrFactory to make sure this is
safe?
> > 
> > Done.
> 
> oops, i'm really sorry.  I forgot that this object is a singelton when I said
my
> last comment about using weakfactory. since it's a singelton, it will outlive
> these threads and base::Unretained is fine as before. so all the weakptr
changes
> aren't necessary.

Done.

[commit-bot: I haz the power, 2011-12-01 21:58:31]

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

[commit-bot: I haz the power, 2011-12-01 23:20:23]

Change committed as 112571