[webkit-reviews] review denied: [Bug 199341] Web Inspector: Styles: Command-X should cut selected properties : [Attachment 373170] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 199341: Web Inspector: Styles: Command-X should cut selected properties
https://bugs.webkit.org/show_bug.cgi?id=199341

Attachment 373170: Patch

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




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

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

r-, due to naming issue and the order of operations when not editable.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:550
> +	       let removed = this.removeSelectedProperties();

I think you mean `_removeSelectedProperties`.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:588
> +    _handleCut(event)

The "cut" event should only have an effect if the style is editable.  If not,
it should cause a beep.  So ... (>612)

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:604
> +	   for (let i = startIndex; i <= endIndex; ++i) {
> +	       let propertyView = this._propertyViews[i];
> +	       formattedProperties.push(propertyView.property.formattedText);
> +	   }

You could rewrite/simplify this as:
```
    let formattedProperties = this._propertyViews.slice(startIndex, endIndex +
1).map((propertyView) => propertyView.property.formattedText);
```

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:612
> +	   if (!this.style.editable)

(>588) ... we should check this earlier (and assert here).

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:493
> +	   // Without this, "copy" and "cut" events wouldn't fire on
SpreadsheetCSSStyleDeclarationEditor.

This comment doesn't help me understand _why_ this is needed.  Please explain.