[webkit-reviews] review granted: [Bug 236580] [CSS Container Queries] Implement full query parser and evaluator : [Attachment 451894] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Feb 14 09:30:47 PST 2022
Sam Weinig <sam at webkit.org> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 236580: [CSS Container Queries] Implement full query parser and evaluator
https://bugs.webkit.org/show_bug.cgi?id=236580
Attachment 451894: Patch
https://bugs.webkit.org/attachment.cgi?id=451894&action=review
--- Comment #5 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 451894
--> https://bugs.webkit.org/attachment.cgi?id=451894
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 :).
More information about the webkit-reviews
mailing list