Bug 236580

Summary: [CSS Container Queries] Implement full query parser and evaluator
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: CSSAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Antti Koivisto 2022-02-14 02:03:04 PST
So far we have been using the media query parser to parse container queries. It does not support nesting and other necessary features. Also the produced data structure are awkward to evaluate.
Comment 1 Antti Koivisto 2022-02-14 02:09:06 PST
Created attachment 451874 [details]
Patch
Comment 2 Antti Koivisto 2022-02-14 03:38:51 PST
Created attachment 451880 [details]
Patch
Comment 3 Antti Koivisto 2022-02-14 04:08:04 PST
Created attachment 451884 [details]
Patch
Comment 4 Antti Koivisto 2022-02-14 05:21:17 PST
Created attachment 451894 [details]
Patch
Comment 5 Sam Weinig 2022-02-14 09:30:47 PST
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 :).
Comment 6 Antti Koivisto 2022-02-14 10:16:40 PST
Created attachment 451921 [details]
Patch for landing
Comment 7 EWS 2022-02-14 11:07:12 PST
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].
Comment 8 Radar WebKit Bug Importer 2022-02-14 11:08:21 PST
<rdar://problem/88916234>