Introduce ash mojo interface for lock screen action handlers

chrome/browser/chromeos/lock_screen_apps/state_controller.cc

 // 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 "chrome/browser/chromeos/lock_screen_apps/state_controller.h"
 
-#include "base/bind.h"
+#include <utility>
+
+#include "ash/public/interfaces/constants.mojom.h"
 #include "base/command_line.h"
-#include "base/location.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chromeos/chromeos_switches.h"
+#include "content/public/common/service_manager_connection.h"
+#include "services/service_manager/public/cpp/connector.h"
+
+using ash::mojom::TrayActionState;
 
 namespace lock_screen_apps {
 
 namespace {
 
-base::LazyInstance<StateController>::Leaky g_instance =
-    LAZY_INSTANCE_INITIALIZER;
+StateController* g_instance = nullptr;

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

The way you're handling instance creation and singleton management here seems a
little unusual and I'm finding it hard to follow. I recommend copying the
pattern xiyuan@ used in SessionControllerClient. Basically:

public constructor
in constuctor, DCHECK(IsEnabled()), DCHECK(!g_instance) and assign g_instance

public destructor
DCHECK_EQ(this, g_instance)

public Init method

Allow tests to reset the tray_action_ptr_ between construction and calling
Init(), either by the tests being a friend or by providing a SetFooForTesting()
method.

public Get() method that does DCHECK(g_instance)

Then you don't need special Create/CreateForTesting methods, and you don't need
to check IsEnabled() so many places.

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

This pretty much what I'm doing now, but I have the creation/initialization
logic here, in Create methods, but I can move it to call sites.

 
 }  // namespace
 
 // static
+bool StateController::IsEnabled() {
+  return base::CommandLine::ForCurrentProcess()->HasSwitch(
+      chromeos::switches::kEnableLockScreenApps);
+}
+
+// static
 StateController* StateController::Get() {
-  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
-          chromeos::switches::kEnableLockScreenApps)) {
-    return nullptr;
-  }
-  return g_instance.Pointer();
+  CHECK(g_instance || !IsEnabled());

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

Do all these really need to be CHECK or would DCHECK be enough?

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

I guess it could be dcheck

+  return g_instance;
+}
+
+// static
+std::unique_ptr<StateController> StateController::Create() {
+  CHECK(!g_instance);
+  CHECK(IsEnabled());
+
+  std::unique_ptr<StateController> instance(new StateController);
+  instance->RegisterAsAshTrayActionClient();
+  return instance;
+}
+
+// static
+std::unique_ptr<StateController> StateController::CreateForTesting(
+    ash::mojom::TrayActionPtr tray_action_ptr) {
+  DCHECK(tray_action_ptr);
+  CHECK(!g_instance);
+  CHECK(IsEnabled());
+
+  std::unique_ptr<StateController> instance(new StateController);
+  instance->tray_action_ptr_ = std::move(tray_action_ptr);
+  instance->RegisterAsAshTrayActionClient();
+  instance->tray_action_ptr_.FlushForTesting();
+  return instance;
+}
+
+StateController::~StateController() {
+  CHECK(g_instance);
+  g_instance = nullptr;

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

Nice that you reset the pointer at the start of the destructor and not at the
end! I've been burned by people doing it the other way around.

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

Acknowledged.

+  tray_action_ptr_->SetClient(nullptr, TrayActionState::kNotSupported);
 }
 
 void StateController::AddObserver(StateObserver* observer) {
   observers_.AddObserver(observer);
 }
 
 void StateController::RemoveObserver(StateObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
-ActionState StateController::GetActionState(Action action) const {
-  DCHECK_EQ(Action::kNewNote, action);
-  return new_note_state_;
+TrayActionState StateController::GetLockScreenNoteState() const {
+  return lock_screen_note_state_;
 }
 
-bool StateController::HandleAction(Action action) {
-  DCHECK_EQ(Action::kNewNote, action);
-
-  if (new_note_state_ != ActionState::kAvailable &&
-      new_note_state_ != ActionState::kHidden) {
-    return false;
+void StateController::RequestNewLockScreenNote() {
+  if (lock_screen_note_state_ != TrayActionState::kAvailable &&
+      lock_screen_note_state_ != TrayActionState::kBackground) {
+    return;
   }
 
-  // TODO(tbarzic): Implement this.
-  NOTIMPLEMENTED();
+  // TODO(tbarzic): Implement this properly.
+  UpdateLockScreenNoteState(TrayActionState::kActive);
+}
+
+void StateController::MoveToBackground() {
+  UpdateLockScreenNoteState(TrayActionState::kBackground);
+}
+
+void StateController::SetLockScreenNoteStateForTesting(
+    ash::mojom::TrayActionState state) {
+  lock_screen_note_state_ = state;
+}
+
+StateController::StateController() : binding_(this) {
+  g_instance = this;
+}
+
+void StateController::RegisterAsAshTrayActionClient() {

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

This could become the Init() method.

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

Done.

+  // The tray action ptr might be set previously if the client was being created
+  // for testing.
+  if (!tray_action_ptr_) {
+    service_manager::Connector* connector =
+        content::ServiceManagerConnection::GetForProcess()->GetConnector();
+    connector->BindInterface(ash::mojom::kServiceName, &tray_action_ptr_);
+  }
+  tray_action_ptr_->SetClient(binding_.CreateInterfacePtrAndBind(),
+                              lock_screen_note_state_);
+}
+
+bool StateController::UpdateLockScreenNoteState(TrayActionState state) {
+  const TrayActionState old_state = GetLockScreenNoteState();
+  if (old_state == state)
+    return false;
+
+  // Action state can be moved to background only if the action is currently
+  // active.
+  if (state == TrayActionState::kBackground &&
+      old_state != TrayActionState::kActive)
+    return false;
+
+  lock_screen_note_state_ = state;
+  NotifyLockScreenNoteStateChanged();
   return true;
 }
 
-void StateController::MoveToBackground() {
-  UpdateActionState(Action::kNewNote, ActionState::kHidden);
-}
+void StateController::NotifyLockScreenNoteStateChanged() {
+  for (auto& observer : observers_)
+    observer.OnLockScreenNoteStateChanged(lock_screen_note_state_);
 
-StateController::StateController() {}
-
-StateController::~StateController() {}
-
-bool StateController::UpdateActionState(Action action,
-                                        ActionState action_state) {
-  DCHECK_EQ(Action::kNewNote, action);
-
-  const ActionState old_state = GetActionState(action);
-  if (old_state == action_state)
-    return false;
-
-  if (action_state == ActionState::kHidden && old_state != ActionState::kActive)
-    return false;
-
-  new_note_state_ = action_state;
-  NotifyStateChanged(Action::kNewNote);
-  return true;
-}
-
-void StateController::NotifyStateChanged(Action action) {
-  DCHECK_EQ(Action::kNewNote, action);
-
-  for (auto& observer : observers_)
-    observer.OnLockScreenAppsStateChanged(Action::kNewNote, new_note_state_);
+  tray_action_ptr_->UpdateLockScreenNoteState(lock_screen_note_state_);
 }
 
 }  // namespace lock_screen_apps