Use a dummy LocalInterfaceProvider for spellcheck test code

Several spellcheck classes hold a LocalInterfaceProvider pointer,
which is always non-null in production code, but can be null in unit
tests. This makes the production code performing unnecessary nullptr
checks and doing hacky things.

This patch adds a dummy LocalInterfaceProvider that doesn't bind any
remote application, and uses it in test code, so that spellcheck
production code no longer needs to perform nullptr checks.

Bug: 714480
Change-Id: I30a9c82efd4298a62bda52d357fcfa9a6c6d991b
Reviewed-on: https://chromium-review.googlesource.com/910650
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537163}

components/spellcheck/renderer/BUILD.gn

@@ -67,6 +67,8 @@ source_set("unit_tests") {
   testonly = true
   sources = [
     "custom_dictionary_engine_unittest.cc",
+    "empty_local_interface_provider.cc",
+    "empty_local_interface_provider.h",
     "spellcheck_provider_mac_unittest.cc",
     "spellcheck_provider_test.cc",
     "spellcheck_provider_test.h",
@@ -92,6 +94,7 @@ source_set("unit_tests") {
   deps = [
     ":renderer",
     "//base:i18n",
+    "//base/test:test_support",
     "//components/spellcheck/common",
     "//ipc:ipc",
     "//testing/gtest",

components/spellcheck/renderer/empty_local_interface_provider.cc

+// 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.
+
+#include "components/spellcheck/renderer/empty_local_interface_provider.h"
+
+namespace spellcheck {
+
+void EmptyLocalInterfaceProvider::GetInterface(
+    const std::string& name,
+    mojo::ScopedMessagePipeHandle request_handle) {}
+
+}  // namespace spellcheck

components/spellcheck/renderer/empty_local_interface_provider.h

+// 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.
+
+#ifndef COMPONENTS_SPELLCHECK_EMPTY_LOCAL_INTERFACE_PROVIDER_H_
+#define COMPONENTS_SPELLCHECK_EMPTY_LOCAL_INTERFACE_PROVIDER_H_
+
+#include "services/service_manager/public/cpp/local_interface_provider.h"
+
+namespace spellcheck {
+
+// A dummy LocalInterfaceProvider that doesn't bind any remote application.
+// May require a base::test::ScopedTaskEnvironment if GetInterface() is expected
+// to be called.
+class EmptyLocalInterfaceProvider
+    : public service_manager::LocalInterfaceProvider {
+ public:
+  void GetInterface(const std::string& name,
+                    mojo::ScopedMessagePipeHandle request_handle) override;
+};
+
+}  // namespace spellcheck
+
+#endif  // COMPONENTS_SPELLCHECK_EMPTY_LOCAL_INTERFACE_PROVIDER_H_

components/spellcheck/renderer/hunspell_engine.cc

@@ -39,6 +39,7 @@ namespace {
 #if !BUILDFLAG(USE_BROWSER_SPELLCHECKER)
 SpellingEngine* CreateNativeSpellingEngine(
     service_manager::LocalInterfaceProvider* embedder_provider) {
+  DCHECK(embedder_provider);
   return new HunspellEngine(embedder_provider);
 }
 #endif
@@ -123,12 +124,9 @@ void HunspellEngine::FillSuggestionList(
 
 bool HunspellEngine::InitializeIfNeeded() {
   if (!initialized_ && !dictionary_requested_) {
-    // |embedder_provider_| will be nullptr in tests.
-    if (embedder_provider_) {
-      spellcheck::mojom::SpellCheckHostPtr spell_check_host;
-      embedder_provider_->GetInterface(&spell_check_host);
-      spell_check_host->RequestDictionary();
-    }
+    spellcheck::mojom::SpellCheckHostPtr spell_check_host;
+    embedder_provider_->GetInterface(&spell_check_host);
+    spell_check_host->RequestDictionary();
     dictionary_requested_ = true;
     return true;
   }

components/spellcheck/renderer/platform_spelling_engine.cc

@@ -11,6 +11,7 @@ using content::RenderThread;
 
 SpellingEngine* CreateNativeSpellingEngine(
     service_manager::LocalInterfaceProvider* embedder_provider) {
+  DCHECK(embedder_provider);
   return new PlatformSpellingEngine();
 }
 

components/spellcheck/renderer/spellcheck.cc

@@ -176,6 +176,7 @@ SpellCheck::SpellCheck(
     : embedder_provider_(embedder_provider),
       spellcheck_enabled_(true),
       weak_factory_(this) {
+  DCHECK(embedder_provider);
   if (!registry)
     return;  // Can be NULL in tests.
   registry->AddInterface(base::BindRepeating(&SpellCheck::SpellCheckerRequest,

components/spellcheck/renderer/spellcheck_multilingual_unittest.cc

@@ -14,8 +14,10 @@
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_task_environment.h"
 #include "components/spellcheck/common/spellcheck_common.h"
 #include "components/spellcheck/common/spellcheck_result.h"
+#include "components/spellcheck/renderer/empty_local_interface_provider.h"
 #include "components/spellcheck/renderer/spellcheck.h"
 #include "components/spellcheck/renderer/spellcheck_provider_test.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -50,8 +52,9 @@ class MultilingualSpellCheckTest : public testing::Test {
   MultilingualSpellCheckTest() {}
 
   void ReinitializeSpellCheck(const std::string& unsplit_languages) {
-    spellcheck_ = new SpellCheck(nullptr, nullptr);
-    provider_.reset(new TestingSpellCheckProvider(spellcheck_));
+    spellcheck_ = new SpellCheck(nullptr, &embedder_provider_);
+    provider_.reset(
+        new TestingSpellCheckProvider(spellcheck_, &embedder_provider_));
     InitializeSpellCheck(unsplit_languages);
   }
 
@@ -111,6 +114,9 @@ class MultilingualSpellCheckTest : public testing::Test {
   }
 
  private:
+  base::test::ScopedTaskEnvironment task_environment_;
+  spellcheck::EmptyLocalInterfaceProvider embedder_provider_;
+
   // Owned by |provider_|.
   SpellCheck* spellcheck_;
   std::unique_ptr<TestingSpellCheckProvider> provider_;

components/spellcheck/renderer/spellcheck_panel.cc

@@ -21,6 +21,7 @@ SpellCheckPanel::SpellCheckPanel(
       spelling_panel_visible_(false),
       embedder_provider_(embedder_provider) {
   DCHECK(render_frame);
+  DCHECK(embedder_provider);
   registry->AddInterface(base::BindRepeating(
       &SpellCheckPanel::SpellCheckPanelRequest, base::Unretained(this)));
   render_frame->GetWebFrame()->SetSpellCheckPanelHostClient(this);

components/spellcheck/renderer/spellcheck_provider.cc

@@ -49,6 +49,7 @@ SpellCheckProvider::SpellCheckProvider(
       embedder_provider_(embedder_provider),
       weak_factory_(this) {
   DCHECK(spellcheck_);
+  DCHECK(embedder_provider);
   if (render_frame)  // NULL in unit tests.
     render_frame->GetWebFrame()->SetTextCheckClient(this);
 }
@@ -60,9 +61,7 @@ spellcheck::mojom::SpellCheckHost& SpellCheckProvider::GetSpellCheckHost() {
   if (spell_check_host_)
     return *spell_check_host_;
 
-  // nullptr in tests.
-  if (embedder_provider_)
-    embedder_provider_->GetInterface(&spell_check_host_);
+  embedder_provider_->GetInterface(&spell_check_host_);
   return *spell_check_host_;
 }
 
@@ -93,8 +92,6 @@ void SpellCheckProvider::RequestTextChecking(
   Send(new SpellCheckHostMsg_RequestTextCheck(routing_id(), last_identifier_,
                                               text));
 #else
-  if (!spell_check_host_ && !content::RenderThread::Get())
-    return;  // NULL in tests that do not provide a spell_check_host_.
   GetSpellCheckHost().CallSpellingService(
       text, base::BindOnce(&SpellCheckProvider::OnRespondSpellingService,
                            weak_factory_.GetWeakPtr(), last_identifier_, text));
@@ -155,8 +152,6 @@ void SpellCheckProvider::CheckSpelling(
     UMA_HISTOGRAM_COUNTS("SpellCheck.api.check", word.size());
     // If optional_suggestions is not requested, the API is called
     // for marking.  So we use this for counting markable words.
-    if (!spell_check_host_ && !content::RenderThread::Get())
-      return;  // NULL in tests that do not provide a spell_check_host_.
     GetSpellCheckHost().NotifyChecked(word, 0 < length);
   }
 }

components/spellcheck/renderer/spellcheck_provider_test.cc

@@ -30,13 +30,18 @@ void FakeTextCheckingCompletion::DidCancelCheckingText() {
   ++cancellation_count_;
 }
 
-TestingSpellCheckProvider::TestingSpellCheckProvider()
-    : SpellCheckProvider(nullptr, new SpellCheck(nullptr, nullptr), nullptr),
+TestingSpellCheckProvider::TestingSpellCheckProvider(
+    service_manager::LocalInterfaceProvider* embedder_provider)
+    : SpellCheckProvider(nullptr,
+                         new SpellCheck(nullptr, embedder_provider),
+                         embedder_provider),
       spelling_service_call_count_(0),
       binding_(this) {}
 
-TestingSpellCheckProvider::TestingSpellCheckProvider(SpellCheck* spellcheck)
-    : SpellCheckProvider(nullptr, spellcheck, nullptr),
+TestingSpellCheckProvider::TestingSpellCheckProvider(
+    SpellCheck* spellcheck,
+    service_manager::LocalInterfaceProvider* embedder_provider)
+    : SpellCheckProvider(nullptr, spellcheck, embedder_provider),
       spelling_service_call_count_(0),
       binding_(this) {}
 
@@ -125,5 +130,6 @@ bool TestingSpellCheckProvider::SatisfyRequestFromCache(
   return SpellCheckProvider::SatisfyRequestFromCache(text, completion);
 }
 
-SpellCheckProviderTest::SpellCheckProviderTest() {}
+SpellCheckProviderTest::SpellCheckProviderTest()
+    : provider_(&embedder_provider_) {}
 SpellCheckProviderTest::~SpellCheckProviderTest() {}

components/spellcheck/renderer/spellcheck_provider_test.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/strings/string16.h"
+#include "components/spellcheck/renderer/empty_local_interface_provider.h"
 #include "components/spellcheck/renderer/spellcheck_provider.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -42,9 +43,10 @@ class FakeTextCheckingCompletion : public blink::WebTextCheckingCompletion {
 class TestingSpellCheckProvider : public SpellCheckProvider,
                                   public spellcheck::mojom::SpellCheckHost {
  public:
-  TestingSpellCheckProvider();
+  explicit TestingSpellCheckProvider(service_manager::LocalInterfaceProvider*);
   // Takes ownership of |spellcheck|.
-  explicit TestingSpellCheckProvider(SpellCheck* spellcheck);
+  TestingSpellCheckProvider(SpellCheck* spellcheck,
+                            service_manager::LocalInterfaceProvider*);
 
   ~TestingSpellCheckProvider() override;
 
@@ -86,6 +88,7 @@ class SpellCheckProviderTest : public testing::Test {
   ~SpellCheckProviderTest() override;
 
  protected:
+  spellcheck::EmptyLocalInterfaceProvider embedder_provider_;
   TestingSpellCheckProvider provider_;
 };
 

components/spellcheck/renderer/spellcheck_unittest.cc

@@ -20,6 +20,7 @@
 #include "build/build_config.h"
 #include "components/spellcheck/common/spellcheck_common.h"
 #include "components/spellcheck/common/spellcheck_result.h"
+#include "components/spellcheck/renderer/empty_local_interface_provider.h"
 #include "components/spellcheck/renderer/hunspell_engine.h"
 #include "components/spellcheck/renderer/spellcheck_language.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -59,7 +60,7 @@ class SpellCheckTest : public testing::Test {
   }
 
   void UninitializeSpellCheck() {
-    spell_check_ = std::make_unique<SpellCheck>(nullptr, nullptr);
+    spell_check_ = std::make_unique<SpellCheck>(nullptr, &embedder_provider_);
   }
 
   bool InitializeIfNeeded() {
@@ -76,9 +77,9 @@ class SpellCheckTest : public testing::Test {
     // TODO(groby): Forcing spellcheck to use hunspell, even on OSX.
     // Instead, tests should exercise individual spelling engines.
     spell_check_->languages_.push_back(
-        std::make_unique<SpellcheckLanguage>(nullptr));
+        std::make_unique<SpellcheckLanguage>(&embedder_provider_));
     spell_check_->languages_.front()->platform_spelling_engine_ =
-        std::make_unique<HunspellEngine>(nullptr);
+        std::make_unique<HunspellEngine>(&embedder_provider_);
     spell_check_->languages_.front()->Init(std::move(file), language);
 #else
     spell_check_->AddSpellcheckLanguage(std::move(file), language);
@@ -124,6 +125,7 @@ class SpellCheckTest : public testing::Test {
 #endif
 
  private:
+  spellcheck::EmptyLocalInterfaceProvider embedder_provider_;
   std::unique_ptr<SpellCheck> spell_check_;
   base::MessageLoop loop_;
 };