[webkit-reviews] review granted: [Bug 247124] [MQ4] Use the new parser and evaluator in mediaAttributeMatches() functions : [Attachment 463274] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 247124: [MQ4] Use the new parser and evaluator in mediaAttributeMatches()
functions
https://bugs.webkit.org/show_bug.cgi?id=247124

Attachment 463274: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 463274
  --> https://bugs.webkit.org/attachment.cgi?id=463274
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.