[webkit-reviews] review denied: [Bug 50514] WebKit2: Context menu support on Windows : [Attachment 76145] [PATChH] CROSS_PLATFORM_CONTEXT_MENUS + Adam's feedback

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 50514: WebKit2: Context menu support on Windows
https://bugs.webkit.org/show_bug.cgi?id=50514

Attachment 76145: [PATChH] CROSS_PLATFORM_CONTEXT_MENUS + Adam's feedback
https://bugs.webkit.org/attachment.cgi?id=76145&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
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.