Add device policy to disallow shutdown - ash UI modifications

Add device policy to disallow shutdown - ash UI modifications

This CL is a follow-up to 776093004 which wires up the UI (ash only!) to the disallow shutdown policy.

CL 811283003: Clean-up of the system shutdown code paths should land first as it cleans up the existing shutdown code paths as suggested by stevenjb.

BUG=312541
Committed: https://crrev.com/7f630ba4f4c3f4075ba718fa18ee0a2eeb36cbb7
Cr-Commit-Position: refs/heads/master@{#312603}

[cschuet (SLOW), 2014-12-17 12:45:16]

cschuet@chromium.org changed reviewers:
+ antrim@chromium.org, derat@chromium.org, stevenjb@chromium.org

[cschuet (SLOW), 2014-12-17 12:45:17]

derat@chromium.org: Please review changes in 
chrome/browser/ui/ash/*
ash/ash_strings.grd
ash/test/*

antrim@chromium.org: Please review changes in 
chrome/browser/resources/chromeos/login/*
chrome/browser/ui/webui/chromeos/login/*

stevenjb@chromium.org: Please review changes in 
ash/system/*
chrome/browser/chromeos/*

[cschuet (SLOW), 2014-12-17 13:09:47]

cschuet@chromium.org changed reviewers:
- antrim@chromium.org

[cschuet (SLOW), 2014-12-17 13:12:54]

cschuet@chromium.org changed reviewers:
+ zelidrag@chromium.org

[cschuet (SLOW), 2014-12-17 13:12:55]

zelidrag@chromium.org: Please review changes in 
chrome/browser/resources/chromeos/login/*
chrome/browser/ui/webui/chromeos/login/*

[Daniel Erat, 2014-12-17 16:21:04]

https://codereview.chromium.org/811033002/diff/20001/ash/system/date/date_def...
File ash/system/date/date_default_view.cc (right):

https://codereview.chromium.org/811033002/diff/20001/ash/system/date/date_def...
ash/system/date/date_default_view.cc:143: } else if (sender == reboot_) {
you should probably add a metric for this to match the metrics for all the other
buttons.

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right):

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:27: #include
"chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer.h"
it seems strange to me for this file to depend on a file in a webui directory
for doing non-webui things. should the webui file be moved somewhere else if
it's being used more generally?

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:286:
scoped_ptr<ShutdownPolicyObserver> shutdown_policy_observer_;
please improve the naming here. it's confusing to have both a concrete
chromeos::ShutdownPolicyObserver class and an ash::ShutdownPolicyObserver
interface that are unrelated inheritance-wise, and then have both
shutdown_policy_observer_ and shutdown_policy_observers_ members here.

i'd recommend renaming the non-ash class to something like
ShutdownPolicyWatcher. maybe someone else can come up with a better name, but it
doesn't seem like a non-interface should be called "Observer".

[stevenjb, 2014-12-17 19:09:03]

https://codereview.chromium.org/811033002/diff/20001/ash/system/date/date_def...
File ash/system/date/date_default_view.cc (right):

https://codereview.chromium.org/811033002/diff/20001/ash/system/date/date_def...
ash/system/date/date_default_view.cc:87: view->AddButton(reboot_);
Rather than having two separate buttons and the UI kind of (but hopefully not
actually) determining the behavior, I think it would be better to use a single
button with a single delegate method 'ShutdownOrRestart' and just update the
text and tooltip to match the policy.

https://codereview.chromium.org/811033002/diff/20001/ash/system/date/date_def...
ash/system/date/date_default_view.cc:100: SystemTrayDelegate* systemTrayDelegate
=
system_tray_delegate

https://codereview.chromium.org/811033002/diff/20001/ash/system/tray/system_t...
File ash/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/811033002/diff/20001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:102: using RebootOnShutdownCallback =
base::Callback<void(bool)>;
Update other typedefs in this header to 'using'

https://codereview.chromium.org/811033002/diff/20001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:205: // Attempts to reboot the system.
restart

https://codereview.chromium.org/811033002/diff/20001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:314: // asynchronously calls
|RebootOnShutdownCallback| once a trusted policy
Use variable name |callback| instead of type.

https://codereview.chromium.org/811033002/diff/20001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:317: const RebootOnShutdownCallback&
callback) = 0;
This should be named something like 'RequestShouldRebootOnShutdown', otherwise
it looks like a setter.

https://codereview.chromium.org/811033002/diff/20001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:319: virtual void
RemoveShutdownPolicyObserver(
Remove should follow Add here and elsewhere.

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/shutdown_policy_browsertest.cc (right):

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/shutdown_policy_browsertest.cc:41: #include
"ui/views/view.h"
Does this really need all of these dependencies, or were they copied from
elsewhere? :)

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/shutdown_policy_browsertest.cc:72: virtual
~ShutdownPolicyBaseTest() {}
override instead of virtual

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/shutdown_policy_browsertest.cc:75: virtual void
OwnershipStatusChanged() override {}
no 'virtual' in overrides
Blank line between implemented methods.

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/header_bar.js (right):

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/header_bar.js:167:
chrome.send('restartSystem');
I think that this should also just call 'shutdownOrRestartSystem' and update the
label based on policy, rather than permitting a UI bug to attempt to do the
wrong thing.

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:606:
ash::LockStateController::RESTART);
It seems super silly for the ash system tray delegate to call back into
ash::Shell. Why not have the UI call ash::Shell directly? (I realize we are
already doing that with Shutdown(), but we should fix that too).

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:611:
base::Bind(&TriggerShutdown));
This has gotten a bit out of hand:
1. The UI asks this delegate to Shutdown.
2. This delegate calls ShutdownPolicy{Handler} to check policy.
3. This delegate then calls into ash::LockStateController to request either
Shutdown or Restart.
4. ash::LockStateController eventually calls
SessionStateControllerDelegateChromeos to request the actual shutdown or
restart.

Unless we have concrete plans allow a non-policy enforced 'Restart' in the UI, I
recommend doing the following instead:

* Remove both Restart() and ShutDown() from this delegate
* Call
ash::Shell::GetInstance()->lock_state_controller()->RequestShutdownOrRestart()
directly from the UI.
* Have SessionStateControllerDelegateChromeos check the policy and translate
Shutdown -> Restart

That way the UI is only using ShutdownPolicy{Handler} to inform what label to
show, and we don't rely on a delegate to do the policy checking.

WDYT?

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right):

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:27: #include
"chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer.h"
On 2014/12/17 16:21:04, Daniel Erat wrote:
> it seems strange to me for this file to depend on a file in a webui directory
> for doing non-webui things. should the webui file be moved somewhere else if
> it's being used more generally?

+1. shutdown_policy_observer.h should probably be moved to
chrome/browser/chromeos/settings.

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:286:
scoped_ptr<ShutdownPolicyObserver> shutdown_policy_observer_;
On 2014/12/17 16:21:04, Daniel Erat wrote:
> please improve the naming here. it's confusing to have both a concrete
> chromeos::ShutdownPolicyObserver class and an ash::ShutdownPolicyObserver
> interface that are unrelated inheritance-wise, and then have both
> shutdown_policy_observer_ and shutdown_policy_observers_ members here.
> 
> i'd recommend renaming the non-ash class to something like
> ShutdownPolicyWatcher. maybe someone else can come up with a better name, but
it
> doesn't seem like a non-interface should be called "Observer".
+1, I suggest the following:
* The existing ShutdownPolicyObserver should be renamed, either
ShutdownPolicyWatcher, or I might recommend ShutdownPolicyHandler.
* The new class should maybe be ShutdownPolicySettingObserver just to be extra
clear and avoid confusing git.

[cschuet (SLOW), 2015-01-07 14:37:08]

https://codereview.chromium.org/811033002/diff/20001/ash/system/date/date_def...
File ash/system/date/date_default_view.cc (right):

https://codereview.chromium.org/811033002/diff/20001/ash/system/date/date_def...
ash/system/date/date_default_view.cc:87: view->AddButton(reboot_);
On 2014/12/17 19:09:02, stevenjb wrote:
> Rather than having two separate buttons and the UI kind of (but hopefully not
> actually) determining the behavior, I think it would be better to use a single
> button with a single delegate method 'ShutdownOrRestart' and just update the
> text and tooltip to match the policy.

Done.

https://codereview.chromium.org/811033002/diff/20001/ash/system/date/date_def...
ash/system/date/date_default_view.cc:100: SystemTrayDelegate* systemTrayDelegate
=
On 2014/12/17 19:09:02, stevenjb wrote:
> system_tray_delegate

Done.

https://codereview.chromium.org/811033002/diff/20001/ash/system/date/date_def...
ash/system/date/date_default_view.cc:143: } else if (sender == reboot_) {
On 2014/12/17 16:21:04, Daniel Erat wrote:
> you should probably add a metric for this to match the metrics for all the
other
> buttons.
I have followed stevenjb's suggestion and there is now a joined code path in
DateDefaultView and LockStateController for both Restart and Shutdown actions.
It is only in the SessionStateControllerDelegateChromeos that the two branch
depending on the state of the policy.

https://codereview.chromium.org/811033002/diff/20001/ash/system/tray/system_t...
File ash/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/811033002/diff/20001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:102: using RebootOnShutdownCallback =
base::Callback<void(bool)>;
On 2014/12/17 19:09:02, stevenjb wrote:
> Update other typedefs in this header to 'using'

Done.

https://codereview.chromium.org/811033002/diff/20001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:205: // Attempts to reboot the system.
On 2014/12/17 19:09:02, stevenjb wrote:
> restart
This function has been removed in the new version of this CL.

https://codereview.chromium.org/811033002/diff/20001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:314: // asynchronously calls
|RebootOnShutdownCallback| once a trusted policy
On 2014/12/17 19:09:02, stevenjb wrote:
> Use variable name |callback| instead of type.

Done.

https://codereview.chromium.org/811033002/diff/20001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:317: const RebootOnShutdownCallback&
callback) = 0;
On 2014/12/17 19:09:02, stevenjb wrote:
> This should be named something like 'RequestShouldRebootOnShutdown', otherwise
> it looks like a setter.
> 

ShouldRebootOnShutdown is supposed to sound like a question "Should we reboot on
shutdown?". I think a setter would be called SetRebootOnShutdown. If you feel
strongly about this I will change it, but 'RequestShouldRebootOnShutdown' is a
bit clunky, I think.

https://codereview.chromium.org/811033002/diff/20001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:319: virtual void
RemoveShutdownPolicyObserver(
On 2014/12/17 19:09:02, stevenjb wrote:
> Remove should follow Add here and elsewhere.

Done.

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:606:
ash::LockStateController::RESTART);
On 2014/12/17 19:09:02, stevenjb wrote:
> It seems super silly for the ash system tray delegate to call back into
> ash::Shell. Why not have the UI call ash::Shell directly? (I realize we are
> already doing that with Shutdown(), but we should fix that too).

Done.

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:611:
base::Bind(&TriggerShutdown));
On 2014/12/17 19:09:02, stevenjb wrote:
> This has gotten a bit out of hand:
> 1. The UI asks this delegate to Shutdown.
> 2. This delegate calls ShutdownPolicy{Handler} to check policy.
> 3. This delegate then calls into ash::LockStateController to request either
> Shutdown or Restart.
> 4. ash::LockStateController eventually calls
> SessionStateControllerDelegateChromeos to request the actual shutdown or
> restart.
> 
> Unless we have concrete plans allow a non-policy enforced 'Restart' in the UI,
I
> recommend doing the following instead:
> 
> * Remove both Restart() and ShutDown() from this delegate
> * Call
> ash::Shell::GetInstance()->lock_state_controller()->RequestShutdownOrRestart()
> directly from the UI.
> * Have SessionStateControllerDelegateChromeos check the policy and translate
> Shutdown -> Restart
> 
> That way the UI is only using ShutdownPolicy{Handler} to inform what label to
> show, and we don't rely on a delegate to do the policy checking.
> 
> WDYT?

Done.

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right):

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:27: #include
"chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer.h"
On 2014/12/17 19:09:03, stevenjb wrote:
> On 2014/12/17 16:21:04, Daniel Erat wrote:
> > it seems strange to me for this file to depend on a file in a webui
directory
> > for doing non-webui things. should the webui file be moved somewhere else if
> > it's being used more generally?
> 
> +1. shutdown_policy_observer.h should probably be moved to
> chrome/browser/chromeos/settings.

Done.

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:27: #include
"chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer.h"
On 2014/12/17 16:21:04, Daniel Erat wrote:
> it seems strange to me for this file to depend on a file in a webui directory
> for doing non-webui things. should the webui file be moved somewhere else if
> it's being used more generally?

Done.

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:286:
scoped_ptr<ShutdownPolicyObserver> shutdown_policy_observer_;
On 2014/12/17 16:21:04, Daniel Erat wrote:
> please improve the naming here. it's confusing to have both a concrete
> chromeos::ShutdownPolicyObserver class and an ash::ShutdownPolicyObserver
> interface that are unrelated inheritance-wise, and then have both
> shutdown_policy_observer_ and shutdown_policy_observers_ members here.
> 
> i'd recommend renaming the non-ash class to something like
> ShutdownPolicyWatcher. maybe someone else can come up with a better name, but
it
> doesn't seem like a non-interface should be called "Observer".

Done.

https://codereview.chromium.org/811033002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:286:
scoped_ptr<ShutdownPolicyObserver> shutdown_policy_observer_;
On 2014/12/17 16:21:04, Daniel Erat wrote:
> please improve the naming here. it's confusing to have both a concrete
> chromeos::ShutdownPolicyObserver class and an ash::ShutdownPolicyObserver
> interface that are unrelated inheritance-wise, and then have both
> shutdown_policy_observer_ and shutdown_policy_observers_ members here.
> 
> i'd recommend renaming the non-ash class to something like
> ShutdownPolicyWatcher. maybe someone else can come up with a better name, but
it
> doesn't seem like a non-interface should be called "Observer".

Done.

[Daniel Erat, 2015-01-07 16:18:40]

you might want to hold off for a bit on any changes to make sure that steven
agrees with my comments. :-)

https://codereview.chromium.org/811033002/diff/40001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/811033002/diff/40001/ash/ash_strings.grd#newc...
ash/ash_strings.grd:355: <message name="IDS_ASH_STATUS_TRAY_REBOOT" desc="The
accessible text for the reboot button.">
nit: "the restart button"?

i think it's a bit weird that we call it rebooting in the code and restarting in
the ui, but i don't know if it's worth changing. "restarting" is a bit vague
when reading the code but i can understand if we don't want to show users the
word "reboot".

https://codereview.chromium.org/811033002/diff/40001/ash/system/chromeos/shut...
File ash/system/chromeos/shutdown_policy_observer.h (right):

https://codereview.chromium.org/811033002/diff/40001/ash/system/chromeos/shut...
ash/system/chromeos/shutdown_policy_observer.h:10: // Used by the
SystemTrayDelegate to observe the |kRebootOnShutdown| policy.
the "used by" in this comment seems a bit unclear. can you change it to
"Registered with SystemTrayDelegate to observe ..."?

https://codereview.chromium.org/811033002/diff/40001/ash/system/date/date_def...
File ash/system/date/date_default_view.cc (right):

https://codereview.chromium.org/811033002/diff/40001/ash/system/date/date_def...
ash/system/date/date_default_view.cc:141: if (reboot_on_shutdown) {
just do:

shutdown_->SetTooltipText(l10n_util::GetStringUTF16(
    reboot_on_shutdown ?
    IDS_ASH_STATUS_TRAY_REBOOT :
    IDS_ASH_STATUS_TRAY_SHUTDOWN));

https://codereview.chromium.org/811033002/diff/40001/ash/system/date/date_def...
File ash/system/date/date_default_view.h (right):

https://codereview.chromium.org/811033002/diff/40001/ash/system/date/date_def...
ash/system/date/date_default_view.h:44: TrayPopupHeaderButton* shutdown_;
could you rename these to help_button_, shutdown_button_, and lock_button_? the
current names are too vague (i was confused when i saw "if (lock_)" in the .cc
file).

https://codereview.chromium.org/811033002/diff/40001/ash/wm/lock_state_contro...
File ash/wm/lock_state_controller.h (right):

https://codereview.chromium.org/811033002/diff/40001/ash/wm/lock_state_contro...
ash/wm/lock_state_controller.h:43: virtual void RequestShutdownOrRestart() = 0;
sorry for all the conflicting messages you're getting here, but i'd prefer that
this method were just named "RequestShutdown". the force-reboot policy seems
like an edge case / implementation detail that callers shouldn't need to think
about.

https://codereview.chromium.org/811033002/diff/40001/ash/wm/lock_state_contro...
ash/wm/lock_state_controller.h:192: void RequestShutdownOrRestart();
i think that this would be better as "RequestShutdown" as well

https://codereview.chromium.org/811033002/diff/40001/ash/wm/lock_state_contro...
ash/wm/lock_state_controller.h:243: // Request that the machine is shut down.
nit: s/is/be/

https://codereview.chromium.org/811033002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/session_state_controller_delegate_chromeos.cc
(right):

https://codereview.chromium.org/811033002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/session_state_controller_delegate_chromeos.cc:44:
if (reboot_on_shutdown) {
nit: it's a line shorter to do:

  if (reboot_on_shutdown)
    DBusThreadManager::Get()->GetPowerManagerClient()->RequestRestart();
  else
    DBusThreadManager::Get()->GetPowerManagerClient()->RequestShutdown();

https://codereview.chromium.org/811033002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/shutdown_policy_handler_unittest.cc
(right):

https://codereview.chromium.org/811033002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/shutdown_policy_handler_unittest.cc:24: public
ShutdownPolicyHandler::Delegate {
nit: fix indenting

https://codereview.chromium.org/811033002/diff/40001/chrome/chrome_browser_ch...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/811033002/diff/40001/chrome/chrome_browser_ch...
chrome/chrome_browser_chromeos.gypi:876:
'browser/chromeos/settings/shutdown_policy_handler.cc',
nit: alphabetize ("shutdown" < "stub")

[stevenjb, 2015-01-07 19:03:37]

https://codereview.chromium.org/811033002/diff/40001/ash/wm/lock_state_contro...
File ash/wm/lock_state_controller.h (right):

https://codereview.chromium.org/811033002/diff/40001/ash/wm/lock_state_contro...
ash/wm/lock_state_controller.h:43: virtual void RequestShutdownOrRestart() = 0;
On 2015/01/07 16:18:40, Daniel Erat wrote:
> sorry for all the conflicting messages you're getting here, but i'd prefer
that
> this method were just named "RequestShutdown". the force-reboot policy seems
> like an edge case / implementation detail that callers shouldn't need to think
> about.

Agreed.

[stevenjb, 2015-01-07 19:24:20]

https://codereview.chromium.org/811033002/diff/40001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/811033002/diff/40001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:236: new
ShutdownPolicyHandler(CrosSettings::Get(), this));
It's a shame that we have to provide this level of indirection just because
ShutdownPolicyHandler lives in src/chrome.

If we provided a DeviceSettingsProvider interface and moved
DeviceSettingsProvider -> DeviceSettingsProviderImpl, and passed
DeviceSettingsProviderImpl::Get() to CrosSettings::Initialize(), then
CrosSettings and ShutdownPolicyHandler could live in src/chromeos, and
DateDefaultView could depend on ShutdownPolicyHandler directly.

I can understand not wanting to make this change dependent on that, but if this
feature is targeted for M42 I would recommend considering making that change
first instead of adding more methods to SystemTrayDelegate.

Created http://crbug.com/446937

[cschuet (SLOW), 2015-01-13 10:38:28]

ptal

https://codereview.chromium.org/811033002/diff/40001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/811033002/diff/40001/ash/ash_strings.grd#newc...
ash/ash_strings.grd:355: <message name="IDS_ASH_STATUS_TRAY_REBOOT" desc="The
accessible text for the reboot button.">
On 2015/01/07 16:18:40, Daniel Erat wrote:
> nit: "the restart button"?
> 
> i think it's a bit weird that we call it rebooting in the code and restarting
in
> the ui, but i don't know if it's worth changing. "restarting" is a bit vague
> when reading the code but i can understand if we don't want to show users the
> word "reboot".

Indeed, the rationale was that all user-facing strings mention "restart" rather
than "reboot".

https://codereview.chromium.org/811033002/diff/40001/ash/system/chromeos/shut...
File ash/system/chromeos/shutdown_policy_observer.h (right):

https://codereview.chromium.org/811033002/diff/40001/ash/system/chromeos/shut...
ash/system/chromeos/shutdown_policy_observer.h:10: // Used by the
SystemTrayDelegate to observe the |kRebootOnShutdown| policy.
On 2015/01/07 16:18:40, Daniel Erat wrote:
> the "used by" in this comment seems a bit unclear. can you change it to
> "Registered with SystemTrayDelegate to observe ..."?

Done.

https://codereview.chromium.org/811033002/diff/40001/ash/system/date/date_def...
File ash/system/date/date_default_view.cc (right):

https://codereview.chromium.org/811033002/diff/40001/ash/system/date/date_def...
ash/system/date/date_default_view.cc:141: if (reboot_on_shutdown) {
On 2015/01/07 16:18:40, Daniel Erat wrote:
> just do:
> 
> shutdown_->SetTooltipText(l10n_util::GetStringUTF16(
>     reboot_on_shutdown ?
>     IDS_ASH_STATUS_TRAY_REBOOT :
>     IDS_ASH_STATUS_TRAY_SHUTDOWN));

Done.

https://codereview.chromium.org/811033002/diff/40001/ash/system/date/date_def...
File ash/system/date/date_default_view.h (right):

https://codereview.chromium.org/811033002/diff/40001/ash/system/date/date_def...
ash/system/date/date_default_view.h:44: TrayPopupHeaderButton* shutdown_;
On 2015/01/07 16:18:40, Daniel Erat wrote:
> could you rename these to help_button_, shutdown_button_, and lock_button_?
the
> current names are too vague (i was confused when i saw "if (lock_)" in the .cc
> file).

Done.

https://codereview.chromium.org/811033002/diff/40001/ash/wm/lock_state_contro...
File ash/wm/lock_state_controller.h (right):

https://codereview.chromium.org/811033002/diff/40001/ash/wm/lock_state_contro...
ash/wm/lock_state_controller.h:43: virtual void RequestShutdownOrRestart() = 0;
On 2015/01/07 16:18:40, Daniel Erat wrote:
> sorry for all the conflicting messages you're getting here, but i'd prefer
that
> this method were just named "RequestShutdown". the force-reboot policy seems
> like an edge case / implementation detail that callers shouldn't need to think
> about.

Done.

https://codereview.chromium.org/811033002/diff/40001/ash/wm/lock_state_contro...
ash/wm/lock_state_controller.h:192: void RequestShutdownOrRestart();
On 2015/01/07 16:18:40, Daniel Erat wrote:
> i think that this would be better as "RequestShutdown" as well

Done.

https://codereview.chromium.org/811033002/diff/40001/ash/wm/lock_state_contro...
ash/wm/lock_state_controller.h:243: // Request that the machine is shut down.
On 2015/01/07 16:18:40, Daniel Erat wrote:
> nit: s/is/be/

Done.

https://codereview.chromium.org/811033002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/session_state_controller_delegate_chromeos.cc
(right):

https://codereview.chromium.org/811033002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/session_state_controller_delegate_chromeos.cc:44:
if (reboot_on_shutdown) {
On 2015/01/07 16:18:40, Daniel Erat wrote:
> nit: it's a line shorter to do:
> 
>   if (reboot_on_shutdown)
>     DBusThreadManager::Get()->GetPowerManagerClient()->RequestRestart();
>   else
>     DBusThreadManager::Get()->GetPowerManagerClient()->RequestShutdown();

Done.

https://codereview.chromium.org/811033002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/shutdown_policy_handler_unittest.cc
(right):

https://codereview.chromium.org/811033002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/shutdown_policy_handler_unittest.cc:24: public
ShutdownPolicyHandler::Delegate {
On 2015/01/07 16:18:40, Daniel Erat wrote:
> nit: fix indenting

Done.

https://codereview.chromium.org/811033002/diff/40001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/811033002/diff/40001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:236: new
ShutdownPolicyHandler(CrosSettings::Get(), this));
On 2015/01/07 19:24:19, stevenjb wrote:
> It's a shame that we have to provide this level of indirection just because
> ShutdownPolicyHandler lives in src/chrome.
> 
> If we provided a DeviceSettingsProvider interface and moved
> DeviceSettingsProvider -> DeviceSettingsProviderImpl, and passed
> DeviceSettingsProviderImpl::Get() to CrosSettings::Initialize(), then
> CrosSettings and ShutdownPolicyHandler could live in src/chromeos, and
> DateDefaultView could depend on ShutdownPolicyHandler directly.
> 
> I can understand not wanting to make this change dependent on that, but if
this
> feature is targeted for M42 I would recommend considering making that change
> first instead of adding more methods to SystemTrayDelegate.
> 
> Created http://crbug.com/446937

As discussed in http://crbug.com/446937, the scope of this proposed change has
now increased quite a bit. I agree, that we can live with the addition and clean
it up later and should not block on this.

https://codereview.chromium.org/811033002/diff/40001/chrome/chrome_browser_ch...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/811033002/diff/40001/chrome/chrome_browser_ch...
chrome/chrome_browser_chromeos.gypi:876:
'browser/chromeos/settings/shutdown_policy_handler.cc',
On 2015/01/07 16:18:40, Daniel Erat wrote:
> nit: alphabetize ("shutdown" < "stub")

Done.

[stevenjb, 2015-01-13 16:59:13]

lgtm

[cschuet (SLOW), 2015-01-21 10:05:58]

On 2015/01/13 16:59:13, stevenjb wrote:
> lgtm

@derat: friendly review ping :)

[Daniel Erat, 2015-01-21 15:25:40]

lgtm with a nit. thanks!

https://codereview.chromium.org/811033002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/811033002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1042:
->RequestShutdown();
nit: this can all fit on a single line now

[cschuet (SLOW), 2015-01-21 15:30:39]

cschuet@chromium.org changed reviewers:
+ nkostylev@chromium.org
- zelidrag@chromium.org

[cschuet (SLOW), 2015-01-21 15:34:41]

@nkostylev: ptal @
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc

https://codereview.chromium.org/811033002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/811033002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1042:
->RequestShutdown();
On 2015/01/21 15:25:40, Daniel Erat wrote:
> nit: this can all fit on a single line now

Done.

[cschuet (SLOW), 2015-01-22 09:43:11]

On 2015/01/21 15:34:41, cschuet wrote:
> @nkostylev: ptal @
> chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc
> 
>
https://codereview.chromium.org/811033002/diff/80001/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):
> 
>
https://codereview.chromium.org/811033002/diff/80001/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1042:
> ->RequestShutdown();
> On 2015/01/21 15:25:40, Daniel Erat wrote:
> > nit: this can all fit on a single line now
> 
> Done.

TBA'ing Nikita as it involves only a use of the modified API.

[cschuet (SLOW), 2015-01-22 09:43:27]

The CQ bit was checked by cschuet@chromium.org

[commit-bot: I haz the power, 2015-01-22 09:43:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/811033002/100001

[commit-bot: I haz the power, 2015-01-22 09:47:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-22 09:47:11]

Try jobs failed on following builders:
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_gn_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_...)

[cschuet (SLOW), 2015-01-22 12:22:16]

The CQ bit was checked by cschuet@chromium.org

[commit-bot: I haz the power, 2015-01-22 12:22:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/811033002/120001

[cschuet (SLOW), 2015-01-22 12:27:10]

The CQ bit was unchecked by cschuet@chromium.org

[cschuet (SLOW), 2015-01-22 12:28:31]

The CQ bit was checked by cschuet@chromium.org

[commit-bot: I haz the power, 2015-01-22 12:29:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/811033002/120001

[Nikita (slow), 2015-01-22 12:37:24]

lgtm

[cschuet (SLOW), 2015-01-22 12:39:52]

The CQ bit was unchecked by cschuet@chromium.org

[cschuet (SLOW), 2015-01-22 12:40:33]

The CQ bit was checked by cschuet@chromium.org

[commit-bot: I haz the power, 2015-01-22 12:41:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/811033002/120001

[commit-bot: I haz the power, 2015-01-22 13:17:35]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-01-22 13:18:37]

Patchset 7 (id:??) landed as
https://crrev.com/7f630ba4f4c3f4075ba718fa18ee0a2eeb36cbb7
Cr-Commit-Position: refs/heads/master@{#312603}