chromeos: Move EnableScreenLock to PowerManagerClient from PowerLibrary

chromeos: Move EnableScreenLock to PowerManagerClient from PowerLibrary

Part of libcros removal.  Directly writing to the prefs file, instead of going
thru libcros.

BUG=chromium-os:16558
TEST=Go to VT2 and make sure /var/lib/power_manager/lock_on_idle_suspend does not exist (delete if it does).  Type 'stop powerd'.  Sign in as a user.  Press power button to lock screen.  Go to VT2 and look at /var/lib/power_manager/lock_on_idle_suspend.  The file should have been created.

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

[satorux1, 2011-11-16 00:52:49]

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

http://codereview.chromium.org/8573025/diff/1/chrome/browser/chromeos/dbus/po...
chrome/browser/chromeos/dbus/power_manager_client.cc:182: config.size());
Let's do this somewhere else. PowerManagerClient is a D-Bus client, and not
supposed to do other things.

Per git grep 'EnableScreenLock(', there seems to be only one caller:

chrome/browser/chromeos/preferences.cc:
    CrosLibrary::Get()->GetPowerLibrary()->EnableScreenLock(

I'd suggest you to do this in the file.

[satorux1, 2011-11-16 00:55:52]

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

http://codereview.chromium.org/8573025/diff/1/chrome/browser/chromeos/dbus/po...
chrome/browser/chromeos/dbus/power_manager_client.cc:182: config.size());
Besides, please be sure to do this in FILE thread. You cannot do file operations
on UI thread.

[satorux1, 2011-11-16 00:59:48]

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

http://codereview.chromium.org/8573025/diff/1/chrome/browser/chromeos/dbus/po...
chrome/browser/chromeos/dbus/power_manager_client.cc:182: config.size());
On 2011/11/16 00:52:49, satorux1 wrote:
> Let's do this somewhere else. PowerManagerClient is a D-Bus client, and not
> supposed to do other things.
> 
> Per git grep 'EnableScreenLock(', there seems to be only one caller:
> 
> chrome/browser/chromeos/preferences.cc:
>     CrosLibrary::Get()->GetPowerLibrary()->EnableScreenLock(
> 
> I'd suggest you to do this in the file.

I changed my mind. Putting the code in chrome/browser/chromeos/system would be
cleaner. See touchpad_settings.cc and timezone_settings.cc there.

[Simon Que, 2011-11-16 01:40:02]

PTAL

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

http://codereview.chromium.org/8573025/diff/1/chrome/browser/chromeos/dbus/po...
chrome/browser/chromeos/dbus/power_manager_client.cc:182: config.size());
On 2011/11/16 00:55:52, satorux1 wrote:
> Besides, please be sure to do this in FILE thread. You cannot do file
operations
> on UI thread.

Done.

http://codereview.chromium.org/8573025/diff/1/chrome/browser/chromeos/dbus/po...
chrome/browser/chromeos/dbus/power_manager_client.cc:182: config.size());
On 2011/11/16 00:59:48, satorux1 wrote:
> On 2011/11/16 00:52:49, satorux1 wrote:
> > Let's do this somewhere else. PowerManagerClient is a D-Bus client, and not
> > supposed to do other things.
> > 
> > Per git grep 'EnableScreenLock(', there seems to be only one caller:
> > 
> > chrome/browser/chromeos/preferences.cc:
> >     CrosLibrary::Get()->GetPowerLibrary()->EnableScreenLock(
> > 
> > I'd suggest you to do this in the file.
> 
> I changed my mind. Putting the code in chrome/browser/chromeos/system would be
> cleaner. See touchpad_settings.cc and timezone_settings.cc there.

Done.

[satorux1, 2011-11-16 02:50:20]

You forgot to upload the new files...

[satorux1, 2011-11-16 23:58:04]

http://codereview.chromium.org/8573025/diff/1017/chrome/browser/chromeos/pref...
File chrome/browser/chromeos/preferences.cc (right):

http://codereview.chromium.org/8573025/diff/1017/chrome/browser/chromeos/pref...
chrome/browser/chromeos/preferences.cc:15: #include
"chrome/browser/chromeos/dbus/power_manager_client.h"
You don't need this.

http://codereview.chromium.org/8573025/diff/1017/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/screen_locker_settings.cc (right):

http://codereview.chromium.org/8573025/diff/1017/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/screen_locker_settings.cc:32:
base::Bind(&EnableScreenLock, enable));
Please don't be call itself. Please create a separate function in anonymous
namespace like EnableScreenLockOnFileThread() and call it.

http://codereview.chromium.org/8573025/diff/1017/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/screen_locker_settings.cc:37:
file_util::WriteFile(FilePath(kLockOnIdleSuspendPath),
We shouldn't write this file on linux desktop. Please use
system::runtime_eivnronment::IsRunningOnChromeOS() and write code for Linux
desktop, that does nothing.

[satorux1, 2011-11-16 23:59:15]

http://codereview.chromium.org/8573025/diff/1017/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/screen_locker_settings.cc (right):

http://codereview.chromium.org/8573025/diff/1017/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/screen_locker_settings.cc:32:
base::Bind(&EnableScreenLock, enable));
On 2011/11/16 23:58:04, satorux1 wrote:
> Please don't be call itself. Please create a separate function in anonymous
> namespace like EnableScreenLockOnFileThread() and call it.


Please also be sure to put

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI))

at the beginning of this this function, and 

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)) in
EnableScreenLockOnFileThread()

[Simon Que, 2011-11-18 18:56:08]

PTAL

http://codereview.chromium.org/8573025/diff/1017/chrome/browser/chromeos/pref...
File chrome/browser/chromeos/preferences.cc (right):

http://codereview.chromium.org/8573025/diff/1017/chrome/browser/chromeos/pref...
chrome/browser/chromeos/preferences.cc:15: #include
"chrome/browser/chromeos/dbus/power_manager_client.h"
On 2011/11/16 23:58:04, satorux1 wrote:
> You don't need this.

Done.

http://codereview.chromium.org/8573025/diff/1017/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/screen_locker_settings.cc (right):

http://codereview.chromium.org/8573025/diff/1017/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/screen_locker_settings.cc:32:
base::Bind(&EnableScreenLock, enable));
On 2011/11/16 23:59:15, satorux1 wrote:
> On 2011/11/16 23:58:04, satorux1 wrote:
> > Please don't be call itself. Please create a separate function in anonymous
> > namespace like EnableScreenLockOnFileThread() and call it.
> 
> 
> Please also be sure to put
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI))
> 
> at the beginning of this this function, and 
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)) in
> EnableScreenLockOnFileThread()

Done.

http://codereview.chromium.org/8573025/diff/1017/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/screen_locker_settings.cc:32:
base::Bind(&EnableScreenLock, enable));
On 2011/11/16 23:58:04, satorux1 wrote:
> Please don't be call itself. Please create a separate function in anonymous
> namespace like EnableScreenLockOnFileThread() and call it.

Done.

http://codereview.chromium.org/8573025/diff/1017/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/screen_locker_settings.cc:37:
file_util::WriteFile(FilePath(kLockOnIdleSuspendPath),
On 2011/11/16 23:58:04, satorux1 wrote:
> We shouldn't write this file on linux desktop. Please use
> system::runtime_eivnronment::IsRunningOnChromeOS() and write code for Linux
> desktop, that does nothing.

Done.

[satorux1, 2011-11-18 19:11:48]

http://codereview.chromium.org/8573025/diff/8003/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/screen_locker_settings.cc (right):

http://codereview.chromium.org/8573025/diff/8003/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/screen_locker_settings.cc:42: if
(!BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
Please remove this line. We know that we are on UI thread.

[Simon Que, 2011-11-18 19:33:16]

PTAL

http://codereview.chromium.org/8573025/diff/8003/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/screen_locker_settings.cc (right):

http://codereview.chromium.org/8573025/diff/8003/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/screen_locker_settings.cc:42: if
(!BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
On 2011/11/18 19:11:48, satorux1 wrote:
> Please remove this line. We know that we are on UI thread.

Done.

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

LGTM

[satorux1, 2011-11-22 21:08:40]

Still LGTM

[commit-bot: I haz the power, 2011-11-22 21:11:42]

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

[commit-bot: I haz the power, 2011-11-22 23:26:52]

Change committed as 111248