Bug 247124 – [MQ4] Use the new parser and evaluator in mediaAttributeMatches() functions

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antti Koivisto]

In HTMLMetaElement and HTMLLinkElement.

[Radar WebKit Bug Importer]

<rdar://problem/101631539>

[Antti Koivisto]

Created attachment 463274 [details]
Patch

[Darin Adler]

Comment on attachment 463274 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=463274&action=review

> Source/WebCore/css/query/GenericMediaQueryParser.h:31
> +#include "MediaQueryParserContext.h"

Doesn’t seem like we need to include this header to compile references to the type. Would a forward declaration suffice?

> Source/WebCore/css/query/MediaQueryParser.cpp:53
> +    auto range = tokenizer->tokenRange();

I don’t think we need a local variable for this.

> Source/WebCore/css/query/MediaQueryParser.h:35
> +    MediaQueryParser(const MediaQueryParserContext&);

Maybe this constructor should be explicit.

> Source/WebCore/css/query/MediaQueryParser.h:37
> +    static MediaQueryList parse(const String&, const MediaQueryParserContext&);

Some day we should make these be StringView. Right now, though, I am guessing this uses a parser that requires an actual String.

> Source/WebCore/html/HTMLLinkElement.cpp:524
> +    MQ::MediaQueryEvaluator evaluator(AtomString(document().frame()->view()->mediaType()), document(), documentStyle ? &*documentStyle : nullptr);

Surprised this isn’t already an AtomString. Maybe we should change that to save a little memory and make this a little faster?

> Source/WebCore/html/HTMLMetaElement.cpp:80
> +            mediaType = AtomString(frameView->mediaType());

More of the same.

> Source/WebCore/html/HTMLMetaElement.h:26
> +#include "MediaQuery.h"

A little sad to have to include this header. We could have used std::unique_ptr to pay a little more cost at runtime but less at compile time. I’ll assume this is the right tradeoff, though.

[Antti Koivisto]

> > Source/WebCore/css/query/MediaQueryParser.cpp:53
> > +    auto range = tokenizer->tokenRange();
> 
> I don’t think we need a local variable for this.

tokenRange() returns a temporary and the reference can't bind to it.

Not that the token range type is small, the actual tokens are owned by the tokenizer.
 
> Surprised this isn’t already an AtomString. Maybe we should change that to
> save a little memory and make this a little faster?

Me too. Better to be fixed separately.
 
> > Source/WebCore/html/HTMLMetaElement.h:26
> > +#include "MediaQuery.h"
> 
> A little sad to have to include this header. We could have used
> std::unique_ptr to pay a little more cost at runtime but less at compile
> time. I’ll assume this is the right tradeoff, though.

There is unlikely be any practical cost in using unique_ptr but std::optional feels better semantically. I agree that that the include is annoying.

[Antti Koivisto]

> Some day we should make these be StringView. Right now, though, I am
> guessing this uses a parser that requires an actual String.

Yeah, CSSTokenizer takes a String&

[Antti Koivisto]

Created attachment 463291 [details]
Patch for landing

[EWS]

Committed 256093@main (7bceda660698): <https://commits.webkit.org/256093@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 463291 [details].