[webkit-reviews] review denied: [Bug 50514] WebKit2: Context menu support on Windows : [Attachment 75952] [PATCH] Context Menu Refactoring - Reordered Functions

Wed Dec 8 13:41:41 PST 2010

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 75952: [PATCH] Context Menu Refactoring - Reordered Functions
https://bugs.webkit.org/attachment.cgi?id=75952&action=review

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