Refactor code to avoid direct dependency upon ICU: phishing_term_feature_extractor

chrome/renderer/safe_browsing/phishing_term_feature_extractor.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.
 
 #include "chrome/renderer/safe_browsing/phishing_term_feature_extractor.h"
 
 #include <list>
 #include <map>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
+#include "base/i18n/break_iterator.h"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/renderer/safe_browsing/feature_extractor_clock.h"
 #include "chrome/renderer/safe_browsing/features.h"
 #include "chrome/renderer/safe_browsing/murmurhash3_util.h"
 #include "crypto/sha2.h"
-#include "third_party/icu/source/common/unicode/ubrk.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace safe_browsing {
 
 // This time should be short enough that it doesn't noticeably disrupt the
 // user's interaction with the page.
 const int PhishingTermFeatureExtractor::kMaxTimePerChunkMs = 10;
 
 // Experimenting shows that we get a reasonable gain in performance by
 // increasing this up to around 10, but there's not much benefit in
@@ skipping 11 matching lines @@
 struct PhishingTermFeatureExtractor::ExtractionState {
   // Stores up to max_words_per_term_ previous words separated by spaces.
   std::string previous_words;
 
   // Stores the sizes of the words in previous_words.  Note: the size includes
   // the space after each word.  In other words, the sum of all sizes in this
   // list is equal to the length of previous_words.
   std::list<size_t> previous_word_sizes;
 
   // An iterator for word breaking.
-  UBreakIterator* iterator;
+  base::i18n::BreakIterator* iterator;

[mattm, 2014/05/07 00:37:03]

scoped_ptr

[Andrew Hayden (chromium.org), 2014/05/07 15:31:58]

Done.

 
-  // Our current position in the text that was passed to the ExtractionState
-  // constructor, speciailly, the most recent break position returned by our
-  // iterator.
-  int position;
-
-  // True if position has been initialized.
-  bool position_initialized;
+  // True until we have advanced once.
+  bool initial;
 
   // The time at which we started feature extraction for the current page.
   base::TimeTicks start_time;
 
   // The number of iterations we've done for the current extraction.
   int num_iterations;
 
+  // Stored position from the last time we advanced the iterator.
+  size_t last_position;
+
   ExtractionState(const base::string16& text, base::TimeTicks start_time_ticks)
-      : position(-1),
-        position_initialized(false),
+      : initial(true),
         start_time(start_time_ticks),
         num_iterations(0) {
-    UErrorCode status = U_ZERO_ERROR;
-    // TODO(bryner): We should pass in the language for the document.
-    iterator = ubrk_open(UBRK_WORD, NULL,
-                         text.data(), text.size(),
-                         &status);
-    if (U_FAILURE(status)) {
-      DLOG(ERROR) << "ubrk_open failed: " << status;
+    iterator = new base::i18n::BreakIterator(
+        text, base::i18n::BreakIterator::BREAK_WORD);
+
+    if (!iterator->Init()) {
+      DLOG(ERROR) << "failed to open iterator";
+      delete iterator;
       iterator = NULL;
     }
+    last_position = iterator->pos();
   }
 
   ~ExtractionState() {
-    if (iterator) {
-      ubrk_close(iterator);
-    }
+    delete iterator;
+    iterator = NULL;
   }
 };
 
 PhishingTermFeatureExtractor::PhishingTermFeatureExtractor(
     const base::hash_set<std::string>* page_term_hashes,
     const base::hash_set<uint32>* page_word_hashes,
     size_t max_words_per_term,
     uint32 murmurhash3_seed,
     FeatureExtractorClock* clock)
     : page_term_hashes_(page_term_hashes),
@@ skipping 45 matching lines @@
   ++state_->num_iterations;
   base::TimeTicks current_chunk_start_time = clock_->Now();
 
   if (!state_->iterator) {
     // We failed to initialize the break iterator, so stop now.
     UMA_HISTOGRAM_COUNTS("SBClientPhishing.TermFeatureBreakIterError", 1);
     RunCallback(false);
     return;
   }
 
-  if (!state_->position_initialized) {
-    state_->position = ubrk_first(state_->iterator);
-    if (state_->position == UBRK_DONE) {
+  if (state_->initial) {
+    if (!state_->iterator->IsValid()) {

[mattm, 2014/05/07 00:37:03]

Looks like this doesn't work, since BreakIterator doesn't initialize pos_ from
ubrk_first:
https://code.google.com/p/chromium/codesearch#chromium/src/base/i18n/break_it...

[Andrew Hayden (chromium.org), 2014/05/07 15:31:58]

I had the same thought and went off to investigate. The results were surprising:
https://code.google.com/p/chromium/issues/detail?id=370422

Anyhow, this whole thing is useless since we're using Advance, which will return
false if it ends up being off the end of the string. So we can throw this entire
chunk away and rely on the while loop.

       // No words present, so we're done.
       RunCallback(true);
       return;
     }
-    state_->position_initialized = true;
+    state_->initial = false;
   }
 
   int num_words = 0;
-  for (int next = ubrk_next(state_->iterator);
-       next != UBRK_DONE; next = ubrk_next(state_->iterator)) {
-    if (ubrk_getRuleStatus(state_->iterator) != UBRK_WORD_NONE) {
+  while (state_->iterator->Advance()) {
+    int next = state_->iterator->pos();
+    if (state_->iterator->IsEndOfWord(next)) {
       // next is now positioned at the end of a word.
-      HandleWord(base::StringPiece16(page_text_->data() + state_->position,
-                                     next - state_->position));
+      HandleWord(base::StringPiece16(page_text_->data() + state_->last_position,
+                                     next - state_->last_position));
       ++num_words;
     }
-    state_->position = next;
+    state_->last_position = next;
 

[mattm, 2014/05/07 00:37:03]

I think all of this could be simplified by using the BreakIterator idiomatically
instead of just mechanically converting the code. If you just use IsWord(),
prev(), pos(), it seems like keeping track of next, initial and last_position is
unnecessary.

[Andrew Hayden (chromium.org), 2014/05/07 15:31:58]

Seems reasonable, done.

     if (num_words >= kClockCheckGranularity) {
       num_words = 0;
       base::TimeTicks now = clock_->Now();
       if (now - state_->start_time >=
           base::TimeDelta::FromMilliseconds(kMaxTotalTimeMs)) {
         DLOG(ERROR) << "Feature extraction took too long, giving up";
         // We expect this to happen infrequently, so record when it does.
         UMA_HISTOGRAM_COUNTS("SBClientPhishing.TermFeatureTimeout", 1);
         RunCallback(false);
         return;
@@ skipping 115 matching lines @@
 
 void PhishingTermFeatureExtractor::Clear() {
   page_text_ = NULL;
   features_ = NULL;
   done_callback_.Reset();
   state_.reset(NULL);
   negative_word_cache_.Clear();
 }
 
 }  // namespace safe_browsing