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

Attachment 356891: Patch

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




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

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

r+, but I want to see tests as described in my comments before this lands

> LayoutTests/inspector/css/add-css-property.html:9
> +	       let suite = InspectorTest.createAsyncSuite("AddCSSProperty");

Is there a reason this needs to be an async test suite?  It seems like all test
code is synchronous, so we could use a sync suite.

> LayoutTests/inspector/css/add-css-property.html:21
> +			   reject();

NIT: although this might seem counterintuitive, I'd prefer that you call
`resolve()` here instead, as calling `reject()` will prevent any test cases
after this one from running (it effectively kills the test chain, as none of
the cases have `catch` handlers for their associated promises, so a single
rejection in any test case basically aborts the entire run).  If a problem is
"localized" to a specific test case, we shouldn't prevent the running of
subsequent tests as a result.

> LayoutTests/inspector/css/add-css-property.html:25
> +		       styleDeclaration.locked = true;

What would happen in the case that the style isn't `locked`?  Is that possible?
 We should have some tests for this.

Interestingly, both `set name` and `set rawValue` call through to
`_ownerStyle.set text`, which may cause race-y things to happen.  We should
test what happens when `locked === false`, as in that case the cached
`_ownerStyle._text` isn't immediately updated (it waits for the agent call to
return with the "real" value and uses that).

> LayoutTests/inspector/css/add-css-property.html:28
> +		       newProperty.rawValue = "green";

I noticed that there's a FIXME inside `WI.CSSProperty`:

    // FIXME: Remove current value getter and rename rawValue to value once the
old styles sidebar is removed.

Perhaps now (or a followup in the immediate future) is that time?

> LayoutTests/inspector/css/add-css-property.html:37
> +		   name: "AddCSSProperty.InsertBeforeAndAfter",

There should also be a test for "InsertBetween".

> LayoutTests/inspector/css/add-css-property.html:39
> +		       let getMatchedStyleDeclaration = () => {

Rather than repeat this helper function inside each test case, you should make
one that accepts a selector as an argument (as well as the resolve/reject
objects):

    function getMatchedStyle(selector, resolve) {
	for (let rule of nodeStyles.matchedRules) {
	    if (rule.selectorText === selector)
		return rule.style;
	}

	InspectorTest.fail("No style found for selector " + selector);
	resolve();
    }

> LayoutTests/inspector/css/add-css-property.html:63
> +		       expected = `color: green;\n    outline: 2px solid
brown;display: block;\n`;

So this patch doesn't try to add a prefix newline/spacing based on the previous
line's spacing?  I thought that that was part of this change?  If so, that
should be a followup.

> LayoutTests/inspector/css/modify-css-property.html:175
> +	       let expectedStyleText = `
> +	   /* padding-left: 2em; */
> +	   padding-right: 0px;
> +    `;

NIT: can you shift these to use "\n" like the other test file?

> LayoutTests/inspector/css/modify-css-property.html:196
> +	   name: "ModifyCSSProperty.CommentOutAndUncommentProperty2",

This could use a better name (e.g.
"ModifyCSSProperty.CommentOutAndUncommentPropertyWithoutNewlines", and the
previous test case would be
"ModifyCSSProperty.CommentOutAndUncommentPropertyWithNewlines").

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

This still worries me, as we'd potentially have race-y behavior if the
`_ownerStyle` isn't locked when the text is changed.  I agree that it's a rare
case, so I'm ok with this moving forward, but adding some tests for this
situation (and others like it) would definitely help.