[webkit-reviews] review denied: [Bug 32277] Chromium: support custom WebCore context menu items in Chromium port. : [Attachment 44477] [PATCH] Same with style bot happy.

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 32277: Chromium: support custom WebCore context menu items in Chromium
port.
https://bugs.webkit.org/show_bug.cgi?id=32277

Attachment 44477: [PATCH] Same with style bot happy.
https://bugs.webkit.org/attachment.cgi?id=44477&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ b/WebKit/chromium/public/WebPopupMenuInfo.h
...
> -// Describes the contents of a popup menu.
> +// Describes the contents of a popup menu or custom context menu item
provided from within WebCore.

Perhaps this struct should be renamed to WebMenuInfo since it
is no longer specific to popup menus.


>  struct WebPopupMenuInfo {
>      struct Item {
>	   enum Type {
>	       Option,
> +	       CheckableOption,
>	       Group,
>	       Separator,
>	   };
>	   WebString label;
>	   Type type;
> +	   unsigned action;

Can you define an enumeration for action?  It occurs to me that we
already have WebMediaPlayerAction and performMediaPlayerAction, which
could be incorporated into a more generic action enum and associated
WebView method.

(WebMediaPlayerAction has two fields, but those could be expressed as
enum values instead.)


> +++ b/WebKit/chromium/public/WebView.h
...
>      virtual void hideAutofillPopup() = 0;
>  
> +    // Context menu --------------------------------------------------------

> +
> +    virtual void executeCustomContextMenuAction(unsigned action) = 0;
>  
>      // Visited link state --------------------------------------------------


nit: WebView.h has two new lines above these delimiter comments


> +++ b/WebKit/chromium/src/ContextMenuClientImpl.cpp
...
> +    // Filter out custom menu elements and add them into the data.
> +    Vector<WebPopupMenuInfo::Item> customItems;
> +    for (size_t i = 0; i < defaultMenu->itemCount(); ++i) {
> +	   ContextMenuItem* inputItem = defaultMenu->itemAtIndex(i,
defaultMenu->platformDescription());
> +	   if (inputItem->action() < ContextMenuItemBaseCustomTag ||
inputItem->action() >=	ContextMenuItemBaseApplicationTag)
> +	       continue;
> +
> +	   WebPopupMenuInfo::Item outputItem;
> +	   outputItem.label = inputItem->title();
> +	   outputItem.enabled = inputItem->enabled();
> +	   outputItem.checked = inputItem->checked();
> +	   outputItem.action = static_cast<unsigned>(inputItem->action() -
ContextMenuItemBaseCustomTag);
> +	   switch (inputItem->type()) {
> +	   case ActionType:
> +	       outputItem.type = WebPopupMenuInfo::Item::Option;
> +	       break;
> +	   case CheckableActionType:
> +	       outputItem.type = WebPopupMenuInfo::Item::CheckableOption;
> +	       break;
> +	   case SeparatorType:
> +	       outputItem.type = WebPopupMenuInfo::Item::Separator;
> +	       break;
> +	   }
> +	   customItems.append(outputItem);
> +    }
> +
> +    WebVector<WebPopupMenuInfo::Item> outputItems(customItems.size());
> +    for (size_t i = 0; i < customItems.size(); ++i)
> +	   outputItems[i] = customItems[i];
> +    data.customItems.swap(outputItems);

nit: Please break this code out into a new function.  The existing
function is already way too long.