Strings with surrogate pairs are not correctly handled when using the glib unicode backend.
Created attachment 71728 [details] Patch to handle surrogate pairs in TextBreakIteratorGtk TextBreakIteratorGtk uses utf8 because it's what pango expects, but we need to return indices for the given input string that it's in utf16. The number of characters is the same for both utf16 and ut8 except when the input string contains surrogate pairs. We need to keep both, the index for the utf8 string to be used internally, and the index for the utf16 string to be used as return value of the iterator interface. It fixes test fast/forms/textarea-maxlength.html
Comment on attachment 71728 [details] Patch to handle surrogate pairs in TextBreakIteratorGtk 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.
Created attachment 72019 [details] New patch to handle surrogate pairs This patch is not just an update of the previous one. In order to simplify the code I've added a new class CharacterIterator that takes care of the utf8 and untf16 indices. The break iterator uses this class to iterate over the input string. I've fixed all coding styles issues in the file too.
Created attachment 72020 [details] Previous patch with ChangeLog fixed Sorry, I made a mistake generating changelog in the previous patch.
Comment on attachment 72020 [details] Previous patch with ChangeLog fixed View in context: https://bugs.webkit.org/attachment.cgi?id=72020&action=review Very nice! I just have a couple very small comments. > WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:31 > +using namespace std; I think I'd prefer the functions you're using to be explicit here. using std::max; for example. > WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:33 > +#define IS_SURROGATE(character) (character >= 0x10000 && character <= 0x10FFFF) I think this should probably be called UTF8_IS_SURROGATE just to be clear that it deals with UTF-8 characters. > WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:77 > + long utf8len = 0; According to the style guidelines, this should be utf8Length.
(In reply to comment #5) > I think I'd prefer the functions you're using to be explicit here. > using std::max; for example. Carlos pointed out that I'm wrong about this. The style guidelines are explicit that "using namespace std;" is preferred.
Created attachment 72171 [details] Updated patch according to review
Comment on attachment 72171 [details] Updated patch according to review Thanks!
Comment on attachment 72171 [details] Updated patch according to review Clearing flags on attachment: 72171 Committed r70881: <http://trac.webkit.org/changeset/70881>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/70881 might have broken Leopard Intel Release (Tests) The following tests are not passing: fast/workers/storage/interrupt-database-sync.html