[webkit-reviews] review granted: [Bug 195389] REGRESSION(r240946): Web Inspector: Styles: removing selected property doesn't update overridden status : [Attachment 364465] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 195389: REGRESSION(r240946): Web Inspector: Styles: removing selected
property doesn't update overridden status
https://bugs.webkit.org/show_bug.cgi?id=195389

Attachment 364465: Patch

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




--- Comment #7 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 364465
  --> https://bugs.webkit.org/attachment.cgi?id=364465
Patch

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

r=me. The fix looks good, although I think we should add additional tests, such
as:

1) Test that overriding a property causes it to be flagged as overridden
2) Test that overriding a property multiple times yields the correct effective
property

And probably many more, but this is a good start. It looks like the FIXME will
need to be addressed first. That would be a good time to expand this test
suite.

> LayoutTests/inspector/css/overridden-property.html:37
> +		   // OverriddenStatusChanged may fire more than once. FIXME:
https://webkit.org/b/195651

Nit: // FIXME: <https://webkit.org/b/195651> OverriddenStatusChanged may fire
more than once.

> LayoutTests/inspector/css/overridden-property.html:41
> +		   InspectorTest.log("OverriddenStatusChanged");

Style: we usually log event listener entry with
`InspectorTest.log("OverriddenStatusChanged event fired.")`.

Once the FIXME is resolved, I think we'll want to add:
InspectorTest.expectFalse(styleRuleProperty.overridden, "Style rule property
should not be overridden.");

> LayoutTests/inspector/css/overridden-property.html:50
> +	       if (contentNodeId) {

I'd use an early return here:

if (!contentNodeId) {
    InspectorTest.fail("DOM node not found.");
    InspectorTest.completeTest();
    return;
}

> LayoutTests/inspector/css/overridden-property.html:70
> +    <p>Testing that CSSProperty.prototype.overridden updates as
expected.</p>

I suggest re-wording this: "Test that CSSProperty.prototype.overridden is
cleared when the overriding (effective) property is removed."

Note: Use the imperative form of the verb instead of present continuous: 
"Testing" -> "Test". It's a bit more common (94 uses vs 75 uses) and IMO,
sounds more correct.

> LayoutTests/inspector/css/overridden-property.html:71
> +

Unnecessary blank line.