[webkit-reviews] review denied: [Bug 48227] [GTK] Handle surrogate pairs in TextBreakIteratorGtk : [Attachment 71728] Patch to handle surrogate pairs in TextBreakIteratorGtk

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 48227: [GTK] Handle surrogate pairs in TextBreakIteratorGtk
https://bugs.webkit.org/show_bug.cgi?id=48227

Attachment 71728: Patch to handle surrogate pairs in TextBreakIteratorGtk
https://bugs.webkit.org/attachment.cgi?id=71728&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71728&action=review

Looking good, but here are some suggestions.

> WebCore/ChangeLog:9
> +	   need to return indices for the given input string that it's in
utf16.

I think this should be "that it's in" --> "that are in" Should be 'UTF-8' and
'UTF-16' throughout.

> WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:42
> +    GOwnPtr<char> m_utf8;

This should probably be a CString.

> WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:115
> +static int characterSize(const gchar* str, glong offset)

'str' should be 'string'

> WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:119
> +    gchar* p = g_utf8_offset_to_pointer(str, offset);
> +    gunichar c = g_utf8_get_char(p);
> +    return (c >= 0x10000 && c <= 0x10FFFF) ? 2 : 1;

Avoid using one letter variable names. These should be full words. Can we use
any of the stuff from JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.hto
remove these magic numbers?

> WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:122
> +static int nextUtf16Step(TextBreakIterator* bi, int i)

Again avoid abbreviations here.

> WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:130
> +static int previousUtf16Step(TextBreakIterator* bi, int i)

Ditto.

> WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:140
> +static int getUTF8Index(TextBreakIterator* bi, int i)

Ditto.

> WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:159
>  int textBreakFirst(TextBreakIterator* bi)

Ditto.

> WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:166
>  int textBreakLast(TextBreakIterator* bi)

Ditto.

> WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:188
> +	       if ((whiteSpaceAtTheEnd = bi->m_logAttrs[pos].is_white)) {

Why the extra parenthesis here?

> WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:201
>  int textBreakNext(TextBreakIterator* bi)

Ditto.

> WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:223
>  int textBreakPrevious(TextBreakIterator* bi)

Ditto, etc.