[webkit-reviews] review granted: [Bug 50514] WebKit2: Context menu support on Windows : [Attachment 76224] [PATCH] Adam's Comments + Platform.h

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted 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 76224: [PATCH] Adam's Comments + Platform.h
https://bugs.webkit.org/attachment.cgi?id=76224&action=review

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