Move ash::mojom::ImeKeyset into //ui/base/ime/chromeos/public/interfaces.

and use it everywhere to express keysets.
This CL aims to reduce the risk to pass unexpected values
to InputMethodManager::OverrideKeyboardUrlRef.

- Rename OverrideKeyboardUrlRef to OverrideKeyboardKeyset

This CL addresses the comment in crrev.com/c/950523.

Bug: 819018
Test: No behavior change. Unit tests still pass.
Change-Id: I96139abaa867c91de039113bc9006b72f210be26
Reviewed-on: https://chromium-review.googlesource.com/974668Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Yuichiro Hanada <yhanada@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545931}

ash/ime/ime_controller.cc

@@ -162,12 +162,13 @@ void ImeController::SetCapsLockEnabled(bool caps_enabled) {
     client_->SetCapsLockEnabled(caps_enabled);
 }
 
-void ImeController::OverrideKeyboardKeyset(ash::mojom::ImeKeyset keyset) {
+void ImeController::OverrideKeyboardKeyset(
+    chromeos::input_method::mojom::ImeKeyset keyset) {
   OverrideKeyboardKeyset(keyset, base::DoNothing());
 }
 
 void ImeController::OverrideKeyboardKeyset(
-    ash::mojom::ImeKeyset keyset,
+    chromeos::input_method::mojom::ImeKeyset keyset,
     mojom::ImeControllerClient::OverrideKeyboardKeysetCallback callback) {
   if (client_)
     client_->OverrideKeyboardKeyset(keyset, std::move(callback));

ash/ime/ime_controller.h

@@ -71,9 +71,9 @@ class ASH_EXPORT ImeController : public mojom::ImeController {
   void SwitchImeById(const std::string& ime_id, bool show_message);
   void ActivateImeMenuItem(const std::string& key);
   void SetCapsLockEnabled(bool caps_enabled);
-  void OverrideKeyboardKeyset(mojom::ImeKeyset keyset);
+  void OverrideKeyboardKeyset(chromeos::input_method::mojom::ImeKeyset keyset);
   void OverrideKeyboardKeyset(
-      mojom::ImeKeyset keyset,
+      chromeos::input_method::mojom::ImeKeyset keyset,
       mojom::ImeControllerClient::OverrideKeyboardKeysetCallback callback);
 
   // Returns true if the switch is allowed and the keystroke should be

ash/ime/test_ime_controller_client.cc

@@ -44,7 +44,7 @@ void TestImeControllerClient::SetCapsLockEnabled(bool enabled) {
 }
 
 void TestImeControllerClient::OverrideKeyboardKeyset(
-    ash::mojom::ImeKeyset keyset,
+    chromeos::input_method::mojom::ImeKeyset keyset,
     OverrideKeyboardKeysetCallback callback) {
   last_keyset_ = keyset;
   std::move(callback).Run();

ash/ime/test_ime_controller_client.h

@@ -24,7 +24,7 @@ class TestImeControllerClient : public mojom::ImeControllerClient {
   void SwitchImeById(const std::string& id, bool show_message) override;
   void ActivateImeMenuItem(const std::string& key) override;
   void SetCapsLockEnabled(bool enabled) override;
-  void OverrideKeyboardKeyset(mojom::ImeKeyset keyset,
+  void OverrideKeyboardKeyset(chromeos::input_method::mojom::ImeKeyset keyset,
                               OverrideKeyboardKeysetCallback callback) override;
 
   int next_ime_count_ = 0;
@@ -33,7 +33,8 @@ class TestImeControllerClient : public mojom::ImeControllerClient {
   int set_caps_lock_count_ = 0;
   std::string last_switch_ime_id_;
   bool last_show_message_ = false;
-  mojom::ImeKeyset last_keyset_ = ash::mojom::ImeKeyset::kNone;
+  chromeos::input_method::mojom::ImeKeyset last_keyset_ =
+      chromeos::input_method::mojom::ImeKeyset::kNone;
 
  private:
   mojo::Binding<mojom::ImeControllerClient> binding_;

ash/public/interfaces/BUILD.gn

@@ -65,6 +65,7 @@ mojom("interfaces_internal") {
     "//services/preferences/public/mojom",
     "//skia/public/interfaces",
     "//ui/accessibility:ax_enums_mojo",
+    "//ui/base/ime/chromeos/public/interfaces",
     "//ui/events/mojo:interfaces",
     "//ui/gfx/geometry/mojo",
     "//ui/gfx/image/mojo:interfaces",

ash/public/interfaces/ime_controller.mojom

@@ -5,6 +5,7 @@
 module ash.mojom;
 
 import "ash/public/interfaces/ime_info.mojom";
+import "ui/base/ime/chromeos/public/interfaces/ime_keyset.mojom";
 
 // Interface for ash client (e.g. Chrome) to send input method info to ash.
 interface ImeController {
@@ -47,16 +48,6 @@ interface ImeController {
                                    bool is_voice_enabled);
 };
 
-// Used by the virtual keyboard to represent different key layouts for
-// different purposes. 'kNone' represents the default key layout.
-// Used in UMA, so this enum should not be reordered.
-enum ImeKeyset {
-  kNone = 0,
-  kEmoji = 1,
-  kHandwriting = 2,
-  kVoice = 3,
-};
-
 // Interface for ash to send input method requests to its client (e.g. Chrome).
 interface ImeControllerClient {
   // Switches to the next input method. Does nothing if only one input method
@@ -90,5 +81,5 @@ interface ImeControllerClient {
   // Overrides the keyboard keyset (emoji, handwriting or voice). If keyset is
   // 'kNone', we switch to the default keyset. Because this is asynchronous,
   // any code that needs the keyset to be updated first must use the callback.
-  OverrideKeyboardKeyset(ImeKeyset keyset) => ();
+  OverrideKeyboardKeyset(chromeos.input_method.mojom.ImeKeyset keyset) => ();
 };

ash/system/ime_menu/ime_menu_tray.cc

@@ -177,13 +177,14 @@ class ImeButtonsView : public views::View, public views::ButtonListener {
     // The |keyset| will be used for drawing input view keyset in IME
     // extensions. ImeMenuTray::ShowKeyboardWithKeyset() will deal with
     // the |keyset| string to generate the right input view url.
-    mojom::ImeKeyset keyset = mojom::ImeKeyset::kNone;
+    using chromeos::input_method::mojom::ImeKeyset;
+    ImeKeyset keyset = ImeKeyset::kNone;
     if (sender == emoji_button_)
-      keyset = mojom::ImeKeyset::kEmoji;
+      keyset = ImeKeyset::kEmoji;
     else if (sender == voice_button_)
-      keyset = mojom::ImeKeyset::kVoice;
+      keyset = ImeKeyset::kVoice;
     else if (sender == handwriting_button_)
-      keyset = mojom::ImeKeyset::kHandwriting;
+      keyset = ImeKeyset::kHandwriting;
     else
       NOTREACHED();
 
@@ -338,7 +339,8 @@ void ImeMenuTray::ShowImeMenuBubbleInternal(bool show_by_click) {
   SetIsActive(true);
 }
 
-void ImeMenuTray::ShowKeyboardWithKeyset(ash::mojom::ImeKeyset keyset) {
+void ImeMenuTray::ShowKeyboardWithKeyset(
+    chromeos::input_method::mojom::ImeKeyset keyset) {
   CloseBubble();
 
   // This will override the url ref of the keyboard to make it shown with
@@ -463,7 +465,8 @@ void ImeMenuTray::HideBubble(const views::TrayBubbleView* bubble_view) {
 }
 
 void ImeMenuTray::OnKeyboardClosed() {
-  ime_controller_->OverrideKeyboardKeyset(ash::mojom::ImeKeyset::kNone);
+  ime_controller_->OverrideKeyboardKeyset(
+      chromeos::input_method::mojom::ImeKeyset::kNone);
   keyboard::KeyboardController* keyboard_controller =
       keyboard::KeyboardController::GetInstance();
   if (keyboard_controller)
@@ -490,7 +493,8 @@ void ImeMenuTray::OnKeyboardHidden() {
 
   // If the the IME menu has overriding the input view url, we should write it
   // back to normal keyboard when hiding the input view.
-  ime_controller_->OverrideKeyboardKeyset(ash::mojom::ImeKeyset::kNone);
+  ime_controller_->OverrideKeyboardKeyset(
+      chromeos::input_method::mojom::ImeKeyset::kNone);
   show_keyboard_ = false;
 
   // If the keyboard is forced to be shown by IME menu for once, we need to

ash/system/ime_menu/ime_menu_tray.h

@@ -13,6 +13,7 @@
 #include "ash/system/tray/tray_bubble_wrapper.h"
 #include "ash/system/virtual_keyboard/virtual_keyboard_observer.h"
 #include "base/macros.h"
+#include "ui/base/ime/chromeos/public/interfaces/ime_keyset.mojom.h"
 #include "ui/keyboard/keyboard_controller_observer.h"
 #include "ui/views/bubble/tray_bubble_view.h"
 
@@ -38,7 +39,7 @@ class ASH_EXPORT ImeMenuTray : public TrayBackgroundView,
 
   // Shows the virtual keyboard with the given keyset: emoji, handwriting or
   // voice.
-  void ShowKeyboardWithKeyset(mojom::ImeKeyset keyset);
+  void ShowKeyboardWithKeyset(chromeos::input_method::mojom::ImeKeyset keyset);
 
   // Returns true if the menu should show emoji, handwriting and voice buttons
   // on the bottom. Otherwise, the menu will show a 'Customize...' bottom row

ash/system/ime_menu/ime_menu_tray_unittest.cc

@@ -285,7 +285,8 @@ TEST_F(ImeMenuTrayTest, ShowEmojiKeyset) {
   accessibility_controller->SetVirtualKeyboardEnabled(true);
   EXPECT_TRUE(accessibility_controller->IsVirtualKeyboardEnabled());
 
-  GetTray()->ShowKeyboardWithKeyset(mojom::ImeKeyset::kEmoji);
+  GetTray()->ShowKeyboardWithKeyset(
+      chromeos::input_method::mojom::ImeKeyset::kEmoji);
   // The menu should be hidden.
   EXPECT_FALSE(IsBubbleShown());
   // The virtual keyboard should be enabled.
@@ -306,7 +307,8 @@ TEST_F(ImeMenuTrayTest, ForceToShowEmojiKeyset) {
 
   TestImeControllerClient client;
   ime_controller->SetClient(client.CreateInterfacePtr());
-  GetTray()->ShowKeyboardWithKeyset(mojom::ImeKeyset::kEmoji);
+  GetTray()->ShowKeyboardWithKeyset(
+      chromeos::input_method::mojom::ImeKeyset::kEmoji);
   // Keyboard is shown asynchronously through Mojo
   ime_controller->FlushMojoForTesting();
   // The virtual keyboard should be enabled.

chrome/browser/chromeos/input_method/input_method_manager_impl.cc

@@ -105,6 +105,19 @@ InputMethodCategory GetInputMethodCategory(const std::string& input_method_id,
   return category;
 }
 
+std::string KeysetToString(mojom::ImeKeyset keyset) {
+  switch (keyset) {
+    case mojom::ImeKeyset::kNone:
+      return "";
+    case mojom::ImeKeyset::kEmoji:
+      return "emoji";
+    case mojom::ImeKeyset::kHandwriting:
+      return "hwt";
+    case mojom::ImeKeyset::kVoice:
+      return "voice";
+  }
+}
+
 }  // namespace
 
 // ------------------------ InputMethodManagerImpl::StateImpl
@@ -1262,7 +1275,7 @@ void InputMethodManagerImpl::MaybeNotifyImeMenuActivationChanged() {
                         is_ime_menu_activated_);
 }
 
-void InputMethodManagerImpl::OverrideKeyboardUrlRef(const std::string& keyset) {
+void InputMethodManagerImpl::OverrideKeyboardKeyset(mojom::ImeKeyset keyset) {
   GURL url = state_->GetInputViewUrl();
 
   // If fails to find ref or tag "id" in the ref, it means the current IME is
@@ -1276,7 +1289,7 @@ void InputMethodManagerImpl::OverrideKeyboardUrlRef(const std::string& keyset) {
   if (i == std::string::npos)
     return;
 
-  if (keyset.empty()) {
+  if (keyset == mojom::ImeKeyset::kNone) {
     // Resets the url as the input method default url and notify the hash
     // changed to VK.
     state_->input_view_url = state_->current_input_method.input_view_url();
@@ -1294,9 +1307,9 @@ void InputMethodManagerImpl::OverrideKeyboardUrlRef(const std::string& keyset) {
   // id like: id=${keyset}.emoji/hwt/voice.
   auto j = overridden_ref.find("&", i + 1);
   if (j == std::string::npos) {
-    overridden_ref += "." + keyset;
+    overridden_ref += "." + KeysetToString(keyset);
   } else {
-    overridden_ref.replace(j, 0, "." + keyset);
+    overridden_ref.replace(j, 0, "." + KeysetToString(keyset));
   }
 
   GURL::Replacements replacements;

chrome/browser/chromeos/input_method/input_method_manager_impl.h

@@ -190,7 +190,7 @@ class InputMethodManagerImpl : public InputMethodManager,
       const std::string& engine_id,
       const std::vector<InputMethodManager::MenuItem>& items) override;
   void MaybeNotifyImeMenuActivationChanged() override;
-  void OverrideKeyboardUrlRef(const std::string& keyset) override;
+  void OverrideKeyboardKeyset(mojom::ImeKeyset keyset) override;
   void SetImeMenuFeatureEnabled(ImeMenuFeature feature, bool enabled) override;
   bool GetImeMenuFeatureEnabled(ImeMenuFeature feature) const override;
   void NotifyObserversImeExtraInputStateChange() override;

chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc

@@ -1281,7 +1281,7 @@ TEST_F(InputMethodManagerImplTest, OverrideKeyboardUrlRefWithKeyset) {
       "chrome-extension://"
       "inputview.html#id=us.compact.qwerty.emoji&language=en-US&passwordLayout="
       "us.compact.qwerty&name=keyboard_us");
-  manager_->OverrideKeyboardUrlRef("emoji");
+  manager_->OverrideKeyboardKeyset(mojom::ImeKeyset::kEmoji);
   EXPECT_EQ(overridden_url_emoji, GetActiveIMEState()->GetInputViewUrl());
 
   // Override the keyboard url ref with 'hwt'.
@@ -1290,7 +1290,7 @@ TEST_F(InputMethodManagerImplTest, OverrideKeyboardUrlRefWithKeyset) {
       "chrome-extension://"
       "inputview.html#id=us.compact.qwerty.hwt&language=en-US&passwordLayout="
       "us.compact.qwerty&name=keyboard_us");
-  manager_->OverrideKeyboardUrlRef("hwt");
+  manager_->OverrideKeyboardKeyset(mojom::ImeKeyset::kHandwriting);
   EXPECT_EQ(overridden_url_hwt, GetActiveIMEState()->GetInputViewUrl());
 
   // Override the keyboard url ref with 'voice'.
@@ -1299,7 +1299,7 @@ TEST_F(InputMethodManagerImplTest, OverrideKeyboardUrlRefWithKeyset) {
       "chrome-extension://"
       "inputview.html#id=us.compact.qwerty.voice&language=en-US"
       "&passwordLayout=us.compact.qwerty&name=keyboard_us");
-  manager_->OverrideKeyboardUrlRef("voice");
+  manager_->OverrideKeyboardKeyset(mojom::ImeKeyset::kVoice);
   EXPECT_EQ(overridden_url_voice, GetActiveIMEState()->GetInputViewUrl());
 }
 
@@ -1310,7 +1310,7 @@ TEST_F(InputMethodManagerImplTest, OverrideDefaultKeyboardUrlRef) {
 
   EXPECT_EQ(default_url, GetActiveIMEState()->GetInputViewUrl());
 
-  manager_->OverrideKeyboardUrlRef("emoji");
+  manager_->OverrideKeyboardKeyset(mojom::ImeKeyset::kEmoji);
   EXPECT_EQ(default_url, GetActiveIMEState()->GetInputViewUrl());
 }
 

chrome/browser/ui/ash/ime_controller_client.cc

@@ -113,25 +113,9 @@ void ImeControllerClient::SetCapsLockEnabled(bool caps_enabled) {
 }
 
 void ImeControllerClient::OverrideKeyboardKeyset(
-    ash::mojom::ImeKeyset keyset,
+    chromeos::input_method::mojom::ImeKeyset keyset,
     OverrideKeyboardKeysetCallback callback) {
-  std::string url_ref;
-  switch (keyset) {
-    case ash::mojom::ImeKeyset::kNone:
-      // kNone resets url_ref to the empty string.
-      break;
-    case ash::mojom::ImeKeyset::kEmoji:
-      url_ref = "emoji";
-      break;
-    case ash::mojom::ImeKeyset::kVoice:
-      url_ref = "voice";
-      break;
-    case ash::mojom::ImeKeyset::kHandwriting:
-      url_ref = "hwt";
-      break;
-  }
-
-  input_method_manager_->OverrideKeyboardUrlRef(url_ref);
+  input_method_manager_->OverrideKeyboardKeyset(keyset);
   std::move(callback).Run();
 }
 

chrome/browser/ui/ash/ime_controller_client.h

@@ -42,7 +42,7 @@ class ImeControllerClient
   void SwitchImeById(const std::string& id, bool show_message) override;
   void ActivateImeMenuItem(const std::string& key) override;
   void SetCapsLockEnabled(bool caps_enabled) override;
-  void OverrideKeyboardKeyset(ash::mojom::ImeKeyset keyset,
+  void OverrideKeyboardKeyset(chromeos::input_method::mojom::ImeKeyset keyset,
                               OverrideKeyboardKeysetCallback callback) override;
 
   // chromeos::input_method::InputMethodManager::Observer:

chrome/browser/ui/ash/ime_controller_client_unittest.cc

@@ -120,8 +120,9 @@ class TestInputMethodManager : public MockInputMethodManager {
   void ActivateInputMethodMenuItem(const std::string& key) override {
     last_activate_menu_item_key_ = key;
   }
-  void OverrideKeyboardUrlRef(const std::string& key_set) override {
-    keyboard_url_ref_ = key_set;
+  void OverrideKeyboardKeyset(
+      chromeos::input_method::mojom::ImeKeyset keyset) override {
+    keyboard_keyset_ = keyset;
   }
 
   InputMethodUtil* GetInputMethodUtil() override { return &util_; }
@@ -135,7 +136,7 @@ class TestInputMethodManager : public MockInputMethodManager {
   int add_menu_observer_count_ = 0;
   int remove_menu_observer_count_ = 0;
   std::string last_activate_menu_item_key_;
-  std::string keyboard_url_ref_;
+  chromeos::input_method::mojom::ImeKeyset keyboard_keyset_;
   FakeInputMethodDelegate delegate_;
   InputMethodUtil util_;
 
@@ -337,10 +338,11 @@ TEST_F(ImeControllerClientTest, OverrideKeyboardKeyset) {
   ImeControllerClient client(&input_method_manager_);
   bool callback_called = false;
   client.OverrideKeyboardKeyset(
-      ash::mojom::ImeKeyset::kEmoji,
+      chromeos::input_method::mojom::ImeKeyset::kEmoji,
       base::BindLambdaForTesting(
           [&callback_called]() { callback_called = true; }));
-  EXPECT_EQ("emoji", input_method_manager_.keyboard_url_ref_);
+  EXPECT_EQ(chromeos::input_method::mojom::ImeKeyset::kEmoji,
+            input_method_manager_.keyboard_keyset_);
   EXPECT_TRUE(callback_called);
 }
 

ui/base/ime/BUILD.gn

@@ -118,6 +118,7 @@ jumbo_component("ime") {
     "//net",
     "//third_party/icu",
     "//ui/base",
+    "//ui/base/ime/chromeos/public/interfaces",
     "//ui/display",
     "//ui/events",
     "//ui/gfx",

ui/base/ime/chromeos/input_method_manager.h

@@ -14,6 +14,7 @@
 
 #include "base/memory/ref_counted.h"
 #include "ui/base/ime/chromeos/input_method_descriptor.h"
+#include "ui/base/ime/chromeos/public/interfaces/ime_keyset.mojom.h"
 #include "ui/base/ime/ui_base_ime_export.h"
 
 class Profile;
@@ -327,10 +328,9 @@ class UI_BASE_IME_EXPORT InputMethodManager {
   // is different from previous.
   virtual void MaybeNotifyImeMenuActivationChanged() = 0;
 
-  // Overrides the keyboard url ref (stuff following '#' to the end of the
-  // string) with the given keyset (emoji, hwt or voice). If |keyset| is empty,
-  // it indicates that we should override the url back with the keyboard keyset.
-  virtual void OverrideKeyboardUrlRef(const std::string& keyset) = 0;
+  // Overrides active keyset with the given keyset if the active IME supports
+  // the given keyset.
+  virtual void OverrideKeyboardKeyset(mojom::ImeKeyset keyset) = 0;
 
   // Enables or disables some advanced features, e.g. handwiring, voices input.
   virtual void SetImeMenuFeatureEnabled(ImeMenuFeature feature,

ui/base/ime/chromeos/mock_input_method_manager.cc

@@ -198,8 +198,7 @@ void MockInputMethodManager::NotifyImeMenuItemsChanged(
 
 void MockInputMethodManager::MaybeNotifyImeMenuActivationChanged() {}
 
-void MockInputMethodManager::OverrideKeyboardUrlRef(const std::string& keyset) {
-}
+void MockInputMethodManager::OverrideKeyboardKeyset(mojom::ImeKeyset keyset) {}
 
 void MockInputMethodManager::SetImeMenuFeatureEnabled(ImeMenuFeature feature,
                                                       bool enabled) {

ui/base/ime/chromeos/mock_input_method_manager.h

@@ -109,7 +109,7 @@ class UI_BASE_IME_EXPORT MockInputMethodManager : public InputMethodManager {
       const std::string& engine_id,
       const std::vector<InputMethodManager::MenuItem>& items) override;
   void MaybeNotifyImeMenuActivationChanged() override;
-  void OverrideKeyboardUrlRef(const std::string& keyset) override;
+  void OverrideKeyboardKeyset(mojom::ImeKeyset keyset) override;
   void SetImeMenuFeatureEnabled(ImeMenuFeature feature, bool enabled) override;
   bool GetImeMenuFeatureEnabled(ImeMenuFeature feature) const override;
   void NotifyObserversImeExtraInputStateChange() override;

ui/base/ime/chromeos/public/interfaces/BUILD.gn

+# Copyright 2018 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.
+
+import("//mojo/public/tools/bindings/mojom.gni")
+
+mojom("interfaces") {
+  sources = [
+    "ime_keyset.mojom",
+  ]
+}

ui/base/ime/chromeos/public/interfaces/OWNERS

+per-file *.mojom=set noparent
+per-file *.mojom=file://ipc/SECURITY_OWNERS

ui/base/ime/chromeos/public/interfaces/ime_keyset.mojom

+// Copyright 2018 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.
+
+module chromeos.input_method.mojom;
+
+// Used by the virtual keyboard to represent different key layouts for
+// different purposes. 'kNone' represents the default key layout.
+// Used in UMA, so this enum should not be reordered.
+enum ImeKeyset {
+  kNone = 0,
+  kEmoji = 1,
+  kHandwriting = 2,
+  kVoice = 3,
+};