[webkit-reviews] review granted: [Bug 124747] Web Inspector: Allow showing a context menu on all mouse events. : [Attachment 217678] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Antoine Quint
<graouts at apple.com>'s request for review:
Bug 124747: Web Inspector: Allow showing a context menu on all mouse events.
https://bugs.webkit.org/show_bug.cgi?id=124747

Attachment 217678: Patch
https://bugs.webkit.org/attachment.cgi?id=217678&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=217678&action=review


r=me, but I do have enough questions that I'd be happy to look at an updated
patch if you have enough changes.

> Source/WebCore/inspector/InspectorFrontendHost.cpp:295
> +    m_frontendPage->contextMenuController().showContextMenuAt(frame,
mousePoint);

By using Page's ContextMenuController, does that mean you get things like
"Inspect Element" added to the context menu? Or is the accessibility context
menu path special?

> Source/WebCore/inspector/InspectorFrontendHost.idl:65
> +    [Custom] void simulateContextmenuEvent(MouseEvent event);

Does this need to be custom because of MouseEvent? I think showContextMenu
needed to be custom because of "any".

I'm not sure about the name, but I could live with it. I'd rather see correct
camel casing though. Here are some suggestions:

    void simulateContextMenuEvent(...)
    void simulateContextMenuEventAndShowContextMenu(...)
    void dispatchContextMenuEventWithEvent(...)
    void showContextMenuWithoutEvent(...)
    void showContextMenuAtLocation(x, y)

Will this be difficult to implement in RWIInspectorFrontendHost? Should that
provide a slightly different API?

> Source/WebInspectorUI/UserInterface/ContextMenu.js:199
> +	   this._menuObject = this._buildDescriptor();

I'd like to see:

    console.assert(!this._menuObject);

And the menuObject is deleted when it it no longer needed.

> Source/WebInspectorUI/UserInterface/ContextMenu.js:205
> +	       if (this._event.type !== "contextmenu" && typeof
InspectorFrontendHost.simulateContextmenuEvent === "function") {
> +		   this._event.target.addEventListener("contextmenu", this,
true);
> +		   InspectorFrontendHost.simulateContextmenuEvent(this._event);


Seeing as this only works with a MouseEvent, we should test and ensure that the
event is a context event or a mouse event. Perhaps an assert. You might be able
to just check "instanceof MouseEvent".

> Source/WebInspectorUI/UserInterface/ContextMenu.js:207
> +		   InspectorFrontendHost.showContextMenu(this._event,
this._menuObject);

this can delete this._menuObject because it is done with it.

> Source/WebInspectorUI/UserInterface/ContextMenu.js:218
> +	   InspectorFrontendHost.showContextMenu(event, this._menuObject);

And this can delete this._menuObject.