[webkit-reviews] review granted: [Bug 192919] Web Inspector: Styles: Pressing Esc when editing name/value should select entire property : [Attachment 357866] 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 192919: Web Inspector: Styles: Pressing Esc when editing name/value should
select entire property
https://bugs.webkit.org/show_bug.cgi?id=192919

Attachment 357866: Patch

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




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

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

rs=me, with a few Style/NITs

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:483
> +	   this.deselectProperties();

Rather than deselecting everything, shouldn't we just deselect that property
(assuming it even is selected)?  If not, should we at least limit this so that
it only deselects if the `propertyView` was already selected?

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:496
> +	       this.deselectProperties();

It doesn't seem possible to hit this code.  If the `propertyView` has an `index
=== -1`, that would mean that it's not being shown, which would mean it's just
been deleted.  The only way this function gets called in the first place is if
the property is not being deleted (e.g. not new), therefore meaning that it has
to have been visible.  I think we should replace this with
`console.assert(index !== -1)`.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:394
> +    spreadsheetTextFieldDidPressEsc(textField, valueBeforeEditing)

NIT: rather than use `valueBeforeEditing` (which makes me think of the
property's "value"), perhaps we can use `textBeforeEditing` or
`contentsBeforeEditing` so it's less confusing.  I get that this is a delegate
call from `WI.SpreadsheetTextField`, which has nothing to do with a
`WI.CSSProperty`, but I think that that may be confusing in this context.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:396
> +	   const isNewProperty = !valueBeforeEditing;

Style: use `let`.