In HTMLMetaElement and HTMLLinkElement.
<rdar://problem/101631539>
Created attachment 463274 [details] Patch
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.
> > 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.
> 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&
Created attachment 463291 [details] Patch for landing
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].