[webkit-reviews] review granted: [Bug 126856] TextBreakIterator's should support Latin-1 for all iterator types : [Attachment 220987] Part 1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 126856: TextBreakIterator's should support Latin-1 for all iterator types
https://bugs.webkit.org/show_bug.cgi?id=126856

Attachment 220987: Part 1
https://bugs.webkit.org/attachment.cgi?id=220987&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=220987&action=review


> Source/WebCore/platform/text/TextBreakIteratorICU.cpp:61
> +static TextBreakIterator* setTextForIterator(TextBreakIterator* iterator,
StringView string)

Should this take a reference instead of a pointer?

> Source/WebCore/platform/text/TextBreakIteratorICU.cpp:77
> +	   ubrk_setUText(reinterpret_cast<UBreakIterator*>(iterator), text,
&setTextStatus);

I’d like to see a helper function so we have only one reinterpret_cast instead
of many. Or maybe just transition to use UBreakIterator directly since we don’t
need a non-ICU abstraction for this.

> Source/WebCore/platform/text/TextBreakIteratorICU.cpp:143
> +TextBreakIterator* wordBreakIterator(const UChar* buffer, int length)

Why not make this take a StringView too?

> Source/WebCore/platform/text/TextBreakIteratorICU.cpp:152
> +TextBreakIterator* sentenceBreakIterator(const UChar* buffer, int length)

And this.

> Source/WebCore/platform/text/TextBreakIteratorICU.cpp:161
> +TextBreakIterator* cursorMovementIterator(const UChar* buffer, int length)

And this.

> Source/WebCore/platform/text/TextBreakIteratorICU.cpp:267
> +void releaseLineBreakIterator(TextBreakIterator* iterator)

Reference?

> Source/WebCore/platform/text/TextBreakIteratorICU.cpp:356
> +// Iterator implemenation.
> +
> +int textBreakFirst(TextBreakIterator* iterator)
> +{
> +    return ubrk_first(reinterpret_cast<UBreakIterator*>(iterator));
> +}
> +
> +int textBreakLast(TextBreakIterator* iterator)
> +{
> +    return ubrk_last(reinterpret_cast<UBreakIterator*>(iterator));
> +}
> +
> +int textBreakNext(TextBreakIterator* iterator)
> +{
> +    return ubrk_next(reinterpret_cast<UBreakIterator*>(iterator));
> +}
> +
> +int textBreakPrevious(TextBreakIterator* iterator)
> +{
> +    return ubrk_previous(reinterpret_cast<UBreakIterator*>(iterator));
> +}
> +
> +int textBreakPreceding(TextBreakIterator* iterator, int pos)
> +{
> +    return ubrk_preceding(reinterpret_cast<UBreakIterator*>(iterator), pos);

> +}
> +
> +int textBreakFollowing(TextBreakIterator* iterator, int pos)
> +{
> +    return ubrk_following(reinterpret_cast<UBreakIterator*>(iterator), pos);

> +}
> +
> +int textBreakCurrent(TextBreakIterator* iterator)
> +{
> +    return ubrk_current(reinterpret_cast<UBreakIterator*>(iterator));
> +}
> +
> +bool isTextBreak(TextBreakIterator* iterator, int position)
> +{
> +    return ubrk_isBoundary(reinterpret_cast<UBreakIterator*>(iterator),
position);
> +}
> +
> +bool isWordTextBreak(TextBreakIterator* iterator)
> +{
> +    int ruleStatus =
ubrk_getRuleStatus(reinterpret_cast<UBreakIterator*>(iterator));
> +    return ruleStatus != UBRK_WORD_NONE;
>  }

This should all just go. It’s a wrapper that we don’t need.

> Source/WebCore/platform/text/icu/UTextProviderLatin1.cpp:46
> +    0, 0, 0,

Some nullptr maybe?

> Source/WebCore/platform/text/icu/UTextProviderLatin1.cpp:52
> +    0,
> +    0,

Some nullptr maybe?

> Source/WebCore/platform/text/icu/UTextProviderLatin1.cpp:56
> +    0, 0, 0

Some nullptr maybe?

> Source/WebCore/platform/text/icu/UTextProviderLatin1.cpp:64
> +	   return 0;

nullptr

> Source/WebCore/platform/text/icu/UTextProviderLatin1.cpp:72
> +    /* Point at the same position, but with an empty buffer */

Maybe // instead of /* comment? In this whole file perhaps.

> Source/WebCore/platform/text/icu/UTextProviderLatin1.cpp:98
> +	       uText->chunkOffset = (int32_t)(index - uText->chunkNativeStart);


static_cast?

> Source/WebCore/platform/text/icu/UTextProviderLatin1.cpp:109
> +	       uText->chunkOffset = (int32_t)(index - uText->chunkNativeStart);


static_cast?

> Source/WebCore/platform/text/icu/UTextProviderLatin1.cpp:206
> +    uText->context = 0;

nullptr?

> Source/WebCore/platform/text/icu/UTextProviderLatin1.cpp:224
> +    text->a = (int64_t)length;

static_cast? no cast at all?

> Source/WebCore/platform/text/icu/UTextProviderLatin1.h:42
> +UText* openLatin1UTextProvider(UTextWithBuffer* utWithBuffer, const LChar*
string, unsigned length, UErrorCode* status);
> +UText* openLatin1ContextAwareUTextProvider(UTextWithBuffer* utWithBuffer,
const LChar* string, unsigned length, const UChar* priorContext, int
priorContextLength, UErrorCode* status);

utWithBuffer and status argument names should be omitted.

> Source/WebCore/platform/text/icu/UTextProviderUTF16.cpp:43
> +    0, 0, 0,

nullptr?

> Source/WebCore/platform/text/icu/UTextProviderUTF16.cpp:48
> +    0, 0, 0, 0,

nullptr?

> Source/WebCore/platform/text/icu/UTextProviderUTF16.cpp:50
> +    0, 0, 0,

nullptr?