Summary: | [CSS Cascade Layers] Update CSSOM to the spec | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||
Component: | CSS | Assignee: | Antti Koivisto <koivisto> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | annulen, cdumez, darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hi, joepeck, kondapallykalyan, macpherson, menard, pangle, ryuan.choi, sergio, simon.fraser, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Safari Technology Preview | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 235455 | ||||||||||
Bug Blocks: | 220779, 234012 | ||||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2021-10-06 16:58:01 PDT
Created attachment 446350 [details]
Patch
Created attachment 446353 [details]
Patch
Created attachment 446357 [details]
Patch
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]. 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. 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. > 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.
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. > 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.
> > 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!
|