Refactor code to avoid direct dependency upon ICU: spellcheck_worditerator

chrome/renderer/spellchecker/spellcheck_worditerator.cc

Index: chrome/renderer/spellchecker/spellcheck_worditerator.cc
diff --git a/chrome/renderer/spellchecker/spellcheck_worditerator.cc b/chrome/renderer/spellchecker/spellcheck_worditerator.cc
index 1297c5ab8f202afaefa6606f26724257f3e59de2..f47abeafa6e8f195e54ac58c2e786170611c9318 100644
--- a/chrome/renderer/spellchecker/spellcheck_worditerator.cc
+++ b/chrome/renderer/spellchecker/spellcheck_worditerator.cc
@@ -10,6 +10,7 @@
 #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"
@@ -299,10 +300,8 @@ bool SpellcheckCharAttribute::OutputDefault(UChar c,
 
 SpellcheckWordIterator::SpellcheckWordIterator()
     : text_(NULL),
-      length_(0),
-      position_(UBRK_DONE),
       attribute_(NULL),
-      iterator_(NULL) {
+      iterator_() {
 }
 
 SpellcheckWordIterator::~SpellcheckWordIterator() {
@@ -315,18 +314,22 @@ bool SpellcheckWordIterator::Initialize(
   // 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.
@@ -335,7 +338,7 @@ bool SpellcheckWordIterator::Initialize(
 }
 
 bool SpellcheckWordIterator::IsInitialized() const {
-  // Return true if we have an ICU custom iterator.
+  // Return true iff we have an iterator.
   return !!iterator_;
 }
 
@@ -343,33 +346,31 @@ 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
@@ -377,32 +378,24 @@ bool SpellcheckWordIterator::GetNextWord(base::string16* word_string,
   // 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,