Refactor code to avoid direct dependency upon ICU: phishing_term_feature_extractor

Refactor code to avoid direct dependency upon ICU: phishing_term_feature_extractor

BUG=367677
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269936

[Andrew Hayden (chromium.org), 2014-05-06 13:52:36]

+mattm for the phishing code, +jshin for the trivial .h change to
break_iterator.h

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

https://codereview.chromium.org/266883010/diff/1/chrome/renderer/safe_browsin...
File chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc (right):

https://codereview.chromium.org/266883010/diff/1/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc:54:
base::i18n::BreakIterator* iterator;
scoped_ptr

https://codereview.chromium.org/266883010/diff/1/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc:152: if
(!state_->iterator->IsValid()) {
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...

https://codereview.chromium.org/266883010/diff/1/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc:170: 
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]

I'll also use scoped_ptr, as suggested in
https://codereview.chromium.org/270203003

https://codereview.chromium.org/266883010/diff/1/chrome/renderer/safe_browsin...
File chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc (right):

https://codereview.chromium.org/266883010/diff/1/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc:54:
base::i18n::BreakIterator* iterator;
On 2014/05/07 00:37:03, mattm wrote:
> scoped_ptr

Done.

https://codereview.chromium.org/266883010/diff/1/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc:152: if
(!state_->iterator->IsValid()) {
On 2014/05/07 00:37:03, mattm wrote:
> 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...

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.

https://codereview.chromium.org/266883010/diff/1/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc:170: 
On 2014/05/07 00:37:03, mattm wrote:
> 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.

Seems reasonable, done.

[Andrew Hayden (chromium.org), 2014-05-07 15:32:28]

PTAL, this patchset should address your concerns and also adds the use of
scoped_ptr for cleanliness.

[Andrew Hayden (chromium.org), 2014-05-09 15:45:30]

Hey folks, can you take another look? I believe this is ready to land! Thanks!

[mattm, 2014-05-09 23:07:11]

sorry for the delay
lgtm with nits

https://codereview.chromium.org/266883010/diff/60001/base/i18n/break_iterator.h
File base/i18n/break_iterator.h (right):

https://codereview.chromium.org/266883010/diff/60001/base/i18n/break_iterator...
base/i18n/break_iterator.h:112: }
This change is no longer needed

https://codereview.chromium.org/266883010/diff/60001/chrome/renderer/safe_bro...
File chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc (right):

https://codereview.chromium.org/266883010/diff/60001/chrome/renderer/safe_bro...
chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc:78:
~ExtractionState() {
destructor could be removed

[Andrew Hayden (chromium.org), 2014-05-12 13:07:36]

https://codereview.chromium.org/266883010/diff/60001/base/i18n/break_iterator.h
File base/i18n/break_iterator.h (right):

https://codereview.chromium.org/266883010/diff/60001/base/i18n/break_iterator...
base/i18n/break_iterator.h:112: }
On 2014/05/09 23:07:12, mattm wrote:
> This change is no longer needed

Done.

https://codereview.chromium.org/266883010/diff/60001/chrome/renderer/safe_bro...
File chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc (right):

https://codereview.chromium.org/266883010/diff/60001/chrome/renderer/safe_bro...
chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc:78:
~ExtractionState() {
On 2014/05/09 23:07:12, mattm wrote:
> destructor could be removed

Done.

[mattm, 2014-05-12 20:18:10]

lgtm

[commit-bot: I haz the power, 2014-05-12 20:18:34]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/266883010/10...

[Andrew Hayden (chromium.org), 2014-05-12 20:47:30]

On 2014/05/12 20:18:10, mattm wrote:
> lgtm

Thanks!

[commit-bot: I haz the power, 2014-05-13 00:22:11]

Change committed as 269936