Bug 57307

Created attachment 87280 [details] Patch

Comment on attachment 87280 [details] Patch Can't we use OwnGPtr?

Comment on attachment 87280 [details] Patch Great catch! I think we can use this opportunity to clean this code up a bit. We should probably make utf16_to_utf8 return a char* instead of taking a char*& (which is bad form, IMO). Then in the caller we should immediately place the return value in a GOwnPtr<gchar> and remove the calls to g_free. This will prevent any mistakes in the future as well.

Created attachment 87452 [details] Patch

(In reply to comment #3) > (From update of attachment 87280 [details]) > Great catch! I think we can use this opportunity to clean this code up a bit. We should probably make utf16_to_utf8 return a char* instead of taking a char*& (which is bad form, IMO). Then in the caller we should immediately place the return value in a GOwnPtr<gchar> and remove the calls to g_free. This will prevent any mistakes in the future as well. I update patch as mentioned with refactoring utf16_to_utf8(because of fixing indentation and coding style). And also, can we remove 'from' variable in convertUniCharToUTF8? this value will be passed 0 always. Thanks.

Comment on attachment 87452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87452&action=review This code probably needs quite a bit more cleanup in the future, but this is a good incremental change. Please make the following changes before landing. > Source/WebCore/platform/graphics/gtk/FontGtk.cpp:92 > for (i = 0; i < aLength; i++) { Moving the declaration of i here would be more idiomatic in C++. > Source/WebCore/platform/graphics/gtk/FontGtk.cpp:135 > + g_free((gpointer)aText); You do not need to cast here. > Source/WebCore/platform/graphics/gtk/FontGtk.cpp:142 > gint new_length = 0; new_length --> newLength; > Source/WebCore/platform/graphics/gtk/FontGtk.cpp:154 > if (from > 0) { > // discard the first 'from' characters > // FIXME: we should do this before the conversion probably > - gchar* str_left = g_utf8_offset_to_pointer(utf8, from); > - gchar* tmp = g_strdup(str_left); > - g_free(utf8); > - utf8 = tmp; > + gchar* strLeft = g_utf8_offset_to_pointer(utf8Text.get(), from); > + utf8Text.set(g_strdup(strLeft)); > } > > - gchar* pos = utf8; > + gchar* pos = utf8Text.get(); I think you can easily optimize this section by doing something like: gchar* pos = utf8Text.get(); if (from > 0) { // FIXME: we should do this before the conversion probably pos = g_utf8_offset_to_pointer(pos, from); }

Created attachment 87650 [details] Patch

(In reply to comment #6) > (From update of attachment 87452 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87452&action=review > > This code probably needs quite a bit more cleanup in the future, but this is a good incremental change. Please make the following changes before landing. > > > Source/WebCore/platform/graphics/gtk/FontGtk.cpp:92 > > for (i = 0; i < aLength; i++) { > > Moving the declaration of i here would be more idiomatic in C++. Done. > > > Source/WebCore/platform/graphics/gtk/FontGtk.cpp:135 > > + g_free((gpointer)aText); > > You do not need to cast here. Because aText is const variable, I couldn't remove casting. Instead, I removed g_free itself using GOwnPtr. > > > Source/WebCore/platform/graphics/gtk/FontGtk.cpp:142 > > gint new_length = 0; > > new_length --> newLength; > Done. > > Source/WebCore/platform/graphics/gtk/FontGtk.cpp:154 > > if (from > 0) { > > // discard the first 'from' characters > > // FIXME: we should do this before the conversion probably > > - gchar* str_left = g_utf8_offset_to_pointer(utf8, from); > > - gchar* tmp = g_strdup(str_left); > > - g_free(utf8); > > - utf8 = tmp; > > + gchar* strLeft = g_utf8_offset_to_pointer(utf8Text.get(), from); > > + utf8Text.set(g_strdup(strLeft)); > > } > > > > - gchar* pos = utf8; > > + gchar* pos = utf8Text.get(); > > I think you can easily optimize this section by doing something like: > > gchar* pos = utf8Text.get(); > if (from > 0) { > // FIXME: we should do this before the conversion probably > pos = g_utf8_offset_to_pointer(pos, from); > } Done.

The commit-queue encountered the following flaky tests while processing attachment 87650 [details]: animations/dynamic-stylesheet-loading.html bug 52669 (author: cmarrin@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 87650 [details] Patch Clearing flags on attachment: 87650 Committed r82541: <http://trac.webkit.org/changeset/82541>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/82541 might have broken GTK Linux 32-bit Debug

Created attachment 87963 [details] Patch

I recreate patch because it was reverted with other patchs.

Comment on attachment 87963 [details] Patch Sorry. This patch should not have been rolled out!

Comment on attachment 87963 [details] Patch Clearing flags on attachment: 87963 Committed r82772: <http://trac.webkit.org/changeset/82772>

All reviewed patches have been landed. Closing bug.