Bug 81117

Summary: [GTK] Implement unicode submenu items
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gustavo, japhet, pnormand, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
mrobinson: review-
Updated patch mrobinson: review+

Description Carlos Garcia Campos 2012-03-14 08:50:32 PDT
The Unicode submenu has never worked, it's shown with all of its items disabled because the context menu action is not implemented.
Comment 1 Carlos Garcia Campos 2012-03-14 09:04:08 PDT
Created attachment 131867 [details]
Patch

Moved the implementation back to WebCore (but without using the WebKit layer) so that the menu works also in WebKit2
Comment 2 Martin Robinson 2012-03-14 11:04:45 PDT
Comment on attachment 131867 [details]
Patch

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.
Comment 3 Carlos Garcia Campos 2012-03-14 11:12:42 PDT
(In reply to comment #2)
> (From update of attachment 131867 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131867&action=review
> 
> Great patch! I have a few comments, but very nice.

Thanks for reviewing it.

> > 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.

Ok.

> > 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);

I'll try.

> > 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.

That's not what all other methods of ContextMenuController file do, I prefer to keep consistency with the other method of the same file.

> > 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.

Ok.

> > 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 :)

Oops

> > 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) {

That doesn't work, it means i is a GList*.

> > 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.

Ok.
Comment 4 Martin Robinson 2012-03-14 11:24:48 PDT
Comment on attachment 131867 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131867&action=review

>>> Source/WebCore/page/ContextMenuController.cpp:602
>>> +    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.
> 
> That's not what all other methods of ContextMenuController file do, I prefer to keep consistency with the other method of the same file.

The older code may have been written before that part of the style guide, so you should either do: 

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

or

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

New code should follow the style guide.
Comment 5 Carlos Garcia Campos 2012-03-14 11:56:17 PDT
(In reply to comment #4)
> (From update of attachment 131867 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131867&action=review
> 
> >>> Source/WebCore/page/ContextMenuController.cpp:602
> >>> +    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.
> > 
> > That's not what all other methods of ContextMenuController file do, I prefer to keep consistency with the other method of the same file.
> 
> The older code may have been written before that part of the style guide,

really?

> so you should either do: 
> 
> appendItem(ContextMenuItem(ActionType, ContextMenuItemTagUnicodeInsertLRMMark, contextMenuItemTagUnicodeInsertLRMMark()), &unicodeMenu);

I would have to try, but I think that won't work, appendItem expects a ContextMenuItem&, and I guess that's the reason while all other methods use a variable to create context menu items.

> or
> 
> ContextMenuItem leftToRightMarkItem(ActionType, ContextMenuItemTagUnicodeInsertLRMMark, contextMenuItemTagUnicodeInsertLRMMark());
> appendItem(leftToRightMarkItem &unicodeMenu);

Ok, I'll rename the variables.
Comment 6 Carlos Garcia Campos 2012-03-15 01:54:14 PDT
Created attachment 132003 [details]
Updated patch

Addressed all review comments
Comment 7 Martin Robinson 2012-03-15 10:15:10 PDT
Comment on attachment 132003 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132003&action=review

> Source/WebCore/page/ContextMenuController.cpp:192
> +static void insertUnicodeCharacter(UChar uchar, Frame* frame)
> +{
> +    String text(&uchar, 1);

You should probably just call this UChar char or UChar character. Converting UChar to camel-case leaves uChar.
Comment 8 Carlos Garcia Campos 2012-03-15 10:35:22 PDT
(In reply to comment #7)
> (From update of attachment 132003 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132003&action=review
> 
> > Source/WebCore/page/ContextMenuController.cpp:192
> > +static void insertUnicodeCharacter(UChar uchar, Frame* frame)
> > +{
> > +    String text(&uchar, 1);
> 
> You should probably just call this UChar char or UChar character. Converting UChar to camel-case leaves uChar.

char is a reserved word, I'll use character
Comment 9 Carlos Garcia Campos 2012-03-15 11:29:13 PDT
Committed r110865: <http://trac.webkit.org/changeset/110865>