|Summary:||[GTK] Handle surrogate pairs in TextBreakIteratorGtk|
|Product:||WebKit||Reporter:||Carlos Garcia Campos <cgarcia>|
|Severity:||Normal||CC:||abarth, commit-queue, eric, mrobinson, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
Description Carlos Garcia Campos 2010-10-25 03:58:39 PDT
Strings with surrogate pairs are not correctly handled when using the glib unicode backend.
Comment 1 Carlos Garcia Campos 2010-10-25 04:03:51 PDT
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 2 Martin Robinson 2010-10-25 11:36:49 PDT
Comment 3 Carlos Garcia Campos 2010-10-27 05:40:30 PDT
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.
Comment 4 Carlos Garcia Campos 2010-10-27 05:45:08 PDT
Created attachment 72020 [details] Previous patch with ChangeLog fixed Sorry, I made a mistake generating changelog in the previous patch.
Comment 5 Martin Robinson 2010-10-28 00:07:34 PDT
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.
Comment 6 Martin Robinson 2010-10-28 00:18:35 PDT
(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.
Comment 7 Carlos Garcia Campos 2010-10-28 05:10:41 PDT
Created attachment 72171 [details] Updated patch according to review
Comment 8 Martin Robinson 2010-10-28 09:24:00 PDT
Comment on attachment 72171 [details] Updated patch according to review Thanks!
Comment 9 WebKit Commit Bot 2010-10-29 08:39:14 PDT
Comment on attachment 72171 [details] Updated patch according to review Clearing flags on attachment: 72171 Committed r70881: <http://trac.webkit.org/changeset/70881>
Comment 10 WebKit Commit Bot 2010-10-29 08:39:21 PDT
All reviewed patches have been landed. Closing bug.