[Webkit-unassigned] [Bug 48227] [GTK] Handle surrogate pairs in TextBreakIteratorGtk

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48227


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #71728|review?                     |review-
               Flag|                            |




--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2010-10-25 11:36:49 PST ---
(From update of attachment 71728)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.