[Spellcheck] Cleanup. Resubmit, again

Completely unmodified resubmit of https://codereview.chromium.org/11361265/

Actual bug causing previous submission to fail is fixed in https://codereview.chromium.org/11413054/

TBR=rlp@chromium.org,sky@chromium.org
BUG=154918

Review URL: https://codereview.chromium.org/11316090

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@170983 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 78 insertions, 90 deletions

diff --git a/chrome/renderer/spellchecker/cocoa_spelling_engine_mac.cc b/chrome/renderer/spellchecker/cocoa_spelling_engine_mac.cc
index 463940e..4ad831d 100644
--- a/chrome/renderer/spellchecker/cocoa_spelling_engine_mac.cc
+++ b/chrome/renderer/spellchecker/cocoa_spelling_engine_mac.cc
@@ -13,6 +13,11 @@ SpellingEngine* CreateNativeSpellingEngine() {
   return new CocoaSpellingEngine();
 }
 
+void CocoaSpellingEngine::Init(base::PlatformFile bdict_file,
+                               const std::vector<std::string>&) {
+  DCHECK(bdict_file == base::kInvalidPlatformFileValue);
+}
+
 bool CocoaSpellingEngine::InitializeIfNeeded() {
   return false;  // We never need to initialize.
 }
diff --git a/chrome/renderer/spellchecker/cocoa_spelling_engine_mac.h b/chrome/renderer/spellchecker/cocoa_spelling_engine_mac.h
index c2856c6..0e7c7f4 100644
--- a/chrome/renderer/spellchecker/cocoa_spelling_engine_mac.h
+++ b/chrome/renderer/spellchecker/cocoa_spelling_engine_mac.h
@@ -10,6 +10,8 @@
 
 class CocoaSpellingEngine : public SpellingEngine {
  public:
+  virtual void Init(base::PlatformFile bdict_file,
+                    const std::vector<std::string>& custom_words) OVERRIDE;
   virtual bool InitializeIfNeeded() OVERRIDE;
   virtual bool IsEnabled() OVERRIDE;
   virtual bool CheckSpelling(const string16& word_to_check, int tag) OVERRIDE;
diff --git a/chrome/renderer/spellchecker/hunspell_engine.h b/chrome/renderer/spellchecker/hunspell_engine.h
index f96e5de..981c48e 100644
--- a/chrome/renderer/spellchecker/hunspell_engine.h
+++ b/chrome/renderer/spellchecker/hunspell_engine.h
@@ -22,8 +22,8 @@ class HunspellEngine : public SpellingEngine {
   HunspellEngine();
   virtual ~HunspellEngine();
 
-  void Init(base::PlatformFile file,
-            const std::vector<std::string>& custom_words);
+  virtual void Init(base::PlatformFile file,
+                    const std::vector<std::string>& custom_words) OVERRIDE;
 
   virtual bool InitializeIfNeeded() OVERRIDE;
   virtual bool IsEnabled() OVERRIDE;
diff --git a/chrome/renderer/spellchecker/spellcheck.cc b/chrome/renderer/spellchecker/spellcheck.cc
index 01eec7e..4daac84 100644
--- a/chrome/renderer/spellchecker/spellcheck.cc
+++ b/chrome/renderer/spellchecker/spellcheck.cc
@@ -46,6 +46,7 @@ class SpellCheck::SpellcheckRequest {
 };
 
 SpellCheck::SpellCheck() : auto_spell_correct_turned_on_(false) {
+  platform_spelling_engine_.reset(CreateNativeSpellingEngine());
 }
 
 SpellCheck::~SpellCheck() {
@@ -96,19 +97,8 @@ void SpellCheck::OnEnableAutoSpellCorrect(bool enable) {
 void SpellCheck::Init(base::PlatformFile file,
                       const std::vector<std::string>& custom_words,
                       const std::string& language) {
-  bool use_platform_spelling_engine =
-      file == base::kInvalidPlatformFileValue && !language.empty();
-
-  // Some tests under OSX still exercise hunspell. Only init native engine
-  // when no dictionary was specified.
-  // TODO(groby): Figure out if we can kill the hunspell dependency for OSX.
-  if (use_platform_spelling_engine) {
-    platform_spelling_engine_.reset(CreateNativeSpellingEngine());
-  } else {
-    HunspellEngine* engine = new HunspellEngine;
-    engine->Init(file, custom_words);
-    platform_spelling_engine_.reset(engine);
-  }
+  platform_spelling_engine_->Init(file, custom_words);
+
   character_attributes_.SetDefaultLanguage(language);
   text_iterator_.Reset();
   contraction_iterator_.Reset();
@@ -287,13 +277,8 @@ void SpellCheck::RequestTextChecking(
 }
 #endif
 
-
 bool SpellCheck::InitializeIfNeeded() {
-  // TODO(groby): OSX creates a hunspell engine here, too. That seems
-  // wrong, but is (AFAICT) the existing flow. Fix that.
-  if (!platform_spelling_engine_.get())
-    platform_spelling_engine_.reset(new HunspellEngine);
-
+  DCHECK(platform_spelling_engine_.get());
   return platform_spelling_engine_->InitializeIfNeeded();
 }
 
@@ -365,8 +350,8 @@ bool SpellCheck::IsValidContraction(const string16& contraction, int tag) {
   return true;
 }
 
-#if !defined(OS_MACOSX)
 void SpellCheck::CreateTextCheckingResults(
+    ResultFilter filter,
     int line_offset,
     const string16& line_text,
     const std::vector<SpellCheckResult>& spellcheck_results,
@@ -381,7 +366,8 @@ void SpellCheck::CreateTextCheckingResults(
         static_cast<WebTextCheckingType>(spellcheck_results[i].type);
     int word_location = spellcheck_results[i].location;
     int word_length = spellcheck_results[i].length;
-    if (type == WebKit::WebTextCheckingTypeSpelling) {
+    if (type == WebKit::WebTextCheckingTypeSpelling &&
+        filter == USE_NATIVE_CHECKER) {
       int misspelling_start = 0;
       int misspelling_length = 0;
       if (SpellCheckWord(text + word_location, word_length, 0,
@@ -396,4 +382,3 @@ void SpellCheck::CreateTextCheckingResults(
   }
   textcheck_results->swap(list);
 }
-#endif
diff --git a/chrome/renderer/spellchecker/spellcheck.h b/chrome/renderer/spellchecker/spellcheck.h
index 7c32d66..851b43e 100644
--- a/chrome/renderer/spellchecker/spellcheck.h
+++ b/chrome/renderer/spellchecker/spellcheck.h
@@ -37,6 +37,11 @@ struct WebTextCheckingResult;
 class SpellCheck : public content::RenderProcessObserver,
                    public base::SupportsWeakPtr<SpellCheck> {
  public:
+  enum ResultFilter {
+    DO_NOT_MODIFY = 1,  // Do not modify results.
+    USE_NATIVE_CHECKER,  // Use native checker to double-check.
+  };
+
   SpellCheck();
   virtual ~SpellCheck();
 
@@ -89,12 +94,14 @@ class SpellCheck : public content::RenderProcessObserver,
   // checks misspelled words returned by the Spelling service and changes the
   // underline colors of contextually-misspelled words.
   void CreateTextCheckingResults(
+      ResultFilter filter,
       int line_offset,
       const string16& line_text,
       const std::vector<SpellCheckResult>& spellcheck_results,
       WebKit::WebVector<WebKit::WebTextCheckingResult>* textcheck_results);
 
  private:
+  friend class SpellCheckTest;
   FRIEND_TEST_ALL_PREFIXES(SpellCheckTest, GetAutoCorrectionWord_EN_US);
   FRIEND_TEST_ALL_PREFIXES(SpellCheckTest,
       RequestSpellCheckMultipleTimesWithoutInitialization);
diff --git a/chrome/renderer/spellchecker/spellcheck_provider.cc b/chrome/renderer/spellchecker/spellcheck_provider.cc
index 9f643b4..c71e415a 100644
--- a/chrome/renderer/spellchecker/spellcheck_provider.cc
+++ b/chrome/renderer/spellchecker/spellcheck_provider.cc
@@ -41,28 +41,6 @@ COMPILE_ASSERT(int(WebKit::WebTextCheckingTypeCorrection) ==
 COMPILE_ASSERT(int(WebKit::WebTextCheckingTypeShowCorrectionPanel) ==
                int(SpellCheckResult::SHOWCORRECTIONPANEL), mismatching_enums);
 
-namespace {
-
-// Converts a vector of SpellCheckResult objects (used by Chrome) to a vector of
-// WebTextCheckingResult objects (used by WebKit).
-void CreateTextCheckingResults(
-    int offset,
-    const std::vector<SpellCheckResult>& spellcheck_results,
-    WebKit::WebVector<WebKit::WebTextCheckingResult>* textcheck_results) {
-  size_t result_size = spellcheck_results.size();
-  WebKit::WebVector<WebKit::WebTextCheckingResult> list(result_size);
-  for (size_t i = 0; i < result_size; ++i) {
-    list[i] = WebTextCheckingResult(
-        static_cast<WebTextCheckingType>(spellcheck_results[i].type),
-        spellcheck_results[i].location + offset,
-        spellcheck_results[i].length,
-        spellcheck_results[i].replacement);
-  }
-  textcheck_results->swap(list);
-}
-
-}  // namespace
-
 SpellCheckProvider::SpellCheckProvider(
     content::RenderView* render_view,
     SpellCheck* spellcheck)
@@ -70,14 +48,14 @@ SpellCheckProvider::SpellCheckProvider(
       content::RenderViewObserverTracker<SpellCheckProvider>(render_view),
       spelling_panel_visible_(false),
       spellcheck_(spellcheck) {
+  DCHECK(spellcheck_);
   if (render_view)  // NULL in unit tests.
     render_view->GetWebView()->setSpellCheckClient(this);
 }
 
 SpellCheckProvider::~SpellCheckProvider() {
 #if defined(OS_MACOSX)
-  Send(new SpellCheckHostMsg_DocumentClosed(
-      routing_id(), routing_id()));
+  Send(new SpellCheckHostMsg_DocumentClosed(routing_id(), routing_id()));
 #endif
 }
 
@@ -111,7 +89,6 @@ void SpellCheckProvider::RequestTextChecking(
   last_request_.clear();
   last_results_.assign(WebKit::WebVector<WebKit::WebTextCheckingResult>());
 
-
   Send(new SpellCheckHostMsg_CallSpellingService(
       routing_id(),
       text_check_completions_.Add(completion),
@@ -163,19 +140,16 @@ void SpellCheckProvider::spellCheck(
     int& length,
     WebVector<WebString>* optional_suggestions) {
   string16 word(text);
-  // Will be NULL during unit tests.
-  if (spellcheck_) {
-    std::vector<string16> suggestions;
-    spellcheck_->SpellCheckWord(
-        word.c_str(), word.size(), routing_id(),
-        &offset, &length, optional_suggestions ? & suggestions : NULL);
-    if (optional_suggestions)
-      *optional_suggestions = suggestions;
-    if (!optional_suggestions) {
-      // If optional_suggestions is not requested, the API is called
-      // for marking.  So we use this for counting markable words.
-      Send(new SpellCheckHostMsg_NotifyChecked(routing_id(), word, 0 < length));
-    }
+  std::vector<string16> suggestions;
+  spellcheck_->SpellCheckWord(
+      word.c_str(), word.size(), routing_id(),
+      &offset, &length, optional_suggestions ? & suggestions : NULL);
+  if (optional_suggestions)
+    *optional_suggestions = suggestions;
+  if (!optional_suggestions) {
+    // If optional_suggestions is not requested, the API is called
+    // for marking.  So we use this for counting markable words.
+    Send(new SpellCheckHostMsg_NotifyChecked(routing_id(), word, 0 < length));
   }
 }
 
@@ -192,10 +166,6 @@ void SpellCheckProvider::checkTextOfParagraph(
   if (!(mask & WebKit::WebTextCheckingTypeSpelling))
     return;
 
-  // Will be NULL during unit tets.
-  if (!spellcheck_)
-    return;
-
   spellcheck_->SpellCheckParagraph(string16(text), results);
 #endif
 }
@@ -208,12 +178,8 @@ void SpellCheckProvider::requestCheckingOfText(
 
 WebString SpellCheckProvider::autoCorrectWord(const WebString& word) {
   const CommandLine& command_line = *CommandLine::ForCurrentProcess();
-  if (command_line.HasSwitch(switches::kEnableSpellingAutoCorrect)) {
-    // Will be NULL during unit tests.
-    if (spellcheck_) {
-      return spellcheck_->GetAutoCorrectionWord(word, routing_id());
-    }
-  }
+  if (command_line.HasSwitch(switches::kEnableSpellingAutoCorrect))
+    return spellcheck_->GetAutoCorrectionWord(word, routing_id());
   return string16();
 }
 
@@ -250,22 +216,18 @@ void SpellCheckProvider::OnRespondSpellingService(
 
   // If |succeeded| is false, we use local spellcheck as a fallback.
   if (!succeeded) {
-    // |spellcheck_| may be NULL in unit tests.
-    if (spellcheck_) {
-      spellcheck_->RequestTextChecking(line, offset, completion);
-      return;
-    }
+    spellcheck_->RequestTextChecking(line, offset, completion);
+    return;
   }
 
   // Double-check the returned spellchecking results with our spellchecker to
   // visualize the differences between ours and the on-line spellchecker.
   WebKit::WebVector<WebKit::WebTextCheckingResult> textcheck_results;
-  if (spellcheck_) {
-    spellcheck_->CreateTextCheckingResults(
-        offset, line, results, &textcheck_results);
-  } else {
-    CreateTextCheckingResults(offset, results, &textcheck_results);
-  }
+  spellcheck_->CreateTextCheckingResults(SpellCheck::USE_NATIVE_CHECKER,
+                                         offset,
+                                         line,
+                                         results,
+                                         &textcheck_results);
   completion->didFinishCheckingText(textcheck_results);
 
   // Cache the request and the converted results.
@@ -300,13 +262,18 @@ void SpellCheckProvider::OnAdvanceToNextMisspelling() {
 void SpellCheckProvider::OnRespondTextCheck(
     int identifier,
     const std::vector<SpellCheckResult>& results) {
+  DCHECK(spellcheck_);
   WebTextCheckingCompletion* completion =
       text_check_completions_.Lookup(identifier);
   if (!completion)
     return;
   text_check_completions_.Remove(identifier);
   WebKit::WebVector<WebKit::WebTextCheckingResult> textcheck_results;
-  CreateTextCheckingResults(0, results, &textcheck_results);
+  spellcheck_->CreateTextCheckingResults(SpellCheck::DO_NOT_MODIFY,
+                                         0,
+                                         string16(),
+                                         results,
+                                         &textcheck_results);
   completion->didFinishCheckingText(textcheck_results);
 }
 
diff --git a/chrome/renderer/spellchecker/spellcheck_provider_test.cc b/chrome/renderer/spellchecker/spellcheck_provider_test.cc
index 0762899..22bd6a2 100644
--- a/chrome/renderer/spellchecker/spellcheck_provider_test.cc
+++ b/chrome/renderer/spellchecker/spellcheck_provider_test.cc
@@ -6,8 +6,12 @@
 
 #include "base/stl_util.h"
 #include "chrome/common/spellcheck_messages.h"
+#include "chrome/renderer/spellchecker/spellcheck.h"
 #include "ipc/ipc_message_macros.h"
 
+class MockSpellcheck: public SpellCheck {
+};
+
 FakeTextCheckingCompletion::FakeTextCheckingCompletion()
 : completion_count_(0),
   cancellation_count_(0) {
@@ -27,12 +31,13 @@ void FakeTextCheckingCompletion::didCancelCheckingText() {
 }
 
 TestingSpellCheckProvider::TestingSpellCheckProvider()
-      : SpellCheckProvider(NULL, NULL),
+      : SpellCheckProvider(NULL, new MockSpellcheck),
         offset_(-1) {
 }
 
 TestingSpellCheckProvider::~TestingSpellCheckProvider() {
-    STLDeleteContainerPointers(messages_.begin(), messages_.end());
+  STLDeleteContainerPointers(messages_.begin(), messages_.end());
+  delete spellcheck_;
 }
 
 bool TestingSpellCheckProvider::Send(IPC::Message* message)  {
diff --git a/chrome/renderer/spellchecker/spellcheck_unittest.cc b/chrome/renderer/spellchecker/spellcheck_unittest.cc
index a617a61..e581a0c 100644
--- a/chrome/renderer/spellchecker/spellcheck_unittest.cc
+++ b/chrome/renderer/spellchecker/spellcheck_unittest.cc
@@ -10,6 +10,7 @@
 #include "base/platform_file.h"
 #include "base/sys_string_conversions.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/renderer/spellchecker/hunspell_engine.h"
 #include "chrome/renderer/spellchecker/spellcheck.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/spellcheck_common.h"
@@ -54,6 +55,11 @@ class SpellCheckTest : public testing::Test {
         chrome::spellcheck_common::GetVersionedFileName(language,
             hunspell_directory),
         base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ, NULL, NULL);
+#if defined(OS_MACOSX)
+    // TODO(groby): Forcing spellcheck to use hunspell, even on OSX.
+    // Instead, tests should exercise individual spelling engines.
+    spell_check_->platform_spelling_engine_.reset(new HunspellEngine);
+#endif
     spell_check_->Init(
         file, std::vector<std::string>(), language);
   }
@@ -1064,8 +1070,11 @@ TEST_F(SpellCheckTest, CreateTextCheckingResults) {
     spellcheck_results.push_back(SpellCheckResult(
         SpellCheckResult::SPELLING, 0, 2, string16()));
     WebKit::WebVector<WebKit::WebTextCheckingResult> textcheck_results;
-    spell_check()->CreateTextCheckingResults(
-        0, text, spellcheck_results, &textcheck_results);
+    spell_check()->CreateTextCheckingResults(SpellCheck::USE_NATIVE_CHECKER,
+                                             0,
+                                             text,
+                                             spellcheck_results,
+                                             &textcheck_results);
     EXPECT_EQ(spellcheck_results.size(), textcheck_results.size());
     EXPECT_EQ(WebKit::WebTextCheckingTypeSpelling, textcheck_results[0].type);
     EXPECT_EQ(spellcheck_results[0].location, textcheck_results[0].location);
@@ -1080,8 +1089,11 @@ TEST_F(SpellCheckTest, CreateTextCheckingResults) {
     spellcheck_results.push_back(SpellCheckResult(
         SpellCheckResult::SPELLING, 7, 4, string16()));
     WebKit::WebVector<WebKit::WebTextCheckingResult> textcheck_results;
-    spell_check()->CreateTextCheckingResults(
-        0, text, spellcheck_results, &textcheck_results);
+    spell_check()->CreateTextCheckingResults(SpellCheck::USE_NATIVE_CHECKER,
+                                             0,
+                                             text,
+                                             spellcheck_results,
+                                             &textcheck_results);
     EXPECT_EQ(spellcheck_results.size(), textcheck_results.size());
     EXPECT_EQ(WebKit::WebTextCheckingTypeGrammar, textcheck_results[0].type);
     EXPECT_EQ(spellcheck_results[0].location, textcheck_results[0].location);
diff --git a/chrome/renderer/spellchecker/spelling_engine.h b/chrome/renderer/spellchecker/spelling_engine.h
index 0901843..b3b030c 100644
--- a/chrome/renderer/spellchecker/spelling_engine.h
+++ b/chrome/renderer/spellchecker/spelling_engine.h
@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "base/platform_file.h"
 #include "base/string16.h"
 
 // Creates the platform's "native" spelling engine.
@@ -18,6 +19,10 @@ class SpellingEngine {
  public:
   virtual ~SpellingEngine() {}
 
+  // Initialize spelling engine with browser-side info. Must be called before
+  // any other functions are called.
+  virtual void Init(base::PlatformFile bdict_file,
+                    const std::vector<std::string>& custom_words) = 0;
   virtual bool InitializeIfNeeded() = 0;
   virtual bool IsEnabled() = 0;
   virtual bool CheckSpelling(const string16& word_to_check, int tag) = 0;