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

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted 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 331260: Patch for review

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




--- Comment #18 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 331260
  --> https://bugs.webkit.org/attachment.cgi?id=331260
Patch for review

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

This seems reasonable, lets live on it.

r=me

> LayoutTests/inspector/css/modify-css-property.html:19
> +	   name: "Update value when CSSStyleDeclaration is unlocked",

I'd suggest "not-locked". "unlocked" sounds like the verb, so it comes across
as "when it is unlocked", which is not actually what is happening here.

> LayoutTests/inspector/css/modify-css-property.html:126
> +	       InspectorTest.evaluateInPage("makeWide()");

Style: We've been using template strings for code, especially in cases like
this: evaluateInPage(`code()`)

> LayoutTests/inspector/css/modify-css-property.html:128
> +	       resolve();

This test can resolve in multiple ways. Here (immediately) or above with
awaitEvent. I think you should keep only the awaitEvent version, since that
happens after this one and includes expectations.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:52
> +	   this.element.addEventListener("focus", () => this.focused = true,
true);

Style: We put braces around arrow function bodies if the return value is not
significant:

    () => { this.focused = true; }

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:53
> +	   this.element.addEventListener("blur", (event) => {

I worry that somehow this might miss something, but I didn't have any issues in
practice.