[webkit-reviews] review granted: [Bug 192266] Web Inspector: Styles: can't select properties of read-only rules : [Attachment 356269] 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 192266: Web Inspector: Styles: can't select properties of read-only rules
https://bugs.webkit.org/show_bug.cgi?id=192266

Attachment 356269: Patch

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




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

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

r=me

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:115
> +	   if (this.hasSelectedProperties())
> +	       this.selectProperties(this._anchorIndex, this._focusIndex);

Wouldn't this theoretically invalidate any blank property added by the previous
line?  Should we assert that only one or the other is ever true?

    console.assert(!isNaN(this._pendingAddBlankPropertyIndexOffset) +
this.hasSelectedProperties() < 2);

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:518
>	   } else if (event.key === "Tab" || event.key === "Enter") {

Are we not adding tabbing support, or is that going to be in a different patch?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:53
> -	   if (this._isEditable()) {
> +	   if (!this._readOnly) {

Is this change needed for this patch?  It seems like both conditions would not
pass for a computed property.