| Summary: | [CSS Container Queries] Implement full query parser and evaluator | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||||||
| Component: | CSS | Assignee: | Antti Koivisto <koivisto> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, macpherson, menard, sam, webkit-bug-importer | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=236687 | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 229659 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Antti Koivisto
2022-02-14 02:03:04 PST
Created attachment 451874 [details]
Patch
Created attachment 451880 [details]
Patch
Created attachment 451884 [details]
Patch
Created attachment 451894 [details]
Patch
Comment on attachment 451894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451894&action=review > Source/WebCore/css/ContainerQuery.h:45 > +enum class ComparisonOperator : uint8_t { Less, LessOrEqual, Equal, Greater, GreaterOrEqual, True }; I would replace `Less` and `Greater` with `LessThan` and `GreaterThan` (also in the OrEqual variants) as that is usually what we call these. > Source/WebCore/css/ContainerQueryParser.cpp:42 > + bool isSizeQuery = equalIgnoringASCIICase(range.peek().value(), "size"); Could you instead do: bool isSizeQuery = range.peek().functionId() == CSSValueSize; ? > Source/WebCore/css/ContainerQueryParser.cpp:89 > + if (equalIgnoringASCIICase(range.peek().value(), "not")) { Again, can this be compared against the CSSValueNot ident rather than the explicit string check? > Source/WebCore/css/ContainerQueryParser.cpp:143 > + auto name = nameToken.value().toAtomString(); I think delaying the toAtomString() until it is needed might be a little more efficient and avoid an unnecessary atomization in some cases. > Source/WebCore/css/ContainerQueryParser.cpp:146 > + return CQ::SizeFeature { CQ::ComparisonOperator::True, name, { } }; I would WTFMove(name) here. > Source/WebCore/css/ContainerQueryParser.cpp:161 > + name = StringView(name).substring(4).toAtomString(); I can't exactly remember how this works with AtomStrings, but I think you want to instead do something like: name = name.string().substring(4).toAtomString(); to potentially get the shared substring optimization. > Source/WebCore/css/ContainerQueryParser.cpp:170 > + return CQ::SizeFeature { op, name, length }; I would WTFMove(name) here. > Source/WebCore/style/ContainerQueryEvaluator.cpp:186 > + // FIXME: Support all features. An evergreen comment :). Created attachment 451921 [details]
Patch for landing
Committed r289742 (247226@main): <https://commits.webkit.org/247226@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451921 [details]. |