cros: Prevent shut down on lid-close during migration.

cros: Prevent shut down on lid-close during migration.

In the login screen, the system will shut down on lid close.
Although we ask users not to close the lid during the migration, it should be
better to change the lid-close action to SUSPEND during migration.

BUG=726229
TEST=manually tested that closing lid during migration does not power off the system.

[fukino, 2017-05-26 07:04:57]

fukino@chromium.org changed reviewers:
+ derat@chromium.org, xiyuan@chromium.org

[fukino, 2017-05-26 07:04:58]

derat@, could you review how the power policy is changed?

We'd like to temporarily change the lid-close action to SUSPEND (it seemed
SHUT_DOWN in login screen) during the ext4 migration.

xiyuan@, could you check the patch as an owner?

Thanks!

[Daniel Erat, 2017-05-26 13:45:34]

not lgtm

https://codereview.chromium.org/2907773002/diff/1/chrome/browser/ui/webui/chr...
File
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
(right):

https://codereview.chromium.org/2907773002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:363:
DBusThreadManager::Get()->GetPowerManagerClient()->SetPolicy(policy);
you can't send a policy directly like this; it'll be overwritten as soon as we
decide to send another policy.

i tried to explain earlier that you should add a new method to
PowerPolicyController so that it will continue to be the only thing sending
policies. i think that something like this will work:

1. add PowerPolicyController::SetEncryptionMigrationActive(bool active)
2. make it update a encryption_migration_active_ bool member and call
SendCurrentPolicy()
3. after the chrome_is_exiting_ code near the end of that method, add something
like this:

if (encryption_migration_active_ &&
    policy.lid_closed_action() != ..._Action_DO_NOTHING) {
  policy.set_lid_closed_action(..._Action_SUSPEND);
  causes += (causes.empty() ? "" : ", ") + "encryption migration";
}

[fukino, 2017-05-29 04:01:49]

On 2017/05/26 13:45:34, Daniel Erat wrote:
> not lgtm
> 
>
https://codereview.chromium.org/2907773002/diff/1/chrome/browser/ui/webui/chr...
> File
> chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
> (right):
> 
>
https://codereview.chromium.org/2907773002/diff/1/chrome/browser/ui/webui/chr...
>
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:363:
> DBusThreadManager::Get()->GetPowerManagerClient()->SetPolicy(policy);
> you can't send a policy directly like this; it'll be overwritten as soon as we
> decide to send another policy.
> 
> i tried to explain earlier that you should add a new method to
> PowerPolicyController so that it will continue to be the only thing sending
> policies. i think that something like this will work:
> 
> 1. add PowerPolicyController::SetEncryptionMigrationActive(bool active)
> 2. make it update a encryption_migration_active_ bool member and call
> SendCurrentPolicy()
> 3. after the chrome_is_exiting_ code near the end of that method, add
something
> like this:
> 
> if (encryption_migration_active_ &&
>     policy.lid_closed_action() != ..._Action_DO_NOTHING) {
>   policy.set_lid_closed_action(..._Action_SUSPEND);
>   causes += (causes.empty() ? "" : ", ") + "encryption migration";
> }

I'm so sorry I missed your comment on the bug.
Thank you for the detailed guidance!
I'll update the patch after OOO (or someone might take over this)

[dspaid, 2017-05-30 05:58:12]

On 2017/05/29 04:01:49, fukino wrote:
> On 2017/05/26 13:45:34, Daniel Erat wrote:
> > not lgtm
> > 
> >
>
https://codereview.chromium.org/2907773002/diff/1/chrome/browser/ui/webui/chr...
> > File
> >
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2907773002/diff/1/chrome/browser/ui/webui/chr...
> >
>
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:363:
> > DBusThreadManager::Get()->GetPowerManagerClient()->SetPolicy(policy);
> > you can't send a policy directly like this; it'll be overwritten as soon as
we
> > decide to send another policy.
> > 
> > i tried to explain earlier that you should add a new method to
> > PowerPolicyController so that it will continue to be the only thing sending
> > policies. i think that something like this will work:
> > 
> > 1. add PowerPolicyController::SetEncryptionMigrationActive(bool active)
> > 2. make it update a encryption_migration_active_ bool member and call
> > SendCurrentPolicy()
> > 3. after the chrome_is_exiting_ code near the end of that method, add
> something
> > like this:
> > 
> > if (encryption_migration_active_ &&
> >     policy.lid_closed_action() != ..._Action_DO_NOTHING) {
> >   policy.set_lid_closed_action(..._Action_SUSPEND);
> >   causes += (causes.empty() ? "" : ", ") + "encryption migration";
> > }
> 
> I'm so sorry I missed your comment on the bug.
> Thank you for the detailed guidance!
> I'll update the patch after OOO (or someone might take over this)

I've sent out https://codereview.chromium.org/2912763003/ for review, which
should replace this patch.

[fukino, 2017-06-01 03:23:38]

Closing, as https://codereview.chromium.org/2912763003/ has landed.

[fukino, 2017-06-01 03:23:44]

Description was changed from

==========
cros: Prevent shut down on lid-close during migration.

In the login screen, the system will shut down on lid close.
Although we ask users not to close the lid during the migration, it should be
better to change the lid-close action to SUSPEND during migration.

BUG=726229
TEST=manually tested that closing lid during migration does not power off the
system.
==========

to

==========
cros: Prevent shut down on lid-close during migration.

In the login screen, the system will shut down on lid close.
Although we ask users not to close the lid during the migration, it should be
better to change the lid-close action to SUSPEND during migration.

BUG=726229
TEST=manually tested that closing lid during migration does not power off the
system.
==========