Add functions to get the length or compute a substring of UTF8 and UTF16...

Add functions to get the length or compute a substring of UTF8 and UTF16
encoded strings.

BUG=none
TEST=Added new unit tests for each.


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

[brettw, 2010-08-19 17:17:38]

Do you still need this for your previous patch? I'm confused since I thought
somebody suggested an alternate way for you to deal with the range problem that
I don't remember if it worked.

What I was thinking would be nice to have is a char_iterator.h like we have a
word_iterator that would allow iteration over Unicode code points in UTF-8 or
UTF-16 input strings.

[dmazzoni, 2010-08-19 17:36:53]

On Thu, Aug 19, 2010 at 10:17 AM, <brettw@chromium.org> wrote:

> Do you still need this for your previous patch? I'm confused since I
> thought
> somebody suggested an alternate way for you to deal with the range problem
> that
> I don't remember if it worked.
>

It's not enough; I want the substring corresponding to the part of the
selection that changed; Gtk only gives me the current selection.


> What I was thinking would be nice to have is a char_iterator.h like we have
> a
> word_iterator that would allow iteration over Unicode code points in UTF-8
> or
> UTF-16 input strings.


I'm happy to add this too, and in fact I might need it, but Length and
Substr still seem pretty useful - okay to keep them?

- Dominic

[brettw, 2010-08-19 18:46:26]

On Thu, Aug 19, 2010 at 10:36 AM, Dominic Mazzoni <dmazzoni@chromium.org> wrote:
> On Thu, Aug 19, 2010 at 10:17 AM, <brettw@chromium.org> wrote:
>>
>> Do you still need this for your previous patch? I'm confused since I
>> thought
>> somebody suggested an alternate way for you to deal with the range problem
>> that
>> I don't remember if it worked.
>
> It's not enough; I want the substring corresponding to the part of the
> selection that changed; Gtk only gives me the current selection.
>
>>
>> What I was thinking would be nice to have is a char_iterator.h like we
>> have a
>> word_iterator that would allow iteration over Unicode code points in UTF-8
>> or
>> UTF-16 input strings.
>
> I'm happy to add this too, and in fact I might need it, but Length and
> Substr still seem pretty useful - okay to keep them?

Do you think you'll need to call this in a bunch of places or just
this one place?

The reason I'm asking is the concerns I brought up in your original
patch about trying to split on unicode chars usually being wrong. I'm
worried if we have these very handy functions, it may encourage people
to do the wrong thing.

If you only need this in one place, I'd say we should write a char
iterator in i18n and have your code use that to get the substring you
want. If it will be needed in a number of places, I guess we'll have
to put the helper functions somewhere, although we should have a scary
warning, and I'd like to find a better place than string_util (which
has too much random crap in it).

Brett

[dmazzoni, 2010-08-19 19:07:53]

On Thu, Aug 19, 2010 at 11:46 AM, Brett Wilson <brettw@chromium.org> wrote:

> Do you think you'll need to call this in a bunch of places or just
> this one place?
>

From one file, for now.


> The reason I'm asking is the concerns I brought up in your original
> patch about trying to split on unicode chars usually being wrong. I'm
> worried if we have these very handy functions, it may encourage people
> to do the wrong thing.
>
...

> If you only need this in one place, I'd say we should write a char
> iterator in i18n and have your code use that to get the substring you
> want. If it will be needed in a number of places, I guess we'll have
> to put the helper functions somewhere, although we should have a scary
> warning, and I'd like to find a better place than string_util (which
> has too much random crap in it).
>

Sounds good, working on the char iterator now.

- Dominic

[brettw, 2010-08-19 20:56:59]

I would put this in base/i18n and just use ICU so you don't have to
worry about copying more stuff out of ICU into the forked files.

Brett

[dmazzoni, 2010-08-19 21:38:16]

Done, take another look.

- Dominic

On Thu, Aug 19, 2010 at 1:56 PM, Brett Wilson <brettw@chromium.org> wrote:

> I would put this in base/i18n and just use ICU so you don't have to
> worry about copying more stuff out of ICU into the forked files.
>
> Brett
>

[brettw, 2010-08-19 21:45:11]

A few minor nits, thanks for the nice tests.

http://codereview.chromium.org/3133028/diff/5002/12002
File base/i18n/char_iterators.cc (right):

http://codereview.chromium.org/3133028/diff/5002/12002#newcode40
base/i18n/char_iterators.cc:40: // UTF16CharIterator
I don't think all this extra space and the class comments you added here really
help. Especially on this small file. I'd just do one blank line like elsewhere.

http://codereview.chromium.org/3133028/diff/5002/12003
File base/i18n/char_iterators.h (right):

http://codereview.chromium.org/3133028/diff/5002/12003#newcode5
base/i18n/char_iterators.h:5: #ifndef BASE_I18N_CHAR_ITERATORS_H_
Would you mind renaming this file "char_iterator" (no "s"). I think it makes
more sense to match even though it technically has two different ones in it.

http://codereview.chromium.org/3133028/diff/5002/12003#newcode10
base/i18n/char_iterators.h:10: #include <vector>
You don't need this.

http://codereview.chromium.org/3133028/diff/5002/12003#newcode25
base/i18n/char_iterators.h:25: class UTF8CharIterator {
Can you please put this file in the base namespace?

http://codereview.chromium.org/3133028/diff/5002/12003#newcode117
base/i18n/char_iterators.h:117: #endif  // BASE_I18N_CHAR_ITERATORS_H__
Nit: only one underscore at the end here.

[dmazzoni, 2010-08-19 21:55:02]

All changes made, thanks for the fast review.

On Thu, Aug 19, 2010 at 2:45 PM, <brettw@chromium.org> wrote:

> http://codereview.chromium.org/3133028/diff/5002/12002#newcode40
> base/i18n/char_iterators.cc:40: // UTF16CharIterator
> I don't think all this extra space and the class comments you added here
> really help. Especially on this small file. I'd just do one blank line
> like elsewhere.
>

Sure, done.


> http://codereview.chromium.org/3133028/diff/5002/12003
> File base/i18n/char_iterators.h (right):
>
> http://codereview.chromium.org/3133028/diff/5002/12003#newcode5
> base/i18n/char_iterators.h:5: #ifndef BASE_I18N_CHAR_ITERATORS_H_
> Would you mind renaming this file "char_iterator" (no "s"). I think it
> makes more sense to match even though it technically has two different
> ones in it.
>

Done. I was torn on this.


> http://codereview.chromium.org/3133028/diff/5002/12003#newcode10
> base/i18n/char_iterators.h:10: #include <vector>
> You don't need this.
>

Oops. :)


> http://codereview.chromium.org/3133028/diff/5002/12003#newcode25
> base/i18n/char_iterators.h:25: class UTF8CharIterator {
> Can you please put this file in the base namespace?
>

Sure.


> http://codereview.chromium.org/3133028/diff/5002/12003#newcode117
> base/i18n/char_iterators.h:117: #endif  // BASE_I18N_CHAR_ITERATORS_H__
> Nit: only one underscore at the end here.


Fixed.

[brettw, 2010-08-19 22:00:58]

LGTM