[webkit-reviews] review granted: [Bug 193305] Web Inspector: Styles: easier way to select a single line : [Attachment 361542] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 193305: Web Inspector: Styles: easier way to select a single line
https://bugs.webkit.org/show_bug.cgi?id=193305

Attachment 361542: Patch

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




--- Comment #6 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 361542
  --> https://bugs.webkit.org/attachment.cgi?id=361542
Patch

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

r=me, nice!

> Source/WebInspectorUI/ChangeLog:8
> +	   Start property selection if mouse cursor moved more than 8px after
mousedown.

Why 8px?  I'm guessing it's related to the size of a single character, but
perhaps we should be a bit more "exact" about it.  Some explanation would be
useful.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:49
> +	   this._boundHandleWindowMouseMove =
this._handleWindowMouseMove.bind(this);

Please lazy initialize this when it's first needed (although keep it as null in
the constructor).

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:411
> +	       this._mouseDownPoint = WI.Point.fromEvent(event);

We should also define `_mouseDownPoint` as a `null` value in the constructor.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:412
> +	       window.addEventListener("mousemove",
this._boundHandleWindowMouseMove, false);

You can drop the `false`.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:433
> +	   console.assert(this._mouseDownPoint);

Style: add a newline after.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:434
> +	   if (WI.Point.fromEvent(event).distance(this._mouseDownPoint) > 8) {

NIT: I'd rather you put the "known" value first and compare it to the current
value, like `this._mouseDownPoint.distance(WI.Point.fromEvent(event))`.  This
way, it's a little less "chaining" and a bit easier to read.

NIT: we should invert this and make it an early return.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:435
> +	       if (!this._propertiesEditor.hasSelectedProperties())

Is it actually possible for `hasSelectedProperties()` to be true?  We clear any
selection when we "mousedown" so it should be cleared before are able to move
the mouse (and therefore fire this listener).

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:436
> +		  
this._propertiesEditor.selectProperties(this._mouseDownIndex,
this._mouseDownIndex);

Should we check that `_mouseDownIndex` isn't `NaN`?  We only set it to a value
when there's a property underneath the mouse in "mousedown".  I know we set
`display: none` when there are no properties, but maybe we should assert just
in case.