[Webkit-unassigned] [Bug 81117] [GTK] Implement unicode submenu items

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

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




--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2012-03-14 11:04:45 PST ---
(From update of attachment 131867)
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.

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