Summary: | [MQ4] Use the new parser and evaluator in mediaAttributeMatches() functions | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||
Component: | CSS | Assignee: | Antti Koivisto <koivisto> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cdumez, changseok, darin, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, macpherson, menard, philipj, sergio, webkit-bug-importer, youssefdevelops, y_soliman | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 247118 | ||||||||
Attachments: |
|
Description
Antti Koivisto
2022-10-27 06:12:46 PDT
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]. |