[webkit-reviews] review denied: [Bug 81117] [GTK] Implement unicode submenu items : [Attachment 131867] Patch

[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 81117: [GTK] Implement unicode submenu items
https://bugs.webkit.org/show_bug.cgi?id=81117

Attachment 131867: Patch
https://bugs.webkit.org/attachment.cgi?id=131867&action=review

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


Great patch! I have a few comments, but very nice.

> Source/WebCore/page/ContextMenuController.cpp:190
> +static void insertUnicodeCharacter(gunichar uchar, Frame* frame)

It's better to use UChar here since that's what leftToRightMark and the like
are defined as.

> Source/WebCore/page/ContextMenuController.cpp:200
> +    long int uchar16Length;
> +    GOwnPtr<gunichar2> uchar16(g_ucs4_to_utf16(&uchar, 1, 0, &uchar16Length,
0));
> +    if (!uchar16)
> +	   return;
> +
> +    String text = String(static_cast<UChar*>(uchar16.get()), uchar16Length);

> +    if (text.isEmpty())
> +	   return;
> +

Can't you just do the following?

String text;
text.append(character);

> Source/WebCore/page/ContextMenuController.cpp:602
> +    ContextMenuItem lrm(ActionType, ContextMenuItemTagUnicodeInsertLRMMark,
contextMenuItemTagUnicodeInsertLRMMark());
> +    ContextMenuItem rlm(ActionType, ContextMenuItemTagUnicodeInsertRLMMark,
contextMenuItemTagUnicodeInsertRLMMark());
> +    ContextMenuItem lre(ActionType, ContextMenuItemTagUnicodeInsertLREMark,
contextMenuItemTagUnicodeInsertLREMark());
> +    ContextMenuItem rle(ActionType, ContextMenuItemTagUnicodeInsertRLEMark,
contextMenuItemTagUnicodeInsertRLEMark());
> +    ContextMenuItem lro(ActionType, ContextMenuItemTagUnicodeInsertLROMark,
contextMenuItemTagUnicodeInsertLROMark());
> +    ContextMenuItem rlo(ActionType, ContextMenuItemTagUnicodeInsertRLOMark,
contextMenuItemTagUnicodeInsertRLOMark());
> +    ContextMenuItem pdf(ActionType, ContextMenuItemTagUnicodeInsertPDFMark,
contextMenuItemTagUnicodeInsertPDFMark());
> +    ContextMenuItem zws(ActionType, ContextMenuItemTagUnicodeInsertZWSMark,
contextMenuItemTagUnicodeInsertZWSMark());
> +    ContextMenuItem zwj(ActionType, ContextMenuItemTagUnicodeInsertZWJMark,
contextMenuItemTagUnicodeInsertZWJMark());
> +    ContextMenuItem zwnj(ActionType,
ContextMenuItemTagUnicodeInsertZWNJMark,
contextMenuItemTagUnicodeInsertZWNJMark());

These names don't really follow WebKit coding conventions because they are
abbreviations, but you could avoid them altogether if you do:

appendItem(ContextMenuItem(ActionType, ContextMenuItemTagUnicodeInsertLRMMark,
contextMenuItemTagUnicodeInsertLRMMark()), &unicodeMenu);

etc.

> Source/WebCore/page/ContextMenuController.cpp:1035
> +	       bool shouldShowUnicodeMenu = true;
> +	       if (EditorClient* client = frame->editor()->client())
> +		   shouldShowUnicodeMenu = client->shouldShowUnicodeMenu();
> +	       if (shouldShowUnicodeMenu) {

EditorClient* client = frame->editor()->client();
if (client && client->shouldshowUnicodeMenu())

seems a little cleaner.

> Source/WebKit/gtk/WebCoreSupport/ContextMenuClientGtk.cpp:87
> +	   // Place the im context menu item right before the unicode menut
item
> +	   // if it's present.

s/menut/menu :)

> Source/WebKit/gtk/WebCoreSupport/ContextMenuClientGtk.cpp:92
> +	   for (iter = items.get(), i = 0; iter; iter = g_list_next(iter), ++i)
{

You should define iter inline here:
for (GList* iter = items.get(), i = 0; iter; iter = g_list_next(iter), ++i) {

> Source/WebKit/gtk/WebCoreSupport/ContextMenuClientGtk.cpp:99
> +	       if (GTK_IS_SEPARATOR_MENU_ITEM(item))
> +		   continue;
> +	       if (String::fromUTF8(gtk_menu_item_get_label(item)) ==
contextMenuItemTagUnicode()) {
> +		   unicodeMenuItemPosition = i;
> +		   break;
> +	       }

I think you could split out this code into a helper like bool
unicodeMenuPresent() and it'd be clearer what's going on.