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..633d7923dd32d55f563d41865867a8215975a646 100644
--- a/chrome/renderer/spellchecker/spellcheck_worditerator.cc
+++ b/chrome/renderer/spellchecker/spellcheck_worditerator.cc
@@ -10,7 +10,9 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/i18n/break_iterator.h"
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"

[groby-ooo-7-16, 2014/05/08 17:52:00]

No need to include scoped_ptr.h - transitive dependency

[Andrew Hayden (chromium.org), 2014/05/09 15:16:16]

Done.

 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/renderer/spellchecker/spellcheck.h"
@@ -299,10 +301,8 @@ bool SpellcheckCharAttribute::OutputDefault(UChar c,
 
 SpellcheckWordIterator::SpellcheckWordIterator()
     : text_(NULL),
-      length_(0),
-      position_(UBRK_DONE),
       attribute_(NULL),
-      iterator_(NULL) {
+      iterator_() {
 }
 
 SpellcheckWordIterator::~SpellcheckWordIterator() {
@@ -315,18 +315,19 @@ 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()) {
+    NOTREACHED() << "failed to open iterator";

[groby-ooo-7-16, 2014/05/08 17:52:00]

Why NOTREACHED? If BreakIterator::Init truly can't fail, just have it there. Or
better, have it not return an error. If it can fail, this is not unreachable.

[Andrew Hayden (chromium.org), 2014/05/09 15:16:16]

I'll add a comment here. It can't fail *in this case* unless the rules fail to
parse, which would mean (since they're hardcoded) that there was a serious code
error in this class.

     return false;
+  }
+  iterator_ = iterator.Pass();
 
   // Set the character attributes so we can normalize the words extracted by
   // this iterator.
@@ -335,7 +336,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 +344,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)) {
+    NOTREACHED() << "failed to set text";

[groby-ooo-7-16, 2014/05/08 17:52:00]

See above.

[Andrew Hayden (chromium.org), 2014/05/09 15:16:16]

Done.

     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
+  // input text consists only of Chinese characters while the spellchecker
+  // language is English.)
+  if (!iterator_->IsValid())
     return false;
 
   text_ = text;
-  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())
     return false;
 
   // Find a word that can be checked for spelling. Our rule sets filter out
@@ -377,32 +376,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(0);

[groby-ooo-7-16, 2014/05/08 17:52:00]

Just reset() - more idiomatic.

[Andrew Hayden (chromium.org), 2014/05/09 15:16:16]

Done.

 }
 
 bool SpellcheckWordIterator::Normalize(int input_start,