[webkit-reviews] review denied: [Bug 179461] Web Inspector: Styles Redesign: data corruption when updating values quickly : [Attachment 329870] Patch for review

[bugzilla-daemon at webkit.org]

Devin Rousso <webkit at devinrousso.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 179461: Web Inspector: Styles Redesign: data corruption when updating
values quickly
https://bugs.webkit.org/show_bug.cgi?id=179461

Attachment 329870: Patch for review

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




--- Comment #11 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 329870
  --> https://bugs.webkit.org/attachment.cgi?id=329870
Patch for review

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

r-, only because this definitely needs some tests.  The logic otherwise sounds
fine, but having tests (specifically on CSSStyleDeclaration) would really help
clarify what will happen in the various cases:

 - not locked, style changes via page's JS
 - locked, style changes via page's JS
 - locked, style changes via WebInspector
 - locked, style changes via WebInspector and page's JS (both orderings)
 - etc.

It should be pretty simple to verify just by checking the style's text.

> Source/WebInspectorUI/ChangeLog:20
> +	   To correctly display invalid and overridden properties, the backend
allowed to update

I think you're missing a word in this sentence, like "is".

> Source/WebInspectorUI/ChangeLog:50
> +	   When selector is focused, clicking on the white-space should not add
a new blank property.

I'd move this comment after the final item for this file, so it's a bit easier
to read.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:105
> +    update(text, properties, styleSheetTextRange, dontFireEvents,
suppressLock)

It'd be nice of we could move some of these "optional" properties to an object
parameter,

    update(text, properties, styleSheetTextRange, options = {})

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:247
> +    // spreadsheetStyleProperty delegate

NIT: SpreadsheetStyleProperty

Also, these should probably go underneath the "Public" section.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:354
> +	   if (this._delegate && typeof
this._delegate.stylePropertyInlineSwatchActivated === "function") {

I don't think we ever have a situation where `this._delegate` is invalid.  I
think you can remove this.