chromeos: Move system resume handling to power manager client

chromeos: Move system resume handling to power manager client

BUG=chromium-os:21115
TEST=Note the time.  Suspend for 5 minutes.  Resume.  The time should have
advanced 5 minutes.

Signed-off-by: Simon Que <sque@chromium.org>;

R=satorux@chromium.org,stevenjb@chromium.org


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

[satorux1, 2011-11-23 01:03:26]

looks good overall, but one request.

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/dbus/po...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/dbus/po...
chrome/browser/chromeos/dbus/power_manager_client.cc:227: std::string arg;
arg -> something more descriptive, like power_state_string.

[satorux1, 2011-11-23 08:24:48]

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/accessi...
File chrome/browser/chromeos/accessibility/system_event_observer.cc (right):

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/accessi...
chrome/browser/chromeos/accessibility/system_event_observer.cc:25:
CrosLibrary::Get()->GetScreenLockLibrary()->AddObserver(this);
Looking at chrome_browser_main_chromeos.cc, this seems to be called _before_
DBusThreadManager is created. Hence, we should introduce Init() to do this and
call it after DBusThreadManager is created, just like we did for
NetworkChangeNotifierChromeos. See chrome_browser_main_chromeos.cc, for details.

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/accessi...
chrome/browser/chromeos/accessibility/system_event_observer.cc:30:
DBusThreadManager::Get()->GetPowerManagerClient()->RemoveObserver(this);
Introduce Shutdown() to do this.

[satorux1, 2011-11-23 08:30:09]

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/net/net...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/net/net...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:84:
DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(this);
So we weren't calling this before this change. I wasn't able to catch this
bug...

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/net/net...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:103:
->RemoveObserver(this);
Remove the lines from 102-103. Doing the same thing...

[satorux1, 2011-11-23 18:25:57]

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/accessi...
File chrome/browser/chromeos/accessibility/system_event_observer.cc (right):

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/accessi...
chrome/browser/chromeos/accessibility/system_event_observer.cc:25:
CrosLibrary::Get()->GetScreenLockLibrary()->AddObserver(this);
On 2011/11/23 08:24:48, satorux1 wrote:
> Looking at chrome_browser_main_chromeos.cc, this seems to be called _before_
> DBusThreadManager is created. Hence, we should introduce Init() to do this and
> call it after DBusThreadManager is created, just like we did for
> NetworkChangeNotifierChromeos. See chrome_browser_main_chromeos.cc, for
details.

I take it back. Forget about adding Init(). I think you can just move the call
to chromeos::accessibility::SystemEventObserver::Initialize() to a place after
the DBusThreadManager is created.

You should also move chromeos::accessibility::SystemEventObserver::Shutdown() to
a place before DBusThreadManager is shut down.

[Simon Que, 2011-11-23 18:35:23]

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/accessi...
File chrome/browser/chromeos/accessibility/system_event_observer.cc (right):

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/accessi...
chrome/browser/chromeos/accessibility/system_event_observer.cc:25:
CrosLibrary::Get()->GetScreenLockLibrary()->AddObserver(this);
Isn't Initialize() a static function?

[Simon Que, 2011-11-23 18:36:06]

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/accessi...
File chrome/browser/chromeos/accessibility/system_event_observer.cc (right):

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/accessi...
chrome/browser/chromeos/accessibility/system_event_observer.cc:25:
CrosLibrary::Get()->GetScreenLockLibrary()->AddObserver(this);
On 2011/11/23 18:35:23, Simon Que wrote:
> Isn't Initialize() a static function?


Oh I see that it has a single instance.

[Simon Que, 2011-11-23 19:24:17]

PTAL

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/accessi...
File chrome/browser/chromeos/accessibility/system_event_observer.cc (right):

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/accessi...
chrome/browser/chromeos/accessibility/system_event_observer.cc:25:
CrosLibrary::Get()->GetScreenLockLibrary()->AddObserver(this);
On 2011/11/23 18:36:07, Simon Que wrote:
> On 2011/11/23 18:35:23, Simon Que wrote:
> > Isn't Initialize() a static function?
> 
> 
> Oh I see that it has a single instance.

Done.

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/dbus/po...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/dbus/po...
chrome/browser/chromeos/dbus/power_manager_client.cc:227: std::string arg;
On 2011/11/23 01:03:27, satorux1 wrote:
> arg -> something more descriptive, like power_state_string.

Done.

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/net/net...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

http://codereview.chromium.org/8666007/diff/1/chrome/browser/chromeos/net/net...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:103:
->RemoveObserver(this);
On 2011/11/23 08:30:09, satorux1 wrote:
> Remove the lines from 102-103. Doing the same thing...

Done.

[satorux1, 2011-11-23 19:38:09]

http://codereview.chromium.org/8666007/diff/7001/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8666007/diff/7001/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:95:
chromeos::accessibility::SystemEventObserver::Shutdown();
initialization and shutdown should be done in the reversed order:

initialize A, B, C
shutdown C, B, A.

Please reorder properly.

[Simon Que, 2011-11-23 19:42:29]

PTAL

http://codereview.chromium.org/8666007/diff/7001/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8666007/diff/7001/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:95:
chromeos::accessibility::SystemEventObserver::Shutdown();
On 2011/11/23 19:38:09, satorux1 wrote:
> initialization and shutdown should be done in the reversed order:
> 
> initialize A, B, C
> shutdown C, B, A.
> 
> Please reorder properly.

Done.

[satorux1, 2011-11-23 20:18:01]

http://codereview.chromium.org/8666007/diff/7001/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8666007/diff/7001/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:95:
chromeos::accessibility::SystemEventObserver::Shutdown();
On 2011/11/23 19:42:29, Simon Que wrote:
> On 2011/11/23 19:38:09, satorux1 wrote:
> > initialization and shutdown should be done in the reversed order:
> > 
> > initialize A, B, C
> > shutdown C, B, A.
> > 
> > Please reorder properly.
> 
> Done.

Noooo, this doesn't look like the right order.

The initialization looks like:

chromeos::DBusThreadManager::Initialize();
...
chromeos::disks::DiskMountManager::Initialize();
chromeos::accessibility::SystemEventObserver::Initialize();

Then, the shutdown should look like

chromeos::accessibility::SystemEventObserver::Shutdown();
chromeos::disks::DiskMountManager::Shutdown();
chromeos::DBusThreadManager::Shutdown();

[satorux1, 2011-11-23 20:57:29]

LGTM. Please be sure to run try jobs.

[commit-bot: I haz the power, 2011-11-23 23:27:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sque@chromium.org/8666007/7013

[commit-bot: I haz the power, 2011-11-23 23:27:20]

Presubmit check for 8666007-7013 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/chromeos/status/status_area_view_chromeos.cc,chrome/browser/chromeos/status/status_area_view_chromeos.h

Presubmit checks took 1.6s to calculate.

[stevenjb, 2011-11-23 23:31:48]

lgtm

[commit-bot: I haz the power, 2011-11-23 23:33:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sque@chromium.org/8666007/7013

[commit-bot: I haz the power, 2011-11-24 01:02:17]

Change committed as 111460