[webkit-reviews] review canceled: [Bug 233208] Web Inspector: Support Cascade Layers in the Styles sidebar : [Attachment 444474] Patch v1.0

[bugzilla-daemon at webkit.org]

Patrick Angle <pangle at apple.com> has canceled 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 444474: Patch v1.0

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




--- Comment #6 from Patrick Angle <pangle at apple.com> ---
Comment on attachment 444474
  --> https://bugs.webkit.org/attachment.cgi?id=444474
Patch v1.0

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

I'll upload a screenshot as well - Keep in mind while viewing the screenshot
that I've opened bug 233238 to deal with the fact that the order of groupings
per rule is backwards of what one would expect to see (outermost
@layer/@supports/@media rule first).

>> Source/WebCore/css/CSSImportRule.h:40
>> +	WEBCORE_EXPORT const std::optional<Vector<AtomString>>&
cascadeLayerName() const;
> 
> You should use `std::optional<CascadeLayerName>&` to match other
`cascadeLayerName` methods (including the implementation of this).

Oops, yeah. I'm pretty sure I did this so I could keep working because
importing StyleRule.h here to get `CascadeLayerName` caused build issues with
WebKitLegacy. I'll revisit that.

>> Source/WebCore/css/CSSLayerRule.cpp:66
>> +	result.append("@layer ", layerName(), " {\n");
> 
> Based on the above, the space is only added if the layer has a name.	Not
sure if that matters, but it's a slight difference.

Good catch - Looks like a WPT test also caught this, so this is clearly not
correct.

>> Source/WebCore/css/CSSLayerRule.cpp:76
>> +	if (!layer.isStatement() && !layer.name().isEmpty())
> 
> Are you sure that the `isStatement` check is necessary?

The layer name is actually a variant which can be either a
`Vector<CascadeLayerName>` or just a `CascadeLayerName` - checking
`!isStatement` lets us know if it's actually a `CascadeLayerName` or not.

>> Source/WebCore/css/StyleRule.h:306
>> +	static String stringFromCascadeLayerName(CascadeLayerName);
> 
> It feels a bit odd for this to be on `StyleRuleLayer` instead of
`CSSLayerRule` since that's primarily where it's used.

I put this here to be in the same file as the definition of `CascadeLayerName`,
but I'm happy to move it.

>> Source/WebCore/inspector/InspectorStyleSheet.cpp:480
>> +		    .setText(downcast<CSSLayerRule>(parentRule)->layerName())
> 
> What do/should we do if this is an empty string?

In my view an empty string is a valid value here - it means that the layer is
it's own anonymous, unnamed, layer. It means the frontend will display a
grouping of `@layer` with no name.

>> Source/WebInspectorUI/UserInterface/Models/CSSGrouping.js:79
>> +	LayerImportRule: "layer-import-rule",
> 
> NIT: While I am in favor of making this alphabetical, it currently isn't, so
I'd either make it alphabetical or move these to the end since it's the most
recently added thing.  The latter would also match the order in the protocol
too.

Oops, though it was alphabetical, but clearly I didn't look deeper than the
first character of any of the existing values.

>> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:30
>> +		if (groupings[i].text != null)
> 
> `!==`

I actually purposefully used abstract equality here (and tried to note in the
comment above that we are checking for nullish values) in place of a more
complex check like `typeof(groupings[i].text) !== "undefined" &&
groupings[i].text !== null`. I suppose given that defining the text and making
it null in the provided expectations isn't done anywhere, I could just do the
first half of the more verbose check. This logic will need to change anyways to
accommodate expecting an undefined/null `text` value on the rule's grouping
anyways.

>> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:74
>> +			{type: WI.CSSGrouping.Type.SupportsRule},
> 
> Shouldn't this have `text: "color: red"`?

I skipped checking that since it was kinda tangential to the actual subject of
these tests, but I'm not opposed to checking it either.

>> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:154
>> +			{type: WI.CSSGrouping.Type.LayerRule, text: ""},
> 
> I think this will cause issues in `WI.CSSGrouping` as it assumes that the
`text` must not be empty.  It may also be worth adjusting the protocol so that
`text` is `"optional": true` so that we can just omit it entirely if there's no
value for it.

I think you are right that the correct thing to do is make text optional, since
an anonymous layer will not have any text. An optional is probably cleaner than
using an empty string to represent this. I'm not sure how I managed to avoid
hitting that assertion in `WI.CSSGrouping` during testing...

>> LayoutTests/inspector/css/getMatchedStylesForNodeLayerGrouping.html:176
>> +		    colorPropertyValue: "lemonchiffon",
> 
> For my own knowledge, why is this last when it was the first one declared? 
Is it because it's before the `@layer imported, base, special`?

Yeah, so layers are stacked in the order of first declaration, with each new
layer being placed on top of previously declared layers (reusing a layer name
just adds to an already declared layer), so in this case the first anonymous
layer (lemonchiffon) is declared before base or special, so it is the lowest
layer, followed by base, then special, then the very last anonymous layer
declared (darkviolet), and then the final special layer that contains all the
styles in the implicit outer layer, which get applied on top of any named
layer. (https://www.w3.org/TR/css-cascade-5/#layer-ordering)