Bug 50514 – WebKit2: Context menu support on Windows

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brian Weinstein]

WebKit2 needs support for context menu items on Windows.

Context menus and context menu items on Windows need a major refactoring, because in their current state, they are not copyable (if you try to make a Vector of them, you will see a crash/double free, but the current WebKit1 code doesn't need to do this, so we've gotten away with it so far).

The plan in this patch will be:

1) Refactor ContextMenu to be backed by a Vector<ContextMenuItem> (instead of an HMENU) and refactor ContextMenuItem to be backed by a

struct PlatformMenuItemDescription {
        PlatformMenuItemDescription()
            : type(ActionType)
            , action(ContextMenuItemTagNoAction)
            , title("")
            , checked(false)
            , enabled(true) { }
        ContextMenuItemType type;
        ContextMenuAction action;
        String title;
        Vector<ContextMenuItem> subMenuItems;
        bool checked;
        bool enabled;
    };

to match the other platforms.

2) Implement the necessary stubs in WebContextMenuProxyWin in WebKit2 to be able to create/draw context menus in WebKit2.

<rdar://problem/8608754>

[Brian Weinstein]

Created attachment 75602 [details]
[PATCH] Context Menu Refactoring

I'm not 100% on the lifetimes of the platformRepresentations, so if the reviewer could take an extra-close look at that, that would be great.

[WebKit Review Bot]

Attachment 75602 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/ContextMenu.h', u'WebCore/platform/ContextMenuItem.h', u'WebCore/platform/PlatformMenuDescription.h', u'WebCore/platform/win/ContextMenuItemWin.cpp', u'WebCore/platform/win/ContextMenuWin.cpp', u'WebKit/win/ChangeLog', u'WebKit/win/WebCoreSupport/WebContextMenuClient.cpp', u'WebKit/win/WebCoreSupport/WebContextMenuClient.h', u'WebKit/win/WebView.cpp']" exit_code: 1
WebCore/platform/win/ContextMenuWin.cpp:162:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/platform/win/ContextMenuWin.cpp:203:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/win/ContextMenuWin.cpp:216:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/win/WebView.cpp:1280:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Weinstein]

Created attachment 75603 [details]
[PATCH] Context Menu Refactoring w/ style fixes 

Patch with style nits fixed - still same comments apply about the lifetime questions.

[Brian Weinstein]

Created attachment 75622 [details]
[PATCH] WebKit2 Win Context Menu Support

[WebKit Review Bot]

Attachment 75622 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit2/ChangeLog', u'WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp', u'WebKit2/UIProcess/win/WebContextMenuProxyWin.h', u'WebKit2/UIProcess/win/WebView.cpp']" exit_code: 1
WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:46:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Weinstein]

Created attachment 75623 [details]
[PATCH] WebKit2 Win Context Menu Support w/ Style Fixes

[Darin Adler]

Comment on attachment 75623 [details]
[PATCH] WebKit2 Win Context Menu Support w/ Style Fixes

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

> WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:43
> +    for (unsigned i = 0; i < items.size(); ++i) {

size_t is the correct type for iterating a vector.

> WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:44
> +        WebContextMenuItemData itemData = items[i];

It’s better to use const WebContextMenuItemData& to avoid an unnecessary copy of the data.

> WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:49
> +            UINT flags = 0;
> +            flags |= itemData.enabled() ? MF_ENABLED : MF_DISABLED;

Seems you could combine these two lines and get better clarity.

> WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:53
> +            WTF::String title = itemData.title();
> +            ::AppendMenu(menu, flags, itemData.action(), title.charactersWithNullTermination());

Why WTF::String instead of String here? I also think you should just drop the title local variable and say item.title().charactersWithNullTermination().

> WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:65
> +            WTF::String title = itemData.title();
> +            ::AppendMenu(menu, MF_POPUP, (UINT) subMenu, title.charactersWithNullTermination());

Extra space in "(UINT) subMenu". Also, is this our usual idiom for this cast? Can we use reinterpret_cast instead? Also, same comment about title local variable as above.

> WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:87
> +    UINT flags = TPM_RIGHTBUTTON | TPM_TOPALIGN | TPM_VERPOSANIMATION | TPM_HORIZONTAL
> +            | TPM_LEFTALIGN | TPM_HORPOSANIMATION | TPM_RETURNCMD | TPM_NONOTIFY;

Indentation here is peculiar. Could be either 4-space indented, or all on one line.

> WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:98
> +    if (m_menu)
> +        ::DestroyMenu(m_menu);

Shouldn’t this also set m_menu to 0?

> WebKit2/UIProcess/win/WebContextMenuProxyWin.h:44
> +    WebContextMenuProxyWin(HWND parentWindow, WebPageProxy* page);

Argument name “page” seems unneeded here.

[Darin Adler]

Comment on attachment 75603 [details]
[PATCH] Context Menu Refactoring w/ style fixes 

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

I didn’t have time to review this whole thing, but there is a layering violation here.

> WebCore/platform/ContextMenu.h:55
>          ContextMenu(const HitTestResult&, const PlatformMenuDescription);
> +#if PLATFORM(WIN)
> +        ContextMenu(const HitTestResult&, const HMENU);
> +#endif

The “const HMENU” here is no different from HMENU, and the const should be removed.

Same is true for “const PlatformMenuDescription”.

> WebCore/platform/ContextMenuItem.h:44
> +#include "HitTestResult.h"

This can’t be right. The platform directory is not allowed to include headers from the rest of WebCore. Something needs to be moved instead.

> WebCore/platform/ContextMenuItem.h:281
> +        ContextMenuItem(const HitTestResult&, const LPMENUITEMINFO);

This is not acceptable layering. HitTestResult is a browser level concept that does not belong here at the platform layer.

[Brian Weinstein]

(In reply to comment #8)
> (From update of attachment 75603 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75603&action=review
> 
> I didn’t have time to review this whole thing, but there is a layering violation here.
> 
> > WebCore/platform/ContextMenu.h:55
> >          ContextMenu(const HitTestResult&, const PlatformMenuDescription);
> > +#if PLATFORM(WIN)
> > +        ContextMenu(const HitTestResult&, const HMENU);
> > +#endif
> 
> The “const HMENU” here is no different from HMENU, and the const should be removed.
> 
> Same is true for “const PlatformMenuDescription”.
> 
> > WebCore/platform/ContextMenuItem.h:44
> > +#include "HitTestResult.h"
> 
> This can’t be right. The platform directory is not allowed to include headers from the rest of WebCore. Something needs to be moved instead.
> 
> > WebCore/platform/ContextMenuItem.h:281
> > +        ContextMenuItem(const HitTestResult&, const LPMENUITEMINFO);
> 
> This is not acceptable layering. HitTestResult is a browser level concept that does not belong here at the platform layer.

I'm confused by this, because ContextMenu also has a HitTestResult, and ContextMenu.h/.cpp is in the WebCore/platform directory as well.

[Brian Weinstein]

(In reply to comment #7)
> (From update of attachment 75623 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75623&action=review
> 
> > WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:43
> > +    for (unsigned i = 0; i < items.size(); ++i) {
> 
> size_t is the correct type for iterating a vector.

Fixed.

> 
> > WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:44
> > +        WebContextMenuItemData itemData = items[i];
> 
> It’s better to use const WebContextMenuItemData& to avoid an unnecessary copy of the data.

Fixed.

> 
> > WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:49
> > +            UINT flags = 0;
> > +            flags |= itemData.enabled() ? MF_ENABLED : MF_DISABLED;
> 
> Seems you could combine these two lines and get better clarity.

Fixed.

> 
> > WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:53
> > +            WTF::String title = itemData.title();
> > +            ::AppendMenu(menu, flags, itemData.action(), title.charactersWithNullTermination());
> 
> Why WTF::String instead of String here? I also think you should just drop the title local variable and say item.title().charactersWithNullTermination().

When I do that, I get a compile error: 'WTF::String::charactersWithNullTermination' : cannot convert 'this' pointer from 'const WTF::String' to 'WTF::String &', but I made the local variable be just of type String, instead of WTF::String.

> 
> > WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:65
> > +            WTF::String title = itemData.title();
> > +            ::AppendMenu(menu, MF_POPUP, (UINT) subMenu, title.charactersWithNullTermination());
> 
> Extra space in "(UINT) subMenu". Also, is this our usual idiom for this cast? Can we use reinterpret_cast instead? Also, same comment about title local variable as above.

Same response as above about the title. Switched to using reinterpret_cast.

> 
> > WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:87
> > +    UINT flags = TPM_RIGHTBUTTON | TPM_TOPALIGN | TPM_VERPOSANIMATION | TPM_HORIZONTAL
> > +            | TPM_LEFTALIGN | TPM_HORPOSANIMATION | TPM_RETURNCMD | TPM_NONOTIFY;
> 
> Indentation here is peculiar. Could be either 4-space indented, or all on one line.

Moved to all on one line.

> 
> > WebKit2/UIProcess/win/WebContextMenuProxyWin.cpp:98
> > +    if (m_menu)
> > +        ::DestroyMenu(m_menu);
> 
> Shouldn’t this also set m_menu to 0?

Fixed.

> 
> > WebKit2/UIProcess/win/WebContextMenuProxyWin.h:44
> > +    WebContextMenuProxyWin(HWND parentWindow, WebPageProxy* page);
> 
> Argument name “page” seems unneeded here.

Removed.

Thanks for the review!

[Brian Weinstein]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 75603 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=75603&action=review
> > 
> > I didn’t have time to review this whole thing, but there is a layering violation here.
> > 
> > > WebCore/platform/ContextMenu.h:55
> > >          ContextMenu(const HitTestResult&, const PlatformMenuDescription);
> > > +#if PLATFORM(WIN)
> > > +        ContextMenu(const HitTestResult&, const HMENU);
> > > +#endif
> > 
> > The “const HMENU” here is no different from HMENU, and the const should be removed.
> > 
> > Same is true for “const PlatformMenuDescription”.
> > 
> > > WebCore/platform/ContextMenuItem.h:44
> > > +#include "HitTestResult.h"
> > 
> > This can’t be right. The platform directory is not allowed to include headers from the rest of WebCore. Something needs to be moved instead.
> > 
> > > WebCore/platform/ContextMenuItem.h:281
> > > +        ContextMenuItem(const HitTestResult&, const LPMENUITEMINFO);
> > 
> > This is not acceptable layering. HitTestResult is a browser level concept that does not belong here at the platform layer.
> 
> I'm confused by this, because ContextMenu also has a HitTestResult, and ContextMenu.h/.cpp is in the WebCore/platform directory as well.

One option that comes to mind is that we could move the HitTestResult into ContextMenuController, since the ContextMenuController only keeps information about one ContextMenu.

I'd like to do this in a follow-up however.

[Brian Weinstein]

After discussion with Adam Roben, I'm going to fix the layering violation in ContextMenu first - by moving functions in ContextMenu that interact with the m_hitTestResult (populate, appendInspectElementItem, checkOrEnableIfNeeded into ContextMenuController).

This is tracked by https://bugs.webkit.org/show_bug.cgi?id=50586.

[Brian Weinstein]

Created attachment 75950 [details]
[PATCH] Context Menu Refactoring

[Brian Weinstein]

Created attachment 75952 [details]
[PATCH] Context Menu Refactoring - Reordered Functions

[Adam Roben (:aroben)]

Comment on attachment 75952 [details]
[PATCH] Context Menu Refactoring - Reordered Functions

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

> WebCore/ChangeLog:20
> +            that is a list of ContextMenuItems.

s/list/Vector/

> WebCore/platform/ContextMenu.h:74
> +        static void nativeMenu(PlatformMenuDescription, HMENU);

Can this function return an HMENU instead of filling one in?

> WebCore/platform/ContextMenuItem.h:174
> +            , title("")

Do we really need an empty string (and not a null string) here?

> WebCore/platform/ContextMenuItem.h:176
> +            , enabled(true) { }

These braces should be on their own lines.

> WebCore/platform/ContextMenuItem.h:280
> +        ContextMenuItem(const LPMENUITEMINFO);

The "const" here is meaningless.

> WebCore/platform/ContextMenuItem.h:310
> +        static void nativeMenuItem(const PlatformMenuItemDescription&, LPMENUITEMINFO);

Can this return a MENUITEMINFO instead?

> WebCore/platform/win/ContextMenuItemWin.cpp:50
> +    : m_platformDescription()

This isn't needed.

> WebCore/platform/win/ContextMenuItemWin.cpp:63
> +    m_platformDescription.type = info->fType & MFT_SEPARATOR ? SeparatorType : ActionType;
> +    m_platformDescription.action = static_cast<ContextMenuAction>(info->wID);
> +    m_platformDescription.title = String(info->dwTypeData, wcslen(info->dwTypeData));
> +
> +    if (info->hSubMenu) {
> +        m_platformDescription.type = SubmenuType;
> +        ContextMenu subMenu(info->hSubMenu);
> +        m_platformDescription.subMenuItems = contextMenuItemVector(subMenu.releasePlatformDescription());
>      }
> +
> +    m_platformDescription.checked = info->fState & MFS_CHECKED;
> +    m_platformDescription.enabled = !(info->fState & MFS_DISABLED);

You need to check that info->fMask has the relevant bit set before you use any of info's other members.

I think you should be using == instead of & when comparing with MFT_SEPARATOR.

> WebCore/platform/win/ContextMenuItemWin.cpp:67
> +    : m_platformDescription()

This isn't needed.

> WebCore/platform/win/ContextMenuItemWin.cpp:81
> +    : m_platformDescription()

This isn't needed.

> WebCore/platform/win/ContextMenuItemWin.cpp:87
> +    : m_platformDescription()

This isn't needed.

> WebCore/platform/win/ContextMenuItemWin.cpp:92
> +ContextMenuItem::ContextMenuItem(PlatformMenuItemDescription item)

This should really take a const PlatformMenuItemDescription&.

> WebCore/platform/win/ContextMenuItemWin.cpp:101
> +PlatformMenuItemDescription ContextMenuItem::releasePlatformDescription()

Can we just call this platformDescription instead? And have it return a const PlatformMenuItemDescription&?

> WebCore/platform/win/ContextMenuItemWin.cpp:123
> +    return const_cast<Vector<ContextMenuItem>*>(&m_platformDescription.subMenuItems);

The const_cast here is ugly. Can we make PlatformMenuDescription be a const Vector<ContextMenuItem>* instead?

> WebCore/platform/win/ContextMenuItemWin.cpp:199
> +void ContextMenuItem::nativeMenuItem(const PlatformMenuItemDescription& item, LPMENUITEMINFO info)
> +{
> +    memset(info, 0, sizeof(MENUITEMINFO));
> +    info->cbSize = sizeof(MENUITEMINFO);
> +    
> +    info->fMask = MIIM_FTYPE | MIIM_ID | MIIM_STRING | MIIM_STATE | MIIM_SUBMENU;
> +
> +    if (item.type == SubmenuType) {
> +        HMENU nativeMenu = ::CreatePopupMenu();
> +        ContextMenu::nativeMenu(const_cast<Vector<ContextMenuItem>*>(&item.subMenuItems), nativeMenu);
> +        info->hSubMenu = nativeMenu;
> +    }
> +
> +    if (item.type == SeparatorType)
> +        info->fType = MFT_SEPARATOR;
> +    else
> +        info->fType = MFT_STRING;
> +
> +    info->wID = item.action;
> +
> +    info->fState |= item.enabled ? MFS_ENABLED : MFS_DISABLED;
> +    info->fState |= item.checked ? MFS_CHECKED : MFS_UNCHECKED;
>  }

It looks like you aren't preserving the item's title.

Do callers know they are responsible for the HMENU in info->hSubMenu?

> WebCore/platform/win/ContextMenuWin.cpp:57
> +        int menuStringLength = ::GetMenuString(menu, i, 0, 0, MF_BYPOSITION);

MSDN says you're supposed to use GetMenuItemInfo instead of GetMenuString.

> WebCore/platform/win/ContextMenuWin.cpp:59
> +        if (!menuStringLength) {
> +            // If we don't have a string, then we must be in a separator.

A better way to determine this is to check MENUITEMINFO::fType after calling GetMenuItemInfo.

> WebCore/platform/win/ContextMenuWin.cpp:75
> +        free(menuString);

This is wrong. You can't mix new[] and free(). You should be using delete[] (except that really you should be using OwnArrayPtr).

> WebCore/platform/win/ContextMenuWin.cpp:154
> +static ContextMenuItem* itemWithActionInMenu(unsigned action, Vector<ContextMenuItem>& items)

Can items be a const Vector&?

> WebCore/platform/win/ContextMenuWin.cpp:156
> +    for (unsigned i = 0; i < items.size(); ++i) {

size_t is the type to use to iterate over a Vector.

> WebCore/platform/win/ContextMenuWin.cpp:161
> +            ContextMenuItem* searchSubmenu = itemWithActionInMenu(action, *items[i].platformSubMenu());
> +            if (searchSubmenu)

This is a confusing variable name. And you can put the declaration inside the if itself:

if (ContextMenuItem* foo = itemWithActionInMenu(...))

> WebCore/platform/win/ContextMenuWin.cpp:189
> +    Vector<ContextMenuItem>* items = new Vector<ContextMenuItem>;

Who is responsible for deleting this?

> WebCore/platform/win/ContextMenuWin.cpp:197
> +    Vector<ContextMenuItem>* items = new Vector<ContextMenuItem>;

Who is responsible for deleting this?

Do we really need both platformDescription() and releasePlatformDescription()?

> WebCore/platform/win/ContextMenuWin.cpp:216
> +        WTF::String title = description.title;

No need for the WTF:: here.

> WebCore/platform/win/ContextMenuWin.cpp:217
> +        menuItem.dwTypeData = (LPWSTR) title.charactersWithNullTermination();

Please use C++ casts instead of a C-style cast here.

> WebCore/platform/win/ContextMenuWin.cpp:224
> +Vector<ContextMenuItem> contextMenuItemVector(Vector<ContextMenuItem>* menu)

I don't understand why this function is needed. But if it is needed, it can just be:

return *menu;

> WebCore/platform/win/ContextMenuWin.cpp:242
> +    PlatformMenuDescription platformMenu = new Vector<ContextMenuItem>;

Who is responsible for destroying this?

> WebCore/platform/win/ContextMenuWin.cpp:243
> +    for (unsigned i = 0; i < menuItemVector.size(); ++i)

You should use size_t.

[Brian Weinstein]

Created attachment 76083 [details]
[PATCH] WIP Context Menu Cross-Platform Goodness

[Adam Roben (:aroben)]

Comment on attachment 76083 [details]
[PATCH] WIP Context Menu Cross-Platform Goodness

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

Looks like this is on the right track!

> WebCore/platform/ContextMenu.cpp:74
> +void ContextMenu::setPlatformDescription(Vector<ContextMenuItem>* menuItems)
> +{
> +    ASSERT_ARG(menuItems, menuItems);
> +
> +    m_items.clear();
> +    for (size_t i = 0; i < menuItems->size(); ++i) {
> +        m_items.append(menuItems->at(i));
> +    }
> +}

This whole function could just be: m_items = *menuItems;

Maybe this should be called setItems?

> WebCore/platform/ContextMenu.cpp:84
> +Vector<ContextMenuItem>* ContextMenu::platformDescription()
> +{
> +    // FIXME: I'm not sure who is supposed to free this Vector, but if I just return &m_items, then I get into a state
> +    // in setPlatformDescription where the menuItems == &m_items, and that isn't a good state to be in.
> +    Vector<ContextMenuItem>* menuItems = new Vector<ContextMenuItem>;
> +    for (size_t i = 0; i < m_items.size(); ++i)
> +        menuItems->append(m_items[i]);
> +    return menuItems;
> +}

I think a function like this would be more useful:

const Vector<ContextMenuItem>& items() const { return m_items; }

> WebCore/platform/ContextMenu.cpp:95
> +Vector<ContextMenuItem> contextMenuItemVector(Vector<ContextMenuItem>* menu)
> +{
> +    ASSERT_ARG(menu, menu);
> +    Vector<ContextMenuItem> itemList;
> +
> +    for (size_t i = 0; i < menu->size(); ++i)
> +        itemList.append(menu->at(i));
> +
> +    return itemList;
> +}

This whole function could just be: return *menu;

Do we really need this function in the new model? It doesn't seem helpful.

[Brian Weinstein]

Created attachment 76099 [details]
[PATCH] More WIP Goodness

[Brian Weinstein]

Created attachment 76105 [details]
[PATCH] CROSS_PLATFORM_CONTEXT_MENUS refactoring

[Adam Roben (:aroben)]

Comment on attachment 76105 [details]
[PATCH] CROSS_PLATFORM_CONTEXT_MENUS refactoring

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

> WebCore/platform/ContextMenu.cpp:42
> +void ContextMenu::setItems(Vector<ContextMenuItem> menuItems)

That should be a const Vector<ContextMenuItem>&. I think the implementation of this function could go in the header.

> WebCore/platform/ContextMenu.cpp:47
> +Vector<ContextMenuItem> ContextMenu::items()

That should be a const Vector<ContextMenuItem>&, and this should be a const member function. I think the implementation of this function could go in the header.

> WebCore/platform/ContextMenu.cpp:55
> +ContextMenuItem* ContextMenu::itemAtIndex(unsigned index)
> +{
> +    return &(m_items[index]);
> +}

No need for the parentheses. I think the implementation of this function could go in the header.

> WebCore/platform/ContextMenu.cpp:57
> +static ContextMenuItem* itemWithActionInMenu(unsigned action, Vector<ContextMenuItem>& menu)

That should be a const Vector<ContextMenuItem>&. I'd remove the "InMenu" part of the name and call the parameter "items" instead of "menu".

> WebCore/platform/ContextMenu.cpp:64
> +        if (menu[i].action() == action)
> +            return &menu[i];
> +        if (menu[i].type() == SubmenuType) {
> +            if (ContextMenuItem* subMenuItem = itemWithActionInMenu(action, menu[i].submenu()))
> +                return subMenuItem;

Maybe it would be better to store menu[i] in a local: const ContextMenuItem& item = menu[i];

Some early-continues could reduce the nesting here.

> WebCore/platform/ContextMenu.h:52
> +        typedef HMENU NativeMenuType;

No need for the "Type" in this name.

> WebCore/platform/ContextMenu.h:56
> +        ContextMenu(const Vector<ContextMenuItem>&);
> +        ContextMenu(NativeMenuType);

You should mark both of these constructors explicit.

> WebCore/platform/ContextMenu.h:58
> +        NativeMenuType nativeMenu();

This can be a const member function.

> WebCore/platform/ContextMenu.h:71
> +    private:
> +        Vector<ContextMenuItem> m_items;
> +#else
>      public:

I know it's only meant to be temporary, but I think this #if style is a little strange. I think it would be better to just put #if USE(CROSS_PLATFORM_CONTEXT_MENUS) around the declarations that are different between the two implementations, rather than putting them around the whole class declaration.

> WebCore/platform/ContextMenu.h:81
> +
>          ~ContextMenu();
>  
>          void insertItem(unsigned position, ContextMenuItem&);
>          void appendItem(ContextMenuItem&);
>          
>          ContextMenuItem* itemWithAction(unsigned);
> +        ContextMenuItem* itemAtIndex(unsigned);

Why are these changes needed?

> WebCore/platform/ContextMenuItem.h:167
> +

You should undo this change.

> WebCore/platform/ContextMenuItem.h:258
> +        typedef MENUITEMINFO NativeItemType;

No need for the "Type" in this name.

> WebCore/platform/ContextMenuItem.h:263
> +        ContextMenuItem(NativeItemType);

You should mark this constructor explicit.

> WebCore/platform/ContextMenuItem.h:265
> +        NativeItemType nativeMenuItem();

This can be a const member function.

> WebCore/platform/ContextMenuItem.h:280
> +        ContextMenuItemType type() const { return m_type; }
> +        void setType(ContextMenuItemType type) { m_type = type; }
> +
> +        ContextMenuAction action() const { return m_action; }
> +        void setAction(ContextMenuAction action) { m_action = action; }
> +
> +        String title() const { return m_title; }
> +        void setTitle(const String& title) { m_title = title; }
> +
> +        bool checked() const { return m_checked; }
> +        void setChecked(bool checked = true) { m_checked = checked; }
> +
> +        bool enabled() const { return m_enabled; }
> +        void setEnabled(bool enabled = true) { m_enabled = enabled; }

In general we prefer to put setters first.

> WebCore/platform/ContextMenuItem.h:283
> +        void setSubMenu(ContextMenu*);
> +        Vector<ContextMenuItem>& submenu() { return m_subMenuItems; }

"submenu()" doesn't follow the same capitalization as "setSubMenu" and "m_subMenuItems".

Maybe submenu() should be renamed to subMenuItems?

submenu() should return a const Vector<ContextMenuItem>& and be a const member function.

> WebCore/platform/ContextMenuItem.h:286
> +        ~ContextMenuItem();
> +    private:

You should add a blank line between these two lines.

> WebCore/platform/win/ContextMenuItemWin.cpp:55
> +    m_type = info.fType & MFT_SEPARATOR ? SeparatorType : ActionType;
> +    m_action = static_cast<ContextMenuAction>(info.wID);
> +    m_title = String(info.dwTypeData, wcslen(info.dwTypeData));
> +
> +    if (info.hSubMenu) {

All my comments from comment 15 (about checking info.fMask, etc.) still apply here.

> WebCore/platform/win/ContextMenuItemWin.cpp:58
> +        ContextMenu subMenu(info.hSubMenu);
> +        m_subMenuItems = subMenu.items();

No need for the local variable here.

> WebCore/platform/win/ContextMenuItemWin.cpp:81
> +    info.cch = m_title.length();
> +    info.dwTypeData = (LPWSTR) m_title.charactersWithNullTermination();

You should be using C++ casts here. You also shouldn't be setting these members for separator items.

But this seems problematic even for string items. It isn't clear to the caller that this dwTypeData is only valid while the ContextMenuItem is still around. It also seems strange for the caller to be responsible for cleaning up info.hSubMenu but not info.dwTypeData.

Maybe it would be better to get rid of ContextMenuItem::nativeMenuItem and put the code into a helper function that ContextMenu::nativeMenu calls instead. Then it would be OK for the helper function to do things like what you've done here (where dwTypeData relies on the lifetime of the ContextMenuItem from which it came), because the scope of the code would be so constrained.

> WebCore/platform/win/ContextMenuWin.cpp:64
> +        if (info.fType == MFT_SEPARATOR) {
> +            // If we don't have a string, then we must be in a separator.

I don't think this comment is needed or helpful. "If we don't have a string" isn't what we just found out; we just found out this is a separator item. I'd just ditch the comment entirely.

> WebCore/platform/win/ContextMenuWin.cpp:83
> +    for (int i = 0; i < m_items.size(); ++i) {

As previously mentioned, size_t is the right type for iterating over Vector.

> WebKit2/Shared/WebContextMenuItemData.cpp:73
> +        Vector<WebCore::ContextMenuItem> coreSubmenu = item.submenu();

This should be a const Vector<WebCore::ContextMenuItem>&.

> WebKit2/WebProcess/WebPage/WebContextMenu.cpp:68
> +    Vector<ContextMenuItem> coreItems = menu->items();

This should be a const Vector<ContextMenuItem>&.

> WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:59
> +PassOwnPtr<ContextMenu> WebContextMenuClient::customizeMenu(PassOwnPtr<ContextMenu> menu)
>  {

I think it would be a little better to start this function like this:

customizeMenu(PassOwnPtr<ContextMenu> popMenu)
{
    OwnPtr<ContextMenu> menu = popMenu;

That way you don't have to worry about menu getting nulled out if you accidentally assign it into an OwnPtr or PassOwnPtr.

> WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:74
> +    if (FAILED(uiDelegate->contextMenuItemsForElement(m_webView, propertyBag.get(), (OLE_HANDLE)(ULONG64)nativeMenu, (OLE_HANDLE*)&nativeMenu)))
> +        return menu;
> +    
> +    OwnPtr<ContextMenu> customizedMenu(new ContextMenu(nativeMenu));
> +    return customizedMenu.release();

You're leaking nativeMenu in both of these cases. You should also use adoptPtr when constructing the new menu.

> WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:91
> +    MENUITEMINFO selectedItem = item->nativeMenuItem();
> +    uiDelegate->contextMenuItemSelected(m_webView, &selectedItem, propertyBag.get());

If selectedItem contained an hSubMenu, you'd be leaking it here. But I think it's guaranteed not to contain one (because you can't select submenu items), so you should add an assertion with a comment.

> WebKit/win/WebCoreSupport/WebContextMenuClient.h:28
> +#include <WebCore/PlatformMenuDescription.h>

I don't think this is needed.

[Build Bot]

Attachment 76105 [details] did not build on win:
Build output: http://queues.webkit.org/results/6913009

[Adam Roben (:aroben)]

Did you actually turn on the new macro anywhere?

[Brian Weinstein]

(In reply to comment #22)
> Did you actually turn on the new macro anywhere?

I did in my tree, but it didn't show in the diff, so it borked the EWS bots.

[Brian Weinstein]

Comment on attachment 76105 [details]
[PATCH] CROSS_PLATFORM_CONTEXT_MENUS refactoring

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

>> WebCore/platform/ContextMenu.cpp:42
>> +void ContextMenu::setItems(Vector<ContextMenuItem> menuItems)
> 
> That should be a const Vector<ContextMenuItem>&. I think the implementation of this function could go in the header.

Fixed.

>> WebCore/platform/ContextMenu.cpp:47
>> +Vector<ContextMenuItem> ContextMenu::items()
> 
> That should be a const Vector<ContextMenuItem>&, and this should be a const member function. I think the implementation of this function could go in the header.

Fixed.

>> WebCore/platform/ContextMenu.cpp:55
>> +}
> 
> No need for the parentheses. I think the implementation of this function could go in the header.

Fixed.

>> WebCore/platform/ContextMenu.cpp:57
>> +static ContextMenuItem* itemWithActionInMenu(unsigned action, Vector<ContextMenuItem>& menu)
> 
> That should be a const Vector<ContextMenuItem>&. I'd remove the "InMenu" part of the name and call the parameter "items" instead of "menu".

I couldn't name it just itemWithAction, that was causing compile errors - I named it findItemWithAction, to signify that it is doing work.

>> WebCore/platform/ContextMenu.cpp:64
>> +                return subMenuItem;
> 
> Maybe it would be better to store menu[i] in a local: const ContextMenuItem& item = menu[i];
> 
> Some early-continues could reduce the nesting here.

Fixed, stored it in a non-const (due to the const discussion we had on IRC), and added an early continue.

>> WebCore/platform/ContextMenu.h:52
>> +        typedef HMENU NativeMenuType;
> 
> No need for the "Type" in this name.

Fixed.

>> WebCore/platform/ContextMenu.h:56
>> +        ContextMenu(NativeMenuType);
> 
> You should mark both of these constructors explicit.

Fixed.

>> WebCore/platform/ContextMenu.h:58
>> +        NativeMenuType nativeMenu();
> 
> This can be a const member function.

Fixed.

>> WebCore/platform/ContextMenu.h:71
>>      public:
> 
> I know it's only meant to be temporary, but I think this #if style is a little strange. I think it would be better to just put #if USE(CROSS_PLATFORM_CONTEXT_MENUS) around the declarations that are different between the two implementations, rather than putting them around the whole class declaration.

I think every function is different. Pretty much all of the functions in CROSS_PLATFORM_CONTEXT_MENUS are implemented in the headers, and the only one that isn't (itemWithAction) - has a different signature. I don't know what this would gain us.

>> WebCore/platform/ContextMenu.h:81
>> +        ContextMenuItem* itemAtIndex(unsigned);
> 
> Why are these changes needed?

They're not - removed.

>> WebCore/platform/ContextMenuItem.h:167
>> +
> 
> You should undo this change.

Done.

>> WebCore/platform/ContextMenuItem.h:258
>> +        typedef MENUITEMINFO NativeItemType;
> 
> No need for the "Type" in this name.

Fixed.

>> WebCore/platform/ContextMenuItem.h:263
>> +        ContextMenuItem(NativeItemType);
> 
> You should mark this constructor explicit.

Fixed.

>> WebCore/platform/ContextMenuItem.h:265
>> +        NativeItemType nativeMenuItem();
> 
> This can be a const member function.

Fixed.

>> WebCore/platform/ContextMenuItem.h:280
>> +        void setEnabled(bool enabled = true) { m_enabled = enabled; }
> 
> In general we prefer to put setters first.

Fixed.

>> WebCore/platform/ContextMenuItem.h:283
>> +        Vector<ContextMenuItem>& submenu() { return m_subMenuItems; }
> 
> "submenu()" doesn't follow the same capitalization as "setSubMenu" and "m_subMenuItems".
> 
> Maybe submenu() should be renamed to subMenuItems?
> 
> submenu() should return a const Vector<ContextMenuItem>& and be a const member function.

Renamed to subMenuItems, and returns a const Vector<ContextMenuItem>& and is a const function.

>> WebCore/platform/ContextMenuItem.h:286
>> +    private:
> 
> You should add a blank line between these two lines.

Fixed.

>> WebCore/platform/win/ContextMenuItemWin.cpp:55
>> +    if (info.hSubMenu) {
> 
> All my comments from comment 15 (about checking info.fMask, etc.) still apply here.

Done, checked info.fMask for any accessor before using it (MIIM_STATE for info.fState, MIIM_ID for wID, etc).

>> WebCore/platform/win/ContextMenuItemWin.cpp:58
>> +        m_subMenuItems = subMenu.items();
> 
> No need for the local variable here.

Fixed.

>> WebCore/platform/win/ContextMenuWin.cpp:64
>> +            // If we don't have a string, then we must be in a separator.
> 
> I don't think this comment is needed or helpful. "If we don't have a string" isn't what we just found out; we just found out this is a separator item. I'd just ditch the comment entirely.

Removed, forgot to get rid of it earlier.

>> WebCore/platform/win/ContextMenuWin.cpp:83
>> +    for (int i = 0; i < m_items.size(); ++i) {
> 
> As previously mentioned, size_t is the right type for iterating over Vector.

D'oh, thought I had fixed all of these. Fixed.

>> WebKit2/Shared/WebContextMenuItemData.cpp:73
>> +        Vector<WebCore::ContextMenuItem> coreSubmenu = item.submenu();
> 
> This should be a const Vector<WebCore::ContextMenuItem>&.

Fixed.

>> WebKit2/WebProcess/WebPage/WebContextMenu.cpp:68
>> +    Vector<ContextMenuItem> coreItems = menu->items();
> 
> This should be a const Vector<ContextMenuItem>&.

Fixed (pending Mac building).

>> WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:59
>>  {
> 
> I think it would be a little better to start this function like this:
> 
> customizeMenu(PassOwnPtr<ContextMenu> popMenu)
> {
>     OwnPtr<ContextMenu> menu = popMenu;
> 
> That way you don't have to worry about menu getting nulled out if you accidentally assign it into an OwnPtr or PassOwnPtr.

Fixed.

>> WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:74
>> +    return customizedMenu.release();
> 
> You're leaking nativeMenu in both of these cases. You should also use adoptPtr when constructing the new menu.

Added the adoptPtr call, and called DestroyMenu on he created nativeMenu.

>> WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:91
>> +    uiDelegate->contextMenuItemSelected(m_webView, &selectedItem, propertyBag.get());
> 
> If selectedItem contained an hSubMenu, you'd be leaking it here. But I think it's guaranteed not to contain one (because you can't select submenu items), so you should add an assertion with a comment.

Added the comment and the assertion.

>> WebKit/win/WebCoreSupport/WebContextMenuClient.h:28
>> +#include <WebCore/PlatformMenuDescription.h>
> 
> I don't think this is needed.

It's not - removed.

[Brian Weinstein]

Comment on attachment 76105 [details]
[PATCH] CROSS_PLATFORM_CONTEXT_MENUS refactoring

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

>> WebCore/platform/ContextMenu.cpp:42
>> +void ContextMenu::setItems(Vector<ContextMenuItem> menuItems)
> 
> That should be a const Vector<ContextMenuItem>&. I think the implementation of this function could go in the header.

Fixed.

>> WebCore/platform/ContextMenu.cpp:47
>> +Vector<ContextMenuItem> ContextMenu::items()
> 
> That should be a const Vector<ContextMenuItem>&, and this should be a const member function. I think the implementation of this function could go in the header.

Fixed.

>> WebCore/platform/ContextMenu.cpp:55
>> +}
> 
> No need for the parentheses. I think the implementation of this function could go in the header.

Fixed.

>> WebCore/platform/ContextMenu.cpp:57
>> +static ContextMenuItem* itemWithActionInMenu(unsigned action, Vector<ContextMenuItem>& menu)
> 
> That should be a const Vector<ContextMenuItem>&. I'd remove the "InMenu" part of the name and call the parameter "items" instead of "menu".

I couldn't name it just itemWithAction, that was causing compile errors - I named it findItemWithAction, to signify that it is doing work.

>> WebCore/platform/ContextMenu.cpp:64
>> +                return subMenuItem;
> 
> Maybe it would be better to store menu[i] in a local: const ContextMenuItem& item = menu[i];
> 
> Some early-continues could reduce the nesting here.

Fixed, stored it in a non-const (due to the const discussion we had on IRC), and added an early continue.

>> WebCore/platform/ContextMenu.h:52
>> +        typedef HMENU NativeMenuType;
> 
> No need for the "Type" in this name.

Fixed.

>> WebCore/platform/ContextMenu.h:56
>> +        ContextMenu(NativeMenuType);
> 
> You should mark both of these constructors explicit.

Fixed.

>> WebCore/platform/ContextMenu.h:58
>> +        NativeMenuType nativeMenu();
> 
> This can be a const member function.

Fixed.

>> WebCore/platform/ContextMenu.h:71
>>      public:
> 
> I know it's only meant to be temporary, but I think this #if style is a little strange. I think it would be better to just put #if USE(CROSS_PLATFORM_CONTEXT_MENUS) around the declarations that are different between the two implementations, rather than putting them around the whole class declaration.

I think every function is different. Pretty much all of the functions in CROSS_PLATFORM_CONTEXT_MENUS are implemented in the headers, and the only one that isn't (itemWithAction) - has a different signature. I don't know what this would gain us.

>> WebCore/platform/ContextMenu.h:81
>> +        ContextMenuItem* itemAtIndex(unsigned);
> 
> Why are these changes needed?

They're not - removed.

>> WebCore/platform/ContextMenuItem.h:167
>> +
> 
> You should undo this change.

Done.

>> WebCore/platform/ContextMenuItem.h:258
>> +        typedef MENUITEMINFO NativeItemType;
> 
> No need for the "Type" in this name.

Fixed.

>> WebCore/platform/ContextMenuItem.h:263
>> +        ContextMenuItem(NativeItemType);
> 
> You should mark this constructor explicit.

Fixed.

>> WebCore/platform/ContextMenuItem.h:265
>> +        NativeItemType nativeMenuItem();
> 
> This can be a const member function.

Fixed.

>> WebCore/platform/ContextMenuItem.h:280
>> +        void setEnabled(bool enabled = true) { m_enabled = enabled; }
> 
> In general we prefer to put setters first.

Fixed.

>> WebCore/platform/ContextMenuItem.h:283
>> +        Vector<ContextMenuItem>& submenu() { return m_subMenuItems; }
> 
> "submenu()" doesn't follow the same capitalization as "setSubMenu" and "m_subMenuItems".
> 
> Maybe submenu() should be renamed to subMenuItems?
> 
> submenu() should return a const Vector<ContextMenuItem>& and be a const member function.

Renamed to subMenuItems, and returns a const Vector<ContextMenuItem>& and is a const function.

>> WebCore/platform/ContextMenuItem.h:286
>> +    private:
> 
> You should add a blank line between these two lines.

Fixed.

>> WebCore/platform/win/ContextMenuItemWin.cpp:55
>> +    if (info.hSubMenu) {
> 
> All my comments from comment 15 (about checking info.fMask, etc.) still apply here.

Done, checked info.fMask for any accessor before using it (MIIM_STATE for info.fState, MIIM_ID for wID, etc).

>> WebCore/platform/win/ContextMenuItemWin.cpp:58
>> +        m_subMenuItems = subMenu.items();
> 
> No need for the local variable here.

Fixed.

>> WebCore/platform/win/ContextMenuWin.cpp:64
>> +            // If we don't have a string, then we must be in a separator.
> 
> I don't think this comment is needed or helpful. "If we don't have a string" isn't what we just found out; we just found out this is a separator item. I'd just ditch the comment entirely.

Removed, forgot to get rid of it earlier.

>> WebCore/platform/win/ContextMenuWin.cpp:83
>> +    for (int i = 0; i < m_items.size(); ++i) {
> 
> As previously mentioned, size_t is the right type for iterating over Vector.

D'oh, thought I had fixed all of these. Fixed.

>> WebKit2/Shared/WebContextMenuItemData.cpp:73
>> +        Vector<WebCore::ContextMenuItem> coreSubmenu = item.submenu();
> 
> This should be a const Vector<WebCore::ContextMenuItem>&.

Fixed.

>> WebKit2/WebProcess/WebPage/WebContextMenu.cpp:68
>> +    Vector<ContextMenuItem> coreItems = menu->items();
> 
> This should be a const Vector<ContextMenuItem>&.

Fixed (pending Mac building).

>> WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:59
>>  {
> 
> I think it would be a little better to start this function like this:
> 
> customizeMenu(PassOwnPtr<ContextMenu> popMenu)
> {
>     OwnPtr<ContextMenu> menu = popMenu;
> 
> That way you don't have to worry about menu getting nulled out if you accidentally assign it into an OwnPtr or PassOwnPtr.

Fixed.

>> WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:74
>> +    return customizedMenu.release();
> 
> You're leaking nativeMenu in both of these cases. You should also use adoptPtr when constructing the new menu.

Added the adoptPtr call, and called DestroyMenu on he created nativeMenu.

>> WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:91
>> +    uiDelegate->contextMenuItemSelected(m_webView, &selectedItem, propertyBag.get());
> 
> If selectedItem contained an hSubMenu, you'd be leaking it here. But I think it's guaranteed not to contain one (because you can't select submenu items), so you should add an assertion with a comment.

Added the comment and the assertion.

>> WebKit/win/WebCoreSupport/WebContextMenuClient.h:28
>> +#include <WebCore/PlatformMenuDescription.h>
> 
> I don't think this is needed.

It's not - removed.

[Brian Weinstein]

Comment on attachment 76105 [details]
[PATCH] CROSS_PLATFORM_CONTEXT_MENUS refactoring

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

>> WebCore/platform/win/ContextMenuItemWin.cpp:81
>> +    info.dwTypeData = (LPWSTR) m_title.charactersWithNullTermination();
> 
> You should be using C++ casts here. You also shouldn't be setting these members for separator items.
> 
> But this seems problematic even for string items. It isn't clear to the caller that this dwTypeData is only valid while the ContextMenuItem is still around. It also seems strange for the caller to be responsible for cleaning up info.hSubMenu but not info.dwTypeData.
> 
> Maybe it would be better to get rid of ContextMenuItem::nativeMenuItem and put the code into a helper function that ContextMenu::nativeMenu calls instead. Then it would be OK for the helper function to do things like what you've done here (where dwTypeData relies on the lifetime of the ContextMenuItem from which it came), because the scope of the code would be so constrained.

Fixed the separator issues.

I re-factored this code to not have dwTypeData set inside of nativeMenuItem. Only one caller needs that information (ContextMenu::nativeMenu), and so that caller will handle setting up that information. It makes the lifetime handling much more straightforward.

[Brian Weinstein]

Created attachment 76145 [details]
[PATChH] CROSS_PLATFORM_CONTEXT_MENUS + Adam's feedback

[Build Bot]

Attachment 76145 [details] did not build on win:
Build output: http://queues.webkit.org/results/6958017

[Adam Roben (:aroben)]

Comment on attachment 76145 [details]
[PATChH] CROSS_PLATFORM_CONTEXT_MENUS + Adam's feedback

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

I still don't see a place that turns on CROSS_PLATFORM_CONTEXT_MENUS.

> WebCore/platform/win/ContextMenuItemWin.cpp:49
> +ContextMenuItem::ContextMenuItem(MENUITEMINFO info)

It looks like this constructor is now leaving member variables uninitialized when info.fMask doesn't contain the appropriate bits. You should initialize them!

> WebCore/platform/win/ContextMenuItemWin.cpp:52
> +        m_type = info.fType & MFT_SEPARATOR ? SeparatorType : ActionType;

That should be ==, not &.

> WebCore/platform/win/ContextMenuItemWin.cpp:53
> +        m_title = String(info.dwTypeData, wcslen(info.dwTypeData));

You should only look at this if info.fType is MFT_STRING and info.fMask has MIIM_STRING set.

> WebCore/platform/win/ContextMenuItemWin.cpp:61
> +        m_subMenuItems = ContextMenu(info.hSubMenu).items();

It's unfortunate that this line of code will create and throwaway an extra copy of the items Vector. If you were to add a function like this:

void getContextMenuItems(NativeMenu, Vector<ContextMenuItem>&);

Then you could use it here.

> WebCore/platform/win/ContextMenuItemWin.cpp:73
> +// ContextMenuItem::nativeMenuItem doesn't set the info.dwTypeData. This is
> +// done to make the lifetime handling of the returned MENUITEMINFO easier on
> +// callers. The only caller that uses the title is ContextMenu::nativeMenu,
> +// so we just set the title there.

I'd replace the last sentence here with something more generic, like "Callers can set dwTypeData themselves (and make their own decisions about its lifetime) if they need it."

I think the way you chose to handle this issue. Maybe this comment should go in the header file? You'd obviously have to prefix it with "On Windows,".

> WebCore/platform/win/ContextMenuItemWin.cpp:91
> +    if (m_type == SubmenuType)
> +        info.hSubMenu = ContextMenu(m_subMenuItems).nativeMenu();

Maybe you should only include MIIM_SUBMENU in info.fMask in this case?

It's unfortunate that this line of code will copy m_subMenuItems just to create an HMENU. You could add a function that goes directly from a Vector<ContextMenuItem> to an HMENU. (Then maybe you wouldn't even need the ContextMenu(const Vector<ContextMenuItem>&) constructor? Or maybe you'd still need that for WebKit2?)

> WebCore/platform/win/ContextMenuWin.cpp:57
> +        UINT infoMask = MIIM_FTYPE | MIIM_ID | MIIM_STRING | MIIM_STATE | MIIM_SUBMENU; 
> +        MENUITEMINFO info = {0};
> +        info.cbSize = sizeof(MENUITEMINFO);
> +        info.fMask = infoMask;

I don't think the infoMask variable is helpful here.

> WebCore/platform/win/ContextMenuWin.cpp:89
> +        // lifetime handling easier for callers. This is the only caller that needs to set the title,
> +        // so set it here.

I don't think the "This is the only caller" information is important, and it's likely to become out of date.

> WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:73
> +    if (FAILED(uiDelegate->contextMenuItemsForElement(m_webView, propertyBag.get(), (OLE_HANDLE)(ULONG64)nativeMenu, (OLE_HANDLE*)&nativeMenu)))
> +        return menu.release();

You're leaking nativeMenu here.

> WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:96
> +    ASSERT(item->type() != SubmenuType && item->type() != SeparatorType);

You should split this up into two separate assertions.

[Adam Roben (:aroben)]

Comment on attachment 76145 [details]
[PATChH] CROSS_PLATFORM_CONTEXT_MENUS + Adam's feedback

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

> WebCore/platform/ContextMenuItem.h:261
> +        ContextMenuItem(ContextMenuAction, const String&, bool enabled, bool checked, Vector<ContextMenuItem> subMenuItems);

subMenuItems should be a const Vector<ContextMenuItem>&.

[Brian Weinstein]

Comment on attachment 76145 [details]
[PATChH] CROSS_PLATFORM_CONTEXT_MENUS + Adam's feedback

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

> WebCore/platform/win/ContextMenuItemWin.cpp:30
> +#include "HitTestResult.h"

This was removed, sorry it was in here so long.

>> WebCore/platform/win/ContextMenuItemWin.cpp:49
>> +ContextMenuItem::ContextMenuItem(MENUITEMINFO info)
> 
> It looks like this constructor is now leaving member variables uninitialized when info.fMask doesn't contain the appropriate bits. You should initialize them!

Fixed - wasn't sure what to initialize the string to - I left it null, but initialized the rest.

>> WebCore/platform/win/ContextMenuItemWin.cpp:52
>> +        m_type = info.fType & MFT_SEPARATOR ? SeparatorType : ActionType;
> 
> That should be ==, not &.

Fixed.

>> WebCore/platform/win/ContextMenuItemWin.cpp:53
>> +        m_title = String(info.dwTypeData, wcslen(info.dwTypeData));
> 
> You should only look at this if info.fType is MFT_STRING and info.fMask has MIIM_STRING set.

Fixed, but not sure what to initialize it to if these aren't set.

>> WebCore/platform/win/ContextMenuItemWin.cpp:61
>> +        m_subMenuItems = ContextMenu(info.hSubMenu).items();
> 
> It's unfortunate that this line of code will create and throwaway an extra copy of the items Vector. If you were to add a function like this:
> 
> void getContextMenuItems(NativeMenu, Vector<ContextMenuItem>&);
> 
> Then you could use it here.

Added a static void getContextMenuItems(HMENU, Vector<ContextMenuItem>&) that is called by this and the ContextMenu(HMENU) constructor.

>> WebCore/platform/win/ContextMenuItemWin.cpp:73
>> +// so we just set the title there.
> 
> I'd replace the last sentence here with something more generic, like "Callers can set dwTypeData themselves (and make their own decisions about its lifetime) if they need it."
> 
> I think the way you chose to handle this issue. Maybe this comment should go in the header file? You'd obviously have to prefix it with "On Windows,".

Kept the comment in the implementation, and added a comment to the header to this effect.

>> WebCore/platform/win/ContextMenuItemWin.cpp:91
>> +        info.hSubMenu = ContextMenu(m_subMenuItems).nativeMenu();
> 
> Maybe you should only include MIIM_SUBMENU in info.fMask in this case?
> 
> It's unfortunate that this line of code will copy m_subMenuItems just to create an HMENU. You could add a function that goes directly from a Vector<ContextMenuItem> to an HMENU. (Then maybe you wouldn't even need the ContextMenu(const Vector<ContextMenuItem>&) constructor? Or maybe you'd still need that for WebKit2?)

Added a static function: static HMENU createMenuFromItems(const Vector<ContextMenuItem>&) - and call that from both ContextMenu::nativeMenu() and here. I was able to get rid of the constructor.

>> WebCore/platform/win/ContextMenuWin.cpp:57
>> +        info.fMask = infoMask;
> 
> I don't think the infoMask variable is helpful here.

Removed.

>> WebCore/platform/win/ContextMenuWin.cpp:89
>> +        // so set it here.
> 
> I don't think the "This is the only caller" information is important, and it's likely to become out of date.

Removed.

>> WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:73
>> +        return menu.release();
> 
> You're leaking nativeMenu here.

Fixed.

>> WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:96
>> +    ASSERT(item->type() != SubmenuType && item->type() != SeparatorType);
> 
> You should split this up into two separate assertions.

Fixed.

[Brian Weinstein]

Created attachment 76224 [details]
[PATCH] Adam's Comments + Platform.h

[Brian Weinstein]

Comment on attachment 76145 [details]
[PATChH] CROSS_PLATFORM_CONTEXT_MENUS + Adam's feedback

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

>> WebCore/platform/ContextMenuItem.h:261
>> +        ContextMenuItem(ContextMenuAction, const String&, bool enabled, bool checked, Vector<ContextMenuItem> subMenuItems);
> 
> subMenuItems should be a const Vector<ContextMenuItem>&.

This is fixed, but didn't make it in the verison of the patch that was just uploaded, I didn't see the comment until after I uploaded.

[Adam Roben (:aroben)]

Comment on attachment 76224 [details]
[PATCH] Adam's Comments + Platform.h

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

> WebCore/platform/ContextMenu.cpp:39
> +// FIXME: When more callers adopt CROSS_PLATFORM_CONTEXT_MENUS, this function should take a const Vector<ContextMenuItem>& - and should
> +// return a const ContextMenuItem*.
> +static ContextMenuItem* findItemWithAction(unsigned action, Vector<ContextMenuItem>& items)

Maybe it would be better to make this function take a const Vector and return a const ContextMenuItem* and to put a const_cast and this comment in ContextMenu::itemWithAction. Then all that has to change in the future is the removal of the const_cast.

> WebCore/platform/ContextMenu.h:60
> +        static NativeMenu createNativeMenuFromItems(const Vector<ContextMenuItem>&);
> +        static void getContextMenuItems(NativeMenu, Vector<ContextMenuItem>&);

I don't think these have to be member functions, but it seems OK for them to be.

> WebCore/platform/ContextMenuItem.h:282
> +        ContextMenuItem(ContextMenuItemType, ContextMenuAction, const String&, ContextMenu* subMenu = 0);
> +        ContextMenuItem(ContextMenuItemType, ContextMenuAction, const String&, bool enabled, bool checked);
> +        ContextMenuItem(ContextMenuAction, const String&, bool enabled, bool checked, Vector<ContextMenuItem> subMenuItems);
> +        explicit ContextMenuItem(NativeItem);
> +
> +        // On Windows, the title (dwTypeData of the MENUITEMINFO) is not set in this function. Callers can set the title themselves,
> +        // and handle the lifetime of the title, if they need it.
> +        NativeItem nativeMenuItem() const;
> +
> +        void setType(ContextMenuItemType type) { m_type = type; }
> +        ContextMenuItemType type() const { return m_type; }
> +
> +        void setAction(ContextMenuAction action) { m_action = action; }
> +        ContextMenuAction action() const { return m_action; }
> +
> +        void setTitle(const String& title) { m_title = title; }
> +        const String& title() const { return m_title; }
> +
> +        void setChecked(bool checked = true) { m_checked = checked; }
> +        bool checked() const { return m_checked; }
> +
> +        void setEnabled(bool enabled = true) { m_enabled = enabled; }
> +        bool enabled() const { return m_enabled; }
> +

So many of these functions have the same declaration as in the !USE(CROSS_PLATFORM_CONTEXT_MENUS) case. I think it would be better to share those declarations and only #ifdef the ones that are different. That will also make it more clear which functions are used in both models and which ones are model-specific. (The same comment applies to ContextMenu.h, though there are fewer shared declarations there.) I'd suggest doing it like this:

ContextMenuItem {
public:
    shared declarations
#if USE(CROSS_PLATFORM_CONTEXT_MENUS)
    more declarations
#else
    more declarations
#endif

private
#if USE(CROSS_PLATFORM_CONTEXT_MENUS)
    more declarations
#else
    more declarations
#endif
};

You'll have to move your setters/getters to ContextMenuItem.cpp, but that seems fine to me. Being able to share the declarations is worth it.

> WebCore/platform/win/ContextMenuItemWin.cpp:48
> +ContextMenuItem::ContextMenuItem(MENUITEMINFO info)

This should be a const MENUITEMINFO&.

> WebCore/platform/win/ContextMenuItemWin.cpp:51
> +    if (info.fMask & MIIM_FTYPE)
> +        m_type = info.fType == MFT_SEPARATOR ? SeparatorType : ActionType;

It looks like m_type doesn't get set if MIIM_FTYPE and MIIM_SUBMENU aren't set.

> WebCore/platform/win/ContextMenuItemWin.cpp:54
> +        m_title = String(info.dwTypeData, wcslen(info.dwTypeData));

In theory you could use info.cch instead of wcslen(info.dwTypeData).

> WebCore/platform/win/ContextMenuWin.cpp:58
> +    for (unsigned i = 0; i < count; ++i) {

You should probably use int for i, since count is an int.

> WebCore/platform/win/ContextMenuWin.cpp:62
> +        info.dwTypeData = 0;

This isn't needed. You already initialized it to 0 a few lines earlier.

[Build Bot]

Attachment 76224 [details] did not build on win:
Build output: http://queues.webkit.org/results/6914041

[Brian Weinstein]

Comment on attachment 76224 [details]
[PATCH] Adam's Comments + Platform.h

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

>> WebCore/platform/ContextMenu.cpp:39
>> +static ContextMenuItem* findItemWithAction(unsigned action, Vector<ContextMenuItem>& items)
> 
> Maybe it would be better to make this function take a const Vector and return a const ContextMenuItem* and to put a const_cast and this comment in ContextMenu::itemWithAction. Then all that has to change in the future is the removal of the const_cast.

Fixed.

>> WebCore/platform/ContextMenuItem.h:282
>> +
> 
> So many of these functions have the same declaration as in the !USE(CROSS_PLATFORM_CONTEXT_MENUS) case. I think it would be better to share those declarations and only #ifdef the ones that are different. That will also make it more clear which functions are used in both models and which ones are model-specific. (The same comment applies to ContextMenu.h, though there are fewer shared declarations there.) I'd suggest doing it like this:
> 
> ContextMenuItem {
> public:
>     shared declarations
> #if USE(CROSS_PLATFORM_CONTEXT_MENUS)
>     more declarations
> #else
>     more declarations
> #endif
> 
> private
> #if USE(CROSS_PLATFORM_CONTEXT_MENUS)
>     more declarations
> #else
>     more declarations
> #endif
> };
> 
> You'll have to move your setters/getters to ContextMenuItem.cpp, but that seems fine to me. Being able to share the declarations is worth it.

I wasn't wild about moving the implementation to the cpp file, but it's not particularly hot code, and it should clean up the header.

>> WebCore/platform/win/ContextMenuItemWin.cpp:48
>> +ContextMenuItem::ContextMenuItem(MENUITEMINFO info)
> 
> This should be a const MENUITEMINFO&.

Fixed.

>> WebCore/platform/win/ContextMenuItemWin.cpp:51
>> +        m_type = info.fType == MFT_SEPARATOR ? SeparatorType : ActionType;
> 
> It looks like m_type doesn't get set if MIIM_FTYPE and MIIM_SUBMENU aren't set.

Added an else and set it to SeparatorType.

>> WebCore/platform/win/ContextMenuItemWin.cpp:54
>> +        m_title = String(info.dwTypeData, wcslen(info.dwTypeData));
> 
> In theory you could use info.cch instead of wcslen(info.dwTypeData).

That seems cleaner. Fixed.

>> WebCore/platform/win/ContextMenuWin.cpp:58
>> +    for (unsigned i = 0; i < count; ++i) {
> 
> You should probably use int for i, since count is an int.

Fixed.

>> WebCore/platform/win/ContextMenuWin.cpp:62
>> +        info.dwTypeData = 0;
> 
> This isn't needed. You already initialized it to 0 a few lines earlier.

Removed.

[Brian Weinstein]

Created attachment 76237 [details]
[PATCH] Reordering ContextMenu{Item}.h

[Adam Roben (:aroben)]

Comment on attachment 76237 [details]
[PATCH] Reordering ContextMenu{Item}.h

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

> WebCore/platform/ContextMenu.cpp:45
> +        if (const ContextMenuItem* subMenuItem = findItemWithAction(action, const_cast<Vector<ContextMenuItem>&>(item.subMenuItems())))

No need for the const_cast here anymore.

> WebCore/platform/ContextMenu.h:52
> +        ContextMenuItem* itemWithAction(unsigned);

Looks like you forgot to remove this from the #else case.

[Brian Weinstein]

(In reply to comment #38)
> (From update of attachment 76237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76237&action=review
> 
> > WebCore/platform/ContextMenu.cpp:45
> > +        if (const ContextMenuItem* subMenuItem = findItemWithAction(action, const_cast<Vector<ContextMenuItem>&>(item.subMenuItems())))
> 
> No need for the const_cast here anymore.

Removed.

> 
> > WebCore/platform/ContextMenu.h:52
> > +        ContextMenuItem* itemWithAction(unsigned);
> 
> Looks like you forgot to remove this from the #else case.

That explains the strange build failure I saw on Mac. Fixed.

[WebKit Review Bot]

Attachment 76237 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6975028

[Brian Weinstein]

Created attachment 76242 [details]
[PATCH] For EWS bots + Adam's last comments

[Brian Weinstein]

Landed refactoring in r73802.

[Brian Weinstein]

Created attachment 76271 [details]
[PATCH] Fix Silly Mistake

[Ada Chan]

Comment on attachment 76271 [details]
[PATCH] Fix Silly Mistake

r=me

[Eric Seidel (no email)]

Comment on attachment 76224 [details]
[PATCH] Adam's Comments + Platform.h

Cleared Adam Roben's review+ from obsolete attachment 76224 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 76237 [details]
[PATCH] Reordering ContextMenu{Item}.h

Cleared Adam Roben's review+ from obsolete attachment 76237 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Looks like this was landed?

[Brian Weinstein]

Landed in r73815.