[webkit-reviews] review denied: [Bug 48408] Port ContextMenuWin.cpp to WinCE : [Attachment 74411] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 48408: Port ContextMenuWin.cpp to WinCE
https://bugs.webkit.org/show_bug.cgi?id=48408

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=74411&action=review

All the changes of HMENU -> PlatformMenuDescription don't really seem helpful.
At the very least they seem unrelated to the rest of the patch.

> WebCore/platform/win/ContextMenuWin.cpp:118
> +    UINT flags = MF_BYPOSITION;
> +    UINT newItem = 0;
> +    LPCWSTR title = 0;
> +
> +    if (item.type() == SeparatorType)
> +	   flags |= MF_SEPARATOR;
> +    else {
> +	   flags |= MF_STRING;
> +	   flags |= item.checked() ? MF_CHECKED : MF_UNCHECKED;
> +	   flags |= item.enabled() ? MF_ENABLED : MF_GRAYED;
> +
> +	   PlatformMenuItemDescription description =
item.releasePlatformDescription();
> +	   title = description->dwTypeData;
> +	   description->dwTypeData = 0;
> +
> +	   if (description->hSubMenu) {
> +	       flags |= MF_POPUP;
> +	       newItem = reinterpret_cast<UINT>(description->hSubMenu);
> +	       description->hSubMenu = 0;
> +	   } else
> +	       newItem = description->wID;
> +
> +	   free(description);
> +    }
> +
> +    if (::InsertMenuW(m_platformDescription, position, flags, newItem,
title))
> +	   ++m_itemCount;

I think it would be clearer if this code were pulled out into a separate
function.

> WebCore/platform/win/ContextMenuWin.cpp:152
> +#if OS(WINCE)
> +    UINT type = info->fType & MFT_STRING;
> +#else
>      UINT type = info->fType & ~(MFT_MENUBARBREAK | MFT_MENUBREAK |
MFT_OWNERDRAW | MFT_RADIOCHECK | MFT_RIGHTORDER | MFT_RIGHTJUSTIFY);
> +#endif

I think non-CE Windows can use the CE codepath, too.