Introduce ash mojo interface for lock screen action handlers

ash/tray_action_handler/tray_action_handler_controller_unittest.cc

Index: ash/tray_action_handler/tray_action_handler_controller_unittest.cc
diff --git a/ash/tray_action_handler/tray_action_handler_controller_unittest.cc b/ash/tray_action_handler/tray_action_handler_controller_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..73e13ef64e0a93f4a233dace5c3267810f4d31fb
--- /dev/null
+++ b/ash/tray_action_handler/tray_action_handler_controller_unittest.cc
@@ -0,0 +1,200 @@
+// Copyright 2017 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 <memory>
+#include <vector>
+
+#include "ash/shell.h"
+#include "ash/test/ash_test_base.h"
+#include "ash/tray_action_handler/tray_action_handler_controller.h"

[James Cook, 2017/05/03 23:31:51]

nit: put this include at the top

[tbarzic, 2017/05/04 20:58:30]

Done.

+#include "ash/tray_action_handler/tray_action_handler_observer.h"
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+
+namespace ash {
+
+namespace {
+
+class ScopedTestStateObserver : public TrayActionHandlerObserver {
+ public:
+  explicit ScopedTestStateObserver(TrayActionHandlerController* controller)
+      : state_controller_(controller) {
+    state_controller_->AddObserver(this);
+  }
+
+  ~ScopedTestStateObserver() override {
+    state_controller_->RemoveObserver(this);
+  }
+
+  void OnActionStateChanged(mojom::TrayActionHandlerAction action,
+                            mojom::TrayActionHandlerState state) override {
+    ASSERT_EQ(mojom::TrayActionHandlerAction::kNewLockScreenNote, action);
+
+    observed_states_.push_back(state);
+  }
+
+  const std::vector<mojom::TrayActionHandlerState>& observed_states() const {
+    return observed_states_;
+  }
+
+  void ClearObservedStates() { observed_states_.clear(); }
+
+ private:
+  TrayActionHandlerController* state_controller_;
+
+  std::vector<mojom::TrayActionHandlerState> observed_states_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedTestStateObserver);
+};
+
+class ScopedTestTrayActionHandler : public mojom::TrayActionHandlerClient {

[James Cook, 2017/05/03 23:31:51]

nit: I think the tests would be easier to understand if they manually called a
SetClient() method. You could also call this TestTrayActionHandlerClient if you
want a shorter name.

For example, this class could expose a CreateInterfacePtrAndBind() method and
the test code could call
controller->SetClient(action_handler_client.CreateInterfacePtrAndBind()) and
controller->SetClient(nullptr)

Or if that doesn't seem clearer, just rename this to
TestTrayAction(Handler)Client to emphasize it is setting itself as the client.

[tbarzic, 2017/05/04 20:58:31]

Done.

+ public:
+  explicit ScopedTestTrayActionHandler(TrayActionHandlerController* controller)
+      : state_controller_(controller), binding_(this) {
+    state_controller_->SetClient(binding_.CreateInterfacePtrAndBind());
+  }
+
+  ~ScopedTestTrayActionHandler() override {
+    state_controller_->SetClient(nullptr);
+  }
+
+  void RequestHandleAction(
+      ash::mojom::TrayActionHandlerAction action) override {
+    ASSERT_EQ(mojom::TrayActionHandlerAction::kNewLockScreenNote, action);
+
+    action_requests_count_++;
+  }
+
+  int action_requests_count() const { return action_requests_count_; }
+
+  void reset_action_requests_count() { action_requests_count_ = 0; }
+
+ private:
+  TrayActionHandlerController* state_controller_;
+  mojo::Binding<ash::mojom::TrayActionHandlerClient> binding_;
+
+  int action_requests_count_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedTestTrayActionHandler);
+};
+
+class TrayActionHandlerControllerTest : public test::AshTestBase {

[James Cook, 2017/05/03 23:31:51]

optional: "using TrayActionHandlerControllerTest = test::AshTestBase" and make
GetController() a utility method, or just inline it.

[tbarzic, 2017/05/04 20:58:30]

Done.

+ public:
+  TrayActionHandlerControllerTest() {}

[James Cook, 2017/05/03 23:31:51]

optional nit: = default instead

[tbarzic, 2017/05/04 20:58:30]

Done.

+  ~TrayActionHandlerControllerTest() override {}
+
+  TrayActionHandlerController* GetController() {
+    return Shell::Get()->tray_action_handler_controller();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TrayActionHandlerControllerTest);
+};
+
+}  // namespace

[James Cook, 2017/05/03 23:31:51]

nit: can this go at the bottom (put all the test code in the anonymous
namespace)?

[tbarzic, 2017/05/04 20:58:30]

I vaguely remember a chromium-dev thread that suggested against
anonymous-namespacing TEST_Fs (I think in order to better cope with test name
conflicts). Though, I don't think that was a particularly strong guideline, so I
can move it down, if you'd prefer it that way.

+
+TEST_F(TrayActionHandlerControllerTest, NoTrayActionHandler) {
+  TrayActionHandlerController* controller = GetController();
+  ScopedTestStateObserver observer(controller);
+
+  EXPECT_EQ(
+      mojom::TrayActionHandlerState::kNotSupported,

[James Cook, 2017/05/03 23:31:51]

optional: I think you may be able to do "using mojom::TrayActionHandlerState;"
at file scope to make these names shorter. mojoms generate enum class.

[tbarzic, 2017/05/04 20:58:30]

Done.

+      controller->GetState(mojom::TrayActionHandlerAction::kNewLockScreenNote));
+
+  controller->UpdateActionHandlerState(
+      mojom::TrayActionHandlerAction::kNewLockScreenNote,
+      mojom::TrayActionHandlerState::kAvailable);
+
+  // The effective state should be |kNotSupported| as long as an action handler
+  // is not set.
+  EXPECT_EQ(
+      mojom::TrayActionHandlerState::kNotSupported,
+      controller->GetState(mojom::TrayActionHandlerAction::kNewLockScreenNote));
+  EXPECT_EQ(0u, observer.observed_states().size());
+
+  // Adding action handler should update the controller state.
+  std::unique_ptr<ScopedTestTrayActionHandler> scoped_action_handler =
+      base::MakeUnique<ScopedTestTrayActionHandler>(controller);
+
+  EXPECT_EQ(
+      mojom::TrayActionHandlerState::kAvailable,
+      controller->GetState(mojom::TrayActionHandlerAction::kNewLockScreenNote));
+  // Effective state changed, so observer should be notified of it.
+  ASSERT_EQ(1u, observer.observed_states().size());
+  EXPECT_EQ(mojom::TrayActionHandlerState::kAvailable,
+            observer.observed_states()[0]);
+  observer.ClearObservedStates();
+
+  // When action handler is reset, effective state should go back to
+  // |kNotSupported|.
+  scoped_action_handler.reset();
+
+  EXPECT_EQ(
+      mojom::TrayActionHandlerState::kNotSupported,
+      controller->GetState(mojom::TrayActionHandlerAction::kNewLockScreenNote));
+  // Effective state changed, so observer should be notified of it.
+  ASSERT_EQ(1u, observer.observed_states().size());
+  EXPECT_EQ(mojom::TrayActionHandlerState::kNotSupported,
+            observer.observed_states()[0]);
+  observer.ClearObservedStates();
+}
+
+TEST_F(TrayActionHandlerControllerTest, StateChangesWithHandlerSet) {
+  TrayActionHandlerController* controller = GetController();
+
+  ScopedTestStateObserver observer(controller);
+  ScopedTestTrayActionHandler action_handler(controller);
+
+  controller->UpdateActionHandlerState(
+      mojom::TrayActionHandlerAction::kNewLockScreenNote,
+      mojom::TrayActionHandlerState::kAvailable);
+  EXPECT_EQ(
+      mojom::TrayActionHandlerState::kAvailable,
+      controller->GetState(mojom::TrayActionHandlerAction::kNewLockScreenNote));
+  ASSERT_EQ(1u, observer.observed_states().size());
+  EXPECT_EQ(mojom::TrayActionHandlerState::kAvailable,
+            observer.observed_states()[0]);
+  observer.ClearObservedStates();
+
+  controller->UpdateActionHandlerState(
+      mojom::TrayActionHandlerAction::kNewLockScreenNote,
+      mojom::TrayActionHandlerState::kAvailable);
+  EXPECT_EQ(
+      mojom::TrayActionHandlerState::kAvailable,
+      controller->GetState(mojom::TrayActionHandlerAction::kNewLockScreenNote));
+  // No real state change, so the observer should not be notified.
+  ASSERT_EQ(0u, observer.observed_states().size());
+
+  controller->UpdateActionHandlerState(
+      mojom::TrayActionHandlerAction::kNewLockScreenNote,
+      mojom::TrayActionHandlerState::kActive);
+  EXPECT_EQ(
+      mojom::TrayActionHandlerState::kActive,
+      controller->GetState(mojom::TrayActionHandlerAction::kNewLockScreenNote));
+  ASSERT_EQ(1u, observer.observed_states().size());
+  EXPECT_EQ(mojom::TrayActionHandlerState::kActive,
+            observer.observed_states()[0]);
+  observer.ClearObservedStates();
+}

[James Cook, 2017/05/03 23:31:51]

I suggest one test that moves through the expected state sequence (not
available, available, launching, active, background) then another small test
that checks that observers are not notified for a duplicate transition.

[tbarzic, 2017/05/04 20:58:30]

Done.

+
+TEST_F(TrayActionHandlerControllerTest, RequestAction) {
+  TrayActionHandlerController* controller = GetController();
+
+  ScopedTestTrayActionHandler action_handler(controller);
+
+  EXPECT_EQ(0, action_handler.action_requests_count());
+  controller->RequestHandleAction(
+      mojom::TrayActionHandlerAction::kNewLockScreenNote);
+  RunAllPendingInMessageLoop();
+  EXPECT_EQ(1, action_handler.action_requests_count());
+}
+
+// Tests that there is no crash if handler is not set.
+TEST_F(TrayActionHandlerControllerTest, RequestActionWithNoHandler) {
+  TrayActionHandlerController* controller = GetController();
+  controller->RequestHandleAction(
+      mojom::TrayActionHandlerAction::kNewLockScreenNote);
+}
+
+}  // namespace ash

[James Cook, 2017/05/03 23:31:51]

Thanks for writing a nice test suite!