Add Enable methods to keyboard_controller.mojom (Take 2)

The CL:
* Introduces keyboard.mojom.KeyboardEnableState, an enum used to track
  the various states that control whether the virtual keyboard should
  be enabled. Some states enable the keyboard, others disable it.
* Moves ownership of the states and the state machine to determine when
  the keyboard should be enabled to KeyboardController.
* Adds mojo interfaces to set and clear states, get the current enabled
  state, and explicitly reload the keyboard.

Note: All mojo API calls to change an enable state will also enable or
disable the keyboard accordingly. This is consistent with existing
logic where these calls are made so should not change any behavior.

Note: Not all calls to keyboard_util.cc helpers have been replaced,
including a few in src/chrome, only places where no additional
re-factoring is required were changed.

Bug: 843332
Change-Id: Ic1741188e2e1ccafda4ad95a71063462ac7f52ff

From original CL:
TBR=tsepez@chromium.org,shend@chromium.org

Change-Id: Ic1741188e2e1ccafda4ad95a71063462ac7f52ff
Reviewed-on: https://chromium-review.googlesource.com/c/1291350
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601253}

ash/keyboard/ash_keyboard_controller.cc

@@ -13,10 +13,10 @@
 #include "ui/gfx/geometry/rect.h"
 #include "ui/keyboard/keyboard_controller.h"
 #include "ui/keyboard/keyboard_ui.h"
-#include "ui/keyboard/keyboard_util.h"
 
 using keyboard::mojom::KeyboardConfig;
 using keyboard::mojom::KeyboardConfigPtr;
+using keyboard::mojom::KeyboardEnableFlag;
 
 namespace ash {
 
@@ -41,7 +41,7 @@ void AshKeyboardController::BindRequest(
 }
 
 void AshKeyboardController::EnableKeyboard() {
-  if (!keyboard::IsKeyboardEnabled())
+  if (!keyboard_controller_->IsKeyboardEnableRequested())
     return;
 
   if (keyboard_controller_->IsEnabled()) {
@@ -103,11 +103,34 @@ void AshKeyboardController::SetKeyboardConfig(
   keyboard_controller_->UpdateKeyboardConfig(*keyboard_config);
 }
 
+void AshKeyboardController::IsKeyboardEnabled(
+    IsKeyboardEnabledCallback callback) {
+  std::move(callback).Run(keyboard_controller_->IsEnabled());
+}
+
+void AshKeyboardController::SetEnableFlag(KeyboardEnableFlag flag) {
+  bool was_enabled = keyboard_controller_->IsEnabled();
+  keyboard_controller_->SetEnableFlag(flag);
+  UpdateEnableFlag(was_enabled);
+}
+
+void AshKeyboardController::ClearEnableFlag(KeyboardEnableFlag flag) {
+  bool was_enabled = keyboard_controller_->IsEnabled();
+  keyboard_controller_->ClearEnableFlag(flag);
+  UpdateEnableFlag(was_enabled);
+}
+
+void AshKeyboardController::ReloadKeyboard() {
+  // Test IsKeyboardEnableRequested in case of an unlikely edge case where this
+  // is called while after the enable state changed to disabled (in which case
+  // we do not want to override the requested state).
+  if (keyboard_controller_->IsKeyboardEnableRequested())
+    EnableKeyboard();
+}
+
 void AshKeyboardController::OnSessionStateChanged(
     session_manager::SessionState state) {
-  // NOTE: keyboard::IsKeyboardEnabled() is false in mash, but may not be in
-  // unit tests. crbug.com/646565.
-  if (!keyboard::IsKeyboardEnabled())
+  if (!keyboard_controller_->IsKeyboardEnableRequested())
     return;
 
   switch (state) {
@@ -136,6 +159,15 @@ void AshKeyboardController::ActivateKeyboard() {
       keyboard_controller_.get());
 }
 
+void AshKeyboardController::UpdateEnableFlag(bool was_enabled) {
+  bool is_enabled = keyboard_controller_->IsKeyboardEnableRequested();
+  if (is_enabled && !was_enabled) {
+    EnableKeyboard();
+  } else if (!is_enabled && was_enabled) {
+    DisableKeyboard();
+  }
+}
+
 void AshKeyboardController::OnKeyboardConfigChanged() {
   KeyboardConfigPtr config =
       KeyboardConfig::New(keyboard_controller_->keyboard_config());

ash/keyboard/ash_keyboard_controller.h

@@ -57,11 +57,15 @@ class ASH_EXPORT AshKeyboardController
   void DestroyVirtualKeyboard();
 
   // mojom::KeyboardController:
-  void AddObserver(
-      mojom::KeyboardControllerObserverAssociatedPtrInfo observer) override;
   void GetKeyboardConfig(GetKeyboardConfigCallback callback) override;
   void SetKeyboardConfig(
       keyboard::mojom::KeyboardConfigPtr keyboard_config) override;
+  void IsKeyboardEnabled(IsKeyboardEnabledCallback callback) override;
+  void SetEnableFlag(keyboard::mojom::KeyboardEnableFlag flag) override;
+  void ClearEnableFlag(keyboard::mojom::KeyboardEnableFlag flag) override;
+  void ReloadKeyboard() override;
+  void AddObserver(
+      mojom::KeyboardControllerObserverAssociatedPtrInfo observer) override;
 
   // SessionObserver:
   void OnSessionStateChanged(session_manager::SessionState state) override;
@@ -78,6 +82,10 @@ class ASH_EXPORT AshKeyboardController
   // Ensures that the keyboard controller is activated for the primary window.
   void ActivateKeyboard();
 
+  // Called whenever the enable flags may have changed the enabled state from
+  // |was_enabled|. If changed, enables or disables the keyboard.
+  void UpdateEnableFlag(bool was_enabled);
+
   // keyboard::KeyboardControllerObserver
   void OnKeyboardConfigChanged() override;
   void OnKeyboardVisibilityStateChanged(bool is_visible) override;

ash/keyboard/ash_keyboard_controller_unittest.cc

@@ -6,17 +6,18 @@
 
 #include <memory>
 #include "ash/public/interfaces/keyboard_controller.mojom.h"
+#include "ash/shell.h"
+#include "ash/test/ash_test_base.h"
 #include "base/run_loop.h"
 #include "base/test/scoped_task_environment.h"
 #include "mojo/public/cpp/bindings/associated_binding.h"
 #include "services/service_manager/public/cpp/connector.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/keyboard/keyboard_controller.h"
-#include "ui/keyboard/keyboard_ui.h"
-#include "ui/keyboard/test/keyboard_test_util.h"
 
 using keyboard::mojom::KeyboardConfig;
 using keyboard::mojom::KeyboardConfigPtr;
+using keyboard::mojom::KeyboardEnableFlag;
 
 namespace ash {
 
@@ -32,22 +33,23 @@ class TestObserver : public mojom::KeyboardControllerObserver {
   ~TestObserver() override = default;
 
   // mojom::KeyboardControllerObserver:
-  void OnKeyboardEnabledChanged(bool enabled) override {}
+  void OnKeyboardEnabledChanged(bool enabled) override {
+    if (!enabled)
+      ++destroyed_count_;
+  }
   void OnKeyboardVisibilityChanged(bool visible) override {}
   void OnKeyboardVisibleBoundsChanged(const gfx::Rect& bounds) override {}
   void OnKeyboardConfigChanged(KeyboardConfigPtr config) override {
     config_ = *config;
   }
 
-  const KeyboardConfig& config() const { return config_; }
-  void set_config(const KeyboardConfig& config) { config_ = config; }
+  KeyboardConfig config_;
+  int destroyed_count_ = 0;
 
  private:
   mojo::AssociatedBinding<ash::mojom::KeyboardControllerObserver>
       keyboard_controller_observer_binding_{this};
 
-  KeyboardConfig config_;
-
   DISALLOW_COPY_AND_ASSIGN(TestObserver);
 };
 
@@ -59,6 +61,15 @@ class TestClient {
     test_observer_ = std::make_unique<TestObserver>(keyboard_controller_.get());
   }
 
+  ~TestClient() = default;
+
+  bool GetIsEnabled() {
+    keyboard_controller_->IsKeyboardEnabled(base::BindOnce(
+        &TestClient::OnIsKeyboardEnabled, base::Unretained(this)));
+    keyboard_controller_.FlushForTesting();
+    return is_enabled_;
+  }
+
   void GetKeyboardConfig() {
     keyboard_controller_->GetKeyboardConfig(base::BindOnce(
         &TestClient::OnGetKeyboardConfig, base::Unretained(this)));
@@ -70,39 +81,46 @@ class TestClient {
     keyboard_controller_.FlushForTesting();
   }
 
-  int got_keyboard_config_count() const { return got_keyboard_config_count_; }
-  const KeyboardConfig& keyboard_config() const { return keyboard_config_; }
+  void SetEnableFlag(KeyboardEnableFlag flag) {
+    keyboard_controller_->SetEnableFlag(flag);
+    keyboard_controller_.FlushForTesting();
+  }
+
+  void ClearEnableFlag(KeyboardEnableFlag flag) {
+    keyboard_controller_->ClearEnableFlag(flag);
+    keyboard_controller_.FlushForTesting();
+  }
+
+  void ReloadKeyboard() {
+    keyboard_controller_->ReloadKeyboard();
+    keyboard_controller_.FlushForTesting();
+  }
+
   TestObserver* test_observer() const { return test_observer_.get(); }
 
+  bool is_enabled_ = false;
+  int got_keyboard_config_count_ = 0;
+  KeyboardConfig keyboard_config_;
+
  private:
+  void OnIsKeyboardEnabled(bool enabled) { is_enabled_ = enabled; }
+
   void OnGetKeyboardConfig(KeyboardConfigPtr config) {
     ++got_keyboard_config_count_;
     keyboard_config_ = *config;
   }
 
   mojom::KeyboardControllerPtr keyboard_controller_;
   std::unique_ptr<TestObserver> test_observer_;
-
-  int got_keyboard_config_count_ = 0;
-  KeyboardConfig keyboard_config_;
 };
 
-class AshKeyboardControllerTest : public testing::Test {
+class AshKeyboardControllerTest : public AshTestBase {
  public:
-  AshKeyboardControllerTest()
-      : scoped_task_environment_(
-            base::test::ScopedTaskEnvironment::MainThreadType::DEFAULT,
-            base::test::ScopedTaskEnvironment::ExecutionMode::QUEUED) {}
+  AshKeyboardControllerTest() = default;
   ~AshKeyboardControllerTest() override = default;
 
   void SetUp() override {
-    ash_keyboard_controller_ = std::make_unique<AshKeyboardController>(
-        nullptr /* session_controller */);
-    // Call SetupUI() so that observer methods get called.
-    auto test_ui =
-        std::make_unique<keyboard::TestKeyboardUI>(nullptr /* input_method */);
-    ash_keyboard_controller_->keyboard_controller()->EnableKeyboard(
-        std::move(test_ui), nullptr /* delegate */);
+    AshTestBase::SetUp();
 
     // Create a local service manager connector to handle requests to
     // mojom::KeyboardController.
@@ -118,29 +136,28 @@ class AshKeyboardControllerTest : public testing::Test {
     base::RunLoop().RunUntilIdle();
 
     test_client_ = std::make_unique<TestClient>(connector_.get());
+
+    // Set the initial observer config to the client (default) config.
+    test_client_->test_observer()->config_ = test_client()->keyboard_config_;
   }
 
   void TearDown() override {
     test_client_.reset();
-    keyboard_controller()->DisableKeyboard();
-    ash_keyboard_controller_.reset();
+    AshTestBase::TearDown();
   }
 
   void AddKeyboardControllerBinding(mojo::ScopedMessagePipeHandle handle) {
-    ash_keyboard_controller_->BindRequest(
+    Shell::Get()->ash_keyboard_controller()->BindRequest(
         mojom::KeyboardControllerRequest(std::move(handle)));
   }
 
   keyboard::KeyboardController* keyboard_controller() {
-    return ash_keyboard_controller_->keyboard_controller();
+    return Shell::Get()->ash_keyboard_controller()->keyboard_controller();
   }
   TestClient* test_client() { return test_client_.get(); }
 
  private:
-  base::test::ScopedTaskEnvironment scoped_task_environment_;
   std::unique_ptr<service_manager::Connector> connector_;
-
-  std::unique_ptr<AshKeyboardController> ash_keyboard_controller_;
   std::unique_ptr<TestClient> test_client_;
 
   DISALLOW_COPY_AND_ASSIGN(AshKeyboardControllerTest);
@@ -150,27 +167,67 @@ class AshKeyboardControllerTest : public testing::Test {
 
 TEST_F(AshKeyboardControllerTest, GetKeyboardConfig) {
   test_client()->GetKeyboardConfig();
-  EXPECT_EQ(1, test_client()->got_keyboard_config_count());
+  EXPECT_EQ(1, test_client()->got_keyboard_config_count_);
 }
 
 TEST_F(AshKeyboardControllerTest, SetKeyboardConfig) {
+  // Enable the keyboard so that config changes trigger observer events.
+  test_client()->SetEnableFlag(KeyboardEnableFlag::kExtensionEnabled);
+
   test_client()->GetKeyboardConfig();
-  EXPECT_EQ(1, test_client()->got_keyboard_config_count());
+  EXPECT_EQ(1, test_client()->got_keyboard_config_count_);
   KeyboardConfigPtr config =
-      KeyboardConfig::New(test_client()->keyboard_config());
+      KeyboardConfig::New(test_client()->keyboard_config_);
   // Set the observer config to the client (default) config.
-  test_client()->test_observer()->set_config(*config);
+  test_client()->test_observer()->config_ = *config;
 
-  // Test that the config changes.
+  // Change the keyboard config.
   bool old_auto_complete = config->auto_complete;
   config->auto_complete = !config->auto_complete;
   test_client()->SetKeyboardConfig(std::move(config));
+
+  // Test that the config changes.
   test_client()->GetKeyboardConfig();
-  EXPECT_NE(old_auto_complete, test_client()->keyboard_config().auto_complete);
+  EXPECT_NE(old_auto_complete, test_client()->keyboard_config_.auto_complete);
 
   // Test that the test observer received the change.
   EXPECT_NE(old_auto_complete,
-            test_client()->test_observer()->config().auto_complete);
+            test_client()->test_observer()->config_.auto_complete);
+}
+
+TEST_F(AshKeyboardControllerTest, Enabled) {
+  EXPECT_FALSE(test_client()->GetIsEnabled());
+  // Enable the keyboard.
+  test_client()->SetEnableFlag(KeyboardEnableFlag::kExtensionEnabled);
+  EXPECT_TRUE(test_client()->GetIsEnabled());
+
+  // Set the enable override to disable the keyboard.
+  test_client()->SetEnableFlag(KeyboardEnableFlag::kPolicyDisabled);
+  EXPECT_FALSE(test_client()->GetIsEnabled());
+
+  // Clear the enable override; should enable the keyboard.
+  test_client()->ClearEnableFlag(KeyboardEnableFlag::kPolicyDisabled);
+  EXPECT_TRUE(test_client()->GetIsEnabled());
+}
+
+TEST_F(AshKeyboardControllerTest, ReloadKeyboard) {
+  EXPECT_EQ(0, test_client()->test_observer()->destroyed_count_);
+
+  // Enable the keyboard.
+  test_client()->SetEnableFlag(KeyboardEnableFlag::kExtensionEnabled);
+  EXPECT_EQ(0, test_client()->test_observer()->destroyed_count_);
+
+  // Enable the keyboard again; this should not reload the keyboard.
+  test_client()->SetEnableFlag(KeyboardEnableFlag::kExtensionEnabled);
+  EXPECT_EQ(0, test_client()->test_observer()->destroyed_count_);
+
+  // Reload the keyboard. This should destroy the previous keyboard window.
+  test_client()->ReloadKeyboard();
+  EXPECT_EQ(1, test_client()->test_observer()->destroyed_count_);
+
+  // Disable the keyboard. The keyboard window should be destroyed.
+  test_client()->ClearEnableFlag(KeyboardEnableFlag::kExtensionEnabled);
+  EXPECT_EQ(2, test_client()->test_observer()->destroyed_count_);
 }
 
 }  // namespace ash