Implements the dialog view for logout button tray in public sessions.

ash/system/logout_button/logout_button_tray.cc

Index: ash/system/logout_button/logout_button_tray.cc
diff --git a/ash/system/logout_button/logout_button_tray.cc b/ash/system/logout_button/logout_button_tray.cc
index c06073040c5c26822b641ecb479f29c65cae088f..d4a1994e9490e1f0fd0b98ddfd3d3b06c9a254ee 100644
--- a/ash/system/logout_button/logout_button_tray.cc
+++ b/ash/system/logout_button/logout_button_tray.cc
@@ -24,7 +24,6 @@
 #include "ui/views/painter.h"
 
 namespace ash {
-
 namespace internal {
 
 namespace {
@@ -94,20 +93,47 @@ LogoutButton::LogoutButton(views::ButtonListener* listener)
 LogoutButton::~LogoutButton() {
 }
 
-LogoutButtonTray::LogoutButtonTray(StatusAreaWidget* status_area_widget)
+LogoutButtonTray::LogoutButtonTray(StatusAreaWidget* status_area_widget,

[bartfab (slow), 2013/12/10 12:41:16]

Nit: Indentation: All arguments must be aligned.

[binjin, 2013/12/12 13:56:55]

Done.

+    LogoutConfirmationDialogView::LogoutConfirmationDelegate* delegate)
     : TrayBackgroundView(status_area_widget),
-      button_(NULL),

[bartfab (slow), 2013/12/10 12:41:16]

Nit: Do not remove this. You will be setting |button_| a few lines down. But it
should have a well-defined value until then as well.

[binjin, 2013/12/12 13:56:55]

Done.

       login_status_(user::LOGGED_IN_NONE),
-      show_logout_button_in_tray_(false) {
+      show_logout_button_in_tray_(false),
+      weak_factory_(this) {
+  if (!delegate) {
+    confirmation_delegate_.reset(
+        new LogoutConfirmationDialogView::LogoutConfirmationDelegate());

[bartfab (slow), 2013/12/10 12:41:16]

Nit: No need for ().

[binjin, 2013/12/12 13:56:55]

Done.

+  } else

[bartfab (slow), 2013/12/10 12:41:16]

Nit 1: Style guide: If the if part has curly braces, the else part must have
curly braces as well.

Nit 2: Why not set confirmation_delegate_ in the initializer list, using a
ternary operator, such as:

confirmation_delegate_(
    delegate ? delegate
             : new LogoutConfirmationDialogView::LogoutConfirmationDelegate)

[binjin, 2013/12/12 13:56:55]

Done.

+    confirmation_delegate_.reset(delegate);
   button_ = new LogoutButton(this);
   tray_container()->AddChildView(button_);
   tray_container()->set_border(NULL);
-  Shell::GetInstance()->system_tray_notifier()->AddLogoutButtonObserver(this);
+  // For testing purpose.

[bartfab (slow), 2013/12/10 12:41:16]

Nit: "For testing purpose" is slightly misleading. The AddLogoutButtonObserver()
call is not for testing purposes only. It is the check that is necessary for
tests. How about a comment like "The Shell may not exist in some unit tests."
instead?

[binjin, 2013/12/12 13:56:55]

Done.

+  if (Shell::HasInstance())
+    Shell::GetInstance()->system_tray_notifier()->AddLogoutButtonObserver(this);
 }
 
 LogoutButtonTray::~LogoutButtonTray() {
-  Shell::GetInstance()->system_tray_notifier()->
-      RemoveLogoutButtonObserver(this);
+  EnsureConfirmationDialogIsClosed();
+  if (Shell::HasInstance())

[bartfab (slow), 2013/12/10 12:41:16]

Nit: Style guide: Multi-line conditionals require curly braces.

[binjin, 2013/12/12 13:56:55]

Done.

+    Shell::GetInstance()->system_tray_notifier()->
+        RemoveLogoutButtonObserver(this);
+}
+
+void LogoutButtonTray::EnsureConfirmationDialogIsShowing() {
+  if (!confirmation_dialog_) {
+    confirmation_dialog_.reset(new LogoutConfirmationDialogView(
+          weak_factory_.GetWeakPtr(), confirmation_delegate_.get()));
+    confirmation_dialog_->Show(dialog_duration_);
+  }
+}
+
+void LogoutButtonTray::EnsureConfirmationDialogIsClosed() {
+  if (confirmation_dialog_ && Shell::HasInstance())
+    confirmation_dialog_->GetWidget()->Close();

[bartfab (slow), 2013/12/10 12:41:16]

1) After this method has run, the dialog is closed but |confirmation_dialog_| is
still non-NULL. This means that IsConfirmationDialogShowing() will keep
returning |true| (which is wrong) and EnsureConfirmationDialogIsShowing() would
think that the dialog is still showing (which is wrong).

Why not release the pointer here, i.e.:

if (confirmation_dialog_ && Shell::HasInstance())
  confirmation_dialog_->release()->GetWidget()->Close();

That way, |confirmation_dialog_| becomes NULL but the dialog is *not* actually
deleted. The dialog now manages its own lifetime and will destroy itself
eventually. This would also completely remove the need for the
DeleteConfirmationDialog() method, for passing |this| to the dialog, for the
weak_ptr_factory_... It would simplify the code a lot.

2) This method does nothing when Shell::HasInstance() is |false|. There is no
widget to close but there still is a |confirmation_dialog_| that should be
deleted.

[binjin, 2013/12/12 13:56:55]

Done.

+}
+
+bool LogoutButtonTray::IsConfirmationDialogShowing() {
+  return confirmation_dialog_;
 }
 
 void LogoutButtonTray::SetShelfAlignment(ShelfAlignment alignment) {
@@ -132,10 +158,20 @@ void LogoutButtonTray::OnShowLogoutButtonInTrayChanged(bool show) {
   UpdateVisibility();
 }
 
+void LogoutButtonTray::OnLogoutDialogDurationChanged(base::TimeDelta duration) {
+  dialog_duration_ = duration;
+  if (confirmation_dialog_)
+    confirmation_dialog_->UpdateDialogDuration(dialog_duration_);
+}
+
 void LogoutButtonTray::ButtonPressed(views::Button* sender,
                                      const ui::Event& event) {
   DCHECK_EQ(sender, button_);
-  Shell::GetInstance()->system_tray_delegate()->SignOut();
+  // Sign out immediately if kLogoutDialogDurationMs is non-positive.

[bartfab (slow), 2013/12/10 12:41:16]

Nit: Do not refer to the pref name here. Ash knows nothing about Chrome prefs.
Just say "if |duration_| is non-positive."

[binjin, 2013/12/12 13:56:55]

Done.

+  if (dialog_duration_ <= base::TimeDelta::FromSeconds(0))

[bartfab (slow), 2013/12/10 12:41:16]

Nit: The default constructor base::TimeDelta() produces a zero-length time delta
for you.

[binjin, 2013/12/12 13:56:55]

Done.

+    confirmation_delegate_->LogoutCurrentUser();
+  else
+    EnsureConfirmationDialogIsShowing();
 }
 
 void LogoutButtonTray::UpdateAfterLoginStatusChange(
@@ -148,10 +184,25 @@ void LogoutButtonTray::UpdateAfterLoginStatusChange(
   UpdateVisibility();
 }
 
+void LogoutButtonTray::DeleteConfirmationDialog() {
+  confirmation_dialog_.reset();
+}
+
+LogoutConfirmationDialogView *LogoutButtonTray::GetConfirmationDialogForTest() {

[bartfab (slow), 2013/12/10 12:41:16]

Nit: Style guide: The * goes on the type.

[binjin, 2013/12/12 13:56:55]

Done.

+  return confirmation_dialog_.get();
+}
+
+LogoutConfirmationDialogView::LogoutConfirmationDelegate*
+LogoutButtonTray::GetConfirmationDelegateForTest() {
+  return confirmation_delegate_.get();
+}
+
 void LogoutButtonTray::UpdateVisibility() {
   SetVisible(show_logout_button_in_tray_ &&
              login_status_ != user::LOGGED_IN_NONE &&
              login_status_ != user::LOGGED_IN_LOCKED);
+  if (!show_logout_button_in_tray_)
+    EnsureConfirmationDialogIsClosed();
 }
 
 }  // namespace internal