RESOLVED FIXED 231340
[CSS Cascade Layers] Update CSSOM to the spec
https://bugs.webkit.org/show_bug.cgi?id=231340
Summary [CSS Cascade Layers] Update CSSOM to the spec
Simon Fraser (smfr)
Reported 2021-10-06 16:58:01 PDT
Attachments
Patch (64.07 KB, patch)
2021-12-08 05:20 PST, Antti Koivisto
ews-feeder: commit-queue-
Patch (64.61 KB, patch)
2021-12-08 06:09 PST, Antti Koivisto
no flags
Patch (64.67 KB, patch)
2021-12-08 06:39 PST, Antti Koivisto
no flags
Radar WebKit Bug Importer
Comment 1 2021-10-06 16:58:29 PDT
Antti Koivisto
Comment 2 2021-12-08 05:20:20 PST
Antti Koivisto
Comment 3 2021-12-08 06:09:32 PST
Antti Koivisto
Comment 4 2021-12-08 06:39:42 PST
EWS
Comment 5 2021-12-08 08:56:20 PST
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 6 2021-12-08 09:00:57 PST
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 7 2021-12-08 09:12:38 PST
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
Comment 8 2021-12-08 10:49:12 PST
> 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 9 2021-12-08 10:54:52 PST
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
Comment 10 2021-12-08 11:00:21 PST
> 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
Comment 11 2021-12-08 11:13:55 PST
> > 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!
Note You need to log in before you can comment on or make changes to this bug.