Allow closing root window, or pressing the power button to shutdown ash_shell, and chrome /w chrome…

Allow closing root window, or pressing the power button to shutdown ash_shell, and chrome /w chromeos flag (not running on chromeos) to exit the application


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

[Harry McCleave, 2012-08-07 01:55:25]

Please take a look when you get a chance.

[sadrul, 2012-08-07 13:29:03]

http://codereview.chromium.org/10825191/diff/1/ash/wm/power_button_controller.cc
File ash/wm/power_button_controller.cc (right):

http://codereview.chromium.org/10825191/diff/1/ash/wm/power_button_controller...
ash/wm/power_button_controller.cc:478: Shell::GetInstance()->delegate()->Exit();
Is this change to make sure chrome terminates if the host window is closed by
the user? Can this cause problems if the host-window is closing because chrome
is already terminating?

http://codereview.chromium.org/10825191/diff/1/chrome/browser/chromeos/system...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/10825191/diff/1/chrome/browser/chromeos/system...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:387:
browser::AttemptUserExit();
no braces

[Harry McCleave, 2012-08-07 18:30:14]

https://chromiumcodereview.appspot.com/10825191/diff/1/ash/wm/power_button_co...
File ash/wm/power_button_controller.cc (right):

https://chromiumcodereview.appspot.com/10825191/diff/1/ash/wm/power_button_co...
ash/wm/power_button_controller.cc:478: Shell::GetInstance()->delegate()->Exit();
On 2012/08/07 13:29:03, sadrul wrote:
> Is this change to make sure chrome terminates if the host window is closed by
> the user? Can this cause problems if the host-window is closing because chrome
> is already terminating?

In its current form the host window doesn't actually close, the event that
triggers this is the user clicking the 'x' to close the host window but this
event is ignored, this change causes the application to exit at that point,
though perhaps isn't the correct way to do so?

https://chromiumcodereview.appspot.com/10825191/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/10825191/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:387:
browser::AttemptUserExit();
On 2012/08/07 13:29:03, sadrul wrote:
> no braces

Done. Though I like my braces :'(

[sadrul, 2012-08-07 22:13:24]

LGTM. The comment below is to make sure you have tested that case.

http://codereview.chromium.org/10825191/diff/1/ash/wm/power_button_controller.cc
File ash/wm/power_button_controller.cc (right):

http://codereview.chromium.org/10825191/diff/1/ash/wm/power_button_controller...
ash/wm/power_button_controller.cc:478: Shell::GetInstance()->delegate()->Exit();
On 2012/08/07 18:30:14, Harry McCleave wrote:
> On 2012/08/07 13:29:03, sadrul wrote:
> > Is this change to make sure chrome terminates if the host window is closed
by
> > the user? Can this cause problems if the host-window is closing because
chrome
> > is already terminating?
> 
> In its current form the host window doesn't actually close, the event that
> triggers this is the user clicking the 'x' to close the host window but this
> event is ignored, this change causes the application to exit at that point,
> though perhaps isn't the correct way to do so?

If the user terminates chrome from within chrome (e.g. pressing ctrl-shift-q),
will this code trigger? If it does, will this cause problems?

[sky, 2012-08-07 23:14:08]

I'm not sure if this would have any side effects on an actual device. Maybe Dan
knows.

http://codereview.chromium.org/10825191/diff/2002/ash/wm/power_button_control...
File ash/wm/power_button_controller.cc (right):

http://codereview.chromium.org/10825191/diff/2002/ash/wm/power_button_control...
ash/wm/power_button_controller.cc:477: if(Shell::GetInstance() &&
Shell::GetInstance()->delegate()) {
no {}

[sky, 2012-08-07 23:33:18]

LGTM

[Daniel Erat, 2012-08-07 23:35:17]

FYI

http://codereview.chromium.org/10825191/diff/2002/ash/wm/power_button_control...
File ash/wm/power_button_controller.cc (right):

http://codereview.chromium.org/10825191/diff/2002/ash/wm/power_button_control...
ash/wm/power_button_controller.cc:476: void
PowerButtonController::OnRootWindowHostClosed(const aura::RootWindow*) {
nit: Doesn't need to happen in this change, but this method should be renamed to
more accurately reflect when it's actually called.  It looks like it only gets
invoked in response to a WM_DELETE_WINDOW message from an X window manager, so
something like OnRootWindowHostCloseRequested() would probably be better.

http://codereview.chromium.org/10825191/diff/2002/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/10825191/diff/2002/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:380:
browser::AttemptUserExit();
nit: Wondering if it'd be cleaner to implement this via the
PowerManagerClient::RequestShutdown() stub implementation instead of
special-casing the non-cros case here.

[commit-bot: I haz the power, 2012-08-15 22:54:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/10825191/5004

[commit-bot: I haz the power, 2012-08-16 01:18:19]

Change committed as 151815