Changes to upload tracked_objects data from all renderer

Changes to upload tracked_objects data from all renderer
processes to browser process.

Added the following messages to render_messages.

ChromeViewMsg_GetRendererTrackedData -  Asks the renderer
to send back tracked data (ThreadData in tracked_objects).

ChromeViewMsg_SetTrackingStatus -  Asks the renderer
to set the tracking status.

ChromeViewHostMsg_RendererTrackedData - render's Send back
tracked data (ThreadData in tracked_objects) as a string.

ChromeViewHostMsg_IsTrackingEnabled -  Asks Browser process
to set the current tracking status from browser in the render.

BUG=101933
R=jar
TEST=browser tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108770

[ramant (doing other things), 2011-10-28 21:55:14]

Hi Jim,
  Would appreciate if you could take a look at this CL to get tracking data from
renderer processes.

thanks,
raman

[jar (doing other things), 2011-10-30 08:04:19]

You asked for some comments... so here they are.

I've pulled all the names off the CC list, as I think this was meant to be a
concept and test CL rather than a real review.

Thanks for setting this up!!!

Jim

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/browser_abou...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:903:
chrome_browser_metrics::TrackingSynchronizer::FetchTrackingDataSynchronously(
I'm assuming this is here just to help with debugging... I think you mentioned
such.

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

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:18: using base::Time;
We probably won't use this.  It is "wall clock" time, and only used for external
visible dates and times.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:66:
base::JSONWriter::Write(callback_object->value_, false, output);
To test this equally simply... you could do a PostTask wit the value you get
from the browser's version of ThreadData. That will allow you to exercise the
code that other processes will call.  I suspect you'll really use that path even
for the browser process.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:102: 10000);
You may as well make this a bit longer.  There is no big carrying cost for
delaying the discard... and some browsers will be pretty darn un-responsive at
times.
I'd suggest 60 seconds (rather than the 10 seconds shown).

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:116: return;
This would leak value.  Perhaps you should do the check before you construct the
value.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:132: ++notification_count;
This number is probably changing underneath us. I'd suggest that you count as
you send, rather than counting in advance.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:136: Register(sequence_number,
callback_object, notification_count);
You should probably just set the count to 1 here (so it is always non-zero
during the loop below). You can then have a method which increments the
notifcation count each time just before you sent a request. Finally, after
you've sent out all the requests, you can decrement the count (using the
function that potentially discards the callback when it hits  zero.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:156: base::AutoLock
auto_lock(lock_);
So long as we only access this on the UI thread, we won't need a lock.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:165: 
DCHECK on UI thread.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:186: // |callback_object_| do a
PostTask to UI thread??
yeah... we should have bounced over to the UI thread at the start of this method
(probably have a static version that runs on the IO thread but bounces all
messages to this method)... so we'd be on the UI thread now.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:187:
request->callback_object_->SendData(value);
You need to do this only if value is non-null.  On line 195 you send NULL down
this path... and you just need to avoid the send data.

I think some of that logic might change... but the NULL check seems reasonable.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:224: TimeTicks::Now() -
started);
This duration should almost always be your 10second number.  I think the first
histogram is more helpful (line 220).

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

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:31: // associated with sequence
number and send the unpicked tracking data to the
nit: unpicked --> unpickled

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:44: // |value| is transferred to
the caller who is trying to collect data.
*if* you think this code will continue to exist in the final implementation....

nit: Suggest you change "The ownership ..." sentence to:

Transfers ownership of |value| to the entity that supplied this callback.  It is
then responsibility of the implementation of SendData() to eventually delete
|value|.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:92: ~TrackingSynchronizer();
make destructor private, so that only reference counting friend can delete us
(and make the base ref counting class our friend).

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:116: int
RegisterAndNotifyAllProcesses(CallbackObject* callback_object);
I'm going to guess that these callback objects are ref-counted, so we should
pass around scoped_ref_ptrs rather tan raw pointers.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:145: base::Lock lock_;
Rather than a lock, we *might* restrict this class to doing most all of its work
on a specific thread, such as the UI thread.  If we do that, then we won't need
a lock.  For the few methods that are on other threads, they should do little
more than process data, and post a task to the proper (UI?) thread to do work
involving access to members.

http://codereview.chromium.org/8413009/diff/10006/chrome/common/chrome_consta...
File chrome/common/chrome_constants.h (right):

http://codereview.chromium.org/8413009/diff/10006/chrome/common/chrome_consta...
chrome/common/chrome_constants.h:99: // this reserved constant avoids any chance
of confusion with a response having
I don't know that we'll need this logic for an "unprovoked" transmission.  We
needed it in histograms, since the data was buffered (in an intermediate tally)
on the browser. For this data, we currently don't buffer it anywhere... so we
don't currently need it.  It is fine to leave the logic (I can imagine we could
do buffering), but the explanation abotu after a page load is not really
applicable.

http://codereview.chromium.org/8413009/diff/10006/chrome/common/render_messag...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/8413009/diff/10006/chrome/common/render_messag...
chrome/common/render_messages.h:505: std::string)
nit: Style seems to include comment in arg list.  Suggest:

std::string /* pickled Value containing profile data */

http://codereview.chromium.org/8413009/diff/10006/chrome/renderer/renderer_tr...
File chrome/renderer/renderer_tracking.cc (right):

http://codereview.chromium.org/8413009/diff/10006/chrome/renderer/renderer_tr...
chrome/renderer/renderer_tracking.cc:43: // argument). Get the process id of the
current process.
I think it might be easier to establish process type back in the browser, when
you were itterating over lists. For example, although there is a render process
list, there might be a few items that are not part of a list, or you might have
to itterate on more than one list... and that list would indicate the type of
process.  Assuming this was determined browser side, you could then IPC pass in
the process type (in addition to the sequence number), and then just pop it into
place here (in the Value).

http://codereview.chromium.org/8413009/diff/10006/chrome/renderer/renderer_tr...
chrome/renderer/renderer_tracking.cc:51: // Send the sequence number and list of
pickled tracking data over synchronous
I think you're on the main renderer thread.  I don't think you nede to do
anything synchronously... but you do possibly have to Post back over to the IO
thread.

[ramant (doing other things), 2011-11-01 00:52:15]

Hi Jim,
  Made all the changes you had suggested. Would appreciate if you could take
another look.

thanks,
raman

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/browser_abou...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:903:
chrome_browser_metrics::TrackingSynchronizer::FetchTrackingDataSynchronously(
On 2011/10/30 08:04:19, jar wrote:
> I'm assuming this is here just to help with debugging... I think you mentioned
> such.

Deleted this file from change set.

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

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:18: using base::Time;
On 2011/10/30 08:04:19, jar wrote:
> We probably won't use this.  It is "wall clock" time, and only used for
external
> visible dates and times.

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:66:
base::JSONWriter::Write(callback_object->value_, false, output);
On 2011/10/30 08:04:19, jar wrote:
> To test this equally simply... you could do a PostTask wit the value you get
> from the browser's version of ThreadData. That will allow you to exercise the
> code that other processes will call.  I suspect you'll really use that path
even
> for the browser process.

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:102: 10000);
On 2011/10/30 08:04:19, jar wrote:
> You may as well make this a bit longer.  There is no big carrying cost for
> delaying the discard... and some browsers will be pretty darn un-responsive at
> times.
> I'd suggest 60 seconds (rather than the 10 seconds shown).

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:116: return;
On 2011/10/30 08:04:19, jar wrote:
> This would leak value.  Perhaps you should do the check before you construct
the
> value.

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:132: ++notification_count;
On 2011/10/30 08:04:19, jar wrote:
> This number is probably changing underneath us. I'd suggest that you count as
> you send, rather than counting in advance.

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:136: Register(sequence_number,
callback_object, notification_count);
On 2011/10/30 08:04:19, jar wrote:
> You should probably just set the count to 1 here (so it is always non-zero
> during the loop below). You can then have a method which increments the
> notifcation count each time just before you sent a request. Finally, after
> you've sent out all the requests, you can decrement the count (using the
> function that potentially discards the callback when it hits  zero.

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:156: base::AutoLock
auto_lock(lock_);
On 2011/10/30 08:04:19, jar wrote:
> So long as we only access this on the UI thread, we won't need a lock.

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:165: 
On 2011/10/30 08:04:19, jar wrote:
> DCHECK on UI thread.

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:186: // |callback_object_| do a
PostTask to UI thread??
On 2011/10/30 08:04:19, jar wrote:
> yeah... we should have bounced over to the UI thread at the start of this
method
> (probably have a static version that runs on the IO thread but bounces all
> messages to this method)... so we'd be on the UI thread now.

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:187:
request->callback_object_->SendData(value);
On 2011/10/30 08:04:19, jar wrote:
> You need to do this only if value is non-null.  On line 195 you send NULL down
> this path... and you just need to avoid the send data.
> 
> I think some of that logic might change... but the NULL check seems
reasonable.

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:224: TimeTicks::Now() -
started);
On 2011/10/30 08:04:19, jar wrote:
> This duration should almost always be your 10second number.  I think the first
> histogram is more helpful (line 220).

Deleted this histogram.
Done.

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

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:31: // associated with sequence
number and send the unpicked tracking data to the
On 2011/10/30 08:04:19, jar wrote:
> nit: unpicked --> unpickled

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:44: // |value| is transferred to
the caller who is trying to collect data.
On 2011/10/30 08:04:19, jar wrote:
> *if* you think this code will continue to exist in the final
implementation....
> 
> nit: Suggest you change "The ownership ..." sentence to:
> 
> Transfers ownership of |value| to the entity that supplied this callback.  It
is
> then responsibility of the implementation of SendData() to eventually delete
> |value|.

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:92: ~TrackingSynchronizer();
On 2011/10/30 08:04:19, jar wrote:
> make destructor private, so that only reference counting friend can delete us
> (and make the base ref counting class our friend).

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:116: int
RegisterAndNotifyAllProcesses(CallbackObject* callback_object);
On 2011/10/30 08:04:19, jar wrote:
> I'm going to guess that these callback objects are ref-counted, so we should
> pass around scoped_ref_ptrs rather tan raw pointers.

I am hoping the Callback that is defined in this file, will be deleted.
Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:145: base::Lock lock_;
On 2011/10/30 08:04:19, jar wrote:
> Rather than a lock, we *might* restrict this class to doing most all of its
work
> on a specific thread, such as the UI thread.  If we do that, then we won't
need
> a lock.  For the few methods that are on other threads, they should do little
> more than process data, and post a task to the proper (UI?) thread to do work
> involving access to members.

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/common/chrome_consta...
File chrome/common/chrome_constants.h (right):

http://codereview.chromium.org/8413009/diff/10006/chrome/common/chrome_consta...
chrome/common/chrome_constants.h:99: // this reserved constant avoids any chance
of confusion with a response having
On 2011/10/30 08:04:19, jar wrote:
> I don't know that we'll need this logic for an "unprovoked" transmission.  We
> needed it in histograms, since the data was buffered (in an intermediate
tally)
> on the browser. For this data, we currently don't buffer it anywhere... so we
> don't currently need it.  It is fine to leave the logic (I can imagine we
could
> do buffering), but the explanation abotu after a page load is not really
> applicable.

Deleted this.
Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/common/render_messag...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/8413009/diff/10006/chrome/common/render_messag...
chrome/common/render_messages.h:505: std::string)
On 2011/10/30 08:04:19, jar wrote:
> nit: Style seems to include comment in arg list.  Suggest:
> 
> std::string /* pickled Value containing profile data */

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/renderer/renderer_tr...
File chrome/renderer/renderer_tracking.cc (right):

http://codereview.chromium.org/8413009/diff/10006/chrome/renderer/renderer_tr...
chrome/renderer/renderer_tracking.cc:43: // argument). Get the process id of the
current process.
On 2011/10/30 08:04:19, jar wrote:
> I think it might be easier to establish process type back in the browser, when
> you were itterating over lists. For example, although there is a render
process
> list, there might be a few items that are not part of a list, or you might
have
> to itterate on more than one list... and that list would indicate the type of
> process.  Assuming this was determined browser side, you could then IPC pass
in
> the process type (in addition to the sequence number), and then just pop it
into
> place here (in the Value).

Done.

http://codereview.chromium.org/8413009/diff/10006/chrome/renderer/renderer_tr...
chrome/renderer/renderer_tracking.cc:51: // Send the sequence number and list of
pickled tracking data over synchronous
On 2011/10/30 08:04:19, jar wrote:
> I think you're on the main renderer thread.  I don't think you nede to do
> anything synchronously... but you do possibly have to Post back over to the IO
> thread.

Used logic similar to "renderer histograms".

[jar (doing other things), 2011-11-03 06:23:15]

Here are some questions and comments.

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

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:30: :
last_used_sequence_number_(kNeverUsableSequenceNumber) {
nit: Indent the colon for an initializer list 4 spaces.

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:49: return
tracking_synchronizer_;
You might want to DCHECK() that we're on the UI thread.  Access to this from any
other thread would be racy.

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:156: TrackingSynchronizer*
current_synchronizer = CurrentSynchronizer();
I think we need to bounce over to the UI thread before accessing the
CurrentSynchronizer.

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:231: return;
This will probably leak value.
Probably delete it since you're not passing it on.

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:233: request = it->second;
nit: Move declarations on lines 225 down to here (declare closer to first use.

Probably move line 226 closer... or to just before line 236.

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:242: }
probably:
else {
  delete value;
}

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:267: started =
request->request_start_time_;
Do we need this anymore?  Probably delete declaration at line 256 as well.

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:282: static const int
kReservedSequenceNumber = 0;
Do you anticipate a need for this?

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

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:10: #include <vector>
nit: add #include <map>

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:107: static void
FetchTrackingDataSynchronously(std::string* output);
nit: probably should have todo() to remove this when final.  I think this was
only for testing during development.

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

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_message_filter.cc:232:
sequence_number, tracked_data, ChildProcessInfo::RENDER_PROCESS);
Do we have corresponding returns from other process types?

http://codereview.chromium.org/8413009/diff/22021/chrome/renderer/renderer_tr...
File chrome/renderer/renderer_tracking.cc (right):

http://codereview.chromium.org/8413009/diff/22021/chrome/renderer/renderer_tr...
chrome/renderer/renderer_tracking.cc:55: base::JSONWriter::Write(value.get(),
false, &pickled_tracking_data_);
Why do we need to write the pickled data into a slot?  We have no further need
fro it (after doing the send) I think. Why do we need to hold on to it?

http://codereview.chromium.org/8413009/diff/22021/content/renderer/render_thr...
File content/renderer/render_thread_impl.h (right):

http://codereview.chromium.org/8413009/diff/22021/content/renderer/render_thr...
content/renderer/render_thread_impl.h:37: class RendererTracking;
Is this needed in this file anymore?

[ramant (doing other things), 2011-11-03 21:51:39]

Hi Jim,
  Made all the changes you have suggested. Will add the support for other
process types in another CL.

thanks much,
raman

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

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:30: :
last_used_sequence_number_(kNeverUsableSequenceNumber) {
On 2011/11/03 06:23:15, jar wrote:
> nit: Indent the colon for an initializer list 4 spaces.

Done.

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:49: return
tracking_synchronizer_;
On 2011/11/03 06:23:15, jar wrote:
> You might want to DCHECK() that we're on the UI thread.  Access to this from
any
> other thread would be racy.

Done.

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:156: TrackingSynchronizer*
current_synchronizer = CurrentSynchronizer();
On 2011/11/03 06:23:15, jar wrote:
> I think we need to bounce over to the UI thread before accessing the
> CurrentSynchronizer.

Done.

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:231: return;
On 2011/11/03 06:23:15, jar wrote:
> This will probably leak value.
> Probably delete it since you're not passing it on.

Done.

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:233: request = it->second;
On 2011/11/03 06:23:15, jar wrote:
> nit: Move declarations on lines 225 down to here (declare closer to first use.
> 
> Probably move line 226 closer... or to just before line 236.

Done.

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:242: }
On 2011/11/03 06:23:15, jar wrote:
> probably:
> else {
>   delete value;
> }

Done.

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:267: started =
request->request_start_time_;
On 2011/11/03 06:23:15, jar wrote:
> Do we need this anymore?  Probably delete declaration at line 256 as well.

Done.

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:282: static const int
kReservedSequenceNumber = 0;
On 2011/11/03 06:23:15, jar wrote:
> Do you anticipate a need for this? 

Done.

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

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:10: #include <vector>
On 2011/11/03 06:23:15, jar wrote:
> nit: add #include <map>

Done.

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.h:107: static void
FetchTrackingDataSynchronously(std::string* output);
On 2011/11/03 06:23:15, jar wrote:
> nit: probably should have todo() to remove this when final.  I think this was
> only for testing during development.

Done.

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

http://codereview.chromium.org/8413009/diff/22021/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_message_filter.cc:232:
sequence_number, tracked_data, ChildProcessInfo::RENDER_PROCESS);
On 2011/11/03 06:23:15, jar wrote:
> Do we have corresponding returns from other process types?

Added a TODO to implement for other process types.

http://codereview.chromium.org/8413009/diff/22021/chrome/renderer/renderer_tr...
File chrome/renderer/renderer_tracking.cc (right):

http://codereview.chromium.org/8413009/diff/22021/chrome/renderer/renderer_tr...
chrome/renderer/renderer_tracking.cc:55: base::JSONWriter::Write(value.get(),
false, &pickled_tracking_data_);
On 2011/11/03 06:23:15, jar wrote:
> Why do we need to write the pickled data into a slot?  We have no further need
> fro it (after doing the send) I think. Why do we need to hold on to it?

Done.

http://codereview.chromium.org/8413009/diff/22021/content/renderer/render_thr...
File content/renderer/render_thread_impl.h (right):

http://codereview.chromium.org/8413009/diff/22021/content/renderer/render_thr...
content/renderer/render_thread_impl.h:37: class RendererTracking;
On 2011/11/03 06:23:15, jar wrote:
> Is this needed in this file anymore?

Done.

[ramant (doing other things), 2011-11-04 03:49:32]

Hi Jim and Eric,
  Integrated the changes into Eric's code. Verified tracking_data is being
uploaded from renderer and browser. Verified the tracking_data is being deleted.
Verified if the tab is closed it doesn't call the TrackedUI code.

thanks
raman

[jar (doing other things), 2011-11-04 21:38:21]

A nit and a small questions, and otherwise it looks great.

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

http://codereview.chromium.org/8413009/diff/44054/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:220: if
(--request->processes_pending_ <= 0)
nit: I'd prefer if this happened after you did the callback (after line 228). 
IMO, until we do the callback, we still do have an outstanding issue pending.

Reading the code, I was afraid the request_ instance would do something
automatically when the pending count got low.

...or is there a reason we need to do this now.. and save the result in
completed??

http://codereview.chromium.org/8413009/diff/44054/chrome/browser/ui/webui/tra...
File chrome/browser/ui/webui/tracking_ui.h (right):

http://codereview.chromium.org/8413009/diff/44054/chrome/browser/ui/webui/tra...
chrome/browser/ui/webui/tracking_ui.h:24: // TrackingMessageHandler accesses it.
Maybe you can provide an accessor, or a test and call interface??  Members
should be private.

[ramant (doing other things), 2011-11-04 22:01:38]

thanks Jim for your comments. Made the changes you had suggested.

-raman

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

http://codereview.chromium.org/8413009/diff/44054/chrome/browser/metrics/trac...
chrome/browser/metrics/tracking_synchronizer.cc:220: if
(--request->processes_pending_ <= 0)
On 2011/11/04 21:38:21, jar wrote:
> nit: I'd prefer if this happened after you did the callback (after line 228). 
> IMO, until we do the callback, we still do have an outstanding issue pending.
> 
> Reading the code, I was afraid the request_ instance would do something
> automatically when the pending count got low.
> 
> ...or is there a reason we need to do this now.. and save the result in
> completed??

Done.

http://codereview.chromium.org/8413009/diff/44054/chrome/browser/ui/webui/tra...
File chrome/browser/ui/webui/tracking_ui.h (right):

http://codereview.chromium.org/8413009/diff/44054/chrome/browser/ui/webui/tra...
chrome/browser/ui/webui/tracking_ui.h:24: // TrackingMessageHandler accesses it.
On 2011/11/04 21:38:21, jar wrote:
> Maybe you can provide an accessor, or a test and call interface??  Members
> should be private.

Done.

[jar (doing other things), 2011-11-04 23:37:39]

lgtm

[commit-bot: I haz the power, 2011-11-04 23:38:12]

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

[commit-bot: I haz the power, 2011-11-05 01:37:39]

Try job failure for 8413009-52001 (retry) on win_rel for step
"chrome_frame_net_tests".
It's a second try, previously, step "chrome_frame_net_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-05 02:04:53]

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