Windows Spellcheck: Init service on clicking in editable content

This is another incremental change towards the goal of having the
spellcheck service initialized on demand, so that there is no
performance hit from loading DLLs supporting the Windows platform
spellchecker on browser startup. The previous change in this effort:

    2239682: Windows Spellcheck: Further work on delay init of
    spellcheck service
    https://chromium-review.googlesource.com/c/chromium/src/+/2239682

listed the following remaining work to be done:

1) In first run scenarios we would still need to load the dictionaries
at browser startup, in order to assure that even if a user never uses
spellcheck in their first browser session after installation,
spellcheck will be enabled for their primary preferred language in the
case where the language has Windows platform but not Hunspell support
(a leading example being Arabic).
2) On going to a page with editable content without first visiting
the languages settings page, clicking in the editable content should
initialize the spellcheck service in the browser process.
3) On right-clicking in editable content to bring up the context menu,
the spellcheck service needs to be instantiated in order to populate
the languages presented in the spellcheck languages submenu.

This CL accomplishes (2). When focus enters editable content, the
method SpellCheckProvider::RequestTextChecking is called from
Blink code in the renderer process. Previously if the spellcheck
service was not initialized, the method would exit without performing
a text check, because there were no native or Hunspell dictionaries
loaded. Now a mojo call is made to asynchronously initialize the
spellcheck service in the browser process. When the dictionaries are
loaded, a callback is made to the renderer, and the text checking
can continue as before.

In all my testing so far this change also fixes (3) as a side effect,
since I haven't discovered any way to bring up the context menu for
editable content without putting the focus there, and gaining focus
invokes a spellcheck request (scenario 2) and thus initializes the
spellcheck service. So unless I am still missing some other scenario
that is broken by delayed initialization, only scenario (1) needs
to be fixed before the WinDelaySpellcheckServiceInit feature flag
can be enabled by default.

Bug: 1064351
Change-Id: I972e76c3052f83f9e05d57086a7cb27e5d9f6ef5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2283370Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Guillaume Jenkins <gujen@google.com>
Commit-Queue: Bruce Long <brlong@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#787864}

chrome/browser/site_isolation/spellcheck_per_process_browsertest.cc

@@ -132,6 +132,10 @@ class MockSpellCheckHost : spellcheck::mojom::SpellCheckHost {
   }
 #endif  // defined(OS_WIN)
 
+  void InitializeDictionaries(
+      InitializeDictionariesCallback callback) override {
+    NOTREACHED();
+  }
 #endif  // BUILDFLAG(USE_BROWSER_SPELLCHECKER)
 
 #if defined(OS_ANDROID)

chrome/browser/spellchecker/spell_check_host_chrome_impl.cc

@@ -210,6 +210,22 @@ void SpellCheckHostChromeImpl::GetPerLanguageSuggestions(
 }
 #endif  // defined(OS_WIN)
 
+void SpellCheckHostChromeImpl::InitializeDictionaries(
+    InitializeDictionariesCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // Initialize the spellcheck service if needed. Initialization must
+  // happen on UI thread.
+  SpellcheckService* spellcheck = GetSpellcheckService();
+
+  if (!spellcheck) {  // Teardown.
+    std::move(callback).Run();
+    return;
+  }
+
+  spellcheck->InitializeDictionaries(std::move(callback));
+}
+
 void SpellCheckHostChromeImpl::OnRequestFinished(SpellingRequest* request) {
   auto iterator = requests_.find(request);
   requests_.erase(iterator);

chrome/browser/spellchecker/spell_check_host_chrome_impl.h

@@ -85,6 +85,8 @@ class SpellCheckHostChromeImpl : public SpellCheckHostImpl {
       GetPerLanguageSuggestionsCallback callback) override;
 #endif  // defined(OS_WIN)
 
+  void InitializeDictionaries(InitializeDictionariesCallback callback) override;
+
   // Clears a finished request from |requests_|. Exposed to SpellingRequest.
   void OnRequestFinished(SpellingRequest* request);
 

chrome/browser/spellchecker/spell_check_host_chrome_impl_win_browsertest.cc

@@ -27,8 +27,14 @@ class PlatformSpellChecker;
 
 class SpellCheckHostChromeImplWinBrowserTest : public InProcessBrowserTest {
  public:
-  SpellCheckHostChromeImplWinBrowserTest() {
-    feature_list_.InitAndEnableFeature(spellcheck::kWinUseBrowserSpellChecker);
+  SpellCheckHostChromeImplWinBrowserTest() = default;
+
+  void SetUp() override {
+    // Don't delay initialization of the SpellcheckService on browser launch.
+    feature_list_.InitWithFeatures(
+        /*enabled_features=*/{spellcheck::kWinUseBrowserSpellChecker},
+        /*disabled_features=*/{spellcheck::kWinDelaySpellcheckServiceInit});
+    InProcessBrowserTest::SetUp();
   }
 
   void SetUpOnMainThread() override {
@@ -38,12 +44,16 @@ class SpellCheckHostChromeImplWinBrowserTest : public InProcessBrowserTest {
     SpellCheckHostChromeImpl::Create(
         renderer_->GetID(), spell_check_host_.BindNewPipeAndPassReceiver());
 
+    InitializeSpellcheckService();
+
     platform_spell_checker_ = SpellcheckServiceFactory::GetForContext(context)
                                   ->platform_spell_checker();
   }
 
   void TearDownOnMainThread() override { renderer_.reset(); }
 
+  virtual void InitializeSpellcheckService() {}
+
   void OnSpellcheckResult(const std::vector<SpellCheckResult>& result) {
     received_result_ = true;
     result_ = result;
@@ -76,6 +86,9 @@ class SpellCheckHostChromeImplWinBrowserTest : public InProcessBrowserTest {
     received_result_ = false;
   }
 
+  void RunSpellCheckReturnMessageTest();
+  void RunGetPerLanguageSuggestionsTest();
+
  protected:
   PlatformSpellChecker* platform_spell_checker_;
   base::test::ScopedFeatureList feature_list_;
@@ -91,6 +104,10 @@ class SpellCheckHostChromeImplWinBrowserTest : public InProcessBrowserTest {
 // Uses browsertest to setup chrome threads.
 IN_PROC_BROWSER_TEST_F(SpellCheckHostChromeImplWinBrowserTest,
                        SpellCheckReturnMessage) {
+  RunSpellCheckReturnMessageTest();
+}
+
+void SpellCheckHostChromeImplWinBrowserTest::RunSpellCheckReturnMessageTest() {
   if (!spellcheck::WindowsVersionSupportsSpellchecker()) {
     return;
   }
@@ -118,6 +135,11 @@ IN_PROC_BROWSER_TEST_F(SpellCheckHostChromeImplWinBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(SpellCheckHostChromeImplWinBrowserTest,
                        GetPerLanguageSuggestions) {
+  RunGetPerLanguageSuggestionsTest();
+}
+
+void SpellCheckHostChromeImplWinBrowserTest::
+    RunGetPerLanguageSuggestionsTest() {
   if (!spellcheck::WindowsVersionSupportsSpellchecker()) {
     return;
   }
@@ -140,3 +162,45 @@ IN_PROC_BROWSER_TEST_F(SpellCheckHostChromeImplWinBrowserTest,
   ASSERT_EQ(1U, suggestion_result_.size());
   EXPECT_GT(suggestion_result_[0].size(), 0U);
 }
+
+class SpellCheckHostChromeImplWinBrowserTestDelayInit
+    : public SpellCheckHostChromeImplWinBrowserTest {
+ public:
+  SpellCheckHostChromeImplWinBrowserTestDelayInit() = default;
+
+  void SetUp() override {
+    // Don't initialize the SpellcheckService on browser launch.
+    feature_list_.InitWithFeatures(
+        /*enabled_features=*/{spellcheck::kWinUseBrowserSpellChecker,
+                              spellcheck::kWinDelaySpellcheckServiceInit},
+        /*disabled_features=*/{});
+    InProcessBrowserTest::SetUp();
+  }
+
+  void InitializeSpellcheckService() override {
+    // With the kWinDelaySpellcheckServiceInit feature flag set, the spellcheck
+    // service is not initialized when instantiated. Call InitializeDictionaries
+    // to load the dictionaries.
+    spell_check_host_->InitializeDictionaries(
+        base::BindOnce(&SpellCheckHostChromeImplWinBrowserTestDelayInit::
+                           InitializeDictionariesCallback,
+                       base::Unretained(this)));
+    RunUntilResultReceived();
+  }
+
+  void InitializeDictionariesCallback() {
+    received_result_ = true;
+    if (quit_)
+      std::move(quit_).Run();
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(SpellCheckHostChromeImplWinBrowserTestDelayInit,
+                       SpellCheckReturnMessage) {
+  RunSpellCheckReturnMessageTest();
+}
+
+IN_PROC_BROWSER_TEST_F(SpellCheckHostChromeImplWinBrowserTestDelayInit,
+                       GetPerLanguageSuggestions) {
+  RunGetPerLanguageSuggestionsTest();
+}

chrome/browser/spellchecker/spellcheck_service.cc

@@ -392,6 +392,19 @@ void SpellcheckService::InitForRenderer(content::RenderProcessHost* host) {
   const PrefService* prefs = user_prefs::UserPrefs::Get(context);
   std::vector<spellcheck::mojom::SpellCheckBDictLanguagePtr> dictionaries;
 
+  bool enable_if_uninitialized = false;
+#if defined(OS_WIN)
+  if (spellcheck::UseBrowserSpellChecker() &&
+      base::FeatureList::IsEnabled(
+          spellcheck::kWinDelaySpellcheckServiceInit)) {
+    // If initialization of the spellcheck service is on-demand, the
+    // renderer-side SpellCheck object needs to start out as enabled in order
+    // for a click on editable content to initialize the spellcheck service.
+    if (!dictionaries_loaded())
+      enable_if_uninitialized = true;
+  }
+#endif  // defined(OS_WIN)
+
   for (const auto& hunspell_dictionary : hunspell_dictionaries_) {
     dictionaries.push_back(spellcheck::mojom::SpellCheckBDictLanguage::New(
         hunspell_dictionary->GetDictionaryFile().Duplicate(),
@@ -399,7 +412,7 @@ void SpellcheckService::InitForRenderer(content::RenderProcessHost* host) {
   }
 
   bool enable = prefs->GetBoolean(spellcheck::prefs::kSpellCheckEnable) &&
-                !dictionaries.empty();
+                (!dictionaries.empty() || enable_if_uninitialized);
 
   std::vector<std::string> custom_words;
   if (enable) {

components/spellcheck/browser/spell_check_host_impl.cc

@@ -79,6 +79,13 @@ void SpellCheckHostImpl::GetPerLanguageSuggestions(
 }
 #endif  // defined(OS_WIN)
 
+void SpellCheckHostImpl::InitializeDictionaries(
+    InitializeDictionariesCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  NOTREACHED();
+  std::move(callback).Run();
+}
+
 #endif  //  BUILDFLAG(USE_BROWSER_SPELLCHECKER) &&
         //  !BUILDFLAG(ENABLE_SPELLING_SERVICE)
 

components/spellcheck/browser/spell_check_host_impl.h

@@ -57,6 +57,8 @@ class SpellCheckHostImpl : public spellcheck::mojom::SpellCheckHost {
       GetPerLanguageSuggestionsCallback callback) override;
 #endif  // defined(OS_WIN)
 
+  void InitializeDictionaries(InitializeDictionariesCallback callback) override;
+
 #endif  // BUILDFLAG(USE_BROWSER_SPELLCHECKER) &&
         // !BUILDFLAG(ENABLE_SPELLING_SERVICE)
 

components/spellcheck/common/spellcheck.mojom

@@ -80,6 +80,10 @@ interface SpellCheckHost {
   [EnableIf=is_win, Sync]
   GetPerLanguageSuggestions(mojo_base.mojom.String16 word) =>
       (array<array<mojo_base.mojom.String16>> suggestions);
+
+  // Completes initialization of the spellcheck service by loading dictionaries.
+  [EnableIf=USE_BROWSER_SPELLCHECKER]
+  InitializeDictionaries() => ();
 };
 
 enum Decoration {

components/spellcheck/renderer/spellcheck_provider.cc

@@ -137,6 +137,40 @@ void SpellCheckProvider::RequestTextChecking(
 #if BUILDFLAG(USE_BROWSER_SPELLCHECKER)
   if (spellcheck::UseBrowserSpellChecker()) {
 #if defined(OS_WIN)
+    if (base::FeatureList::IsEnabled(
+            spellcheck::kWinDelaySpellcheckServiceInit) &&
+        !dictionaries_loaded_) {
+      // Initialize the spellcheck service on demand (this spellcheck request
+      // could be the result of the first click in editable content), then
+      // complete the text check request when the dictionaries are loaded.
+      GetSpellCheckHost().InitializeDictionaries(
+          base::BindOnce(&SpellCheckProvider::RequestTextCheckingFromBrowser,
+                         weak_factory_.GetWeakPtr(), text));
+      return;
+    }
+#endif  // defined(OS_WIN)
+
+    RequestTextCheckingFromBrowser(text);
+  }
+#endif  // BUILDFLAG(USE_BROWSER_SPELLCHECKER)
+
+#if BUILDFLAG(USE_RENDERER_SPELLCHECKER)
+  if (!spellcheck::UseBrowserSpellChecker()) {
+    GetSpellCheckHost().CallSpellingService(
+        text,
+        base::BindOnce(&SpellCheckProvider::OnRespondSpellingService,
+                       weak_factory_.GetWeakPtr(), last_identifier_, text));
+  }
+#endif  // BUILDFLAG(USE_RENDERER_SPELLCHECKER)
+}
+
+#if BUILDFLAG(USE_BROWSER_SPELLCHECKER)
+void SpellCheckProvider::RequestTextCheckingFromBrowser(
+    const base::string16& text) {
+  DCHECK(spellcheck::UseBrowserSpellChecker());
+#if defined(OS_WIN)
+  dictionaries_loaded_ = true;
+
   // Determine whether a hybrid check is needed.
   bool use_hunspell = spellcheck_->EnabledLanguageCount() > 0;
   bool use_native =
@@ -174,18 +208,8 @@ void SpellCheckProvider::RequestTextChecking(
       text, routing_id(),
       base::BindOnce(&SpellCheckProvider::OnRespondTextCheck,
                      weak_factory_.GetWeakPtr(), last_identifier_, text));
-  }
-#endif
-
-#if BUILDFLAG(USE_RENDERER_SPELLCHECKER)
-  if (!spellcheck::UseBrowserSpellChecker()) {
-    GetSpellCheckHost().CallSpellingService(
-        text,
-        base::BindOnce(&SpellCheckProvider::OnRespondSpellingService,
-                       weak_factory_.GetWeakPtr(), last_identifier_, text));
-  }
-#endif
 }
+#endif  // BUILDFLAG(USE_BROWSER_SPELLCHECKER)
 
 void SpellCheckProvider::FocusedElementChanged(
     const blink::WebElement& unused) {
@@ -367,7 +391,7 @@ void SpellCheckProvider::OnRespondTextCheck(
   last_request_ = line;
   last_results_.Swap(textcheck_results);
 }
-#endif
+#endif  // BUILDFLAG(USE_BROWSER_SPELLCHECKER)
 
 bool SpellCheckProvider::SatisfyRequestFromCache(
     const base::string16& text,

components/spellcheck/renderer/spellcheck_provider.h

@@ -132,7 +132,17 @@ class SpellCheckProvider : public content::RenderFrameObserver,
       int identifier,
       const base::string16& line,
       const std::vector<SpellCheckResult>& results);
-#endif
+
+  // Makes mojo calls to the browser process to perform platform spellchecking.
+  void RequestTextCheckingFromBrowser(const base::string16& text);
+
+#if defined(OS_WIN)
+  // Flag indicating that the spellcheck service has been initialized and
+  // the dictionaries have been loaded initially. Used to avoid an unnecessary
+  // mojo call to determine this in every text check request.
+  bool dictionaries_loaded_ = false;
+#endif  // defined(OS_WIN)
+#endif  // BUILDFLAG(USE_BROWSER_SPELLCHECKER)
 
   // Holds ongoing spellchecking operations.
   WebTextCheckCompletions text_check_completions_;

components/spellcheck/renderer/spellcheck_provider_test.cc

@@ -10,6 +10,7 @@
 #include "base/run_loop.h"
 #include "build/build_config.h"
 #include "components/spellcheck/common/spellcheck.mojom.h"
+#include "components/spellcheck/common/spellcheck_features.h"
 #include "components/spellcheck/common/spellcheck_result.h"
 #include "components/spellcheck/renderer/hunspell_engine.h"
 #include "components/spellcheck/renderer/spellcheck.h"
@@ -192,6 +193,18 @@ void TestingSpellCheckProvider::GetPerLanguageSuggestions(
 }
 #endif  // defined(OS_WIN)
 
+void TestingSpellCheckProvider::InitializeDictionaries(
+    InitializeDictionariesCallback callback) {
+#if defined(OS_WIN)
+  if (base::FeatureList::IsEnabled(
+          spellcheck::kWinDelaySpellcheckServiceInit)) {
+    std::move(callback).Run();
+    return;
+  }
+#endif  // defined(OS_WIN)
+
+  NOTREACHED();
+}
 #endif  // BUILDFLAG(USE_BROWSER_SPELLCHECKER)
 
 #if defined(OS_ANDROID)

components/spellcheck/renderer/spellcheck_provider_test.h

@@ -151,6 +151,8 @@ class TestingSpellCheckProvider : public SpellCheckProvider,
       GetPerLanguageSuggestionsCallback callback) override;
 #endif  // defined(OS_WIN)
 
+  void InitializeDictionaries(InitializeDictionariesCallback callback) override;
+
 #endif
 
 #if defined(OS_ANDROID)

components/spellcheck/renderer/spellcheck_provider_unittest.cc

@@ -65,12 +65,37 @@ class HybridSpellCheckTest
   HybridSpellCheckTest() : provider_(&embedder_provider_) {}
   ~HybridSpellCheckTest() override {}
 
+  void SetUp() override {
+    // Don't delay initialization of the SpellcheckService on browser launch.
+    feature_list_.InitWithFeatures(
+        /*enabled_features=*/{spellcheck::kWinUseBrowserSpellChecker},
+        /*disabled_features=*/{spellcheck::kWinDelaySpellcheckServiceInit});
+  }
+
+  void RunShouldUseBrowserSpellCheckOnlyWhenNeededTest();
+
  protected:
+  base::test::ScopedFeatureList feature_list_;
   base::test::SingleThreadTaskEnvironment task_environment_;
   spellcheck::EmptyLocalInterfaceProvider embedder_provider_;
   TestingSpellCheckProvider provider_;
 };
 
+// Test fixture for testing hybrid check cases with delayed initialization of
+// the spellcheck service.
+class HybridSpellCheckTestDelayInit : public HybridSpellCheckTest {
+ public:
+  HybridSpellCheckTestDelayInit() = default;
+
+  void SetUp() override {
+    // Don't initialize the SpellcheckService on browser launch.
+    feature_list_.InitWithFeatures(
+        /*enabled_features=*/{spellcheck::kWinUseBrowserSpellChecker,
+                              spellcheck::kWinDelaySpellcheckServiceInit},
+        /*disabled_features=*/{});
+  }
+};
+
 // Test fixture for testing combining results from both the native spell checker
 // and Hunspell.
 class CombineSpellCheckResultsTest
@@ -156,12 +181,7 @@ TEST_F(SpellCheckProviderTest, ShouldNotUseBrowserSpellCheck) {
   EXPECT_EQ(completion.cancellation_count_, 0U);
 }
 
-// Tests that the SpellCheckProvider calls into the native spell checker only
-// when needed.
-INSTANTIATE_TEST_SUITE_P(
-    SpellCheckProviderHybridTests,
-    HybridSpellCheckTest,
-    testing::Values(
+static const HybridSpellCheckTestCase kSpellCheckProviderHybridTestsParams[] = {
     // No languages - should go straight to completion
     HybridSpellCheckTestCase{0U, 0U, 0U, 0U},
     // 1 disabled language - should go to browser
@@ -175,16 +195,25 @@ INSTANTIATE_TEST_SUITE_P(
     // 3 disabled languages - should go to browser
     HybridSpellCheckTestCase{3U, 0U, 0U, 1U},
     // 3 disabled languages, 3 enabled - should go to browser
-        HybridSpellCheckTestCase{6U, 3U, 0U, 1U}));
+    HybridSpellCheckTestCase{6U, 3U, 0U, 1U}};
+
+// Tests that the SpellCheckProvider calls into the native spell checker only
+// when needed.
+INSTANTIATE_TEST_SUITE_P(
+    SpellCheckProviderHybridTests,
+    HybridSpellCheckTest,
+    testing::ValuesIn(kSpellCheckProviderHybridTestsParams));
 
 TEST_P(HybridSpellCheckTest, ShouldUseBrowserSpellCheckOnlyWhenNeeded) {
+  RunShouldUseBrowserSpellCheckOnlyWhenNeededTest();
+}
+
+void HybridSpellCheckTest::RunShouldUseBrowserSpellCheckOnlyWhenNeededTest() {
   if (!spellcheck::WindowsVersionSupportsSpellchecker()) {
     return;
   }
 
   const auto& test_case = GetParam();
-  base::test::ScopedFeatureList local_features;
-  local_features.InitAndEnableFeature(spellcheck::kWinUseBrowserSpellChecker);
 
   FakeTextCheckingResult completion;
   provider_.spellcheck()->SetFakeLanguageCounts(
@@ -202,6 +231,20 @@ TEST_P(HybridSpellCheckTest, ShouldUseBrowserSpellCheckOnlyWhenNeeded) {
   EXPECT_EQ(completion.cancellation_count_, 0U);
 }
 
+// Tests that the SpellCheckProvider calls into the native spell checker only
+// when needed when the code path through
+// SpellCheckProvider::RequestTextChecking is that used when the spellcheck
+// service is initialized on demand.
+INSTANTIATE_TEST_SUITE_P(
+    SpellCheckProviderHybridTests,
+    HybridSpellCheckTestDelayInit,
+    testing::ValuesIn(kSpellCheckProviderHybridTestsParams));
+
+TEST_P(HybridSpellCheckTestDelayInit,
+       ShouldUseBrowserSpellCheckOnlyWhenNeeded) {
+  RunShouldUseBrowserSpellCheckOnlyWhenNeededTest();
+}
+
 // Tests that the SpellCheckProvider can correctly combine results from the
 // native spell checker and Hunspell.
 INSTANTIATE_TEST_SUITE_P(