TextBreakIterator should use the C++ icu API instead of the C one

Use icu::BreakIterator instead of the ubrk_* api. It makes the code cleaner.

BUG=232922
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159441

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159508

[igoroliveira, 2013-09-17 21:41:13]

The currently code depends of one small change in third_party/icu. I will upload
the patch soon.

[leviw_travelin_and_unemployed, 2013-09-17 21:53:39]

lgtm this is way better.

https://codereview.chromium.org/23618052/diff/1/Source/core/editing/TextCheck...
File Source/core/editing/TextCheckingHelper.cpp (right):

https://codereview.chromium.org/23618052/diff/1/Source/core/editing/TextCheck...
Source/core/editing/TextCheckingHelper.cpp:78: int wordEnd =
iterator->current();
It seems strange to me that these 2 lines are the same now.

[igoroliveira, 2013-09-17 21:55:12]

On 2013/09/17 21:53:39, Levi wrote:
> lgtm this is way better.
> 
>
https://codereview.chromium.org/23618052/diff/1/Source/core/editing/TextCheck...
> File Source/core/editing/TextCheckingHelper.cpp (right):
> 
>
https://codereview.chromium.org/23618052/diff/1/Source/core/editing/TextCheck...
> Source/core/editing/TextCheckingHelper.cpp:78: int wordEnd =
> iterator->current();
> It seems strange to me that these 2 lines are the same now.

Yeah my fault. Fixing.

[jungshik at Google, 2013-09-18 00:26:01]

Thank you for the CL. 

You can add "BUG=232922" which is about this kind of changes. That is, instead
of using C API (which was required because Safari on Mac OS X cannot rely on C++
API), use C++ API if it gives us an advantage over C API. In this case, it
clearly does because 1) ICU's C API is a thin wrapper over C++ break iterator
(so using C++ API saves some cycles) 2) Blink code itself gets cleaner by using
C++ API.

[igoroliveira, 2013-09-18 20:54:52]

On 2013/09/18 00:26:01, Jungshik Shin wrote:
> Thank you for the CL. 
> 
> You can add "BUG=232922" which is about this kind of changes. That is, instead
> of using C API (which was required because Safari on Mac OS X cannot rely on
C++
> API), use C++ API if it gives us an advantage over C API. In this case, it
> clearly does because 1) ICU's C API is a thin wrapper over C++ break iterator
> (so using C++ API saves some cycles) 2) Blink code itself gets cleaner by
using
> C++ API.

Updated patch fixing several bugs.

[leviw_travelin_and_unemployed, 2013-09-20 23:24:40]

This patch doesn't compile :(

[igoroliveira, 2013-09-20 23:38:04]

On 2013/09/20 23:24:40, Levi wrote:
> This patch doesn't compile :(

Yes the https://codereview.chromium.org/23480090/ needs to be committed first.

[igoroliveira, 2013-09-24 21:03:42]

On 2013/09/20 23:38:04, Igor Oliveira wrote:
> On 2013/09/20 23:24:40, Levi wrote:
> > This patch doesn't compile :(
> 
> Yes the https://codereview.chromium.org/23480090/ needs to be committed first.

Ok trying again. Improved a bit the code and now
https://codereview.chromium.org/23480090/ was committed.

[jungshik at Google, 2013-09-24 22:56:28]

On 2013/09/24 21:03:42, Igor Oliveira wrote:
> On 2013/09/20 23:38:04, Igor Oliveira wrote:
> > On 2013/09/20 23:24:40, Levi wrote:
> > > This patch doesn't compile :(
> > 
> > Yes the https://codereview.chromium.org/23480090/ needs to be committed
first.
> 
> Ok trying again. Improved a bit the code and now
> https://codereview.chromium.org/23480090/ was committed.

ICU was rolled to include your ICU CL. So, this CL should work fine now.

[igoroliveira, 2013-10-10 22:14:44]

Levi, Could you take a look? I updated/rebased the patch.

On 2013/09/24 22:56:28, Jungshik Shin wrote:
> On 2013/09/24 21:03:42, Igor Oliveira wrote:
> > On 2013/09/20 23:38:04, Igor Oliveira wrote:
> > > On 2013/09/20 23:24:40, Levi wrote:
> > > > This patch doesn't compile :(
> > > 
> > > Yes the https://codereview.chromium.org/23480090/ needs to be committed
> first.
> > 
> > Ok trying again. Improved a bit the code and now
> > https://codereview.chromium.org/23480090/ was committed.
> 
> ICU was rolled to include your ICU CL. So, this CL should work fine now.

[eseidel, 2013-10-10 23:44:22]

lgtm

Looks reasonable to me.

[commit-bot: I haz the power, 2013-10-11 00:38:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/igor.o@sisa.samsung.com/23618052/28001

[commit-bot: I haz the power, 2013-10-11 06:15:11]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-10-11 06:16:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/igor.o@sisa.samsung.com/23618052/28001

[commit-bot: I haz the power, 2013-10-11 09:22:43]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-10-11 09:36:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/igor.o@sisa.samsung.com/23618052/28001

[commit-bot: I haz the power, 2013-10-11 10:17:12]

Change committed as 159441

[ojan, 2013-10-11 17:18:52]

Unfortunately, this broke the android_aosp build. As it's blocking the Blink
roll, I'm going to roll out. I'll file a bug and CC some android people.
Hopefully someone will understand what's wrong.

http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/1...

[ojan, 2013-10-11 18:06:36]

On 2013/10/11 17:18:52, ojan wrote:
> Unfortunately, this broke the android_aosp build. As it's blocking the Blink
> roll, I'm going to roll out. I'll file a bug and CC some android people.
> Hopefully someone will understand what's wrong.
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/1...

It looks like the rollout did fix it. I'm not actually sure what android people
to CC, so I'm not going to file a bug. Peter, do you know who to CC on
android_aosp build issues?

[jungshik at Google, 2013-10-11 18:37:33]

On 2013/10/11 18:06:36, ojan wrote:
> On 2013/10/11 17:18:52, ojan wrote:
> > Unfortunately, this broke the android_aosp build. As it's blocking the Blink
> > roll, I'm going to roll out. I'll file a bug and CC some android people.
> > Hopefully someone will understand what's wrong.
> > 
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/1...
> 
> It looks like the rollout did fix it. I'm not actually sure what android
people
> to CC, so I'm not going to file a bug. Peter, do you know who to CC on
> android_aosp build issues?

Aha.... Android AOSP build uses Android's ICU, doesn't it? ICU's
TextBreakIterator got getRuleStatus() only in ICU 52, but the latest AOSP (for
JB MR2) has ICU 51 or earlier. So, it'll not work with Android AOSP.

[jungshik at Google, 2013-10-11 18:38:33]

On 2013/10/11 18:37:33, Jungshik Shin wrote:
> On 2013/10/11 18:06:36, ojan wrote:
> > On 2013/10/11 17:18:52, ojan wrote:
> > > Unfortunately, this broke the android_aosp build. As it's blocking the
Blink
> > > roll, I'm going to roll out. I'll file a bug and CC some android people.
> > > Hopefully someone will understand what's wrong.
> > > 
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/1...
> > 
> > It looks like the rollout did fix it. I'm not actually sure what android
> people
> > to CC, so I'm not going to file a bug. Peter, do you know who to CC on
> > android_aosp build issues?
> 
> Aha.... Android AOSP build uses Android's ICU, doesn't it? ICU's
> TextBreakIterator got getRuleStatus() only in ICU 52, but the latest AOSP (for
> JB MR2) has ICU 51 or earlier. So, it'll not work with Android AOSP.

A work around is to "downcast" to RuleBasedBreakIterator, but it's not pretty
code-wise.

[jungshik at Google, 2013-10-11 20:18:29]

https://codereview.chromium.org/23618052/diff/28001/Source/platform/text/Text...
File Source/platform/text/TextBreakIteratorICU.cpp (right):

https://codereview.chromium.org/23618052/diff/28001/Source/platform/text/Text...
Source/platform/text/TextBreakIteratorICU.cpp:747: int ruleStatus =
iterator->getRuleStatus();
With ICU 51 or earlier (without a patch to add getRuleStatus to BreakIterator),
this would fail as shown in an AOSP build. Because the only implementation of
icu::BreakIterator in ICU is icu::RuleBasedBreakIterator, we can 'downcast'
iterator to RBBI here if we want to go with this CL.

[igoroliveira, 2013-10-11 20:29:16]

On 2013/10/11 20:18:29, Jungshik Shin wrote:
>
https://codereview.chromium.org/23618052/diff/28001/Source/platform/text/Text...
> File Source/platform/text/TextBreakIteratorICU.cpp (right):
> 
>
https://codereview.chromium.org/23618052/diff/28001/Source/platform/text/Text...
> Source/platform/text/TextBreakIteratorICU.cpp:747: int ruleStatus =
> iterator->getRuleStatus();
> With ICU 51 or earlier (without a patch to add getRuleStatus to
BreakIterator),
> this would fail as shown in an AOSP build. Because the only implementation of
> icu::BreakIterator in ICU is icu::RuleBasedBreakIterator, we can 'downcast'
> iterator to RBBI here if we want to go with this CL.

Updated the patch. Now isWordTextBreak does a downcast of TextBreakIterator to
RuleBasedBreakIterator.

[commit-bot: I haz the power, 2013-10-12 02:00:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/igor.o@sisa.samsung.com/23618052/61001

[commit-bot: I haz the power, 2013-10-12 03:46:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/igor.o@sisa.samsung.com/23618052/61001

[commit-bot: I haz the power, 2013-10-12 13:12:02]

Change committed as 159508