[webkit-reviews] review granted: [Bug 171902] REGRESSION (r?): Web Inspector: Shift-click on color square in Styles sidebar should not select text : [Attachment 310468] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 171902: REGRESSION (r?): Web Inspector: Shift-click on color square in
Styles sidebar should not select text
https://bugs.webkit.org/show_bug.cgi?id=171902

Attachment 310468: Patch

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




--- Comment #2 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 310468
  --> https://bugs.webkit.org/attachment.cgi?id=310468
Patch

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

r=me, with some notes.

>
Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:-455
> -	   if (this._mouseDownCursorPosition.line === cursor.line &&
this._mouseDownCursorPosition.ch === cursor.ch) {

Hmm, it would be nice if we could keep this check and return early if it fails.
This would improve the readability of the function and avoid doing the work to
determine `clickedBookmark` when not needed. To do this would require that
`this._mouseDownCursorPosition = null` happen first, instead of at the end of
the function. A local `mouseDownCursorPosition` would be needed since the rest
of the function uses the value.

I'll leave it up to you. If you think the function is cleanest as-is, I'm fine
with it.