[webkit-reviews] review denied: [Bug 192396] Web Inspector: Styles: toggling selected properties may cause data corruption : [Attachment 356785] 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 192396: Web Inspector: Styles: toggling selected properties may cause data
corruption
https://bugs.webkit.org/show_bug.cgi?id=192396

Attachment 356785: Patch

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




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

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

r-, due to the potential double agent call

> LayoutTests/inspector/css/modify-css-property.html:131
> +	   name: "Comment out and uncomment a property",

This is more of a "description", not a name.  Our usual pattern is to use the
suite name followed by a test name, all camel-cased.

    ModifyCSSProperty.CommentOutAndUncommentProperty

> LayoutTests/inspector/css/modify-css-property.html:148
> +	       let styleDeclaration = getMatchedStyleDeclaration();

This should fail in the case that no style is found (would also require a
`return null;` inside `getMatchedStyleDeclaration`).

    if (!styleDeclaration) {
	InspectorTest.fail("No style declaration found.");
	resolve();
	return;
    }

> LayoutTests/inspector/css/modify-css-property.html:154
> +	       getProperty("padding-right").commentOut(disabled);

Ditto (>148).

    let property = getProperty("padding-right");
    if (!property) {
	InspectorTest.fail("No property found.");
	resolve();
	return;
    }

> LayoutTests/inspector/css/modify-css-property.html:159
> +	       InspectorTest.expectEqual(styleDeclaration.text, `
> +	   /* padding-left: 2em; */
> +	   padding-right: 0px;
> +    `, `Style declaration text should update immediately with uncommented
property.`);

This is unfortunate, especially considering how specific it is with spacing. 
Is there any way we could move this outside the `InspectorText.expectEqual`,
like maybe as a `const`, so it can be styled better?

> LayoutTests/inspector/css/modify-css-property.html:164
> +	       getProperty("padding-right").commentOut(disabled);

Ditto (>154).  Also, is this expected to return a new property, or the same
property?  We should assert that the result is the same/different.

> LayoutTests/inspector/css/modify-css-property.html:169
> +	       InspectorTest.expectEqual(styleDeclaration.text, `
> +	   /* padding-left: 2em; */
> +	   /* padding-right: 0px; */
> +    `, `Style declaration text should update immediately with commented out
property.`);

Ditto (>159).

> LayoutTests/inspector/css/modify-css-property.html:171
> +	       InspectorTest.expectThat(!getProperty("padding-right").enabled,
`Commented out property is disabled.`);

Ditto (>164).

> LayoutTests/inspector/css/modify-css-property.html:212
> +	   /* padding-left: 2em; */
> +	   /* padding-right: 0px; */

We should add tests for different padding situations (e.g. if there is more
than one space before/after the property).

> Source/WebInspectorUI/ChangeLog:24
> +	       /* color: red; */ font-size: 12px

I'm not sure if this is just how you formatted this, but shouldn't we be
keeping the newline as well?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:401
> +		   this.text = property.text.trimRight() + ";" + match[1];

This means that we'll be doing two updates of the owner style.	We should try
to do as few back/forth calls to the backend as possible.

Also, I think this will only work if the owner style is `locked`, as otherwise
the owner style text won't get updated until `CSS.setStyleText` returns.  We
should test this.