Refactor code to avoid direct dependency upon ICU: spellcheck_worditerator

Refactor code to avoid direct dependency upon ICU: spellcheck_worditerator

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

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

PTAL. groby@ as owner of the spellcheck code, jshin@ as owner of the
break_iterator.

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

Also please note this change depends upon an incremental improvement in
break_iterator.h that is under review in
https://codereview.chromium.org/266883010/

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

Mostly nits - thanks for cleaning this up!

https://codereview.chromium.org/270203003/diff/80001/base/i18n/break_iterator.cc
File base/i18n/break_iterator.cc (right):

https://codereview.chromium.org/270203003/diff/80001/base/i18n/break_iterator...
base/i18n/break_iterator.cc:140: if (break_type_ != BREAK_WORD && break_type_ !=
RULE_BASED)
nit: Is there a point in having BreakIterator::IsWordIterator, since this is
shared three places?

https://codereview.chromium.org/270203003/diff/80001/chrome/renderer/spellche...
File chrome/renderer/spellchecker/spellcheck_worditerator.cc (right):

https://codereview.chromium.org/270203003/diff/80001/chrome/renderer/spellche...
chrome/renderer/spellchecker/spellcheck_worditerator.cc:15: #include
"base/memory/scoped_ptr.h"
No need to include scoped_ptr.h - transitive dependency

https://codereview.chromium.org/270203003/diff/80001/chrome/renderer/spellche...
chrome/renderer/spellchecker/spellcheck_worditerator.cc:327: NOTREACHED() <<
"failed to open iterator";
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.

https://codereview.chromium.org/270203003/diff/80001/chrome/renderer/spellche...
chrome/renderer/spellchecker/spellcheck_worditerator.cc:348: NOTREACHED() <<
"failed to set text";
See above.

https://codereview.chromium.org/270203003/diff/80001/chrome/renderer/spellche...
chrome/renderer/spellchecker/spellcheck_worditerator.cc:396: iterator_.reset(0);
Just reset() - more idiomatic.

https://codereview.chromium.org/270203003/diff/80001/chrome/renderer/spellche...
File chrome/renderer/spellchecker/spellcheck_worditerator.h (right):

https://codereview.chromium.org/270203003/diff/80001/chrome/renderer/spellche...
chrome/renderer/spellchecker/spellcheck_worditerator.h:15: #include
"base/i18n/break_iterator.h"
If you forward declare BreakIterator, you don't need this.

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

https://codereview.chromium.org/270203003/diff/80001/base/i18n/break_iterator.cc
File base/i18n/break_iterator.cc (right):

https://codereview.chromium.org/270203003/diff/80001/base/i18n/break_iterator...
base/i18n/break_iterator.cc:140: if (break_type_ != BREAK_WORD && break_type_ !=
RULE_BASED)
On 2014/05/08 17:52:00, groby wrote:
> nit: Is there a point in having BreakIterator::IsWordIterator, since this is
> shared three places?

Maybe. My hope is actually that we can throw some of this away. There's only one
place I can find that needs the [Start|End]OfWord bits, and if we can refactor
that out then those two use cases go away. Unless you strongly object, I'll
leave this for now.

https://codereview.chromium.org/270203003/diff/80001/chrome/renderer/spellche...
File chrome/renderer/spellchecker/spellcheck_worditerator.cc (right):

https://codereview.chromium.org/270203003/diff/80001/chrome/renderer/spellche...
chrome/renderer/spellchecker/spellcheck_worditerator.cc:15: #include
"base/memory/scoped_ptr.h"
On 2014/05/08 17:52:00, groby wrote:
> No need to include scoped_ptr.h - transitive dependency

Done.

https://codereview.chromium.org/270203003/diff/80001/chrome/renderer/spellche...
chrome/renderer/spellchecker/spellcheck_worditerator.cc:327: NOTREACHED() <<
"failed to open iterator";
On 2014/05/08 17:52:00, groby wrote:
> 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.

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.

https://codereview.chromium.org/270203003/diff/80001/chrome/renderer/spellche...
chrome/renderer/spellchecker/spellcheck_worditerator.cc:348: NOTREACHED() <<
"failed to set text";
On 2014/05/08 17:52:00, groby wrote:
> See above.

Done.

https://codereview.chromium.org/270203003/diff/80001/chrome/renderer/spellche...
chrome/renderer/spellchecker/spellcheck_worditerator.cc:396: iterator_.reset(0);
On 2014/05/08 17:52:00, groby wrote:
> Just reset() - more idiomatic.

Done.

https://codereview.chromium.org/270203003/diff/80001/chrome/renderer/spellche...
File chrome/renderer/spellchecker/spellcheck_worditerator.h (right):

https://codereview.chromium.org/270203003/diff/80001/chrome/renderer/spellche...
chrome/renderer/spellchecker/spellcheck_worditerator.h:15: #include
"base/i18n/break_iterator.h"
On 2014/05/08 17:52:00, groby wrote:
> If you forward declare BreakIterator, you don't need this.

Done.

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

PTAL. Thanks!

[groby-ooo-7-16, 2014-05-09 20:05:15]

LGTM for spellcheck

[Andrew Hayden (chromium.org), 2014-05-12 11:37:40]

Thanks, groby@. Jungshik, can you take a look?

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

I removed some code that didn't do anything, and verified that unit tests still
pass:

$ ./out/Debug/unit_tests "--gtest_filter=SpellcheckWordIteratorTest.*"
--single-process-tests                                       
Note: Google Test filter = SpellcheckWordIteratorTest.*
[==========] Running 4 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 4 tests from SpellcheckWordIteratorTest
[ RUN      ] SpellcheckWordIteratorTest.SplitWord
[       OK ] SpellcheckWordIteratorTest.SplitWord (255 ms)
[ RUN      ] SpellcheckWordIteratorTest.RuleSetConsistency
[       OK ] SpellcheckWordIteratorTest.RuleSetConsistency (22 ms)
[ RUN      ] SpellcheckWordIteratorTest.TreatNumbersAsWordCharacters
[       OK ] SpellcheckWordIteratorTest.TreatNumbersAsWordCharacters (223 ms)
[ RUN      ] SpellcheckWordIteratorTest.Initialization
[       OK ] SpellcheckWordIteratorTest.Initialization (21 ms)
[----------] 4 tests from SpellcheckWordIteratorTest (522 ms total)

[----------] Global test environment tear-down
[==========] 4 tests from 1 test case ran. (524 ms total)
[  PASSED  ] 4 tests.

https://codereview.chromium.org/270203003/diff/120001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_worditerator.cc (right):

https://codereview.chromium.org/270203003/diff/120001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_worditerator.cc:354: // Return false if
this text does not have any words. (For example, the
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.

https://codereview.chromium.org/270203003/diff/120001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_worditerator.cc:373: if (!text_ ||
!iterator_->IsValid())
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:48]

Still LGTM % text_ nit

https://codereview.chromium.org/270203003/diff/120001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_worditerator.cc (right):

https://codereview.chromium.org/270203003/diff/120001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_worditerator.cc:354: // Return false if
this text does not have any words. (For example, the
On 2014/05/12 13:19:37, Andrew Hayden wrote:
> 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.

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)

https://codereview.chromium.org/270203003/diff/120001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_worditerator.cc:360: text_ = text;
I _think_ all references to _text can go. There's no reason to keep that alive
any more, either.

https://codereview.chromium.org/270203003/diff/120001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_worditerator.cc:373: if (!text_ ||
!iterator_->IsValid())
On 2014/05/12 13:19:37, Andrew Hayden wrote:
> 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.

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

[Andrew Hayden (chromium.org), 2014-05-12 21:02:41]

A few style nits of my own

https://codereview.chromium.org/270203003/diff/140001/base/i18n/break_iterato...
File base/i18n/break_iterator.cc (right):

https://codereview.chromium.org/270203003/diff/140001/base/i18n/break_iterato...
base/i18n/break_iterator.cc:60: iter_ = ubrk_openRules(rules_.c_str(), // rules
string
Can probably remove the comments here, they don't add much.

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

https://codereview.chromium.org/270203003/diff/140001/base/i18n/break_iterato...
base/i18n/break_iterator.h:92: // Updates the text used by the iterator,
resetting the iterator as if
Grammar: "if if"

https://codereview.chromium.org/270203003/diff/140001/base/i18n/break_iterato...
base/i18n/break_iterator.h:134: // The breaking style (word/space/newline).
Mutually exclusive with rules_
Leftover comment cruft, we have RULE_BASED in the enum.

[jungshik at Google, 2014-05-13 06:32:44]

https://codereview.chromium.org/270203003/diff/140001/base/i18n/break_iterato...
File base/i18n/break_iterator.cc (right):

https://codereview.chromium.org/270203003/diff/140001/base/i18n/break_iterato...
base/i18n/break_iterator.cc:60: iter_ = ubrk_openRules(rules_.c_str(), // rules
string
On 2014/05/12 21:02:41, Andrew Hayden wrote:
> Can probably remove the comments here, they don't add much.

It's not you but I'm just noting here that 
we could have used std::string (instead of string16) if we had used ICU C++ API.

https://codereview.chromium.org/270203003/diff/140001/base/i18n/break_iterato...
base/i18n/break_iterator.cc:129: pos_ =
ubrk_first(static_cast<UBreakIterator*>(iter_));
I'm not sure if it's a good idea to combine two APIs into one just because a
caller (spellchecker_breakiterator does).

[Andrew Hayden (chromium.org), 2014-05-13 08:45:49]

https://codereview.chromium.org/270203003/diff/140001/base/i18n/break_iterato...
File base/i18n/break_iterator.cc (right):

https://codereview.chromium.org/270203003/diff/140001/base/i18n/break_iterato...
base/i18n/break_iterator.cc:129: pos_ =
ubrk_first(static_cast<UBreakIterator*>(iter_));
On 2014/05/13 06:32:44, Jungshik Shin wrote:
> I'm not sure if it's a good idea to combine two APIs into one just because a
> caller (spellchecker_breakiterator does). 

I cracked open the code in rbbi.cpp, and it does first() implicitly in the call
to setText. So we can just get rid of the call to ubrk_first and set pos_=0,
since we know from the ICU mailing list that first() always returns zero.

[Andrew Hayden (chromium.org), 2014-05-14 17:46:09]

Addressed the comment about ubrk_first. Jungshik, can you PTAL?

[Andrew Hayden (chromium.org), 2014-06-10 13:53:55]

On 2014/05/14 17:46:09, Andrew Hayden wrote:
> Addressed the comment about ubrk_first. Jungshik, can you PTAL?

Jungshik: Are you ok with this change? I'd really like to get it landed.

[jungshik at Google, 2014-06-12 23:07:40]

LGTM Sorry for the delay.

[Andrew Hayden (chromium.org), 2014-06-12 23:11:08]

On 2014/06/12 23:07:40, Jungshik Shin wrote:
> LGTM Sorry for the delay.

No worries. Thanks for reviewing. I'll do a rebase and get this moving.

[Andrew Hayden (chromium.org), 2014-06-12 23:12:58]

Sweet, no merge conflicts and all the trybots are happy? Must be my lucky day.
CQ, go!

[Andrew Hayden (chromium.org), 2014-06-12 23:13:56]

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-06-12 23:16:23]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/270203003/16...

[commit-bot: I haz the power, 2014-06-13 00:42:58]

Change committed as 276869