[webkit-reviews] review granted: [Bug 195494] Web Inspector: we should show artificial context menus on mousedown instead of click : [Attachment 364091] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 195494: Web Inspector: we should show artificial context menus on mousedown
instead of click
https://bugs.webkit.org/show_bug.cgi?id=195494

Attachment 364091: Patch

https://bugs.webkit.org/attachment.cgi?id=364091&action=review




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 364091
  --> https://bugs.webkit.org/attachment.cgi?id=364091
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364091&action=review

> Source/WebInspectorUI/ChangeLog:4
> +	   Web Inspector: we should show artificial context menus on mousedown
instead of click
> +	   https://bugs.webkit.org/show_bug.cgi?id=195494

Awesome! It is not clear from the description that this allows you to mousedown
=> mouseover context menu => mouseup to quick select a contextmenu item. But it
does, and that is awesome.

> Source/WebInspectorUI/ChangeLog:14
> +	   fired when/before a "contextmenu" event is fired, each of the below
callers has to maintain

What does it mean they are fired "when/before a contextmenu event is fired"?

> Source/WebInspectorUI/ChangeLog:41
> +	   * UserInterface/Controllers/CanvasManager.js:
> +	   (WI.CanvasManager.supportsRecordingAutoCapture):
> +	   Drive-by: fix usage of InspectorBackend.domains.{CanvasAgent =>
Canvas}

These drive-bys should really be separate patches, they are so far removed from
this.

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:444
> +	   this._ignoreViewShaderButtonMouseDown = true;

Should this move down after the early returns? Otherwise it seems if we hit one
of the early returns we would never recover and this mousedown would always
bail.

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:475
> +	   this._ignoreViewRecordingButtonMouseDown = true;

Same regarding early returns.

> Source/WebInspectorUI/UserInterface/Views/ContextMenu.js:227
> +	   for (let callback of this._beforeShowCallbacks)

This would fail if this._beforeShowCallbacks is null. This happens in the Tab
Bar, if you have many tabs, make a narrow window, and click the ">>" chevron to
switch between hidden tabs.

(1) It would be simpler to just construct it with an array and then
`addBeforeShowCallback` can be simplified and this would avoid a possible
exception.
(2) We should update that menu as well! TabBar and LegacyTabBar have calls to
ContextMenu.prototype.show

Basically it seems might want to do this for any call to
ContextMenu.prototype.show in mousedown. Should we add an assert in show along
the lines of:

    console.assert(event.type !== "mousedown" ||
this._beforeShowCallbacks.length === 1, "ContextMenu.show() in mousedown should
have a show callback handler to enable quick selection.");