Bug 231340 – [CSS Cascade Layers] Update CSSOM to the spec

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Simon Fraser (smfr)]

Per CSS WG resolution:
https://github.com/w3c/csswg-drafts/issues/6576#issuecomment-937246847

[Radar WebKit Bug Importer]

<rdar://problem/83958697>

[Antti Koivisto]

Created attachment 446350 [details]
Patch

[Antti Koivisto]

Created attachment 446353 [details]
Patch

[Antti Koivisto]

Created attachment 446357 [details]
Patch

[EWS]

Committed r286657 (244970@main): <https://commits.webkit.org/244970@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446357 [details].

[Darin Adler]

Comment on attachment 446357 [details]
Patch

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

> Source/WebCore/inspector/InspectorStyleSheet.cpp:455
> +            auto layerName = downcast<CSSImportRule>(parentRule)->layerName();
> +            if (!layerName.isNull()) {

Hey Antti, when making a change like this, please consider the modern C++ way to keep this scoped like it was before:

    if (auto layerName = downcast<CSSImportRule>(parentRule)->layerName(); !layerName.isNull()) {

I think it works and makes the code better even though it can be a little hard to spot the code after the semicolon.

[Patrick Angle]

Comment on attachment 446357 [details]
Patch

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

> Source/WebCore/inspector/InspectorStyleSheet.cpp:481
> +            layerRulePayload->setText(downcast<CSSLayerBlockRule>(parentRule)->name());

The Inspector frontend expects an anonymous layer will not provide the optional `text` parameter. Since this has already landed I've opened Bug 234012 to track cleaning this up.

[Antti Koivisto]

> Hey Antti, when making a change like this, please consider the modern C++
> way to keep this scoped like it was before:
> 
>     if (auto layerName = downcast<CSSImportRule>(parentRule)->layerName();
> !layerName.isNull()) {
> 
> I think it works and makes the code better even though it can be a little
> hard to spot the code after the semicolon.

I did consider it and decided it reads better as two lines. There is no benefit from limiting the scope here.

[Antti Koivisto]

Comment on attachment 446357 [details]
Patch

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

>> Source/WebCore/inspector/InspectorStyleSheet.cpp:481
>> +            layerRulePayload->setText(downcast<CSSLayerBlockRule>(parentRule)->name());
> 
> The Inspector frontend expects an anonymous layer will not provide the optional `text` parameter. Since this has already landed I've opened Bug 234012 to track cleaning this up.

layerName() returns an empty string for anonymous layers. Maybe best to handle that in the frontend.

[Antti Koivisto]

> layerName() returns an empty string for anonymous layers. Maybe best to
> handle that in the frontend.

Or to be exact CSSLayerBlockRule::name() returns empty string for anonymous layer (and never a null string).

CSSImportRule::layerName() returns null string for no layer specified case and empty string for anonymous layer case.

CSSLayerStatementRule::nameList() returns a vector with at least one item and no empty or null strings.

[Patrick Angle]

> > layerName() returns an empty string for anonymous layers. Maybe best to
> > handle that in the frontend.
> 
> Or to be exact CSSLayerBlockRule::name() returns empty string for anonymous
> layer (and never a null string).
> 
> CSSImportRule::layerName() returns null string for no layer specified case
> and empty string for anonymous layer case.
> 
> CSSLayerStatementRule::nameList() returns a vector with at least one item
> and no empty or null strings.

Makes sense - given this I think I will go ahead and just teach the frontend how to handle an empty string here. Thanks for the clarification!