[webkit-reviews] review denied: [Bug 50514] WebKit2: Context menu support on Windows : [Attachment 76105] [PATCH] CROSS_PLATFORM_CONTEXT_MENUS refactoring

[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 76105: [PATCH] CROSS_PLATFORM_CONTEXT_MENUS refactoring
https://bugs.webkit.org/attachment.cgi?id=76105&action=review

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