Refactor code to avoid direct dependency upon ICU: spellcheck_worditerator

chrome/renderer/spellchecker/spellcheck_worditerator.cc

 // Copyright (c) 2012 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.
 
 // Implements a custom word iterator used for our spellchecker.
 
 #include "chrome/renderer/spellchecker/spellcheck_worditerator.h"
 
 #include <map>
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/i18n/break_iterator.h"
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/renderer/spellchecker/spellcheck.h"
 #include "third_party/icu/source/common/unicode/normlzr.h"
 #include "third_party/icu/source/common/unicode/schriter.h"
 #include "third_party/icu/source/common/unicode/uscript.h"
 #include "third_party/icu/source/i18n/unicode/ulocdata.h"
 
 // SpellcheckCharAttribute implementation:
@@ skipping 269 matching lines @@
   UScriptCode script_code = uscript_getScript(c, &status);
   if (script_code == script_code_ || script_code == USCRIPT_COMMON)
     output->push_back(c);
   return true;
 }
 
 // SpellcheckWordIterator implementation:
 
 SpellcheckWordIterator::SpellcheckWordIterator()
     : text_(NULL),
-      length_(0),
-      position_(UBRK_DONE),
       attribute_(NULL),
-      iterator_(NULL) {
+      iterator_() {
 }
 
 SpellcheckWordIterator::~SpellcheckWordIterator() {
   Reset();
 }
 
 bool SpellcheckWordIterator::Initialize(
     const SpellcheckCharAttribute* attribute,
     bool allow_contraction) {
   // Create a custom ICU break iterator with empty text used in this object. (We
   // allow setting text later so we can re-use this iterator.)
   DCHECK(attribute);
-  UErrorCode open_status = U_ZERO_ERROR;
-  UParseError parse_status;
-  base::string16 rule(attribute->GetRuleSet(allow_contraction));
+  const base::string16 rule(attribute->GetRuleSet(allow_contraction));
 
   // If there is no rule set, the attributes were invalid.
   if (rule.empty())
     return false;
 
-  iterator_ = ubrk_openRules(rule.c_str(), rule.length(), NULL, 0,
-                             &parse_status, &open_status);
-  if (U_FAILURE(open_status))
+  scoped_ptr<base::i18n::BreakIterator> iterator(
+      new base::i18n::BreakIterator(base::string16(), rule));
+  if (!iterator->Init()) {
+    // Since we're not passing in any text, the only reason this could fail
+    // is if we fail to parse the rules. Since the rules are hardcoded,
+    // that would be a bug in this class.
+    NOTREACHED() << "failed to open iterator (broken rules)";
     return false;
+  }
+  iterator_ = iterator.Pass();
 
   // Set the character attributes so we can normalize the words extracted by
   // this iterator.
   attribute_ = attribute;
   return true;
 }
 
 bool SpellcheckWordIterator::IsInitialized() const {
-  // Return true if we have an ICU custom iterator.
+  // Return true iff we have an iterator.
   return !!iterator_;
 }
 
 bool SpellcheckWordIterator::SetText(const base::char16* text, size_t length) {
   DCHECK(!!iterator_);
 
   // Set the text to be split by this iterator.
-  UErrorCode status = U_ZERO_ERROR;
-  ubrk_setText(iterator_, text, length, &status);
-  if (U_FAILURE(status))
+  if (!iterator_->SetText(text, length)) {
+    LOG(ERROR) << "failed to set text";
     return false;
+  }
 
-  // Retrieve the position to the first word in this text. We return false if
-  // this text does not have any words. (For example, The input text consists
-  // only of Chinese characters while the spellchecker language is English.)
-  position_ = ubrk_first(iterator_);
-  if (position_ == UBRK_DONE)
+  // Return false if this text does not have any words. (For example, the

[Andrew Hayden (chromium.org), 2014/05/12 13:19:37]

This turns out to be useless now. I asked the ICU mailing list if calling
ubrk_first (which is what happens after setting the text) can ever return a
non-zero value, and the answer is no. IsValid is based on this, so it will
always - ALWAYS - be true after setting text or calling init. This comment has
been incorrect since it was put here, as far as I can tell, and is useless. The
later loop on Advance() will function correctly in all cases. We should take
this out, as there's no way to do this detection correctly without iterating
over the whole string in advance.

[groby-ooo-7-16, 2014/05/12 20:14:49]

Hm. As long as the tests pass, I suppose we're good. There's never been a test
for the "false" case, so they should still pass ;)

(Too bad SetText can still fail... I'd love to make this void. Oh well)

+  // input text consists only of Chinese characters while the spellchecker
+  // language is English.)
+  if (!iterator_->IsValid())
     return false;
 
   text_ = text;

[groby-ooo-7-16, 2014/05/12 20:14:49]

I _think_ all references to _text can go. There's no reason to keep that alive
any more, either.

-  length_ = static_cast<int>(length);
   return true;
 }
 
 bool SpellcheckWordIterator::GetNextWord(base::string16* word_string,
                                          int* word_start,
                                          int* word_length) {
-  DCHECK(!!text_ && length_ > 0);
+  DCHECK(!!text_);
 
   word_string->clear();
   *word_start = 0;
   *word_length = 0;
 
-  if (!text_ || position_ == UBRK_DONE)
+  if (!text_ || !iterator_->IsValid())

[Andrew Hayden (chromium.org), 2014/05/12 13:19:37]

Similarly, here, the only time that IsValid() will be false is after we get
through the entire string. The while loop below already guards on Advance()
returning true, which takes care of it properly. This check is completely
unnecessary.

[groby-ooo-7-16, 2014/05/12 20:14:49]

Since this is documented API behavior via the BreakIterator unittests, yeah, I'd
say this can be removed.

     return false;
 
   // Find a word that can be checked for spelling. Our rule sets filter out
   // invalid words (e.g. numbers and characters not supported by the
   // spellchecker language) so this ubrk_getRuleStatus() call returns
   // UBRK_WORD_NONE when this iterator finds an invalid word. So, we skip such
   // words until we can find a valid word or reach the end of the input string.
-  int next = ubrk_next(iterator_);
-  while (next != UBRK_DONE) {
-    if (ubrk_getRuleStatus(iterator_) != UBRK_WORD_NONE) {
-      if (Normalize(position_, next - position_, word_string)) {
-        *word_start = position_;
-        *word_length = next - position_;
-        position_ = next;
+  while (iterator_->Advance()) {
+    const size_t start = iterator_->prev();
+    const size_t length = iterator_->pos() - start;
+    if (iterator_->IsWord()) {
+      if (Normalize(start, length, word_string)) {
+        *word_start = start;
+        *word_length = length;
         return true;
       }
     }
-    position_ = next;
-    next = ubrk_next(iterator_);
   }
 
-  // There aren't any more words in the given text. Set the position to
-  // UBRK_DONE to prevent from calling ubrk_next() next time when this function
-  // is called.
-  position_ = UBRK_DONE;
+  // There aren't any more words in the given text.
   return false;
 }
 
 void SpellcheckWordIterator::Reset() {
-  if (iterator_) {
-    ubrk_close(iterator_);
-    iterator_ = NULL;
-  }
+  iterator_.reset();
 }
 
 bool SpellcheckWordIterator::Normalize(int input_start,
                                        int input_length,
                                        base::string16* output_string) const {
   // We use NFKC (Normalization Form, Compatible decomposition, followed by
   // canonical Composition) defined in Unicode Standard Annex #15 to normalize
   // this token because it it the most suitable normalization algorithm for our
   // spellchecker. Nevertheless, it is not a perfect algorithm for our
   // spellchecker and we need manual normalization as well. The normalized
   // text does not have to be NUL-terminated since its characters are copied to
   // string16, which adds a NUL character when we need.
   icu::UnicodeString input(FALSE, &text_[input_start], input_length);
   UErrorCode status = U_ZERO_ERROR;
   icu::UnicodeString output;
   icu::Normalizer::normalize(input, UNORM_NFKC, 0, output, status);
   if (status != U_ZERO_ERROR && status != U_STRING_NOT_TERMINATED_WARNING)
     return false;
 
   // Copy the normalized text to the output.
   icu::StringCharacterIterator it(output);
   for (UChar c = it.first(); c != icu::CharacterIterator::DONE; c = it.next())
     attribute_->OutputChar(c, output_string);
 
   return !output_string->empty();
 }