[webkit-reviews] review granted: [Bug 233208] Web Inspector: Support Cascade Layers in the Styles sidebar : [Attachment 444587] Patch v1.1 - Address Devin's review comments

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 233208: Web Inspector: Support Cascade Layers in the Styles sidebar
https://bugs.webkit.org/show_bug.cgi?id=233208

Attachment 444587: Patch v1.1 - Address Devin's review comments

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




--- Comment #10 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 444587
  --> https://bugs.webkit.org/attachment.cgi?id=444587
Patch v1.1 - Address Devin's review comments

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

r=me, with a few suggestions/changes.  Nice work!  :)

> Source/JavaScriptCore/inspector/protocol/CSS.json:231
> +		   { "name": "text", "type": "string", "optional": true,
"description": "Media query text." },

NIT: I dunno if this is actually a requirement/rule, but I think non-optional
things should be before optional things, so I'd move this below `type`.

Aside: the `description` for this is also wrong ��

> Source/WebCore/css/CSSLayerRule.cpp:80
> +	   return
std::optional<String>(stringFromCascadeLayerName(layer.name()));

Is the `std::optional<String>` actually needed?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:484
> +	   for (auto&& ruleGroupingPayload : ruleGroupingPayloads) {

Does this also need a `WTFMove(ruleGroupingPayloads)`?

> Source/WebInspectorUI/UserInterface/Models/CSSGrouping.js:35
> +	   this._text = text || null;

We probably need to add checks anywhere this is used so that we don't suddenly
end up with a "null" somewhere in the UI ��

AFAICT there's only two usages:
- `WI.CSSStyleDeclaration.prototype.generateFormattedText`
- `WI.SpreadsheetCSSStyleDeclarationSection.prototype.initialLayout`

> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:32
> +		   InspectorTest.expectFalse(rule.groupings[i].text, `Grouping
${i} should not have any text.`);

NIT: I think `expectNull` would be more technically correct.