Added task to Observer methods

Added task to Observer methods

By adding the task to the observer methods and GetBirthPlace() to Tracked it
makes it possible to register an observer that can print more information about
the tasks as they are being processed.

BUG=NONE
TEST=MessageLoopTest.*

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

[DaveMoore, 2010-10-24 20:51:21]

By adding the task to the observer methods and GetBirthPlace() to Tracked it
makes it possible to register an observer that can print more information about
the tasks as they are being processed.

[willchan no longer on Chromium, 2010-10-24 22:47:09]

LGTM, but adding Darin as a reviewer since he usually wants to at least know
about this stuff.  You should probably give him at least a day of courtesy
notice in case he wants to make any comment here.

I copied your message into the changelist description so that it'll show up in
the subversion logs as the justification for this.  You may want to add your
motivating use case into the changelist description.  For anyone else that reads
this code review after the fact, it was because Dave wants to log the
birthplaces in a TaskObserver, which can be post-processed later on so we can
better analyze startup.

http://codereview.chromium.org/4081003/diff/2001/3002
File base/message_loop.h (right):

http://codereview.chromium.org/4081003/diff/2001/3002#newcode75
base/message_loop.h:75: virtual void WillProcessTask(const Task* task) = 0;
Turns out I originally wrote it like this, but Darin asked if it was necessary. 
It wasn't necessary for me yet, so I just passed the |birth_time|. 
http://codereview.chromium.org/2098020/diff/1/3#newcode72.

I'm fine with what you have, but my personal style pref here is to pass a const
Task&.  I find this more self-documenting that it can never be NULL.  Darin said
Tasks are usually passed as pointers, but I believe that's because we basically
never pass them as const anyway.  Anyway, up to you / Darin.

http://codereview.chromium.org/4081003/diff/2001/3005
File base/tracked.h (right):

http://codereview.chromium.org/4081003/diff/2001/3005#newcode97
base/tracked.h:97: const Location GetBirthPlace() const;
Nit: how about const Location&?  Then the caller can copy it if he wants.  I
think you only made a copy since the #ifndef just returns a default.  I believe
the temporary will stay in scope as long as there's a valid reference, so you
can safely return const Location&.

[willchan no longer on Chromium, 2010-10-24 22:49:08]

Oops, actually add Darin.

[darin (slow to review), 2010-10-25 05:52:10]

http://codereview.chromium.org/4081003/diff/2001/3002
File base/message_loop.h (right):

http://codereview.chromium.org/4081003/diff/2001/3002#newcode75
base/message_loop.h:75: virtual void WillProcessTask(const Task* task) = 0;
On 2010/10/24 22:47:09, willchan wrote:
> Turns out I originally wrote it like this, but Darin asked if it was
necessary. 
> It wasn't necessary for me yet, so I just passed the |birth_time|. 
> http://codereview.chromium.org/2098020/diff/1/3#newcode72.
> 
> I'm fine with what you have, but my personal style pref here is to pass a
const
> Task&.  I find this more self-documenting that it can never be NULL.  Darin
said
> Tasks are usually passed as pointers, but I believe that's because we
basically
> never pass them as const anyway.  Anyway, up to you / Darin.

I can support making this be |const Task&|.

One thing to keep in mind with references is casting.  If you need to downcast,
then you should do so using pointers instead of references.  Since Task is
always intended to be subclassed, it feels better to pass as a pointer, but
maybe that's just personal bias.

http://codereview.chromium.org/4081003/diff/2001/3005
File base/tracked.h (right):

http://codereview.chromium.org/4081003/diff/2001/3005#newcode97
base/tracked.h:97: const Location GetBirthPlace() const;
On 2010/10/24 22:47:09, willchan wrote:
> Nit: how about const Location&?  Then the caller can copy it if he wants.  I
> think you only made a copy since the #ifndef just returns a default.  I
believe
> the temporary will stay in scope as long as there's a valid reference, so you
> can safely return const Location&.

That doesn't work.  The implementation allocates a temporary object on the stack
and returns that, so you have to return by value.  The copy constructor will get
optimized away.

const Location Tracked::GetBirthPlace() const {
  return Location("NoFunctionName", "NeedToSetBirthPlace", -1);
}

Actually, a better solution would be to create a static Location object that can
be returned.  Then you can return a const Location&, which would be cheaper and
generate less code.

const Location& Tracked::GetBirthPlace() const {
  static const Location kNone("none", "none", -1);
  return kNone;
}

[willchan no longer on Chromium, 2010-10-25 06:12:04]

http://codereview.chromium.org/4081003/diff/2001/3005
File base/tracked.h (right):

http://codereview.chromium.org/4081003/diff/2001/3005#newcode97
base/tracked.h:97: const Location GetBirthPlace() const;
On 2010/10/25 05:52:11, darin wrote:
> On 2010/10/24 22:47:09, willchan wrote:
> > Nit: how about const Location&?  Then the caller can copy it if he wants.  I
> > think you only made a copy since the #ifndef just returns a default.  I
> believe
> > the temporary will stay in scope as long as there's a valid reference, so
you
> > can safely return const Location&.
> 
> That doesn't work.  The implementation allocates a temporary object on the
stack
> and returns that, so you have to return by value.  The copy constructor will
get
> optimized away.

For some reason I thought it'd create a temporary on the caller's stack that
would remain in scope as long as the temporary was in scope.  I clearly was
confused =/

> 
> const Location Tracked::GetBirthPlace() const {
>   return Location("NoFunctionName", "NeedToSetBirthPlace", -1);
> }
> 
> Actually, a better solution would be to create a static Location object that
can
> be returned.  Then you can return a const Location&, which would be cheaper
and
> generate less code.
> 
> const Location& Tracked::GetBirthPlace() const {
>   static const Location kNone("none", "none", -1);
>   return kNone;
> }

Hm, that's what I would have suggested at first, but isn't this not-threadsafe
since we disable locks for function local statics?  Can't we have a race on two
calls to GetBirthPlace() and have one thread access a partially initialized
value?  If I'm wrong, then yeah, let's go with the function local static. 
Otherwise, I'd be fine with just returning by value, since this is such a minor
piece of code anyway since it's usually ifdef'd out.

[willchan no longer on Chromium, 2010-10-25 06:14:39]

http://codereview.chromium.org/4081003/diff/2001/3002
File base/message_loop.h (right):

http://codereview.chromium.org/4081003/diff/2001/3002#newcode75
base/message_loop.h:75: virtual void WillProcessTask(const Task* task) = 0;
On 2010/10/25 05:52:11, darin wrote:
> On 2010/10/24 22:47:09, willchan wrote:
> > Turns out I originally wrote it like this, but Darin asked if it was
> necessary. 
> > It wasn't necessary for me yet, so I just passed the |birth_time|. 
> > http://codereview.chromium.org/2098020/diff/1/3#newcode72.
> > 
> > I'm fine with what you have, but my personal style pref here is to pass a
> const
> > Task&.  I find this more self-documenting that it can never be NULL.  Darin
> said
> > Tasks are usually passed as pointers, but I believe that's because we
> basically
> > never pass them as const anyway.  Anyway, up to you / Darin.
> 
> I can support making this be |const Task&|.
> 
> One thing to keep in mind with references is casting.  If you need to
downcast,
> then you should do so using pointers instead of references.  Since Task is
> always intended to be subclassed, it feels better to pass as a pointer, but
> maybe that's just personal bias.

Oops, missed this one.  I agree with you here about pointers for when we
down_cast.  However, I don't think we ever want to down cast in a TaskObserver,
since we have no idea what the Task subtype is.  If there's a need to down cast,
then I agree, let's make it a pointer.

[darin (slow to review), 2010-10-25 19:26:20]

On Sun, Oct 24, 2010 at 11:12 PM, <willchan@chromium.org> wrote:

>
> http://codereview.chromium.org/4081003/diff/2001/3005
> File base/tracked.h (right):
>
> http://codereview.chromium.org/4081003/diff/2001/3005#newcode97
> base/tracked.h:97: const Location GetBirthPlace() const;
> On 2010/10/25 05:52:11, darin wrote:
>
>> On 2010/10/24 22:47:09, willchan wrote:
>> > Nit: how about const Location&?  Then the caller can copy it if he
>>
> wants.  I
>
>> > think you only made a copy since the #ifndef just returns a default.
>>
>  I
>
>> believe
>> > the temporary will stay in scope as long as there's a valid
>>
> reference, so you
>
>> > can safely return const Location&.
>>
>
>  That doesn't work.  The implementation allocates a temporary object on
>>
> the stack
>
>> and returns that, so you have to return by value.  The copy
>>
> constructor will get
>
>> optimized away.
>>
>
> For some reason I thought it'd create a temporary on the caller's stack
> that would remain in scope as long as the temporary was in scope.  I
> clearly was confused =/
>
>
>
>  const Location Tracked::GetBirthPlace() const {
>>   return Location("NoFunctionName", "NeedToSetBirthPlace", -1);
>> }
>>
>
>  Actually, a better solution would be to create a static Location
>>
> object that can
>
>> be returned.  Then you can return a const Location&, which would be
>>
> cheaper and
>
>> generate less code.
>>
>
>  const Location& Tracked::GetBirthPlace() const {
>>   static const Location kNone("none", "none", -1);
>>   return kNone;
>> }
>>
>
> Hm, that's what I would have suggested at first, but isn't this
> not-threadsafe since we disable locks for function local statics?  Can't
> we have a race on two calls to GetBirthPlace() and have one thread
> access a partially initialized value?  If I'm wrong, then yeah, let's go
> with the function local static.  Otherwise, I'd be fine with just
> returning by value, since this is such a minor piece of code anyway
> since it's usually ifdef'd out.


I think the risk is that we'd potentially initialize kNone twice, which
should
be harmless.  Or, do you know for a fact that the static initializer guard
is
toggled before running the Location constructor?

-Darin


>
>
> http://codereview.chromium.org/4081003/show
>

[willchan no longer on Chromium, 2010-10-25 20:33:06]

On Mon, Oct 25, 2010 at 12:26 PM, Darin Fisher <darin@chromium.org> wrote:

> On Sun, Oct 24, 2010 at 11:12 PM, <willchan@chromium.org> wrote:
>
>>
>> http://codereview.chromium.org/4081003/diff/2001/3005
>> File base/tracked.h (right):
>>
>> http://codereview.chromium.org/4081003/diff/2001/3005#newcode97
>> base/tracked.h:97: const Location GetBirthPlace() const;
>> On 2010/10/25 05:52:11, darin wrote:
>>
>>> On 2010/10/24 22:47:09, willchan wrote:
>>> > Nit: how about const Location&?  Then the caller can copy it if he
>>>
>> wants.  I
>>
>>> > think you only made a copy since the #ifndef just returns a default.
>>>
>>  I
>>
>>> believe
>>> > the temporary will stay in scope as long as there's a valid
>>>
>> reference, so you
>>
>>> > can safely return const Location&.
>>>
>>
>>  That doesn't work.  The implementation allocates a temporary object on
>>>
>> the stack
>>
>>> and returns that, so you have to return by value.  The copy
>>>
>> constructor will get
>>
>>> optimized away.
>>>
>>
>> For some reason I thought it'd create a temporary on the caller's stack
>> that would remain in scope as long as the temporary was in scope.  I
>> clearly was confused =/
>>
>>
>>
>>  const Location Tracked::GetBirthPlace() const {
>>>   return Location("NoFunctionName", "NeedToSetBirthPlace", -1);
>>> }
>>>
>>
>>  Actually, a better solution would be to create a static Location
>>>
>> object that can
>>
>>> be returned.  Then you can return a const Location&, which would be
>>>
>> cheaper and
>>
>>> generate less code.
>>>
>>
>>  const Location& Tracked::GetBirthPlace() const {
>>>   static const Location kNone("none", "none", -1);
>>>   return kNone;
>>> }
>>>
>>
>> Hm, that's what I would have suggested at first, but isn't this
>> not-threadsafe since we disable locks for function local statics?  Can't
>> we have a race on two calls to GetBirthPlace() and have one thread
>> access a partially initialized value?  If I'm wrong, then yeah, let's go
>> with the function local static.  Otherwise, I'd be fine with just
>> returning by value, since this is such a minor piece of code anyway
>> since it's usually ifdef'd out.
>
>
> I think the risk is that we'd potentially initialize kNone twice, which
> should
> be harmless.  Or, do you know for a fact that the static initializer guard
> is
> toggled before running the Location constructor?
>

I have no idea :)  If you know what you're suggesting here, then I'm happy
with the function local static.


>
> -Darin
>
>
>>
>>
>> http://codereview.chromium.org/4081003/show
>>
>
>

[DaveMoore, 2010-10-26 17:10:43]

There was an early LGTM followed by some discussion. 

I've kept the task as a const Task* in the observer methods and made the default
Location in GetBirthPlace() when not compiled with TRACK_ALL_TASK_OBJECTS as a
local static.

[darin (slow to review), 2010-10-26 21:25:31]

LGTM