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

ash/system/logout_button/logout_button_tray_unittest.cc

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/system/logout_button/logout_button_tray.h"
+
+#include "ash/system/logout_button/logout_button_observer.h"
+#include "ash/system/logout_button/logout_confirmation_dialog_view.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/single_thread_task_runner.h"
+#include "base/thread_task_runner_handle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace ash {
+namespace internal {
+
+// A SingleThreadTaskRunner that mocks the current time and allows it to be
+// fast-forwarded.
+class MockTimeSingleThreadTaskRunner : public base::SingleThreadTaskRunner {

[bartfab (slow), 2013/12/03 19:46:04]

This entire class is a copy & paste from another place. Please move it to a
shared place instead.

[binjin, 2013/12/04 10:47:03]

Can you suggest a proper place to move the shared class to? 

[bartfab (slow), 2013/12/04 12:51:56]

It should probably move to a file ending in test_util.h/test_util.cc. Since this
is used in different places throughout the chromeos code,
chrome/browser/chromeos/test_util.h would be a good start. But that is a generic
file name, so some reviewer may object and ask you to move it into a
subdirectory. This is easy to do, so I suggest you start with that name and see
how it goes.

[binjin, 2013/12/05 10:08:05]

I'm not sure if it's okay to use chrome/* classes from ash/*, but I think the
better idea is implementing a priority_queue(instead of deque) version of
TestSimpleTaskRunner (base/test/test_simple_task_runner.h), like
TestForwardableTaskRunner or TestOrderedTaskRunner, upon the existing
TestPendingTask(base/test/test_pending_task.h). 

On 2013/12/04 12:51:56, bartfab wrote:

[bartfab (slow), 2013/12/06 13:50:04]

As discussed offline, I agree that having this code copy & pasted is probably
our best solution for now. It is needed by tests under chrome/ and tests under
ash/ but is not generic enough to warrant placing it in a shared place like
base/. Hence, there will be two implementations of
MockTimeSingleThreadTaskRunner, one here and one in chrome/.

Feel free to reimplement the MockTimeSingleThreadTaskRunner here in a nicer way.
What you copy & pasted from the tests I wrote works but if you want to improve
it, by all means, go ahead.

+ public:
+  MockTimeSingleThreadTaskRunner();
+
+  // base::SingleThreadTaskRunner:
+  virtual bool RunsTasksOnCurrentThread() const OVERRIDE;
+  virtual bool PostDelayedTask(const tracked_objects::Location& from_here,
+                               const base::Closure& task,
+                               base::TimeDelta delay) OVERRIDE;
+  virtual bool PostNonNestableDelayedTask(
+      const tracked_objects::Location& from_here,
+      const base::Closure& task,
+      base::TimeDelta delay) OVERRIDE;
+
+  const base::TimeTicks& GetCurrentTime() const;
+
+  void FastForwardBy(int64 milliseconds);
+  void FastForwardUntilNoTasksRemain();
+
+ private:
+  // Strict weak temporal ordering of tasks.
+  class TemporalOrder {
+   public:
+    bool operator()(
+        const std::pair<base::TimeTicks, base::Closure>& first_task,
+        const std::pair<base::TimeTicks, base::Closure>& second_task) const;
+  };
+
+  virtual ~MockTimeSingleThreadTaskRunner();
+
+  base::TimeTicks now_;
+  std::priority_queue<std::pair<base::TimeTicks, base::Closure>,
+                      std::vector<std::pair<base::TimeTicks, base::Closure> >,
+                      TemporalOrder> tasks_;
+};
+
+class MockLogoutConfirmationSettingsProvider
+    : public LogoutConfirmationSettingsProvider {

[bartfab (slow), 2013/12/03 19:46:04]

Nit: s/SettingsProvider/Delegate/. This name is more appropriate because the
helper class not only supplies settings but also handles commands sent to it
(such as LogoutCurrentUser).

[binjin, 2013/12/05 10:08:05]

Done.

+ public:
+  MockLogoutConfirmationSettingsProvider(

[bartfab (slow), 2013/12/03 19:46:04]

Single-argument constructors should be marked explicit.

[binjin, 2013/12/04 10:47:03]

Done.

+      MockTimeSingleThreadTaskRunner* runner);
+
+  // LogoutConfirmationSettingsProvider

[bartfab (slow), 2013/12/03 19:46:04]

Nit: Add a colon at the end of the line.

[binjin, 2013/12/04 10:47:03]

Done.

+  virtual void LogoutCurrentUser(LogoutConfirmationDialogView*) OVERRIDE;

[bartfab (slow), 2013/12/03 19:46:04]

Nit: Add argument name.

[binjin, 2013/12/04 10:47:03]

Done.

+  virtual base::TimeTicks GetCurrentTime() OVERRIDE;
+
+  void SetLogoutCalled(bool called);

[bartfab (slow), 2013/12/03 19:46:04]

Nit: This is never used.

[binjin, 2013/12/04 10:47:03]

Done.

+  bool IsLogoutCalled();

[bartfab (slow), 2013/12/03 19:46:04]

Nit 1: WasLogoutCalled() would be more correct.
Nit 2: const

[binjin, 2013/12/04 10:47:03]

Done.

+
+ private:
+  bool logout_called_;
+
+  MockTimeSingleThreadTaskRunner* runner_;
+};

[bartfab (slow), 2013/12/03 19:46:04]

Nit: Add DISALLOW_COPY_AND_ASSIGN(MockLogoutConfirmationSettingsProvider);

[binjin, 2013/12/04 10:47:03]

Done.

+
+MockTimeSingleThreadTaskRunner::MockTimeSingleThreadTaskRunner() {
+}
+
+bool MockTimeSingleThreadTaskRunner::RunsTasksOnCurrentThread() const {
+  return true;
+}
+
+bool MockTimeSingleThreadTaskRunner::PostDelayedTask(
+    const tracked_objects::Location& from_here,
+    const base::Closure& task,
+    base::TimeDelta delay) {
+  tasks_.push(std::pair<base::TimeTicks, base::Closure>(now_ + delay, task));
+  return true;
+}
+
+bool MockTimeSingleThreadTaskRunner::PostNonNestableDelayedTask(
+    const tracked_objects::Location& from_here,
+    const base::Closure& task,
+    base::TimeDelta delay) {
+  NOTREACHED();
+  return false;
+}
+
+const base::TimeTicks& MockTimeSingleThreadTaskRunner::GetCurrentTime() const {
+  return now_;
+}
+
+void MockTimeSingleThreadTaskRunner::FastForwardBy(int64 delta) {
+  const base::TimeTicks latest =
+      now_ + base::TimeDelta::FromMilliseconds(delta);
+  while (!tasks_.empty() && tasks_.top().first <= latest) {
+    now_ = tasks_.top().first;
+    base::Closure task = tasks_.top().second;
+    tasks_.pop();
+    task.Run();
+  }
+  now_ = latest;
+}
+
+void MockTimeSingleThreadTaskRunner::FastForwardUntilNoTasksRemain() {
+  while (!tasks_.empty()) {
+    now_ = tasks_.top().first;
+    base::Closure task = tasks_.top().second;
+    tasks_.pop();
+    task.Run();
+  }
+}
+
+bool MockTimeSingleThreadTaskRunner::TemporalOrder::operator()(
+    const std::pair<base::TimeTicks, base::Closure>& first_task,
+    const std::pair<base::TimeTicks, base::Closure>& second_task) const {
+  return first_task.first >= second_task.first;
+}
+
+MockTimeSingleThreadTaskRunner::~MockTimeSingleThreadTaskRunner() {
+}
+
+MockLogoutConfirmationSettingsProvider::MockLogoutConfirmationSettingsProvider(
+    MockTimeSingleThreadTaskRunner* runner) {
+  runner_ = runner;
+  logout_called_ = false;
+}
+
+void MockLogoutConfirmationSettingsProvider::LogoutCurrentUser(
+    LogoutConfirmationDialogView* dialog) {
+  logout_called_ = true;
+  if (dialog)
+    dialog->DeleteDelegate();
+}
+
+base::TimeTicks MockLogoutConfirmationSettingsProvider::GetCurrentTime() {
+  return runner_->GetCurrentTime();
+}
+
+void MockLogoutConfirmationSettingsProvider::SetLogoutCalled(bool called) {
+  logout_called_ = called;
+}
+
+bool MockLogoutConfirmationSettingsProvider::IsLogoutCalled() {
+  return logout_called_;
+}
+
+class LogoutConfirmationDialogTest : public testing::Test {
+ public:
+  LogoutConfirmationDialogTest() {}

[bartfab (slow), 2013/12/03 19:46:04]

Nit: You should be consistent with your inlining style. Since you did not inline
any other methods, the constructor and destructor here should not be inlined
either.

[binjin, 2013/12/04 10:47:03]

Done.

+  virtual ~LogoutConfirmationDialogTest() {}
+
+  // testing::Test

[bartfab (slow), 2013/12/03 19:46:04]

Nit: Add a colon at the end of the line.

[binjin, 2013/12/04 10:47:03]

Done.

+  virtual void SetUp() OVERRIDE;
+  virtual void TearDown() OVERRIDE;
+
+  void ChangeDialogDuration(int duration_ms);

[bartfab (slow), 2013/12/03 19:46:04]

Nit: Make the argument a base::TimeDelta.

[binjin, 2013/12/05 10:08:05]

Done.

+
+ protected:
+  scoped_ptr<LogoutButtonTray> tray_;

[bartfab (slow), 2013/12/03 19:46:04]

Nit: s/tray_/button_/ or s/tray_/logout_button_/ would be more appropriate.

[binjin, 2013/12/04 10:47:03]

Done.

+  scoped_refptr<MockTimeSingleThreadTaskRunner> runner_;
+  scoped_ptr<MockLogoutConfirmationSettingsProvider> provider_;

[bartfab (slow), 2013/12/03 19:46:04]

Nit: In line with renaming the class, this member should be renamed to
|delegate_|.

[binjin, 2013/12/05 10:08:05]

Done.

+};
+
+void LogoutConfirmationDialogTest::SetUp() {
+  runner_ = new MockTimeSingleThreadTaskRunner;
+  tray_.reset(new LogoutButtonTray(NULL));
+  ChangeDialogDuration(20 * 1000); // default value, 20 seconds.
+  provider_.reset(new MockLogoutConfirmationSettingsProvider(runner_.get()));
+  LogoutConfirmationDialogView::SetProvider(provider_.get());
+}
+
+void LogoutConfirmationDialogTest::TearDown() {
+}
+
+void LogoutConfirmationDialogTest::ChangeDialogDuration(int duration_ms) {
+  tray_->OnLogoutDialogDurationChanged(
+      base::TimeDelta::FromMilliseconds(duration_ms));
+}
+
+TEST_F(LogoutConfirmationDialogTest, NoClickWithDefaultValue) {
+  base::ThreadTaskRunnerHandle runner_handle(runner_);

[bartfab (slow), 2013/12/03 19:46:04]

Nit: Could this be handled by the LogoutConfirmationDialogTest class? As it
stands, you have to duplicate it in every test case.

[binjin, 2013/12/05 10:08:05]

Done.

+
+  EXPECT_FALSE(tray_->IsConfirmationDialogShowing());
+
+  tray_->OnUserLogoutEvent();
+
+  runner_->FastForwardBy(1 * 1000); // 1 second after start

[bartfab (slow), 2013/12/03 19:46:04]

Nit 1: Here and elsewhere: Terminate comments with a full stop.

Nit 2: "after start" of what? How about "1 second since the logout button was
pressed?"

[binjin, 2013/12/05 10:08:05]

Done.

+
+  EXPECT_TRUE(tray_->IsConfirmationDialogShowing());
+  EXPECT_FALSE(provider_->IsLogoutCalled());
+
+  runner_->FastForwardBy(18 * 1000); // 19 seconds after start

[bartfab (slow), 2013/12/03 19:46:04]

Nit: Fast-forwarding by 1s and then 18s is rather arbitrary. I presume you want
to verify that the dialog shows immediately and then want to verify that shortly
before the countdown expires, is still being seen. How about this:

runner_->FastForwardBy(0);
// Now the dialog should have popped up
runner_->FastForwardBy(19 * 1000);
// Now the dialog should still be showing
runner_->FastForwardBy(2 * 1000);
// Now the user should have been logged out.

[binjin, 2013/12/05 10:08:05]

Done.

+
+  EXPECT_TRUE(tray_->IsConfirmationDialogShowing());
+  EXPECT_FALSE(provider_->IsLogoutCalled());
+
+  runner_->FastForwardBy(2 * 1000); // 21 seconds after start
+
+  EXPECT_FALSE(tray_->IsConfirmationDialogShowing());
+  EXPECT_TRUE(provider_->IsLogoutCalled());
+}
+
+TEST_F(LogoutConfirmationDialogTest, ZeroPreferenceValue) {
+  base::ThreadTaskRunnerHandle runner_handle(runner_);
+
+  ChangeDialogDuration(0); // set preference to 0

[bartfab (slow), 2013/12/03 19:46:04]

Nit: The comment does not really add much.

[binjin, 2013/12/05 10:08:05]

Done.

+
+  EXPECT_FALSE(tray_->IsConfirmationDialogShowing());
+
+  tray_->OnUserLogoutEvent();
+
+  EXPECT_FALSE(tray_->IsConfirmationDialogShowing());
+
+  runner_->FastForwardBy(1 * 1000); // 1 second

[bartfab (slow), 2013/12/03 19:46:04]

Nit: Same as above: Why not runner_->FastForwardBy(0) to see if there is any
immediate reaction?

[binjin, 2013/12/05 10:08:05]

Done.

+
+  EXPECT_FALSE(tray_->IsConfirmationDialogShowing());
+  EXPECT_TRUE(provider_->IsLogoutCalled());
+}

[bartfab (slow), 2013/12/03 19:46:04]

Nit: Add runner_->FastForwardUntilNoTasksRemain() and verify that the the dialog
is not shown and the user is not logged out.

[binjin, 2013/12/05 10:08:05]

I didn't follow you on this.

On 2013/12/03 19:46:04, bartfab wrote:

[bartfab (slow), 2013/12/06 13:50:04]

At the end of the test, you verify that the confirmation dialog is not showing
*at this moment* and logout has not been called *at this moment*. But maybe the
dialog would show or a logout would happen a few moments later? My suggestion
was to add this:

runner_->FastForwardUntilNoTasksRemain();

EXPECT_FALSE(tray_->IsConfirmationDialogShowing());
EXPECT_TRUE(provider_->IsLogoutCalled());

This fast-forwards time once more, until absolutely all tasks that were ever
posted have been run. There are guaranteed to be zero tasks in the queue. If at
that point, no dialog is showing and no logout has happened, we know that the
user will not be logged out.

[binjin, 2013/12/09 18:50:02]

Done.

+
+TEST_F(LogoutConfirmationDialogTest, OnTheFlyDialogDurationChange) {
+  base::ThreadTaskRunnerHandle runner_handle(runner_);
+
+  ChangeDialogDuration(5 * 1000); // set dialog duration to 5 seconds

[bartfab (slow), 2013/12/03 19:46:04]

Nit: Here and elsewhere: If you change the argument to a base::TimeDelta, the
call will be very self-documenting and no explicit comment will be needed.

[binjin, 2013/12/05 10:08:05]

Done.

+
+  EXPECT_FALSE(tray_->IsConfirmationDialogShowing());
+
+  tray_->OnUserLogoutEvent();
+
+  runner_->FastForwardBy(3 * 1000); // 3 seconds after start

[bartfab (slow), 2013/12/03 19:46:04]

Nit: Same as above: Why not runner_->FastForwardBy(0) to see if there is any
immediate reaction?

[binjin, 2013/12/05 10:08:05]

Done.

+
+  EXPECT_TRUE(tray_->IsConfirmationDialogShowing());
+  EXPECT_FALSE(provider_->IsLogoutCalled());
+
+  ChangeDialogDuration(10 * 1000); // change dialog duration to 10 seconds
+
+  runner_->FastForwardBy(6 * 1000); // 9 seconds after start
+
+  EXPECT_TRUE(tray_->IsConfirmationDialogShowing());
+  EXPECT_FALSE(provider_->IsLogoutCalled());
+
+  runner_->FastForwardBy(2 * 1000); // 11 seconds after start
+
+  EXPECT_FALSE(tray_->IsConfirmationDialogShowing());
+  EXPECT_TRUE(provider_->IsLogoutCalled());
+}
+
+TEST_F(LogoutConfirmationDialogTest, UserClickedButton) {
+  base::ThreadTaskRunnerHandle runner_handle(runner_);
+
+  tray_->OnUserLogoutEvent();
+
+  runner_->FastForwardBy(3 * 1000); // 3 seconds after start

[bartfab (slow), 2013/12/03 19:46:04]

Nit: Same as above: Why not runner_->FastForwardBy(0) to see if there is any
immediate reaction?

[binjin, 2013/12/05 10:08:05]

Done.

+
+  EXPECT_FALSE(provider_->IsLogoutCalled());
+
+  tray_->GetConfirmationDialog()->Accept();
+
+  runner_->FastForwardBy(1 * 1000); // 4 seconds after start

[bartfab (slow), 2013/12/03 19:46:04]

Nit: Same as above: Why not runner_->FastForwardBy(0) to see if there is any
immediate reaction?

[binjin, 2013/12/05 10:08:05]

Done.

+
+  EXPECT_TRUE(provider_->IsLogoutCalled());
+}
+
+}  // namespace internal
+}  // namespace ash