[webkit-reviews] review granted: [Bug 200419] Web Inspector: Styles: show @supports CSS groupings : [Attachment 375494] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 200419: Web Inspector: Styles: show @supports CSS groupings
https://bugs.webkit.org/show_bug.cgi?id=200419

Attachment 375494: Patch

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




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

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

r=me! Nice!

> Source/JavaScriptCore/inspector/protocol/CSS.json:224
> -	       "id": "CSSMedia",
> +	       "id": "Grouping",

I'm not sure `Grouping` is clear but I'm warming up to it.

I considered `AtRuleQuery`. Since these primarily match a media query or
feature query features, with <link media="..."> / <style media="..."> special
cases.
https://developer.mozilla.org/en-US/docs/Web/CSS/At-rule

I suspect other at-rule features like `@page {...}`, `@font-face {...}` and
`@keyframes {...}` will require their own types.

> Source/JavaScriptCore/inspector/protocol/CSS.json:226
> +	       "description": "CSS @media/@supports descriptor.",

This says "@media/@supports" but has a longer list of supported things (@media,
@supports, @import, and synthesized from <link media="..."> / <style
media="...">).

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:458
> +	   let groupings = this.groupings.filter((grouping) => grouping.text
!== "all");

Does the frontend filter out media queries of `all`? Maybe then the backend
shouldn't filter them itself?

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:109
> +	       this._groupingElements = groupings.map((grouping, i) => {

Nit: This doesn't make use of `i`

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:118
> +		   if (groupingTypeElement.children.length)
> +		       groupingTypeElement.append(", ");

This just blew my mind. I did not know that `d = document.createElement("div");
d.textContent = "foo"; d.children.length` is zero. I guess `children` is not
`childNodes`.

Maybe using `groupingTypeElement.childElementCount` might be better? I'd
probably have carried a boolean around to say whether or not we're appending
more to the currentGroupingType.