Add LogoutConfirmationController to show LogoutConfirmationDialogs

Add LogoutConfirmationController to show LogoutConfirmationDialogs

LogoutButtonTray uses and owns LogoutConfirmationDialogView, a dialog that
asks the user to confirm or deny logout. This CL generalizes the dialog
and moves its ownership to a LogoutConfirmationController accessible via
ash::Shell, allowing other classes to show the same dialog.

BUG=278247
TEST=Updated unit tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255316

[bartfab (slow), 2014-02-27 10:30:49]

Hi Steven,

Could you take a look at ash/system/*?

Hi Daniel,

Could you take a look at ash/ash.gyp and ash/shell.*?

Bin, FYI.

[Daniel Erat, 2014-02-27 11:59:15]

lgtm for top-level ash/ files

[stevenjb, 2014-02-27 17:58:44]

https://codereview.chromium.org/178183005/diff/1/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/178183005/diff/1/ash/ash.gyp#newcode374
ash/ash.gyp:374: 'system/logout_confirmation/logout_confirmation_dialog.h',
Are logout_button and logout_conformation related at this point? It seems a bit
confusing to have two separate directories, maybe we can put them all in a
'logout' directory?

https://codereview.chromium.org/178183005/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/178183005/diff/1/ash/shell.cc#newcode964
ash/shell.cc:964: scoped_ptr<base::TickClock>(new base::DefaultTickClock),
nit: use make_scoped_ptr?

https://codereview.chromium.org/178183005/diff/1/ash/shell.cc#newcode966
ash/shell.cc:966: base::Unretained(system_tray_delegate_.get()))));
Even though we are explicitly creating/destroying LogoutConfirmationController
after/before SystemTrayDelegate, it might be more clear/robust/maintainable to
use a local function that calls ash::Shell::GetInstance(), checks
shell->system_tray_delegate(), and calls
shell->system_tray_delegate()->SignOut(). That way even if the callback somehow
outlives LogoutConfirmationController, or code gets moved around, the callback
will still be safe.

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_button/log...
File ash/system/logout_button/logout_button_tray.cc (right):

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_button/log...
ash/system/logout_button/logout_button_tray.cc:147: base::TimeTicks::Now() +
dialog_duration_);
Is there an edge case here where the tray can outlive the controller? (i.e. I
think we may need a test here).

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
File ash/system/logout_confirmation/logout_confirmation_controller.cc (right):

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller.cc:46: else
} else {

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
File ash/system/logout_confirmation/logout_confirmation_controller.h (right):

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller.h:32:
LogoutConfirmationController(scoped_ptr<base::TickClock> clock,
Are we passing in clock (instead of constructing it) just for tests? If so, I
think it would be more clear to add a SetClockForTest() method that resets
clock_ instead.

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller.h:34: virtual
~LogoutConfirmationController();
This doesn't appear to need to be virtual.

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller.h:36:
base::TickClock* Clock() const;
This can be:
  base::TickClock* clock() const { return clock_.get(); }

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller.h:44: void
OnLogoutConfirmed();
WS

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller.h:53:
LogoutConfirmationDialog* dialog_;
Comment ownership of raw pointer. (Views hierarchy I assume?)

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
File ash/system/logout_confirmation/logout_confirmation_controller_unittest.cc
(right):

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller_unittest.cc:28: //
Consolidate them (crbug.com/329911).
Would be nice to get this done sooner than later.

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller_unittest.cc:95:
tasks_.push(std::pair<base::TimeTicks, base::Closure>(now_ + delay, task));
nit: std::make_pair(now_ + delay, task)

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
File ash/system/logout_confirmation/logout_confirmation_dialog.cc (right):

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_dialog.cc:99: if
(time_remaining >= base::TimeDelta::FromMilliseconds(500)) {
const for 500

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
File ash/system/logout_confirmation/logout_confirmation_dialog.h (right):

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_dialog.h:39:
GetDialogButtonLabel(ui::DialogButton button) const OVERRIDE;
clang format says:
 virtual base::string16 GetDialogButtonLabel(ui::DialogButton button) const
      OVERRIDE;

[bartfab (slow), 2014-02-28 12:13:49]

https://codereview.chromium.org/178183005/diff/1/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/178183005/diff/1/ash/ash.gyp#newcode374
ash/ash.gyp:374: 'system/logout_confirmation/logout_confirmation_dialog.h',
On 2014/02/27 17:58:44, stevenjb wrote:
> Are logout_button and logout_conformation related at this point? It seems a
bit
> confusing to have two separate directories, maybe we can put them all in a
> 'logout' directory?

As of this CL, the logout button is the only class that uses the logout
confirmation dialog. But I am doing this whole refactoring only because my next
CL is adding another consumer of the dialog. If you want me to, I can still move
all the classes into a joint "logout" directory but that would imply a stronger
relationship between dialog and button than there really is.

https://codereview.chromium.org/178183005/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/178183005/diff/1/ash/shell.cc#newcode964
ash/shell.cc:964: scoped_ptr<base::TickClock>(new base::DefaultTickClock),
On 2014/02/27 17:58:44, stevenjb wrote:
> nit: use make_scoped_ptr?

make_scoped_ptr cannot be used when upcasting, as done here from a
DefaultTickClock to a TickClock.

https://codereview.chromium.org/178183005/diff/1/ash/shell.cc#newcode966
ash/shell.cc:966: base::Unretained(system_tray_delegate_.get()))));
On 2014/02/27 17:58:44, stevenjb wrote:
> Even though we are explicitly creating/destroying LogoutConfirmationController
> after/before SystemTrayDelegate, it might be more clear/robust/maintainable to
> use a local function that calls ash::Shell::GetInstance(), checks
> shell->system_tray_delegate(), and calls
> shell->system_tray_delegate()->SignOut(). That way even if the callback
somehow
> outlives LogoutConfirmationController, or code gets moved around, the callback
> will still be safe.

The reason for doing it this way is that I can supply a different closure for
tests. It is essentially a delegate interface. SystemTrayDelegate::SignOut() is
one possible delegate. Tests will use another delegate that records that the
method has been called only.

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_button/log...
File ash/system/logout_button/logout_button_tray.cc (right):

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_button/log...
ash/system/logout_button/logout_button_tray.cc:147: base::TimeTicks::Now() +
dialog_duration_);
On 2014/02/27 17:58:44, stevenjb wrote:
> Is there an edge case here where the tray can outlive the controller? (i.e. I
> think we may need a test here).

The controller is destroyed in the Shell's destructor. Even if a tray was still
alive, it would no longer reach the ButttonPressed() method for lack of input
events. But it is easy to add a check to be on the safe side. Done.

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
File ash/system/logout_confirmation/logout_confirmation_controller.cc (right):

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller.cc:46: else
On 2014/02/27 17:58:44, stevenjb wrote:
> } else {

Done.

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
File ash/system/logout_confirmation/logout_confirmation_controller.h (right):

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller.h:32:
LogoutConfirmationController(scoped_ptr<base::TickClock> clock,
On 2014/02/27 17:58:44, stevenjb wrote:
> Are we passing in clock (instead of constructing it) just for tests? If so, I
> think it would be more clear to add a SetClockForTest() method that resets
> clock_ instead.

Done.

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller.h:34: virtual
~LogoutConfirmationController();
On 2014/02/27 17:58:44, stevenjb wrote:
> This doesn't appear to need to be virtual.

You were right in the version of the CL you looked at. The class now inherits
from ShellObserver though so that the virtual is warranted :).

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller.h:36:
base::TickClock* Clock() const;
On 2014/02/27 17:58:44, stevenjb wrote:
> This can be:
>   base::TickClock* clock() const { return clock_.get(); }

Done.

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller.h:44: void
OnLogoutConfirmed();
On 2014/02/27 17:58:44, stevenjb wrote:
> WS

Done.

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller.h:53:
LogoutConfirmationDialog* dialog_;
On 2014/02/27 17:58:44, stevenjb wrote:
> Comment ownership of raw pointer. (Views hierarchy I assume?)

Done.

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
File ash/system/logout_confirmation/logout_confirmation_controller_unittest.cc
(right):

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller_unittest.cc:28: //
Consolidate them (crbug.com/329911).
On 2014/02/27 17:58:44, stevenjb wrote:
> Would be nice to get this done sooner than later.

The problem is that it requires buy-in from base owners because the class is
used by tests in different components. I will start that discussion with the
owners.

In the meantime, this CL does not make the situation worse - it moves the
MockTimeSingleThreadTaskRunner from logout_button_tray_unittest.cc (which is
being deleted) to logout_confirmation_controller_unittest.cc (which is being
added).

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_controller_unittest.cc:95:
tasks_.push(std::pair<base::TimeTicks, base::Closure>(now_ + delay, task));
On 2014/02/27 17:58:44, stevenjb wrote:
> nit: std::make_pair(now_ + delay, task)

Done.

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
File ash/system/logout_confirmation/logout_confirmation_dialog.cc (right):

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_dialog.cc:99: if
(time_remaining >= base::TimeDelta::FromMilliseconds(500)) {
On 2014/02/27 17:58:44, stevenjb wrote:
> const for 500

Done.

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
File ash/system/logout_confirmation/logout_confirmation_dialog.h (right):

https://codereview.chromium.org/178183005/diff/1/ash/system/logout_confirmati...
ash/system/logout_confirmation/logout_confirmation_dialog.h:39:
GetDialogButtonLabel(ui::DialogButton button) const OVERRIDE;
On 2014/02/27 17:58:44, stevenjb wrote:
> clang format says:
>  virtual base::string16 GetDialogButtonLabel(ui::DialogButton button) const
>       OVERRIDE;

Either one is allowed by the style guide. I have no preference for one or the
other. Done.

[bartfab (slow), 2014-03-04 22:45:42]

Hi Steven,

Friendly review ping.

[stevenjb, 2014-03-04 23:33:29]

https://codereview.chromium.org/178183005/diff/1/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/178183005/diff/1/ash/ash.gyp#newcode374
ash/ash.gyp:374: 'system/logout_confirmation/logout_confirmation_dialog.h',
On 2014/02/28 12:13:49, bartfab wrote:
> On 2014/02/27 17:58:44, stevenjb wrote:
> > Are logout_button and logout_conformation related at this point? It seems a
> bit
> > confusing to have two separate directories, maybe we can put them all in a
> > 'logout' directory?
> 
> As of this CL, the logout button is the only class that uses the logout
> confirmation dialog. But I am doing this whole refactoring only because my
next
> CL is adding another consumer of the dialog. If you want me to, I can still
move
> all the classes into a joint "logout" directory but that would imply a
stronger
> relationship between dialog and button than there really is.

I wouldn't say that implies a strong relationship, I would just prefer to keep
the number of subfolders under ash/system reasonable; it doens't make sense to
have a folder for every class. Maybe a better grouping would be
ash/system/session and everything logout and session_length_limit related could
be moved there. That can be in a separate CL however.

https://codereview.chromium.org/178183005/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/178183005/diff/1/ash/shell.cc#newcode964
ash/shell.cc:964: scoped_ptr<base::TickClock>(new base::DefaultTickClock),
On 2014/02/28 12:13:49, bartfab wrote:
> On 2014/02/27 17:58:44, stevenjb wrote:
> > nit: use make_scoped_ptr?
> 
> make_scoped_ptr cannot be used when upcasting, as done here from a
> DefaultTickClock to a TickClock.
Ah, I missed the upcast. I like this version better without the need for a cast.

https://codereview.chromium.org/178183005/diff/1/ash/shell.cc#newcode966
ash/shell.cc:966: base::Unretained(system_tray_delegate_.get()))));
On 2014/02/28 12:13:49, bartfab wrote:
> On 2014/02/27 17:58:44, stevenjb wrote:
> > Even though we are explicitly creating/destroying
LogoutConfirmationController
> > after/before SystemTrayDelegate, it might be more clear/robust/maintainable
to
> > use a local function that calls ash::Shell::GetInstance(), checks
> > shell->system_tray_delegate(), and calls
> > shell->system_tray_delegate()->SignOut(). That way even if the callback
> somehow
> > outlives LogoutConfirmationController, or code gets moved around, the
callback
> > will still be safe.
> 
> The reason for doing it this way is that I can supply a different closure for
> tests. It is essentially a delegate interface. SystemTrayDelegate::SignOut()
is
> one possible delegate. Tests will use another delegate that records that the
> method has been called only.
Testability is important, but stability even more so. It looks like this would
be safe since logout_confirmation_controller_ is destroyed before
system_tray_delegate_, but that's only true so long as
LogoutConfirtmationController never passes along the callback. We might want to
at least make a comment to that effect.

Meanwhile, in convincing myself of that I think we may have a different edge
case, I'll comment that part of the code separately.

https://codereview.chromium.org/178183005/diff/40001/ash/system/logout_confir...
File ash/system/logout_confirmation/logout_confirmation_controller.cc (right):

https://codereview.chromium.org/178183005/diff/40001/ash/system/logout_confir...
ash/system/logout_confirmation/logout_confirmation_controller.cc:30:
Shell::GetInstance()->RemoveShellObserver(this);
I think we need to destroy dialog_ here (if not NULL), or at least clear its
pointer to us. Otherwise, since LogoutConfirmationDialog is owned by the Views
hierarchy, which gets destroyed *after* LogoutConfirtmationController, it could
access this class after it is destroyed (e.g. if Logout were clicked as Chrome
is shutting down).

[bartfab (slow), 2014-03-05 18:19:44]

https://codereview.chromium.org/178183005/diff/1/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/178183005/diff/1/ash/ash.gyp#newcode374
ash/ash.gyp:374: 'system/logout_confirmation/logout_confirmation_dialog.h',
On 2014/03/04 23:33:29, stevenjb wrote:
> On 2014/02/28 12:13:49, bartfab wrote:
> > On 2014/02/27 17:58:44, stevenjb wrote:
> > > Are logout_button and logout_conformation related at this point? It seems
a
> > bit
> > > confusing to have two separate directories, maybe we can put them all in a
> > > 'logout' directory?
> > 
> > As of this CL, the logout button is the only class that uses the logout
> > confirmation dialog. But I am doing this whole refactoring only because my
> next
> > CL is adding another consumer of the dialog. If you want me to, I can still
> move
> > all the classes into a joint "logout" directory but that would imply a
> stronger
> > relationship between dialog and button than there really is.
> 
> I wouldn't say that implies a strong relationship, I would just prefer to keep
> the number of subfolders under ash/system reasonable; it doens't make sense to
> have a folder for every class. Maybe a better grouping would be
> ash/system/session and everything logout and session_length_limit related
could
> be moved there. That can be in a separate CL however.

Moving everything under ash/system/session makes sense. I modified the CL do to
this move straight away.

https://codereview.chromium.org/178183005/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/178183005/diff/1/ash/shell.cc#newcode966
ash/shell.cc:966: base::Unretained(system_tray_delegate_.get()))));
On 2014/03/04 23:33:29, stevenjb wrote:
> On 2014/02/28 12:13:49, bartfab wrote:
> > On 2014/02/27 17:58:44, stevenjb wrote:
> > > Even though we are explicitly creating/destroying
> LogoutConfirmationController
> > > after/before SystemTrayDelegate, it might be more
clear/robust/maintainable
> to
> > > use a local function that calls ash::Shell::GetInstance(), checks
> > > shell->system_tray_delegate(), and calls
> > > shell->system_tray_delegate()->SignOut(). That way even if the callback
> > somehow
> > > outlives LogoutConfirmationController, or code gets moved around, the
> callback
> > > will still be safe.
> > 
> > The reason for doing it this way is that I can supply a different closure
for
> > tests. It is essentially a delegate interface. SystemTrayDelegate::SignOut()
> is
> > one possible delegate. Tests will use another delegate that records that the
> > method has been called only.
> Testability is important, but stability even more so. It looks like this would
> be safe since logout_confirmation_controller_ is destroyed before
> system_tray_delegate_, but that's only true so long as
> LogoutConfirtmationController never passes along the callback. We might want
to
> at least make a comment to that effect.
> 
> Meanwhile, in convincing myself of that I think we may have a different edge
> case, I'll comment that part of the code separately.

Added a comment.

https://codereview.chromium.org/178183005/diff/40001/ash/system/logout_confir...
File ash/system/logout_confirmation/logout_confirmation_controller.cc (right):

https://codereview.chromium.org/178183005/diff/40001/ash/system/logout_confir...
ash/system/logout_confirmation/logout_confirmation_controller.cc:30:
Shell::GetInstance()->RemoveShellObserver(this);
On 2014/03/04 23:33:30, stevenjb wrote:
> I think we need to destroy dialog_ here (if not NULL), or at least clear its
> pointer to us. Otherwise, since LogoutConfirmationDialog is owned by the Views
> hierarchy, which gets destroyed *after* LogoutConfirtmationController, it
could
> access this class after it is destroyed (e.g. if Logout were clicked as Chrome
> is shutting down).

I think the code was actually safe: In my understanding, the loops are no longer
spun at this point in the shutdown sequence, so input and timer events should no
longer be generated or processed. But I agree that closing the dialog is even
safer. Done.

[stevenjb, 2014-03-05 18:57:19]

Looks good, just a couple final comments.

https://codereview.chromium.org/178183005/diff/80001/ash/system/session/logou...
File ash/system/session/logout_confirmation_controller.cc (right):

https://codereview.chromium.org/178183005/diff/80001/ash/system/session/logou...
ash/system/session/logout_confirmation_controller.cc:20: :
clock_(reinterpret_cast<base::TickClock*>(new base::DefaultTickClock)),
reinterpret_cast shouldn't be necessary here.

https://codereview.chromium.org/178183005/diff/80001/ash/system/session/logou...
File ash/system/session/logout_confirmation_controller_unittest.cc (right):

https://codereview.chromium.org/178183005/diff/80001/ash/system/session/logou...
ash/system/session/logout_confirmation_controller_unittest.cc:270: }
Can we add a test to ensure that if a user declines, and we call ConfirmLogout
again (e.g. the timeout was triggered a second time), it works as expected? (The
logic looked OK to me, but I wasn't 100% certain).

[bartfab (slow), 2014-03-05 22:24:55]

https://codereview.chromium.org/178183005/diff/80001/ash/system/session/logou...
File ash/system/session/logout_confirmation_controller.cc (right):

https://codereview.chromium.org/178183005/diff/80001/ash/system/session/logou...
ash/system/session/logout_confirmation_controller.cc:20: :
clock_(reinterpret_cast<base::TickClock*>(new base::DefaultTickClock)),
On 2014/03/05 18:57:20, stevenjb wrote:
> reinterpret_cast shouldn't be necessary here.

Done.

https://codereview.chromium.org/178183005/diff/80001/ash/system/session/logou...
File ash/system/session/logout_confirmation_controller_unittest.cc (right):

https://codereview.chromium.org/178183005/diff/80001/ash/system/session/logou...
ash/system/session/logout_confirmation_controller_unittest.cc:270: }
On 2014/03/05 18:57:20, stevenjb wrote:
> Can we add a test to ensure that if a user declines, and we call ConfirmLogout
> again (e.g. the timeout was triggered a second time), it works as expected?
(The
> logic looked OK to me, but I wasn't 100% certain).

Done.

[stevenjb, 2014-03-05 22:37:50]

lgtm

[bartfab (slow), 2014-03-06 00:57:32]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-03-06 00:59:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/178183005/100001

[commit-bot: I haz the power, 2014-03-06 02:54:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/178183005/100001

[commit-bot: I haz the power, 2014-03-06 03:18:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/178183005/100001

[commit-bot: I haz the power, 2014-03-06 11:41:42]

Change committed as 255316