[webkit-reviews] review granted: [Bug 32200] Web Inspector: provide custom context menu in the front-end window. : [Attachment 44366] [PATCH] Proposed change

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has granted Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 32200: Web Inspector: provide custom context menu in the front-end window.
https://bugs.webkit.org/show_bug.cgi?id=32200

Attachment 44366: [PATCH] Proposed change
https://bugs.webkit.org/attachment.cgi?id=44366&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>

> +    class MenuSelectionHandler : public ContextMenuSelectionHandler
> +    {

Brace should be on the same line.


> +	   virtual ~MenuSelectionHandler() {}

I think we informally like a space between the braces. { } vs {}.


> +	   MenuSelectionHandler(InspectorFrontendHost* frontendHost)
> +	       : m_frontendHost(frontendHost)
> +	   {
> +	   }

We informally like to put empty constructors on one line.


> +    this._appendItem(WebInspector.UIString("Edit as HTML"),
this._noop.bind(this));
> +    this._appendItem(WebInspector.UIString("Add attribute"),
this._noop.bind(this));
> +    this._appendSeparator();
> +    this._appendItem(WebInspector.UIString("Copy"), this._copy.bind(this));
> +    this._appendItem(WebInspector.UIString("Delete"),
this._delete.bind(this));

This confused me, since those file is generic. We should land this file mostly
empty until it is ready to work.


> +    _appendItem: function(label, handler)
> +    {
> +	   var id = this._items.length;
> +	   this._items.push({id: id, label: label});
> +	   this._handlers[id] = handler;
> +    },
> +
> +    _appendSeparator: function()
> +    {
> +	   this._items.push({});
> +    },

I think in the end these functions will be publicly used and should drop the
underscore. But maybe I don't understand how the final design will work.


> +WebInspector.documentContextMenu = function(event)

Maybe contextMenuEventFired is a more descriptive name.


> +    if (item->action() >= ContextMenuItemBaseCustomTag) {
> +	   ASSERT(m_selectionHandler);
> +	   m_selectionHandler->contextMenuItemSelected(item);
> +	   return;
> +    }

Does ContextMenuItemBaseApplicationTag get handled before this block? What
prevents another tag higher that isn't a "ContextMenuItemBaseCustomTag" from
hitting this?


> +    class ContextMenuSelectionHandler : public
RefCounted<ContextMenuSelectionHandler>
> +    {

Brace should be on the same line.


> +	   ContextMenuSelectionHandler() {}
> +	   virtual ~ContextMenuSelectionHandler() {};

Put a space in the braces.