[webkit-reviews] review granted: [Bug 198629] Web Inspector: longhand CSS properties overridden by shorthands miss strikethrough : [Attachment 371613] 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 198629: Web Inspector: longhand CSS properties overridden by shorthands
miss strikethrough
https://bugs.webkit.org/show_bug.cgi?id=198629

Attachment 371613: Patch

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




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

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

r=me, with some final fixes before landing.

> LayoutTests/inspector/css/overridden-property-expected.txt:15
> +PASS: border-top-color is overridden.

Nice! This is MUCH easier to read/understand.

> LayoutTests/inspector/css/overridden-property.html:38
> +    function getStyleDeclaration(selector, callback, resolve) {

Now that you're passing a `callback`, I'd replace the `resolve` with a
`reject`, as we really should be throwing an error if we couldn't find the
selector.

> LayoutTests/inspector/css/overridden-property.html:92
> +	   name: "OverriddenByShorthand",

Style: please prefix the test case name with the suite name, so
"OverriddenProperty.OverriddenByShorthand".

> LayoutTests/inspector/css/overridden-property.html:95
> +		  
InspectorTest.expectTrue(style.visibleProperties[0].overridden,
"border-top-color is overridden.");

We should be using `propertyForName` instead of expecting that it's the first
visible property.
```
    const dontCreateIfMissing = true;
    let borderTopColorProperty = style.propertyForName("border-top-color",
dontCreateIfMissing);
```
Alternatively, adding an `InspectorTest.assert(style.visibleProperties[0].name
=== "border-top-color");` would be fine too.

> LayoutTests/inspector/css/overridden-property.html:120
> +	       });

Shouldn't you also pass `reject` in here?

> LayoutTests/inspector/css/overridden-property.html:131
> +	       });

Ditto (>120).

> LayoutTests/inspector/css/overridden-property.html:146
> +    .shorthand {

A better name could be used here, like `.longhand-overridden-by-shorthand`.

> LayoutTests/inspector/css/overridden-property.html:161
> +    .shorthand-not-overridden-longhand {

NIT: this is missing a `-by-`, so `.shorthand-not-overridden-by-longhand`.

> LayoutTests/inspector/css/overridden-property.html:170
> +    <div class="shorthand-overridden-by-important-longhand"></div>
> +    <div class="shorthand-overridden-by-important-longhand"></div>

Duplicate.  Please remove (or explain why it's still needed).