Introduce ash mojo interface for lock screen action handlers

ash/tray_action/tray_action_unittest.cc

Index: ash/tray_action/tray_action_unittest.cc
diff --git a/ash/tray_action/tray_action_unittest.cc b/ash/tray_action/tray_action_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8e6a8704d911566dc26a73341f831b12feceee33
--- /dev/null
+++ b/ash/tray_action/tray_action_unittest.cc
@@ -0,0 +1,262 @@
+// 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 "ash/tray_action/tray_action.h"
+
+#include <memory>
+#include <vector>
+
+#include "ash/shell.h"
+#include "ash/test/ash_test_base.h"
+#include "ash/tray_action/tray_action_observer.h"
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/run_loop.h"
+
+using ash::mojom::TrayActionState;
+
+namespace ash {
+
+namespace {
+
+class ScopedTestStateObserver : public TrayActionObserver {
+ public:
+  explicit ScopedTestStateObserver(TrayAction* tray_action)
+      : tray_action_(tray_action) {
+    tray_action_->AddObserver(this);
+  }
+
+  ~ScopedTestStateObserver() override { tray_action_->RemoveObserver(this); }
+
+  void OnLockScreenNoteStateChanged(TrayActionState state) override {
+    observed_states_.push_back(state);
+  }
+
+  const std::vector<TrayActionState>& observed_states() const {
+    return observed_states_;
+  }
+
+  void ClearObservedStates() { observed_states_.clear(); }
+
+ private:
+  TrayAction* tray_action_;
+
+  std::vector<TrayActionState> observed_states_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedTestStateObserver);
+};
+
+class TestTrayActionClient : public mojom::TrayActionClient {
+ public:
+  TestTrayActionClient() : binding_(this) {}
+
+  ~TestTrayActionClient() override = default;
+
+  mojom::TrayActionClientPtr CreateInterfacePtrAndBind() {
+    return binding_.CreateInterfacePtrAndBind();
+  }
+
+  void RequestNewLockScreenNote() override { action_requests_count_++; }

[James Cook, 2017/05/05 02:55:44]

nit: "// mojom::TrayActionClient:" above

[tbarzic, 2017/05/05 04:46:27]

Done.

+
+  int action_requests_count() const { return action_requests_count_; }
+
+  void reset_action_requests_count() { action_requests_count_ = 0; }

[James Cook, 2017/05/05 02:55:45]

nit: ResetActionRequestsCount().

[tbarzic, 2017/05/05 04:46:26]

Done.

+
+ private:
+  mojo::Binding<ash::mojom::TrayActionClient> binding_;
+
+  int action_requests_count_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(TestTrayActionClient);
+};
+
+using TrayActionTest = test::AshTestBase;
+
+}  // namespace
+
+TEST_F(TrayActionTest, NoTrayAction) {
+  TrayAction* tray_action = Shell::Get()->tray_action();
+  ScopedTestStateObserver observer(tray_action);
+
+  EXPECT_EQ(TrayActionState::kNotSupported,
+            tray_action->GetLockScreenNoteState());
+
+  tray_action->UpdateLockScreenNoteState(TrayActionState::kAvailable);
+
+  // The effective state should be |kNotSupported| as long as an action handler
+  // is not set.
+  EXPECT_EQ(TrayActionState::kNotSupported,
+            tray_action->GetLockScreenNoteState());
+  EXPECT_EQ(0u, observer.observed_states().size());
+
+  // Adding action handler should update the tray_action state.
+  std::unique_ptr<TestTrayActionClient> action_client =
+      base::MakeUnique<TestTrayActionClient>();
+  tray_action->SetClient(action_client->CreateInterfacePtrAndBind(),
+                         TrayActionState::kNotSupported);
+
+  EXPECT_EQ(TrayActionState::kNotSupported,
+            tray_action->GetLockScreenNoteState());
+  // Effective state hasn't changed, so observer should be notified of it.

[James Cook, 2017/05/05 02:55:45]

nit: "should be" -> "should not be", or just remove the comment.

[tbarzic, 2017/05/05 04:46:26]

Done.

+  ASSERT_EQ(0u, observer.observed_states().size());

[James Cook, 2017/05/05 02:55:44]

EXPECT_EQ

[tbarzic, 2017/05/05 04:46:26]

Done.

+
+  tray_action->SetClient(nullptr, TrayActionState::kNotSupported);
+  action_client.reset();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(TrayActionState::kNotSupported,
+            tray_action->GetLockScreenNoteState());
+  ASSERT_EQ(0u, observer.observed_states().size());

[James Cook, 2017/05/05 02:55:45]

EXPECT_EQ

[tbarzic, 2017/05/05 04:46:26]

Done.

+}
+
+TEST_F(TrayActionTest, SettingInitialState) {
+  TrayAction* tray_action = Shell::Get()->tray_action();
+
+  ScopedTestStateObserver observer(tray_action);
+  TestTrayActionClient action_client;
+  tray_action->SetClient(action_client.CreateInterfacePtrAndBind(),
+                         TrayActionState::kAvailable);
+
+  EXPECT_EQ(TrayActionState::kAvailable, tray_action->GetLockScreenNoteState());
+  ASSERT_EQ(1u, observer.observed_states().size());
+  EXPECT_EQ(TrayActionState::kAvailable, observer.observed_states()[0]);
+  observer.ClearObservedStates();

[James Cook, 2017/05/05 02:55:45]

do you have to do this?

[tbarzic, 2017/05/05 04:46:26]

Done.

+
+  tray_action->SetClient(nullptr, TrayActionState::kNotSupported);

[James Cook, 2017/05/05 02:55:45]

do you have to do this?

[tbarzic, 2017/05/05 04:46:27]

Done.

+}
+
+TEST_F(TrayActionTest, StateChangeNotificationOnConnectionLoss) {
+  TrayAction* tray_action = Shell::Get()->tray_action();
+
+  ScopedTestStateObserver observer(tray_action);
+  std::unique_ptr<TestTrayActionClient> action_client(
+      new TestTrayActionClient());
+  tray_action->SetClient(action_client->CreateInterfacePtrAndBind(),
+                         TrayActionState::kAvailable);
+
+  EXPECT_EQ(TrayActionState::kAvailable, tray_action->GetLockScreenNoteState());
+  ASSERT_EQ(1u, observer.observed_states().size());
+  EXPECT_EQ(TrayActionState::kAvailable, observer.observed_states()[0]);
+  observer.ClearObservedStates();
+
+  action_client.reset();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(TrayActionState::kNotSupported,
+            tray_action->GetLockScreenNoteState());
+  ASSERT_EQ(1u, observer.observed_states().size());
+  EXPECT_EQ(TrayActionState::kNotSupported, observer.observed_states()[0]);
+  observer.ClearObservedStates();

[James Cook, 2017/05/05 02:55:45]

ditto

[tbarzic, 2017/05/05 04:46:26]

Done.

+
+  tray_action->SetClient(nullptr, TrayActionState::kNotSupported);

[James Cook, 2017/05/05 02:55:44]

ditto, and below

[tbarzic, 2017/05/05 04:46:26]

Done.

+}
+
+TEST_F(TrayActionTest, StateChangesWithHandlerSet) {

[James Cook, 2017/05/05 02:55:44]

This test is kinda duplicated by the NormalStateProgression and the
ObserversNotNotifiedOnDuplicateState. Maybe just delete this one?

[tbarzic, 2017/05/05 04:46:27]

Done.

+  TrayAction* tray_action = Shell::Get()->tray_action();
+
+  ScopedTestStateObserver observer(tray_action);
+  TestTrayActionClient action_client;
+  tray_action->SetClient(action_client.CreateInterfacePtrAndBind(),
+                         TrayActionState::kNotSupported);
+
+  tray_action->UpdateLockScreenNoteState(TrayActionState::kAvailable);
+  EXPECT_EQ(TrayActionState::kAvailable, tray_action->GetLockScreenNoteState());
+  ASSERT_EQ(1u, observer.observed_states().size());
+  EXPECT_EQ(TrayActionState::kAvailable, observer.observed_states()[0]);
+  observer.ClearObservedStates();
+
+  tray_action->UpdateLockScreenNoteState(TrayActionState::kAvailable);
+  EXPECT_EQ(TrayActionState::kAvailable, tray_action->GetLockScreenNoteState());
+  // No real state change, so the observer should not be notified.
+  ASSERT_EQ(0u, observer.observed_states().size());
+
+  tray_action->UpdateLockScreenNoteState(TrayActionState::kActive);
+  EXPECT_EQ(TrayActionState::kActive, tray_action->GetLockScreenNoteState());
+  ASSERT_EQ(1u, observer.observed_states().size());
+  EXPECT_EQ(TrayActionState::kActive, observer.observed_states()[0]);
+  observer.ClearObservedStates();
+
+  tray_action->SetClient(nullptr, TrayActionState::kNotSupported);
+}
+
+TEST_F(TrayActionTest, NormalStateProgression) {
+  TrayAction* tray_action = Shell::Get()->tray_action();
+
+  ScopedTestStateObserver observer(tray_action);
+  TestTrayActionClient action_client;
+  tray_action->SetClient(action_client.CreateInterfacePtrAndBind(),
+                         TrayActionState::kNotSupported);
+
+  tray_action->UpdateLockScreenNoteState(TrayActionState::kAvailable);
+  EXPECT_EQ(TrayActionState::kAvailable, tray_action->GetLockScreenNoteState());
+  ASSERT_EQ(1u, observer.observed_states().size());
+  EXPECT_EQ(TrayActionState::kAvailable, observer.observed_states()[0]);
+  observer.ClearObservedStates();
+
+  tray_action->UpdateLockScreenNoteState(TrayActionState::kLaunching);
+  EXPECT_EQ(TrayActionState::kLaunching, tray_action->GetLockScreenNoteState());
+  ASSERT_EQ(1u, observer.observed_states().size());
+  EXPECT_EQ(TrayActionState::kLaunching, observer.observed_states()[0]);
+  observer.ClearObservedStates();
+
+  tray_action->UpdateLockScreenNoteState(TrayActionState::kBackground);
+  EXPECT_EQ(TrayActionState::kBackground,
+            tray_action->GetLockScreenNoteState());
+  ASSERT_EQ(1u, observer.observed_states().size());
+  EXPECT_EQ(TrayActionState::kBackground, observer.observed_states()[0]);
+  observer.ClearObservedStates();
+
+  tray_action->UpdateLockScreenNoteState(TrayActionState::kNotSupported);
+  EXPECT_EQ(TrayActionState::kNotSupported,
+            tray_action->GetLockScreenNoteState());
+  ASSERT_EQ(1u, observer.observed_states().size());
+  EXPECT_EQ(TrayActionState::kNotSupported, observer.observed_states()[0]);
+  observer.ClearObservedStates();
+
+  tray_action->SetClient(nullptr, TrayActionState::kNotSupported);
+}
+
+TEST_F(TrayActionTest, ObserversNotNotifiedOnDuplicateState) {
+  TrayAction* tray_action = Shell::Get()->tray_action();
+
+  ScopedTestStateObserver observer(tray_action);
+  TestTrayActionClient action_client;
+  tray_action->SetClient(action_client.CreateInterfacePtrAndBind(),
+                         TrayActionState::kNotSupported);
+
+  tray_action->UpdateLockScreenNoteState(TrayActionState::kAvailable);
+  EXPECT_EQ(TrayActionState::kAvailable, tray_action->GetLockScreenNoteState());
+  ASSERT_EQ(1u, observer.observed_states().size());
+  EXPECT_EQ(TrayActionState::kAvailable, observer.observed_states()[0]);
+  observer.ClearObservedStates();
+
+  tray_action->UpdateLockScreenNoteState(TrayActionState::kAvailable);
+  EXPECT_EQ(TrayActionState::kAvailable, tray_action->GetLockScreenNoteState());
+  ASSERT_EQ(0u, observer.observed_states().size());
+
+  tray_action->SetClient(nullptr, TrayActionState::kNotSupported);
+}
+
+TEST_F(TrayActionTest, RequestAction) {
+  TrayAction* tray_action = Shell::Get()->tray_action();
+
+  TestTrayActionClient action_client;
+  tray_action->SetClient(action_client.CreateInterfacePtrAndBind(),
+                         TrayActionState::kNotSupported);
+
+  EXPECT_EQ(0, action_client.action_requests_count());
+  tray_action->RequestNewLockScreenNote();
+  RunAllPendingInMessageLoop();
+  EXPECT_EQ(1, action_client.action_requests_count());

[James Cook, 2017/05/05 02:55:45]

Why does it get a request if it set the state as "not supported"? That seems
odd.

Maybe there shouldn't be a kNotSupported state at all (and you don't set a
client if you don't support the action)? Or maybe the ash TrayAction controller
shouldn't send requests if the action isn't supported.

[tbarzic, 2017/05/05 04:46:26]

Yeah, updated the logic to check that the action can be handled before sending
the request.

In practice, client will be unset when the state is kNotSupported (when the
feature is implemented, at least).

[James Cook, 2017/05/05 17:22:46]

After more consideration, I think you should eliminate the possibility of having
two states ("no client" vs. "yes client but not available"). It seems like that
exists to support having multiple different actions, which you don't have. If
you had multiple actions, I think it would be better to have a "client" or
"observer" per action. Either way, I think it would be better to get rid of
kNotSupported. Feel free to do this in a follow-up CL.

[tbarzic, 2017/05/05 17:51:17]

OK, there are subtle differences, but probably not relevant enough at the moment
(especially not in ash) - replaced kNotSupported with kNotAvailable. If I Chrome
code ends up needing to separate the two, I can always add another state later.

+
+  tray_action->SetClient(nullptr, TrayActionState::kNotSupported);
+}
+
+// Tests that there is no crash if handler is not set.
+TEST_F(TrayActionTest, RequestActionWithNoHandler) {
+  TrayAction* tray_action = Shell::Get()->tray_action();
+  tray_action->RequestNewLockScreenNote();
+}
+
+}  // namespace ash