Make the SystemMonitor observer list thread safe....

Make the SystemMonitor observer list thread safe.
This is done by using the new observer_list_threadsafe.


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

[Mike Belshe, 2008-09-26 15:52:58]

Don't review this yet.  Reworking it.

[Mike Belshe, 2008-09-26 17:02:13]

The SystemMonitor will be thread safe and maintain some restrictions on how
notifications occur.

* Add/Remove Observer can be called from any thread.
* Callbacks can be run on any thread.
* Callers calling Add/Remove observer while notifications are in progress will
block until the notification is complete.

Because the Observers are not reference counted, we can't really copy the
observer list and use it outside of a lock; if we did, the observer could remove
itself from the list and delete itself before the notification had finished
using the observer callback.  For this particular API (SystemMonitor), we don't
need to support this anyway.

[Mike Belshe, 2008-09-26 19:53:21]

Based on feedback from Darin; added a check in case the caller does attempt to
Add/RemoveObserver while notifications are in progress.

[darin (slow to review), 2008-09-30 17:31:01]

LGTM

http://codereview.chromium.org/4288/diff/17/412
File base/system_monitor.h (right):

http://codereview.chromium.org/4288/diff/17/412#newcode35
Line 35: PowerStateEvent,  // The Power status of the system has changed.
nit: I think google style says these should be POWER_STATE_EVENT, etc.

[Dean McNamee, 2008-09-30 20:02:19]

Can you move all of the code in the header into a .cc?  It will help code bloat
from overinlining, clean up the interface, and speed up build times and stuff...

http://codereview.chromium.org/4288/diff/603/804
File base/system_monitor.h (right):

http://codereview.chromium.org/4288/diff/603/804#newcode31
Line 31: bool BatteryPower() { return battery_in_use_; }
Does anything protect this?

http://codereview.chromium.org/4288/diff/603/804#newcode67
Line 67: observer_list_.AddObserver(obs);
Does this stuff really need to be implemented inline in the header?  It is not
important for performance, makes the interface a bit less clear, and also
requires including logging.h, etc.

http://codereview.chromium.org/4288/diff/603/801
File base/system_monitor_unittest.cc (right):

http://codereview.chromium.org/4288/diff/603/801#newcode38
Line 38: for (int index=0; index < kObservers; index++)
index = 0, ++index

[Mike Belshe, 2008-09-30 20:14:35]

On 2008/09/30 20:02:19, Dean McNamee wrote:
> Can you move all of the code in the header into a .cc?  It will help code
bloat
> from overinlining, clean up the interface, and speed up build times and
stuff...

OK.

> 
> http://codereview.chromium.org/4288/diff/603/804
> File base/system_monitor.h (right):
> 
> http://codereview.chromium.org/4288/diff/603/804#newcode31
> Line 31: bool BatteryPower() { return battery_in_use_; }
> Does anything protect this?

I don't think it needs to be protected.


> 
> http://codereview.chromium.org/4288/diff/603/804#newcode67
> Line 67: observer_list_.AddObserver(obs);
> Does this stuff really need to be implemented inline in the header?  It is not
> important for performance, makes the interface a bit less clear, and also
> requires including logging.h, etc.

I'll move it.  For the record, though, I think its more clear in the header. 
Mostly I disagree with C++'s desire to separate headers from implementations. 
This is a level of convolution that good compilers shouldn't need.

> 
> http://codereview.chromium.org/4288/diff/603/801
> File base/system_monitor_unittest.cc (right):
> 
> http://codereview.chromium.org/4288/diff/603/801#newcode38
> Line 38: for (int index=0; index < kObservers; index++)
> index = 0, ++index

OK.

[Mike Belshe, 2008-10-31 20:54:24]

After the first review, I went back and redid ObserverListThreadSafe.  Here is
an updated version which picks that up and also delays the initial battery
check.

There is a second CL which integrates further into Chrome (including running the
initial battery check).

[darin (slow to review), 2008-10-31 22:36:51]

LGTM

http://codereview.chromium.org/4288/diff/1808/1618
File base/observer_list_threadsafe.h (right):

http://codereview.chromium.org/4288/diff/1808/1618#newcode90
Line 90: DCHECK(list)  << "RemoveObserver called on for unknown thread";
nit: could also be a NOTREACHED inside the "if (!list)" to avoid repeating the
conditional.

http://codereview.chromium.org/4288/diff/1808/1615
File base/system_monitor.cc (right):

http://codereview.chromium.org/4288/diff/1808/1615#newcode18
Line 18: #pragma warning(suppress: 4355)  // Okay to pass "this" here.
nit: please use the ALLOW_THIS_IN_INITIALIZER_LIST macro instead.  it helps
avoid issues with other compilers that complain about unknown pragmas.

http://codereview.chromium.org/4288/diff/1808/1615#newcode76
Line 76: MessageLoop::current()->PostDelayedTask(FROM_HERE,
optional:  you might find that OneShotTimer<T> is a bit nicer here.  instead of
a ScopedRunnableMethodFactory<T> as a member variable, you'd have a
OneShotTimer<T> as a member variable.  the end result is pretty similar so
either way is fine.  just thought i'd mention OneShotTimer<T> in case you hadn't
considered it.

http://codereview.chromium.org/4288/diff/1808/1619
File base/time_win.cc (right):

http://codereview.chromium.org/4288/diff/1808/1619#newcode237
Line 237: if (monitor)
Q: why do you need to worry about this being null?

[Dean McNamee, 2008-11-01 09:53:02]

I don't see where Start() is called, is it?  I'm guessing it's going to be
called somewhere else so that we guarantee where on a thread with a message loop
etc?  Where will it be called?

http://codereview.chromium.org/4288/diff/1808/1615
File base/system_monitor.cc (right):

http://codereview.chromium.org/4288/diff/1808/1615#newcode13
Line 13: static int kDelayedBatteryCheck = 10*1000;
spaces around *, put the units in the name like CheckMs.

[Mike Belshe, 2008-11-02 00:22:45]

On 2008/10/31 22:36:51, darin wrote:
> LGTM
> 
> http://codereview.chromium.org/4288/diff/1808/1618
> File base/observer_list_threadsafe.h (right):
> 
> http://codereview.chromium.org/4288/diff/1808/1618#newcode90
> Line 90: DCHECK(list)  << "RemoveObserver called on for unknown thread";
> nit: could also be a NOTREACHED inside the "if (!list)" to avoid repeating the
> conditional.

ok

> 
> http://codereview.chromium.org/4288/diff/1808/1615
> File base/system_monitor.cc (right):
> 
> http://codereview.chromium.org/4288/diff/1808/1615#newcode18
> Line 18: #pragma warning(suppress: 4355)  // Okay to pass "this" here.
> nit: please use the ALLOW_THIS_IN_INITIALIZER_LIST macro instead.  it helps
> avoid issues with other compilers that complain about unknown pragmas.

cool.  didn't know that.  ok

> 
> http://codereview.chromium.org/4288/diff/1808/1615#newcode76
> Line 76: MessageLoop::current()->PostDelayedTask(FROM_HERE,
> optional:  you might find that OneShotTimer<T> is a bit nicer here.  instead
of
> a ScopedRunnableMethodFactory<T> as a member variable, you'd have a
> OneShotTimer<T> as a member variable.  the end result is pretty similar so
> either way is fine.  just thought i'd mention OneShotTimer<T> in case you
hadn't
> considered it.

I like this, and switched to it.  The only drawback is that the oneshottimer
doesn't provide for taking function arguments nicely like PostTask does.

> 
> http://codereview.chromium.org/4288/diff/1808/1619
> File base/time_win.cc (right):
> 
> http://codereview.chromium.org/4288/diff/1808/1619#newcode237
> Line 237: if (monitor)
> Q: why do you need to worry about this being null?

I like to do the extra check defensively.  Also, I think I sometimes get nitted
if I don't!  I think it should be safe.

[Mike Belshe, 2008-11-02 00:23:30]

On 2008/11/01 09:53:02, Dean McNamee wrote:
> I don't see where Start() is called, is it?  I'm guessing it's going to be
> called somewhere else so that we guarantee where on a thread with a message
loop
> etc?  Where will it be called?

Sorry - as mentioned there is another CL coming.  Integration into the browser
isn't fully done.  Expect it on monday.

> 
> http://codereview.chromium.org/4288/diff/1808/1615
> File base/system_monitor.cc (right):
> 
> http://codereview.chromium.org/4288/diff/1808/1615#newcode13
> Line 13: static int kDelayedBatteryCheck = 10*1000;
> spaces around *, put the units in the name like CheckMs.

ok