Refactor SpellCheckHost::ToggleSpellCheck

The mojo API is a no-op other than on Android with |enabled| being false. This patch refactors it and renames it into |DisconnectSessionBridge| to make the behavior more explicit. Bug: 814225 Change-Id: I786411375edd594806fb2f7bcdc7b884be4352fe Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1500016 Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org> Reviewed-by: Ken Buchanan <kenrb@chromium.org> Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org> Cr-Commit-Position: refs/heads/master@{#637847}
Denger-Network · Mar 5, 2019 · 551a8c1 · 551a8c1
1 parent 22ea659
commit 551a8c1
Show file tree

Hide file tree

Showing 8 changed files with 43 additions and 25 deletions.
diff --git a/chrome/browser/chrome_site_per_process_browsertest.cc b/chrome/browser/chrome_site_per_process_browsertest.cc
@@ -15,6 +15,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/post_task.h"
+#include "build/build_config.h"
 #include "chrome/browser/chrome_content_browser_client.h"
 #include "chrome/browser/external_protocol/external_protocol_handler.h"
 #include "chrome/browser/profiles/profile.h"
@@ -947,14 +948,18 @@ class MockSpellCheckHost : spellcheck::mojom::SpellCheckHost {
     TextReceived(text);
   }
 
-  void ToggleSpellCheck(bool, bool) override {}
   void CheckSpelling(const base::string16& word,
                      int,
                      CheckSpellingCallback) override {}
   void FillSuggestionList(const base::string16& word,
                           FillSuggestionListCallback) override {}
 #endif
 
+#if defined(OS_ANDROID)
+  // spellcheck::mojom::SpellCheckHost:
+  void DisconnectSessionBridge() override {}
+#endif
+
   content::RenderProcessHost* process_host_;
   bool text_received_ = false;
   base::string16 text_;

diff --git a/components/spellcheck/browser/spell_check_host_impl.cc b/components/spellcheck/browser/spell_check_host_impl.cc
@@ -63,14 +63,6 @@ void SpellCheckHostImpl::RequestTextCheck(const base::string16& text,
 #endif
 }
 
-void SpellCheckHostImpl::ToggleSpellCheck(bool enabled, bool checked) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-#if defined(OS_ANDROID)
-  if (!enabled)
-    session_bridge_.DisconnectSession();
-#endif
-}
-
 void SpellCheckHostImpl::CheckSpelling(const base::string16& word,
                                        int route_id,
                                        CheckSpellingCallback callback) {
@@ -89,3 +81,10 @@ void SpellCheckHostImpl::FillSuggestionList(
   std::move(callback).Run(std::vector<base::string16>());
 }
 #endif  // BUILDFLAG(USE_BROWSER_SPELLCHECKER)
+
+#if defined(OS_ANDROID)
+void SpellCheckHostImpl::DisconnectSessionBridge() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  session_bridge_.DisconnectSession();
+}
+#endif
diff --git a/components/spellcheck/browser/spell_check_host_impl.h b/components/spellcheck/browser/spell_check_host_impl.h
@@ -45,14 +45,18 @@ class SpellCheckHostImpl : public spellcheck::mojom::SpellCheckHost {
   void RequestTextCheck(const base::string16& text,
                         int route_id,
                         RequestTextCheckCallback callback) override;
-  void ToggleSpellCheck(bool enabled, bool checked) override;
   void CheckSpelling(const base::string16& word,
                      int route_id,
                      CheckSpellingCallback callback) override;
   void FillSuggestionList(const base::string16& word,
                           FillSuggestionListCallback callback) override;
 #endif  // BUILDFLAG(USE_BROWSER_SPELLCHECKER)
 
+#if defined(OS_ANDROID)
+  // spellcheck::mojom::SpellCheckHost:
+  void DisconnectSessionBridge() override;
+#endif
+
  private:
 #if defined(OS_ANDROID)
   // Android-specific object used to query the Android spellchecker.

diff --git a/components/spellcheck/browser/spellchecker_session_bridge_android.cc b/components/spellcheck/browser/spellchecker_session_bridge_android.cc
@@ -55,10 +55,10 @@ void SpellCheckerSessionBridge::RequestTextCheck(
   }
 
   // RequestTextCheck API call arrives at the SpellCheckHost before
-  // ToggleSpellCheck when the user focuses an input field that already
+  // DisconnectSessionBridge when the user focuses an input field that already
   // contains completed text.  We need to initialize the spellchecker here
-  // rather than in response to ToggleSpellCheck so that the existing text
-  // will be spellchecked immediately.
+  // rather than in response to DisconnectSessionBridge so that the existing
+  // text will be spellchecked immediately.
   if (java_object_.is_null()) {
     java_object_.Reset(Java_SpellCheckerSessionBridge_create(
         base::android::AttachCurrentThread(),

diff --git a/components/spellcheck/common/spellcheck.mojom b/components/spellcheck/common/spellcheck.mojom
@@ -57,9 +57,9 @@ interface SpellCheckHost {
   RequestTextCheck(mojo_base.mojom.String16 text, int32 route_id) =>
       (array<SpellCheckResult> results);
 
-  // Toggles the enabled state of the platform-specific spell checker.
-  [EnableIf=USE_BROWSER_SPELLCHECKER]
-  ToggleSpellCheck(bool enabled, bool checked);
+  // Disconnects the Android spell checker session bridge.
+  [EnableIf=is_android]
+  DisconnectSessionBridge();
 
   // Checks the correctness of a word with a platform-specific spell checker.
   // TODO(groby): This needs to originate from SpellcheckProvider.

diff --git a/components/spellcheck/renderer/spellcheck_provider.cc b/components/spellcheck/renderer/spellcheck_provider.cc
@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/metrics/histogram_macros.h"
+#include "build/build_config.h"
 #include "components/spellcheck/common/spellcheck.mojom.h"
 #include "components/spellcheck/common/spellcheck_result.h"
 #include "components/spellcheck/renderer/spellcheck.h"
@@ -144,15 +145,18 @@ void SpellCheckProvider::RequestTextChecking(
 }
 
 void SpellCheckProvider::FocusedNodeChanged(const blink::WebNode& unused) {
-#if BUILDFLAG(USE_BROWSER_SPELLCHECKER)
+#if defined(OS_ANDROID)
   WebLocalFrame* frame = render_frame()->GetWebFrame();
   WebElement element = frame->GetDocument().IsNull()
                            ? WebElement()
                            : frame->GetDocument().FocusedElement();
   bool enabled = !element.IsNull() && element.IsEditable();
-  bool checked = enabled && IsSpellCheckingEnabled();
-  GetSpellCheckHost().ToggleSpellCheck(enabled, checked);
-#endif  // USE_BROWSER_SPELLCHECKER
+
+  // TODO(xiaochengh): Don't call GetSpellCheckHost() if SpellCheckHost is not
+  // bound yet. crbug.com/814225
+  if (!enabled)
+    GetSpellCheckHost().DisconnectSessionBridge();
+#endif  // defined(OS_ANDROID)
 }
 
 bool SpellCheckProvider::IsSpellCheckingEnabled() const {

diff --git a/components/spellcheck/renderer/spellcheck_provider_test.cc b/components/spellcheck/renderer/spellcheck_provider_test.cc
@@ -109,10 +109,6 @@ void TestingSpellCheckProvider::RequestTextCheck(
   text_check_requests_.push_back(std::make_pair(text, std::move(callback)));
 }
 
-void TestingSpellCheckProvider::ToggleSpellCheck(bool, bool) {
-  NOTREACHED();
-}
-
 void TestingSpellCheckProvider::CheckSpelling(const base::string16&,
                                               int,
                                               CheckSpellingCallback) {
@@ -125,6 +121,12 @@ void TestingSpellCheckProvider::FillSuggestionList(const base::string16&,
 }
 #endif  // BUILDFLAG(USE_BROWSER_SPELLCHECKER)
 
+#if defined(OS_ANDROID)
+void TestingSpellCheckProvider::DisconnectSessionBridge() {
+  NOTREACHED();
+}
+#endif
+
 void TestingSpellCheckProvider::SetLastResults(
     const base::string16 last_request,
     blink::WebVector<blink::WebTextCheckingResult>& last_results) {

diff --git a/components/spellcheck/renderer/spellcheck_provider_test.h b/components/spellcheck/renderer/spellcheck_provider_test.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/strings/string16.h"
+#include "build/build_config.h"
 #include "components/spellcheck/renderer/empty_local_interface_provider.h"
 #include "components/spellcheck/renderer/spellcheck_provider.h"
 #include "mojo/public/cpp/bindings/binding.h"
@@ -88,7 +89,6 @@ class TestingSpellCheckProvider : public SpellCheckProvider,
   void RequestTextCheck(const base::string16&,
                         int,
                         RequestTextCheckCallback) override;
-  void ToggleSpellCheck(bool, bool) override;
   using SpellCheckProvider::CheckSpelling;
   void CheckSpelling(const base::string16&,
                      int,
@@ -97,6 +97,10 @@ class TestingSpellCheckProvider : public SpellCheckProvider,
                           FillSuggestionListCallback) override;
 #endif
 
+#if defined(OS_ANDROID)
+  void DisconnectSessionBridge() override;
+#endif
+
   // Message loop (if needed) to deliver the SpellCheckHost request flow.
   std::unique_ptr<base::MessageLoop> loop_;