Make SystemMonitor not a Singleton and move it out of base

Make SystemMonitor not a Singleton and move it out of base

SystemMonitor makes an assumption that through its lifetime a MessageLoop exists and stays the same. It is difficult and error-prone to satisfy that when it is a Singleton. It has caused problems in the past.

Additionally, extract HighResoltionTimerManager out of time_win.cc, eliminating yet another Singleton.

TEST=none
BUG=none

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

[Paweł Hajdan Jr., 2009-11-23 14:36:24]

This is not yet tested on other systems, etc. Just to show my idea how to fix
things.

I think that SystemMonitor cannot live in base, because it assumes that the
MessageLoop::current() is valid and stays the same through its lifetime. The
only place when we can guarantee that is chrome. The code is used in the Main
procedures of each type of process, therefore chrome/common.

One thing about HighResolutionTimerManager: I'm not sure whether it should have
stubs for non-Windows platforms, or have #ifdefs used in each place. I generally
don't like #ifdefs, but I don't have a strong opinion in this case.

[Mike Belshe, 2009-11-23 16:43:45]

Overall, I would have left the SystemMonitor in base.  I think it is potentially
useful elsewhere.  But I like the decoupling so that it is manually invoked
rather than the singleton dangling, and I really like the removal from Time().  

What would you think of leaving it in base?

http://codereview.chromium.org/431008/diff/1/11
File base/system_monitor.h (left):

http://codereview.chromium.org/431008/diff/1/11#oldcode1
base/system_monitor.h:1: // Copyright (c) 2009 The Chromium Authors. All rights
reserved.
We can move this file; but I did kind of like it in base.  If any application
*uses* it, then it may need the messageloop; and that seems like a fine
restriction.  If it is unused, it doesn't have requirements, right?

It's hard to tell from this CL if you are only moving it or also changing the
code.

http://codereview.chromium.org/431008/diff/1/12
File base/time.h (right):

http://codereview.chromium.org/431008/diff/1/12#newcode247
base/time.h:247: static bool UseHighResolutionTimer(bool use);
Note:  If someone uses this to switch back and forth from hi-res to lo-res, they
might have unexpected results.

http://codereview.chromium.org/431008/diff/1/5
File base/time_win.cc (left):

http://codereview.chromium.org/431008/diff/1/5#oldcode91
base/time_win.cc:91: class HighResolutionTimerManager : public
base::SystemMonitor::PowerObserver {
I like the fact that you are pulling this out of the Time code; this seems like
good refactoring.

But I can't find where this code has moved to in this CL?

[Paweł Hajdan Jr., 2009-11-23 17:40:49]

On 2009/11/23 16:43:45, Mike Belshe wrote:
> Overall, I would have left the SystemMonitor in base.  I think it is
potentially
> useful elsewhere.  But I like the decoupling so that it is manually invoked
> rather than the singleton dangling, and I really like the removal from Time().
 
> 
> What would you think of leaving it in base?

I'd rather prefer it in chrome, but it's not a strong opinion. If you think it
will be needed in base, I see no problem in leaving it there.

> http://codereview.chromium.org/431008/diff/1/11
> File base/system_monitor.h (left):
> 
> http://codereview.chromium.org/431008/diff/1/11#oldcode1
> base/system_monitor.h:1: // Copyright (c) 2009 The Chromium Authors. All
rights
> reserved.
> We can move this file; but I did kind of like it in base.  If any application
> *uses* it, then it may need the messageloop; and that seems like a fine
> restriction.  If it is unused, it doesn't have requirements, right?
> 
> It's hard to tell from this CL if you are only moving it or also changing the
> code.

I can do it in two steps if you prefer, or just leave SM in base.

> http://codereview.chromium.org/431008/diff/1/12
> File base/time.h (right):
> 
> http://codereview.chromium.org/431008/diff/1/12#newcode247
> base/time.h:247: static bool UseHighResolutionTimer(bool use);
> Note:  If someone uses this to switch back and forth from hi-res to lo-res,
they
> might have unexpected results.

After a discussion on gtalk I added a little TODO about that. This patch doesn't
change anything about this.

> http://codereview.chromium.org/431008/diff/1/5
> File base/time_win.cc (left):
> 
> http://codereview.chromium.org/431008/diff/1/5#oldcode91
> base/time_win.cc:91: class HighResolutionTimerManager : public
> base::SystemMonitor::PowerObserver {
> I like the fact that you are pulling this out of the Time code; this seems
like
> good refactoring.
> 
> But I can't find where this code has moved to in this CL?

Oops. svn-added the files and updated the patch.

[Evan Martin, 2009-11-23 17:56:47]

Re base/ vs chrome/: one metric for this is that base is starting to get shared
by more stuff.  o3d now depends on base, and I think the ChromeOS window manager
does.  So the question to ask is: "will this code be shared by chrome,
test_shell, o3d, and the WM?"

I don't know the answer to that question for this.  :)

[Peter Kasting, 2009-11-23 18:22:16]

I don't think I'm qualified to review this.

[darin (slow to review), 2009-11-24 05:26:54]

To avoid clutter in base/, we have generally used the rule that things
should only live in base/ if they are used by more than one consumer of
base/.  For example, if net/ or webkit/ share a dependency with chrome/,
then base/ might be the right place for that something.

We now also have app/ for things like this that are really more about
building a desktop app.  If net/ doesn't need it, but both chrome/ and
webkit/ need it, then perhaps app/ is the right place in other words.

In this case, the move to chrome/common seems to conform to that general
rule.  Although app/ seems fairly reasonable if we expect any other "apps"
to need this.

-Darin


On Mon, Nov 23, 2009 at 9:56 AM, <evan@chromium.org> wrote:

> Re base/ vs chrome/: one metric for this is that base is starting to get
> shared
> by more stuff.  o3d now depends on base, and I think the ChromeOS window
> manager
> does.  So the question to ask is: "will this code be shared by chrome,
> test_shell, o3d, and the WM?"
>
> I don't know the answer to that question for this.  :)
>
>
> http://codereview.chromium.org/431008
>

[Paweł Hajdan Jr., 2009-11-24 18:30:35]

Should be ready for the final review.

[Paweł Hajdan Jr., 2009-11-26 09:44:21]

ping!

[Mike Belshe, 2009-11-26 17:03:12]

lgtm.

Maybe we should add a unit test.

Also - what happens if someone manually instantiates a system monitor observer
before starting the system monitor?  I hope it fails nicely.  I suspect it would
be easier to make this mistake now? 

 It would also be easier to just forget to instantiate these.  I wonder if the
time could assert that a HiResTimerManager is running somehow?  This would at
least catch the case where we accidentally forgot to run one.  The singleton
code didn't have this problem because encapsulation was better.

http://codereview.chromium.org/431008/diff/1068/88
File chrome/browser/session_history_uitest.cc (right):

http://codereview.chromium.org/431008/diff/1068/88#newcode495
chrome/browser/session_history_uitest.cc:495:
ASSERT_TRUE(tab_->NavigateToURLBlockUntilNavigationsComplete(
Is this an accidental file inclusion to this CL?  It doesn't seem related to
SystemMonitor.

[Paweł Hajdan Jr., 2009-11-26 20:34:17]

On 2009/11/26 17:03:12, Mike Belshe wrote:
> Also - what happens if someone manually instantiates a system monitor observer
> before starting the system monitor?

NULL pointer dereference.
> I hope it fails nicely.  I suspect it would
> be easier to make this mistake now? 

Actually it's neither harder nor easier. It just fails in a more predictable
way. Previously you could attempt to use it without a MessageLoop, or destroy
the loop while SM was still running. That was failing in a very mysterious way.
NULL pointer dereference on SystemMonitor::Get is quite obvious.

>  It would also be easier to just forget to instantiate these.  I wonder if the
> time could assert that a HiResTimerManager is running somehow?

I don't see a good way to test that other than some forms of global data.

> This would at least catch the case where we accidentally forgot to run one. 
The singleton
> code didn't have this problem because encapsulation was better.

The same is true for most of the initialization code, although it's probably
harder to spot that we run the lower-resolution timer. Anyway, IMHO being more
careful when reviewing changes to process startup code (which is copy-pasted
anyway, and copy-pasting on current code will yield correct results) is better
than really nasty debugging problems.

> http://codereview.chromium.org/431008/diff/1068/88
> File chrome/browser/session_history_uitest.cc (right):
> 
> http://codereview.chromium.org/431008/diff/1068/88#newcode495
> chrome/browser/session_history_uitest.cc:495:
> ASSERT_TRUE(tab_->NavigateToURLBlockUntilNavigationsComplete(
> Is this an accidental file inclusion to this CL?  It doesn't seem related to
> SystemMonitor.

Good catch. Removed and committed.