Move BiDiLineIterator to base/i18n/ directory.

Move BiDiLineIterator to base/i18n/ directory.

BUG=23581
TEST=trybots

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71884

[tfarina, 2011-01-14 02:26:27]

Please, take a look.

[tony, 2011-01-14 17:48:55]

I told tfarina that this move sounded fine, but now I'm not so sure.  Brett,
what do you think about moving app/bidi_line_iterator* into base/i18n?

[brettw, 2011-01-14 19:09:51]

I think moving this is OK (I'm more willing to move stuff to base/i18n than into
base since it's a separate project that isn't necessary for everybody to
include). I would like to do a bit of cleanup at the same time, though.

http://codereview.chromium.org/6249007/diff/8001/base/i18n/bidi_line_iterator.cc
File base/i18n/bidi_line_iterator.cc (right):

http://codereview.chromium.org/6249007/diff/8001/base/i18n/bidi_line_iterator...
base/i18n/bidi_line_iterator.cc:18: UBool BiDiLineIterator::Open(const
std::wstring& text,
Can we replace wstring with string16? Then you can remove the WCHAR_T_IS_UTF32
ifdef below.

Also, can we use bool instead of UBool as the return value here?

http://codereview.chromium.org/6249007/diff/8001/base/i18n/bidi_line_iterator.h
File base/i18n/bidi_line_iterator.h (right):

http://codereview.chromium.org/6249007/diff/8001/base/i18n/bidi_line_iterator...
base/i18n/bidi_line_iterator.h:11: #include "unicode/ubidi.h"
I think we can remove this dependency and move it to the .cc file, and forward
declare this in the header as "struct UBiDi;" If this gives you weird errors,
try "typedef struct UBiDi UBiDi;"

http://codereview.chromium.org/6249007/diff/8001/base/i18n/bidi_line_iterator...
base/i18n/bidi_line_iterator.h:18: class BiDiLineIterator {
Can we put this in the base::i18n namespace?

[tfarina, 2011-01-14 23:16:10]

Brett, comments addressed. PTAL!

http://codereview.chromium.org/6249007/diff/8001/base/i18n/bidi_line_iterator.cc
File base/i18n/bidi_line_iterator.cc (right):

http://codereview.chromium.org/6249007/diff/8001/base/i18n/bidi_line_iterator...
base/i18n/bidi_line_iterator.cc:18: UBool BiDiLineIterator::Open(const
std::wstring& text,
On 2011/01/14 19:09:51, brettw wrote:
> Can we replace wstring with string16? Then you can remove the WCHAR_T_IS_UTF32
> ifdef below.
> 
> Also, can we use bool instead of UBool as the return value here?

Done.

http://codereview.chromium.org/6249007/diff/8001/base/i18n/bidi_line_iterator.h
File base/i18n/bidi_line_iterator.h (right):

http://codereview.chromium.org/6249007/diff/8001/base/i18n/bidi_line_iterator...
base/i18n/bidi_line_iterator.h:11: #include "unicode/ubidi.h"
On 2011/01/14 19:09:51, brettw wrote:
> I think we can remove this dependency and move it to the .cc file, and forward
> declare this in the header as "struct UBiDi;" If this gives you weird errors,
> try "typedef struct UBiDi UBiDi;"

It's possible to forward this struct and UBiDiLevel (an uint8_t), but it isn't
possible to forward the enum UBiDiDirection.

http://codereview.chromium.org/6249007/diff/8001/base/i18n/bidi_line_iterator...
base/i18n/bidi_line_iterator.h:18: class BiDiLineIterator {
On 2011/01/14 19:09:51, brettw wrote:
> Can we put this in the base::i18n namespace?

Done.

[tony, 2011-01-14 23:18:39]

Thiago: Some of these changes are already covered by
http://codereview.chromium.org/6315002/.  I thought you said you were going to
wait until that landed first?

[tfarina, 2011-01-14 23:21:58]

On 2011/01/14 23:18:39, tony wrote:
> Thiago: Some of these changes are already covered by
> http://codereview.chromium.org/6315002/.  I thought you said you were going to
> wait until that landed first?

Yeah, but Brett asked to do this change. So I did what he asked. Sorry. Let me
know if I have to revert the string16 changes (I assume that is what we are
talking about).

[brettw, 2011-01-14 23:26:10]

You probably should have just pointed out the problem. In this case, I would
wait for Tony's patch to land and then upload the latest diff so I can review
it.

[tfarina, 2011-01-18 19:18:49]

On 2011/01/14 23:26:10, brettw wrote:
> You probably should have just pointed out the problem. In this case, I would
> wait for Tony's patch to land and then upload the latest diff so I can review
> it.

Rebased. PTAL!

[tony, 2011-01-18 19:24:29]

http://codereview.chromium.org/6249007/diff/26001/app/app_base.gypi
File app/app_base.gypi (right):

http://codereview.chromium.org/6249007/diff/26001/app/app_base.gypi#newcode152
app/app_base.gypi:152: 'combobox_model.h',
Did you mean to add this here?

http://codereview.chromium.org/6249007/diff/26001/base/i18n/bidi_line_iterator.h
File base/i18n/bidi_line_iterator.h (right):

http://codereview.chromium.org/6249007/diff/26001/base/i18n/bidi_line_iterato...
base/i18n/bidi_line_iterator.h:11: #include "unicode/ubidi.h"
What happened when you tried to move this include into the .cc file?

[tfarina, 2011-01-18 19:34:47]

http://codereview.chromium.org/6249007/diff/26001/app/app_base.gypi
File app/app_base.gypi (right):

http://codereview.chromium.org/6249007/diff/26001/app/app_base.gypi#newcode152
app/app_base.gypi:152: 'combobox_model.h',
On 2011/01/18 19:24:29, tony wrote:
> Did you mean to add this here?

No. It seems it was a merge conflict. I didn't remove beacuse I wasn't 100%. It
seems this file is now in ui/base/models/

http://codereview.chromium.org/6249007/diff/26001/base/i18n/bidi_line_iterator.h
File base/i18n/bidi_line_iterator.h (right):

http://codereview.chromium.org/6249007/diff/26001/base/i18n/bidi_line_iterato...
base/i18n/bidi_line_iterator.h:11: #include "unicode/ubidi.h"
On 2011/01/18 19:24:29, tony wrote:
> What happened when you tried to move this include into the .cc file?

As I told to Brett:
"It's possible to forward this struct and UBiDiLevel (an uint8_t), but it isn't
possible to forward the enum UBiDiDirection."

I have the error saved somewhere, but I don't have it right now.

[tfarina, 2011-01-18 19:42:56]

http://codereview.chromium.org/6249007/diff/26001/app/app_base.gypi
File app/app_base.gypi (right):

http://codereview.chromium.org/6249007/diff/26001/app/app_base.gypi#newcode152
app/app_base.gypi:152: 'combobox_model.h',
On 2011/01/18 19:24:29, tony wrote:
> Did you mean to add this here?

Removed.

[tony, 2011-01-18 19:45:08]

LGTM!

Sorry, I missed your comment about trying to forward the enum UBiDiDirection.

[tony, 2011-01-19 18:37:43]

Still LGTM.  Please wait for the trybots to pass.