[UMA] Use proper C++ objects to serialize tracked_objects across process boundaries.

[UMA] Use proper C++ objects to serialize tracked_objects across process boundaries.

In more detail:

(1) Adds serialized versions of the key tracked_objects:: classes.

(2) Rename Snapshot -> SerializedSnapshot (to match other serialized class names).  Move a bit of complexity out of the class into the calling code, so that the class is just a dumb data container.

(3) Rename DataCollector -> SerializedProcessData.  Move most of the complexity out of the class into the calling code, so that the class is just a dumb data container.

(4) Safer memory management: Rather than returning a heap-allocated object that the caller is expected to take ownership of, have the caller pass in an allocated object (either stack- or heap-allocated -- whichever is more convenient for the client).


BUG=103480
TEST=none (refactoring, no functional change expected)
TBR=ajwong@chromium.org,estade@chromium.org

[Ilya Sherman, 2012-03-14 06:03:45]

Jim and/or Eric, please take a look at all of the changes.
John, please look over the content/ changes for OWNER approval.

The motivation for this CL is to upload the profiler data to UMA: [
https://chromiumcodereview.appspot.com/9702015/ ].  I will rebase that CL once
this one is reviewed and committed, so that the actual logic changes are
separated from the refactoring.  I know this CL still looks kind of monstrous,
but most of the changes are mechanical...

https://chromiumcodereview.appspot.com/9702014/diff/1/base/profiler/tracked_t...
File base/profiler/tracked_time.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/1/base/profiler/tracked_t...
base/profiler/tracked_time.h:15: // TODO(isherman): Shouldn't this be explicitly
an int32 or something like that?
Jim, thoughts?

https://chromiumcodereview.appspot.com/9702014/diff/1/chrome/browser/metrics/...
File chrome/browser/metrics/tracking_synchronizer.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/1/chrome/browser/metrics/...
chrome/browser/metrics/tracking_synchronizer.cc:30: // roll our own version?
Reviewers, thoughts?  Just less code this way?

[eroman, 2012-03-14 07:37:33]

I'll defer to jar (since I've mostly just edited profiler_ui.cc)

[jam, 2012-03-14 17:45:31]

http://codereview.chromium.org/9702014/diff/29/content/public/common/serializ...
File content/public/common/serialized_profiler_data.h (right):

http://codereview.chromium.org/9702014/diff/29/content/public/common/serializ...
content/public/common/serialized_profiler_data.h:24: public
tracked_objects::SerializedProcessData {
in content/public we put interfaces and dont derived from concrete types. it's
similar to WebKit API
see http://www.chromium.org/developers/content-module/content-api

[Ilya Sherman, 2012-03-14 18:01:28]

http://codereview.chromium.org/9702014/diff/29/content/public/common/serializ...
File content/public/common/serialized_profiler_data.h (right):

http://codereview.chromium.org/9702014/diff/29/content/public/common/serializ...
content/public/common/serialized_profiler_data.h:24: public
tracked_objects::SerializedProcessData {
On 2012/03/14 17:45:31, John Abd-El-Malek wrote:
> in content/public we put interfaces and dont derived from concrete types. it's
> similar to WebKit API
> see http://www.chromium.org/developers/content-module/content-api

This is currently a struct, which is just a dumb data container used for IPC
serialization.  (There is also a trivial ToValue() method, to re-serialize to
JSON.  That doesn't really have to live in content/, though it is consistent
with how the similar dumb data containers are defined in base/.)  The link you
pointed me at seems to include an exception for structs: "it's acceptable to put
implementation files that hold constructors/destructors of interfaces/structs
which might have member variables. For structs..."

If that exception is not relevant to this class, could you please explain why
not, and perhaps update the API docs?  Also, if that exception is not relevant,
what would you recommend -- reworking this struct to be a proper class, with
private member variables and public accessors?  That seems unduly tedious just
to provide a handy container for serialization...

[jam, 2012-03-14 19:01:59]

On 2012/03/14 18:01:28, Ilya Sherman wrote:
>
http://codereview.chromium.org/9702014/diff/29/content/public/common/serializ...
> File content/public/common/serialized_profiler_data.h (right):
> 
>
http://codereview.chromium.org/9702014/diff/29/content/public/common/serializ...
> content/public/common/serialized_profiler_data.h:24: public
> tracked_objects::SerializedProcessData {
> On 2012/03/14 17:45:31, John Abd-El-Malek wrote:
> > in content/public we put interfaces and dont derived from concrete types.
it's
> > similar to WebKit API
> > see http://www.chromium.org/developers/content-module/content-api
> 
> This is currently a struct, which is just a dumb data container used for IPC
> serialization.  (There is also a trivial ToValue() method, to re-serialize to
> JSON.  That doesn't really have to live in content/, though it is consistent
> with how the similar dumb data containers are defined in base/.)  The link you
> pointed me at seems to include an exception for structs: "it's acceptable to
put
> implementation files that hold constructors/destructors of interfaces/structs
> which might have member variables. For structs..."
> 
> If that exception is not relevant to this class, could you please explain why
> not, and perhaps update the API docs?

we put structs, but not ones that derive from other structs.

>  Also, if that exception is not relevant,
> what would you recommend -- reworking this struct to be a proper class, with
> private member variables and public accessors?  That seems unduly tedious just
> to provide a handy container for serialization...

it's quicker for you if you can take a look at the rest of the api and see how
other classes deal with this. for me to block off time to look through this
patch and understand how it's doing things, and recommend a solution, it'll take
a while.

[jam, 2012-03-14 19:14:03]

http://codereview.chromium.org/9702014/diff/29/content/public/common/serializ...
File content/public/common/serialized_profiler_data.h (right):

http://codereview.chromium.org/9702014/diff/29/content/public/common/serializ...
content/public/common/serialized_profiler_data.h:29: void
ToValue(base::DictionaryValue* dictionary) const;
one more note: this is against the style guide for a struct

[Ilya Sherman, 2012-03-16 22:56:42]

There are still some unittest files that need fixing up due to the migration of
the ToValue() code; but those changes will all be fairly mechanical, and
shouldn't need to block the review.

https://chromiumcodereview.appspot.com/9702014/diff/29/content/public/common/...
File content/public/common/serialized_profiler_data.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/29/content/public/common/...
content/public/common/serialized_profiler_data.h:24: public
tracked_objects::SerializedProcessData {
On 2012/03/14 17:45:31, John Abd-El-Malek wrote:
> in content/public we put interfaces and dont derived from concrete types. it's
> similar to WebKit API
> see http://www.chromium.org/developers/content-module/content-api

Ok, removed this struct entirely in favor of passing two separate parameters.

https://chromiumcodereview.appspot.com/9702014/diff/29/content/public/common/...
content/public/common/serialized_profiler_data.h:29: void
ToValue(base::DictionaryValue* dictionary) const;
On 2012/03/14 19:14:03, John Abd-El-Malek wrote:
> one more note: this is against the style guide for a struct

Very well, extracted this out to a static function in chrome/browser instead.

[Ilya Sherman, 2012-03-17 03:33:47]

Eric, you might want to double check my changes to profiler.js (new as of patch
set 5).

Jim, I'd prefer to hold off on fixing up the tracked_objects unittest until I
get at least an initial round of review comments from you confirming that you're
ok with the direction this CL is headed in.

[eroman, 2012-03-17 03:45:22]

changes to the .js look good

[jam, 2012-03-18 19:31:54]

(just looked at content)

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

http://codereview.chromium.org/9702014/diff/8002/content/common/child_process...
content/common/child_process_messages.h:92: int,                  /* sequence
number */
there's no need to tell the child process what type it is, just to have it send
it back. this information is usually just kept in the browser process. when you
create the filter for these messages in the browser, pass in the process type
there and hold it as a member

http://codereview.chromium.org/9702014/diff/8002/content/public/browser/profi...
File content/public/browser/profiler_subscriber.h (right):

http://codereview.chromium.org/9702014/diff/8002/content/public/browser/profi...
content/public/browser/profiler_subscriber.h:33: content::ProcessType
process_type) = 0;
nit: no need for content::

[Ilya Sherman, 2012-03-19 22:01:23]

https://chromiumcodereview.appspot.com/9702014/diff/8002/content/common/child...
File content/common/child_process_messages.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/8002/content/common/child...
content/common/child_process_messages.h:92: int,                  /* sequence
number */
On 2012/03/18 19:31:54, John Abd-El-Malek wrote:
> there's no need to tell the child process what type it is, just to have it
send
> it back. this information is usually just kept in the browser process. when
you
> create the filter for these messages in the browser, pass in the process type
> there and hold it as a member

Done.

https://chromiumcodereview.appspot.com/9702014/diff/8002/content/public/brows...
File content/public/browser/profiler_subscriber.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/8002/content/public/brows...
content/public/browser/profiler_subscriber.h:33: content::ProcessType
process_type) = 0;
On 2012/03/18 19:31:54, John Abd-El-Malek wrote:
> nit: no need for content::

Done.

[jam, 2012-03-19 22:39:37]

content lgtm

[jar (doing other things), 2012-03-21 18:19:55]

+1 for the concept and direction: Moving to a real C++ objects.

The use of Value was strongly targeted at the only real consumer of the data (at
the time), which was JS.  The fact that I couldn't add methods to the Value
class is what forced me to use them as simple untyped container classes...
although I didn't like doin' it.

When moving to real objects, it would be much better to use objects rather than
mere structs.  I hate structs, and vastly prefer classes with restricted
accessors and mutators. IMO, in this case, since you're moving to real objects,
the objects should have private members (so that they are easier to review and
understand), and should have methods.

Summary uber-comment: I think all the structs should be classes (members made
private).  Those serialized classes should have methods that can emit Value
instance representations (recursively calling each other's methods).  They might
also have methods to do the emission of the protobufs.... but I'm not sure about
what the interfaces of protobufs are like... and how plausible that would be.

Am I off base and missing something?

http://codereview.chromium.org/9702014/diff/8002/base/location.h
File base/location.h (right):

http://codereview.chromium.org/9702014/diff/8002/base/location.h#newcode69
base/location.h:69: struct BASE_EXPORT SerializedLocation {
IMO: It is much better to use classes, and accessors, than to use a struct. The
privacy of the members makes it easy to read locally, without fears that other
folks will change the interior values.

http://codereview.chromium.org/9702014/diff/8002/base/profiler/tracked_time.h
File base/profiler/tracked_time.h (right):

http://codereview.chromium.org/9702014/diff/8002/base/profiler/tracked_time.h...
base/profiler/tracked_time.h:15: // TODO(isherman): Shouldn't this be explicitly
an int32 or something like that?
I think a change to int32 would be good.  This typedef was (as I recall) used as
an internal optimization to get closer to what we needed sooner in the call
stack.

When reading and writing these values, we do count on them being atomically
written or read (i.e., you can't get a partial read or write) on all our
platforms.  I'm careful to avoid the need for barriers and locks.  It *could* be
that to be super careful, these should be changed to atomics, with non-barrier
accesses...  but the smaller change to an explicit int32 seems like a good idea.

http://codereview.chromium.org/9702014/diff/8002/base/tracked_objects.cc
File base/tracked_objects.cc (right):

http://codereview.chromium.org/9702014/diff/8002/base/tracked_objects.cc#newc...
base/tracked_objects.cc:17: #include "base/port.h"
I'm not sure.... but I think port.h needs to come after build_config.h.  IF so,
we should have a comment to help keep it in place.

http://codereview.chromium.org/9702014/diff/8002/base/tracked_objects.h
File base/tracked_objects.h (right):

http://codereview.chromium.org/9702014/diff/8002/base/tracked_objects.h#newco...
base/tracked_objects.h:201: const Location location() const { return location_;
}
I think these simple methods were in the CC file because there was a move toward
not putting method bodies in headers (and relying on global optimization for
inlining).  

Is there a new policy to reverse this trend?

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

http://codereview.chromium.org/9702014/diff/8002/chrome/browser/metrics/track...
chrome/browser/metrics/tracking_synchronizer.cc:29: // roll our own version?
You'd have to look closely at the semantics we use.  Singleton would do for the
lazy construction, but once we start teardown, and have destroyed it, we
wouldn't want to accidentally re-incarnate it.

In general, I vastly prefer explicit construction and destruction, unless there
is a strong reason to be lazy (and implicitly have weakly defined construction
and destruction ordering).

http://codereview.chromium.org/9702014/diff/8002/chrome/browser/metrics/track...
File chrome/browser/metrics/tracking_synchronizer.h (right):

http://codereview.chromium.org/9702014/diff/8002/chrome/browser/metrics/track...
chrome/browser/metrics/tracking_synchronizer.h:98: content::ProcessType
process_type);
nit/comment/suggest/question: Shouldn't process_type be embedded in the profiler
data?

http://codereview.chromium.org/9702014/diff/8002/chrome/browser/task_profiler...
File chrome/browser/task_profiler/task_profiler_data_serializer.h (right):

http://codereview.chromium.org/9702014/diff/8002/chrome/browser/task_profiler...
chrome/browser/task_profiler/task_profiler_data_serializer.h:32: const
tracked_objects::SerializedProcessData& process_data,
IMO, this should be a method on the SerializedProcessData class.

I also suspect that the type should be embedded in that class.

http://codereview.chromium.org/9702014/diff/8002/content/browser/profiler_con...
File content/browser/profiler_controller_impl.h (left):

http://codereview.chromium.org/9702014/diff/8002/content/browser/profiler_con...
content/browser/profiler_controller_impl.h:47: // Contact child processes and
set profiler status to |enable|.
Historically there was a plan to turn the profiler on and off dynamically.  This
API was meant to support that... but I now think that won't be an issue... so
this API can indeed be vanquished.

[Ilya Sherman, 2012-03-21 19:23:02]

https://chromiumcodereview.appspot.com/9702014/diff/8002/base/location.h
File base/location.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/8002/base/location.h#newc...
base/location.h:69: struct BASE_EXPORT SerializedLocation {
On 2012/03/21 18:19:55, jar wrote:
> IMO: It is much better to use classes, and accessors, than to use a struct.
The
> privacy of the members makes it easy to read locally, without fears that other
> folks will change the interior values.

Hmm.  If these were to be classes, they would need both setters and getters, as
(to the best of my knowledge) the IPC code requires some way to initialize data
other than through the constructor.  So, making these be proper classes, rather
than structs, would not add any safety, but it would add a bunch of extra
boiler-plate code...

https://chromiumcodereview.appspot.com/9702014/diff/8002/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/8002/base/tracked_objects...
base/tracked_objects.cc:17: #include "base/port.h"
On 2012/03/21 18:19:55, jar wrote:
> I'm not sure.... but I think port.h needs to come after build_config.h.  IF
so,
> we should have a comment to help keep it in place.

It looks like port.h already #includes build_config.h.  If that's the only
reason that build_config.h is being #included here, I guess we can remove it
entirely.

https://chromiumcodereview.appspot.com/9702014/diff/8002/base/tracked_objects.h
File base/tracked_objects.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/8002/base/tracked_objects...
base/tracked_objects.h:201: const Location location() const { return location_;
}
On 2012/03/21 18:19:55, jar wrote:
> I think these simple methods were in the CC file because there was a move
toward
> not putting method bodies in headers (and relying on global optimization for
> inlining).  
> 
> Is there a new policy to reverse this trend?

The style guide [
http://dev.chromium.org/developers/coding-style#TOC-Inline-functions ]
explicitly makes an exception for simple accessors, the same functions for which
the style guide recommends unix_hacker_style() names.

https://chromiumcodereview.appspot.com/9702014/diff/8002/chrome/browser/metri...
File chrome/browser/metrics/tracking_synchronizer.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/8002/chrome/browser/metri...
chrome/browser/metrics/tracking_synchronizer.h:98: content::ProcessType
process_type);
On 2012/03/21 18:19:55, jar wrote:
> nit/comment/suggest/question: Shouldn't process_type be embedded in the
profiler
> data?

I also wanted to write it that way, but our visibility rules don't allow it: the
ProcessType enum is defined under content/ whereas the profiler code lives in
base/.  We could do something hacky like casting the enum to an int and back,
but that really seems like we'd just be unwisely circumventing the visibility
rules.

https://chromiumcodereview.appspot.com/9702014/diff/8002/chrome/browser/task_...
File chrome/browser/task_profiler/task_profiler_data_serializer.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/8002/chrome/browser/task_...
chrome/browser/task_profiler/task_profiler_data_serializer.h:32: const
tracked_objects::SerializedProcessData& process_data,
On 2012/03/21 18:19:55, jar wrote:
> IMO, this should be a method on the SerializedProcessData class.

I'd strongly prefer to keep the JSON serialization code under chrome/browser/,
as there's no need for the base/ code to know how to serialize this data to JSON
-- the only client of it is a Chrome feature.  In theory, JSON serialization
could be useful for other projects building off of base/; but in practice, I'm
concerned that the JSON format is too closely tied to the chrome://profiler
page's JavaScript for this to work out smoothly.

[Ilya Sherman, 2012-03-21 21:45:42]

https://chromiumcodereview.appspot.com/9702014/diff/8002/base/profiler/tracke...
File base/profiler/tracked_time.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/8002/base/profiler/tracke...
base/profiler/tracked_time.h:15: // TODO(isherman): Shouldn't this be explicitly
an int32 or something like that?
On 2012/03/21 18:19:55, jar wrote:
> I think a change to int32 would be good.  This typedef was (as I recall) used
as
> an internal optimization to get closer to what we needed sooner in the call
> stack.
> 
> When reading and writing these values, we do count on them being atomically
> written or read (i.e., you can't get a partial read or write) on all our
> platforms.  I'm careful to avoid the need for barriers and locks.  It *could*
be
> that to be super careful, these should be changed to atomics, with non-barrier
> accesses...  but the smaller change to an explicit int32 seems like a good
idea.

Ah, looks like we are already using int32 within this header.  I've split this
bit of cleanup off into https://chromiumcodereview.appspot.com/9818004/

https://chromiumcodereview.appspot.com/9702014/diff/8002/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/8002/base/tracked_objects...
base/tracked_objects.cc:17: #include "base/port.h"
On 2012/03/21 19:23:02, Ilya Sherman wrote:
> On 2012/03/21 18:19:55, jar wrote:
> > I'm not sure.... but I think port.h needs to come after build_config.h.  IF
> so,
> > we should have a comment to help keep it in place.
> 
> It looks like port.h already #includes build_config.h.  If that's the only
> reason that build_config.h is being #included here, I guess we can remove it
> entirely.

Yep, looks like it's not needed for this file.  Removed.

https://chromiumcodereview.appspot.com/9702014/diff/8002/chrome/browser/metri...
File chrome/browser/metrics/tracking_synchronizer.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/8002/chrome/browser/metri...
chrome/browser/metrics/tracking_synchronizer.cc:29: // roll our own version?
On 2012/03/21 18:19:55, jar wrote:
> You'd have to look closely at the semantics we use.  Singleton would do for
the
> lazy construction, but once we start teardown, and have destroyed it, we
> wouldn't want to accidentally re-incarnate it.
> 
> In general, I vastly prefer explicit construction and destruction, unless
there
> is a strong reason to be lazy (and implicitly have weakly defined construction
> and destruction ordering).

Ok. :)

[jar (doing other things), 2012-04-04 17:55:35]

I *think* the code is fine.... but as per our discussion, changing names will
probably make it easier to read and verify.

http://codereview.chromium.org/9702014/diff/29001/base/tracked_objects.cc
File base/tracked_objects.cc (right):

http://codereview.chromium.org/9702014/diff/29001/base/tracked_objects.cc#new...
base/tracked_objects.cc:862: : parent(*parent_child.first),
nit: indent 4 from left margin

http://codereview.chromium.org/9702014/diff/29001/base/tracked_objects.h
File base/tracked_objects.h (right):

http://codereview.chromium.org/9702014/diff/29001/base/tracked_objects.h#newc...
base/tracked_objects.h:147: // The SerializedProcessData struct is a serialized
representation of the list
nit:  The use of the word "Serialized..." as a prefix is probably not the best. 
Usually this means you've translated to something like a string, or some serial
data.  In truth, you've put it into a persistent struct that can be copied
around freely.  I'm not sure what the name should be... and need to think
more...

Some plausible words would be:
Snapshot
Frozen

I guess you're using the word Serialized to suggest the mental model that "this
class has the moral equivalent of serialized data, and you can think of it as
such."  It is a bit unexpected that the data then effectively has accessors
(slotted access to members).

http://codereview.chromium.org/9702014/diff/29001/base/tracked_objects.h#newc...
base/tracked_objects.h:333: struct BASE_EXPORT SerializedSnapshot {
nit: The historical name "Snapshot" is probably not good to preserve, given that
you're developing a bunch of snapshot classes now.

As per our discussion, this class shows the bottom level leaf in the data
structure, describing data on a set of "similar" tasks.  It could almost be a
Lifetime, except it lists aggregate data about a whole set of tasks that had the
same birth location and death thread (and Lifetime sounds like a singular run,
not a set of runs of a task).  Perhaps we'd call this collection of "similar
lives" a Family, or we could think of it as as a LineItem or Row in the final
profiler report.  Plausible names are then:

Family
LineItem
TaskGroup
TaskRow
Lifetimes  (note the plural)
Lives

For any of the above names, the word "Snapshot" should probably be a postfix,
such as:
FamilySnapshot
LivesSnapshot

...but Frozen should probably be a prefix:
FrozenFamily
FrozenLives

http://codereview.chromium.org/9702014/diff/29001/base/tracked_objects.h#newc...
base/tracked_objects.h:540: // of living instances of the task.  If |reset_max|
is true, then the max
Comment should be clear that the count includes only births for which a death
has not yet been recorded.  (or includes all births, including dead ones.. if I
have it backwards).  Comment (probably copied from my words) only says it "keeps
track" and  I wasn't sure if we had to contrast with process_data to get live
counts.

http://codereview.chromium.org/9702014/diff/29001/base/tracked_objects_unitte...
File base/tracked_objects_unittest.cc (right):

http://codereview.chromium.org/9702014/diff/29001/base/tracked_objects_unitte...
base/tracked_objects_unittest.cc:16: return
base::IntToString(base::GetCurrentProcId());
nit: I'm not sure which is better.... but you could put this in as a slot in the
TrackedObjectTest class, that is initialized during the constructor.

I can't make up my mind... so leaving it as is should be fine... but I thought
I'd mention the alternative.

http://codereview.chromium.org/9702014/diff/29001/base/tracked_objects_unitte...
base/tracked_objects_unittest.cc:40: }
These lines (surely written by me) are useless, since we call directly into the
ThreadData method on lines 28 and 34.

http://codereview.chromium.org/9702014/diff/29001/base/tracked_objects_unitte...
base/tracked_objects_unittest.cc:182: scoped_ptr<base::DictionaryValue>
value(new base::DictionaryValue);
I think the scoped pointer was only useful because we used to get a return of a
pointer.  Now that you're creating the value locally, you can just use a local
instance (instead of a scoped pointer).

http://codereview.chromium.org/9702014/diff/29001/base/tracked_objects_unitte...
base/tracked_objects_unittest.cc:259: scoped_ptr<base::DictionaryValue>
dictionary(new base::DictionaryValue);
Suggest using just an instance (not scoped pointer).  Here and below.

http://codereview.chromium.org/9702014/diff/29001/chrome/browser/task_profile...
File chrome/browser/task_profiler/task_profiler_data_serializer.cc (right):

http://codereview.chromium.org/9702014/diff/29001/chrome/browser/task_profile...
chrome/browser/task_profiler/task_profiler_data_serializer.cc:103:
scoped_ptr<DictionaryValue> snapshot(new DictionaryValue);
Can a value instance work here, instead of a scoped ptr?

http://codereview.chromium.org/9702014/diff/29001/chrome/browser/task_profile...
chrome/browser/task_profiler/task_profiler_data_serializer.cc:117:
scoped_ptr<base::DictionaryValue> parent_child(new base::DictionaryValue);
instance vs scoped ptr?

http://codereview.chromium.org/9702014/diff/29001/chrome/browser/task_profile...
chrome/browser/task_profiler/task_profiler_data_serializer.cc:146:
scoped_ptr<base::DictionaryValue> this_process_data_json(
instance?

http://codereview.chromium.org/9702014/diff/29001/chrome/browser/task_profile...
File chrome/browser/task_profiler/task_profiler_data_serializer_unittest.cc
(right):

http://codereview.chromium.org/9702014/diff/29001/chrome/browser/task_profile...
chrome/browser/task_profiler/task_profiler_data_serializer_unittest.cc:80:
process_data.snapshots.back().death_data.queue_duration_sum = 13;
sums should probably be larger than max or sample values.  It would be tempting
to push asserts into the object... so we may as well "keep it real" in case such
happens.

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

http://codereview.chromium.org/9702014/diff/29001/content/browser/profiler_co...
content/browser/profiler_controller_impl.h:29: // Send the number of
|pending_processes| to |subscriber_|. |end| is set to
This comment seemed very hard to read and understand... so I'd suggest:

Notify the |subscriber_| that it should expect data from at least
|pending_processes| additional distinct notifications.  OnPendingProcess may be
called repeatedly, and the last call should have |end| set to true (indicating
that there is no longer a possibility for the count of pending processes to
increase).

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

http://codereview.chromium.org/9702014/diff/29001/content/public/browser/prof...
content/public/browser/profiler_subscriber.h:33: content::ProcessType
process_type) = 0;
jam mentioned in a nit that you don't need "content" but I think your change got
lost (didn't get uploaded??)

[Ilya Sherman, 2012-04-05 02:51:03]

https://chromiumcodereview.appspot.com/9702014/diff/29001/base/tracked_object...
File base/tracked_objects.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/29001/base/tracked_object...
base/tracked_objects.cc:862: : parent(*parent_child.first),
On 2012/04/04 17:55:35, jar wrote:
> nit: indent 4 from left margin

Done.

https://chromiumcodereview.appspot.com/9702014/diff/29001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/29001/base/tracked_object...
base/tracked_objects.h:147: // The SerializedProcessData struct is a serialized
representation of the list
On 2012/04/04 17:55:35, jar wrote:
> nit:  The use of the word "Serialized..." as a prefix is probably not the
best. 
> Usually this means you've translated to something like a string, or some
serial
> data.  In truth, you've put it into a persistent struct that can be copied
> around freely.  I'm not sure what the name should be... and need to think
> more...
> 
> Some plausible words would be:
> Snapshot
> Frozen
> 
> I guess you're using the word Serialized to suggest the mental model that
"this
> class has the moral equivalent of serialized data, and you can think of it as
> such."  It is a bit unexpected that the data then effectively has accessors
> (slotted access to members).

Went with 'Snapshot'

https://chromiumcodereview.appspot.com/9702014/diff/29001/base/tracked_object...
base/tracked_objects.h:333: struct BASE_EXPORT SerializedSnapshot {
On 2012/04/04 17:55:35, jar wrote:
> nit: The historical name "Snapshot" is probably not good to preserve, given
that
> you're developing a bunch of snapshot classes now.
> 
> As per our discussion, this class shows the bottom level leaf in the data
> structure, describing data on a set of "similar" tasks.  It could almost be a
> Lifetime, except it lists aggregate data about a whole set of tasks that had
the
> same birth location and death thread (and Lifetime sounds like a singular run,
> not a set of runs of a task).  Perhaps we'd call this collection of "similar
> lives" a Family, or we could think of it as as a LineItem or Row in the final
> profiler report.  Plausible names are then:
> 
> Family
> LineItem
> TaskGroup
> TaskRow
> Lifetimes  (note the plural)
> Lives

Went with just 'TaskSnapshot' -- does that sound ok to you?

> For any of the above names, the word "Snapshot" should probably be a postfix,
> such as:
> FamilySnapshot
> LivesSnapshot
> 
> ...but Frozen should probably be a prefix:
> FrozenFamily
> FrozenLives

https://chromiumcodereview.appspot.com/9702014/diff/29001/base/tracked_object...
base/tracked_objects.h:540: // of living instances of the task.  If |reset_max|
is true, then the max
On 2012/04/04 17:55:35, jar wrote:
> Comment should be clear that the count includes only births for which a death
> has not yet been recorded.  (or includes all births, including dead ones.. if
I
> have it backwards).  Comment (probably copied from my words) only says it
"keeps
> track" and  I wasn't sure if we had to contrast with process_data to get live
> counts.

Done.

https://chromiumcodereview.appspot.com/9702014/diff/29001/base/tracked_object...
File base/tracked_objects_unittest.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/29001/base/tracked_object...
base/tracked_objects_unittest.cc:40: }
On 2012/04/04 17:55:35, jar wrote:
> These lines (surely written by me) are useless, since we call directly into
the
> ThreadData method on lines 28 and 34.

This seems to be used by the TrackedObjectsTest.MinimalStartupShutdown test (and
is needed to change the visibility of
ThreadData::ShutdownSingleThreadedCleanup() for that test).

https://chromiumcodereview.appspot.com/9702014/diff/29001/base/tracked_object...
base/tracked_objects_unittest.cc:182: scoped_ptr<base::DictionaryValue>
value(new base::DictionaryValue);
On 2012/04/04 17:55:35, jar wrote:
> I think the scoped pointer was only useful because we used to get a return of
a
> pointer.  Now that you're creating the value locally, you can just use a local
> instance (instead of a scoped pointer).

This test code is actually completely out of sync with the rest of the CL,
because I didn't want to spend time updating it if we decided to go in a
different direction.  There is no longer any way to generate the DictionaryValue
serialization within base/ -- I'll need to fix up the tests, which I'll do in a
follow-up patch set.  (Note: patch set, not CL :)

https://chromiumcodereview.appspot.com/9702014/diff/29001/chrome/browser/task...
File chrome/browser/task_profiler/task_profiler_data_serializer.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/29001/chrome/browser/task...
chrome/browser/task_profiler/task_profiler_data_serializer.cc:103:
scoped_ptr<DictionaryValue> snapshot(new DictionaryValue);
On 2012/04/04 17:55:35, jar wrote:
> Can a value instance work here, instead of a scoped ptr?

Unfortunately no, because the |snapshots_list| will take ownership of this
pointer on line 105.  Saves a copy, but definitely makes code a little trickier
to follow...

https://chromiumcodereview.appspot.com/9702014/diff/29001/chrome/browser/task...
chrome/browser/task_profiler/task_profiler_data_serializer.cc:117:
scoped_ptr<base::DictionaryValue> parent_child(new base::DictionaryValue);
On 2012/04/04 17:55:35, jar wrote:
> instance vs scoped ptr?

Ditto.

https://chromiumcodereview.appspot.com/9702014/diff/29001/chrome/browser/task...
chrome/browser/task_profiler/task_profiler_data_serializer.cc:146:
scoped_ptr<base::DictionaryValue> this_process_data_json(
On 2012/04/04 17:55:35, jar wrote:
> instance?

Ditto.

https://chromiumcodereview.appspot.com/9702014/diff/29001/chrome/browser/task...
File chrome/browser/task_profiler/task_profiler_data_serializer_unittest.cc
(right):

https://chromiumcodereview.appspot.com/9702014/diff/29001/chrome/browser/task...
chrome/browser/task_profiler/task_profiler_data_serializer_unittest.cc:80:
process_data.snapshots.back().death_data.queue_duration_sum = 13;
On 2012/04/04 17:55:35, jar wrote:
> sums should probably be larger than max or sample values.  It would be
tempting
> to push asserts into the object... so we may as well "keep it real" in case
such
> happens.

Oops, maxes should probably be larger than samples, too.  That's how I'd meant
to set things up, but I guess the variables got swapped around...  Thanks for
catching that!

https://chromiumcodereview.appspot.com/9702014/diff/29001/content/browser/pro...
File content/browser/profiler_controller_impl.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/29001/content/browser/pro...
content/browser/profiler_controller_impl.h:29: // Send the number of
|pending_processes| to |subscriber_|. |end| is set to
On 2012/04/04 17:55:35, jar wrote:
> This comment seemed very hard to read and understand... so I'd suggest:
> 
> Notify the |subscriber_| that it should expect data from at least
> |pending_processes| additional distinct notifications.  OnPendingProcess may
be
> called repeatedly, and the last call should have |end| set to true (indicating
> that there is no longer a possibility for the count of pending processes to
> increase).

Done (with some further tweaks to the wording).

https://chromiumcodereview.appspot.com/9702014/diff/29001/content/public/brow...
File content/public/browser/profiler_subscriber.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/29001/content/public/brow...
content/public/browser/profiler_subscriber.h:33: content::ProcessType
process_type) = 0;
On 2012/04/04 17:55:35, jar wrote:
> jam mentioned in a nit that you don't need "content" but I think your change
got
> lost (didn't get uploaded??)

Oops.  Re-fixed.

[Ilya Sherman, 2012-04-05 05:10:56]

Unit tests are now fixed up to live in the brave new world as well.

[jar (doing other things), 2012-04-09 23:32:59]

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/location.h
File base/location.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/location.h#new...
base/location.h:74: ~LocationSnapshot();
Probably no need for a destructor. The data items take care of themselves.

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
File base/tracked_objects.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects.cc:29: const bool kTrackParentChildLinks = true;
I haven't checked to see if this has a perf implication.  I really should
:-/.... but I don't know that you meant to include this change in the CL.

Could you add a TODO(jar) for me to enable this (and be sure the impact is not
bad in general).

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_objects.h
File base/tracked_objects.h (left):

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects.h:699: typedef std::map<const BirthOnThread*, int>
BirthCount;
I'm not sure where this typedef should move.... but by keeping it, you can avoid
having to repeat this long declaration about 4-6 times in the header and CC
file.

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects.h:223: ~BirthOnThreadSnapshot();
Probably not needed.

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects.h:315: ~DeathDataSnapshot();
Not needed?

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects.h:338: ~TaskSnapshot();
not needed probably.

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects.h:526: // true, then the max values in each DeathData
instance should be reset during
nit (on my own comment): "...should be reset during..."  --> "... are reset
by..."

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects.h:540: // be reset during the scan.
nit: "...should be reset during..."  --> "... are reset by..."

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects.h:698: ~ParentChildPairSnapshot();
not needed.

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects.h:710: ~ProcessDataSnapshot();
not needed.

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
File base/tracked_objects_unittest.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects_unittest.cc:31: void Reset() {
Your name is much better... but I was worried that folks would try to call this
dangerous function without realizing that "it doesn't work" when you're live, in
a multi-threaded scenario.  I think it is fine here in the unit test, but you
should definitely not propogate this further (into the ThreadData class).

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects_unittest.cc:144: Births* first_birth =
ThreadData::TallyABirthIfActive(location);
Hmm... it looks like I left this out :-(.  Good catch.  Interesting.

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects_unittest.cc:159: // Don't conclude the run, so that we
don't use the actual timer that we
It looks like you concluded the run (contrary to this comment) on line 155.  As
a positive note, you should be able to calculate the duration from (end_time -
start_time).InMilliseconds(), and contrast it with data that you gather.

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects_unittest.cc:189: EXPECT_EQ(0,
process_data.tasks[0].death_data.run_duration_sum);
I *suspect* these times are 0 only because the duration is really small.  I bet
this will turn to a larger number on a slow machine, or when you get unlucky and
the time-calls are made across the border of an timer update.  I'd *suggest* you
calculate the time using end_time and start_time, and use that for most of these
comparisons.

...or am I confused about what is being timed here?

You can "test" my hypothesis by putting a 30ms sleep on line 153.  That should
probably cause this test to fail (currently).

https://chromiumcodereview.appspot.com/9702014/diff/48001/chrome/browser/task...
File chrome/browser/task_profiler/task_profiler_data_serializer.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/chrome/browser/task...
chrome/browser/task_profiler/task_profiler_data_serializer.cc:29: void
LocationSnapshotToJson(const LocationSnapshot& location,
Most of these functions names should be changed into something closer to:
...ToValue()

https://chromiumcodereview.appspot.com/9702014/diff/48001/chrome/browser/task...
File chrome/browser/task_profiler/task_profiler_data_serializer.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/chrome/browser/task...
chrome/browser/task_profiler/task_profiler_data_serializer.h:31: static void
SerializeToJson(
This should probably be called something more like:
ToValue().

Would it be reasonable to take the first two arguments in the constructor, and
hold the Value() representation as a member?

https://chromiumcodereview.appspot.com/9702014/diff/48001/content/common/chil...
File content/common/child_thread.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/content/common/chil...
content/common/child_thread.cc:229: ThreadData::Snapshot(false, &process_data);
Probably not worth trying to plumb now, but we need to pass in the first
argument to reset_max or not.  Adding a TODO(jar) would be helpful.  Thanks!

[Ilya Sherman, 2012-04-10 00:37:42]

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/location.h
File base/location.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/location.h#new...
base/location.h:74: ~LocationSnapshot();
On 2012/04/09 23:32:59, jar wrote:
> Probably no need for a destructor. The data items take care of themselves.

We need a destructor so that the compiler's auto-generated destructor is not
inlined -- not inlining speeds up compilation & linking.  The explicit
destructors in tracked_objects.h are needed for essentially the same reason.

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
File base/tracked_objects.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects.cc:29: const bool kTrackParentChildLinks = true;
On 2012/04/09 23:32:59, jar wrote:
> I haven't checked to see if this has a perf implication.  I really should
> :-/.... but I don't know that you meant to include this change in the CL.
> 
> Could you add a TODO(jar) for me to enable this (and be sure the impact is not
> bad in general).

Done.  Definitely didn't mean to include this in this CL -- thanks for catching
that!

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_objects.h
File base/tracked_objects.h (left):

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects.h:699: typedef std::map<const BirthOnThread*, int>
BirthCount;
On 2012/04/09 23:32:59, jar wrote:
> I'm not sure where this typedef should move.... but by keeping it, you can
avoid
> having to repeat this long declaration about 4-6 times in the header and CC
> file.

Done.

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects.h:526: // true, then the max values in each DeathData
instance should be reset during
On 2012/04/09 23:32:59, jar wrote:
> nit (on my own comment): "...should be reset during..."  --> "... are reset
> by..."

Done.

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects.h:540: // be reset during the scan.
On 2012/04/09 23:32:59, jar wrote:
> nit: "...should be reset during..."  --> "... are reset by..."

Done.

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
File base/tracked_objects_unittest.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects_unittest.cc:31: void Reset() {
On 2012/04/09 23:32:59, jar wrote:
> Your name is much better... but I was worried that folks would try to call
this
> dangerous function without realizing that "it doesn't work" when you're live,
in
> a multi-threaded scenario.  I think it is fine here in the unit test, but you
> should definitely not propogate this further (into the ThreadData class).

Yeah, I agree that we don't want to propagate this beyond the unit test file.

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects_unittest.cc:159: // Don't conclude the run, so that we
don't use the actual timer that we
On 2012/04/09 23:32:59, jar wrote:
> It looks like you concluded the run (contrary to this comment) on line 155. 
As
> a positive note, you should be able to calculate the duration from (end_time -
> start_time).InMilliseconds(), and contrast it with data that you gather.

Removed this comment in a more recent patch set :)

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects_unittest.cc:189: EXPECT_EQ(0,
process_data.tasks[0].death_data.run_duration_sum);
On 2012/04/09 23:32:59, jar wrote:
> I *suspect* these times are 0 only because the duration is really small.  I
bet
> this will turn to a larger number on a slow machine, or when you get unlucky
and
> the time-calls are made across the border of an timer update.  I'd *suggest*
you
> calculate the time using end_time and start_time, and use that for most of
these
> comparisons.
> 
> ...or am I confused about what is being timed here?
> 
> You can "test" my hypothesis by putting a 30ms sleep on line 153.  That should
> probably cause this test to fail (currently).

Good catch!  Fixed.

https://chromiumcodereview.appspot.com/9702014/diff/48001/chrome/browser/task...
File chrome/browser/task_profiler/task_profiler_data_serializer.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/chrome/browser/task...
chrome/browser/task_profiler/task_profiler_data_serializer.cc:29: void
LocationSnapshotToJson(const LocationSnapshot& location,
On 2012/04/09 23:32:59, jar wrote:
> Most of these functions names should be changed into something closer to:
> ...ToValue()

Done.

https://chromiumcodereview.appspot.com/9702014/diff/48001/chrome/browser/task...
File chrome/browser/task_profiler/task_profiler_data_serializer.h (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/chrome/browser/task...
chrome/browser/task_profiler/task_profiler_data_serializer.h:31: static void
SerializeToJson(
On 2012/04/09 23:32:59, jar wrote:
> This should probably be called something more like:
> ToValue().

Done.

> Would it be reasonable to take the first two arguments in the constructor, and
> hold the Value() representation as a member?

Probably not, as this function is also called from profiler_ui.cc, whereas
TaskProfilerDataSerializer() is mostly geared toward serialization to disk.  If
we decide to use protocol buffers for serializing to disk, then I'll move this
code out into profiler_ui.cc.  In the meanwhile, I think it makes sense to keep
it as a static helper function here.

[jar (doing other things), 2012-04-10 15:09:36]

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
File base/tracked_objects_unittest.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects_unittest.cc:132: TEST_F(TrackedObjectsTest,
ParentChildTest) {
It looks like much of this test (and certainly its name) vanished.  Where did
the corresponding code go?

https://chromiumcodereview.appspot.com/9702014/diff/63002/base/tracked_object...
File base/tracked_objects_unittest.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/63002/base/tracked_object...
base/tracked_objects_unittest.cc:19: const char kStillAlive[] = "Still_Alive";
The standardizing on the names, and line numbers tends to weaken these tests  a
bit.  This code all tends to deal with centralized data, and the fear is that
some data will be pushed into the database, and not be reset in a later test. 
When you have the same file name, function name, etc., you can't catch that
class of error.  Note for instance that we use atoms to record birth locations,
and this change would tend to cause us to reuse a single atom (or not realize we
had failed to update a pointer to a new atom).

Although it is slightly painful to list new values in each test, I think it adds
to the coverage.

https://chromiumcodereview.appspot.com/9702014/diff/63002/base/tracked_object...
base/tracked_objects_unittest.cc:44: void TallyABirth(const Location& location,
const std::string& thread_name) {
This was a nice refactor to create these two helper functions.

https://chromiumcodereview.appspot.com/9702014/diff/63002/chromeos/dbus/flimf...
File chromeos/dbus/flimflam_profile_client.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/63002/chromeos/dbus/flimf...
chromeos/dbus/flimflam_profile_client.cc:9: #include "base/values.h"
Although these flimflam changes look good.... did you mean to include them in
this CL?

[Ilya Sherman, 2012-04-10 20:34:40]

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
File base/tracked_objects_unittest.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/48001/base/tracked_object...
base/tracked_objects_unittest.cc:132: TEST_F(TrackedObjectsTest,
ParentChildTest) {
On 2012/04/10 15:09:36, jar wrote:
> It looks like much of this test (and certainly its name) vanished.  Where did
> the corresponding code go?

This test just added additional EXPECTations to the TinyStartupShutdown test, so
I combined the two.

https://chromiumcodereview.appspot.com/9702014/diff/63002/base/tracked_object...
File base/tracked_objects_unittest.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/63002/base/tracked_object...
base/tracked_objects_unittest.cc:19: const char kStillAlive[] = "Still_Alive";
On 2012/04/10 15:09:36, jar wrote:
> The standardizing on the names, and line numbers tends to weaken these tests 
a
> bit.  This code all tends to deal with centralized data, and the fear is that
> some data will be pushed into the database, and not be reset in a later test. 
> When you have the same file name, function name, etc., you can't catch that
> class of error.  Note for instance that we use atoms to record birth
locations,
> and this change would tend to cause us to reuse a single atom (or not realize
we
> had failed to update a pointer to a new atom).
> 
> Although it is slightly painful to list new values in each test, I think it
adds
> to the coverage.

Does the DifferentLives test not give us the desired coverage?  As I understand,
unit tests should be written to be independent -- there's no guarantee that
they'll be run in any particular sequence or that all of them will be run
together.  So, if we're missing coverage, we should probably add new tests
providing this coverage.  But... I think we are already explicitly testing this.
 Am I misunderstanding your comment?

https://chromiumcodereview.appspot.com/9702014/diff/63002/chromeos/dbus/flimf...
File chromeos/dbus/flimflam_profile_client.cc (right):

https://chromiumcodereview.appspot.com/9702014/diff/63002/chromeos/dbus/flimf...
chromeos/dbus/flimflam_profile_client.cc:9: #include "base/values.h"
On 2012/04/10 15:09:36, jar wrote:
> Although these flimflam changes look good.... did you mean to include them in
> this CL?

Sadly, yes.  I removed the #include "base/values.h" from tracked_objects.h in
this CL, and somehow the flimflam code had previously been inheriting that
#include...

[jar (doing other things), 2012-04-12 00:39:15]

lgtm

[Evan Stade, 2012-04-12 00:39:44]

rubber stamp on webui

[Ilya Sherman, 2012-04-12 00:44:08]

+James for webui OWNERS review

[Ilya Sherman, 2012-04-12 00:44:56]

Oops, missed Evan's rubber stamp -- thanks Evan :)

[commit-bot: I haz the power, 2012-04-12 00:45:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/9702014/76034

[James Hawkins, 2012-04-12 00:48:40]

webui lgtm

[commit-bot: I haz the power, 2012-04-12 01:06:22]

Try job failure for 9702014-76034 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...