[webkit-reviews] review denied: [Bug 119012] Web Inspector: pressing the Cmd key over a CSS property should underline it immediately (jump to definition mode) : [Attachment 213249] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Antoine Quint
<graouts at apple.com>'s request for review:
Bug 119012: Web Inspector: pressing the Cmd key over a CSS property should
underline it immediately (jump to definition mode)
https://bugs.webkit.org/show_bug.cgi?id=119012

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

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


I think you can remove a bunch of code here now right? If
CodeMirrorTokenTracking controller is always tracking when appropriate then:

    1. Make CodeMirrorTokenTrackingController.prototype.{start,stop}Tracking
private, or roll them into _mouseEntered and _mouseLeft
    2. Remove CodeMirrorTokenTrackingController.prototype.tracking and code
that uses it
      - Remove SourceCodeTextEditor.prototype._shouldTrackTokenHovering,
_startTrackingTokenHoveringIfNeeded, _stopTrackingTokenHoveringIfNeeded,
_debuggerDidPause
      - Remove registration and callers of _debuggerDidPause

The only thing I am unsure of is the bottom two lines of this patch (if we
disable jump to symbol tracking by unpressing Cmd, the removal highlighted
range). I suspect you can just remove the if tracking branch.

I'd like to see a follow up patch removing the dead code!

> Source/WebInspectorUI/ChangeLog:22
> +	   tracking since otherwise pressing the Cmd key over a read-only
editor could highlight the previously selected
> +	   candidate of another editor.

Is that really true? Each editor gets a separate tokenTrackingController
instance, how could a range be highlighted in a separate editor?

> Source/WebInspectorUI/UserInterface/Main.js:110
> +    window.addEventListener("mousemove", this._mousemoved.bind(this), true);


Style: "_mousemoved" => "_mouseMoved"