chromeos: move screen lock handling to power manager client

chromeos: move screen lock handling to power manager client

Part of libcros removal -- removing chromeos_screen_lock

BUG=chromium-os:16559
TEST=Make sure screen lock/unlock works

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=112369

[satorux1, 2011-11-29 01:29:12]

Looks good overall. Please be sure to test that screen locks work as before.

http://codereview.chromium.org/8664014/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/8664014/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider_chromeos.cc:19: #include
"chrome/browser/chromeos/dbus/dbus_thread_manager.h"
Shouldn't we include power_manager_client.h?

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

http://codereview.chromium.org/8664014/diff/1/chrome/browser/chromeos/dbus/po...
chrome/browser/chromeos/dbus/power_manager_client.cc:18: #include
"content/public/browser/browser_thread.h"
don't need this.

http://codereview.chromium.org/8664014/diff/1/chrome/browser/chromeos/dbus/po...
chrome/browser/chromeos/dbus/power_manager_client.cc:342:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
Don't need this. We don't do this in other functions. Let's make it consistent.

http://codereview.chromium.org/8664014/diff/1/chrome/browser/chromeos/dbus/po...
chrome/browser/chromeos/dbus/power_manager_client.cc:353: }
What about defining a function for each signal, rather than processing all
signals in this function. That may be a bit lengthier but would be cleaner.

[Simon Que, 2011-11-29 01:47:31]

PTAL

http://codereview.chromium.org/8664014/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/8664014/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider_chromeos.cc:19: #include
"chrome/browser/chromeos/dbus/dbus_thread_manager.h"
On 2011/11/29 01:29:12, satorux1 wrote:
> Shouldn't we include power_manager_client.h?

Done.

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

http://codereview.chromium.org/8664014/diff/1/chrome/browser/chromeos/dbus/po...
chrome/browser/chromeos/dbus/power_manager_client.cc:18: #include
"content/public/browser/browser_thread.h"
On 2011/11/29 01:29:12, satorux1 wrote:
> don't need this.

Done.

http://codereview.chromium.org/8664014/diff/1/chrome/browser/chromeos/dbus/po...
chrome/browser/chromeos/dbus/power_manager_client.cc:342:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2011/11/29 01:29:12, satorux1 wrote:
> Don't need this. We don't do this in other functions. Let's make it
consistent.

Done.

http://codereview.chromium.org/8664014/diff/1/chrome/browser/chromeos/dbus/po...
chrome/browser/chromeos/dbus/power_manager_client.cc:353: }
On 2011/11/29 01:29:12, satorux1 wrote:
> What about defining a function for each signal, rather than processing all
> signals in this function. That may be a bit lengthier but would be cleaner.

Done.

[hashimoto, 2011-11-29 02:58:28]

http://codereview.chromium.org/8664014/diff/3001/chrome/browser/chromeos/acce...
File chrome/browser/chromeos/accessibility/system_event_observer.cc (right):

http://codereview.chromium.org/8664014/diff/3001/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/system_event_observer.cc:23:
DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(this);
AddObserver is also called in Initialize.
Please remove one of them.

http://codereview.chromium.org/8664014/diff/3001/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/system_event_observer.cc:27:
DBusThreadManager::Get()->GetPowerManagerClient()->RemoveObserver(this);
ditto.

[Simon Que, 2011-11-29 23:31:24]

PTAL

http://codereview.chromium.org/8664014/diff/3001/chrome/browser/chromeos/acce...
File chrome/browser/chromeos/accessibility/system_event_observer.cc (right):

http://codereview.chromium.org/8664014/diff/3001/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/system_event_observer.cc:23:
DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(this);
On 2011/11/29 02:58:31, hashimoto wrote:
> AddObserver is also called in Initialize.
> Please remove one of them.

Done.

http://codereview.chromium.org/8664014/diff/3001/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/system_event_observer.cc:27:
DBusThreadManager::Get()->GetPowerManagerClient()->RemoveObserver(this);
On 2011/11/29 02:58:31, hashimoto wrote:
> ditto.

Done.

[stevenjb, 2011-11-29 23:44:09]

lgtm

[hashimoto, 2011-11-30 00:54:52]

LGTM about system_event_observer.cc, thanks!

[commit-bot: I haz the power, 2011-11-30 23:47:50]

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

[commit-bot: I haz the power, 2011-12-01 01:33:17]

Change committed as 112369

[oshima, 2011-12-01 18:12:05]

On 2011/12/01 01:33:17, I haz the power (commit-bot) wrote:
> Change committed as 112369

Hi, this broke linux_chromeos bot. Could you please fix it ASAP?
I'll also look into it, but if i couldn't fix it soon, i'll revert this.