Bug 183677

Summary: Create MediaQueryParserContext to provide additional context for the evaluation of media queries
Product: WebKit Reporter: Megan Gardner <megan_gardner>
Component: New BugsAssignee: Megan Gardner <megan_gardner>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, ews-watchlist, koivisto, rniwa, thorton, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews101 for mac-sierra
none
Patch
none
Patch thorton: review+

Description Megan Gardner 2018-03-15 14:28:54 PDT
Create MediaQueryParserContext to provide additional context for the evaluation of media queries
Comment 1 Megan Gardner 2018-03-15 14:29:57 PDT
Created attachment 335885 [details]
Patch
Comment 2 Megan Gardner 2018-03-15 14:31:12 PDT
<rdar://problem/38382934>
Comment 3 EWS Watchlist 2018-03-15 14:31:54 PDT
Attachment 335885 [details] did not pass style-queue:


ERROR: Source/WebCore/css/MediaList.h:47:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 EWS Watchlist 2018-03-15 15:29:43 PDT
Comment on attachment 335885 [details]
Patch

Attachment 335885 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6970887

New failing tests:
css2.1/20110323/at-import-004.htm
fast/media/mq-invalid-syntax-03.html
fast/dom/css-dom-read-2.html
imported/w3c/web-platform-tests/cssom/cssimportrule.html
cssom/cssimportrule-media.html
fast/media/mq-invalid-media-feature-03.html
Comment 5 EWS Watchlist 2018-03-15 15:29:45 PDT
Created attachment 335894 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 6 Tim Horton 2018-03-15 15:36:11 PDT
Comment on attachment 335885 [details]
Patch

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

> Source/WebCore/css/MediaList.cpp:85
> +    auto result = create(mediaString); // TODO: pass in a context if we end up needing that bit

I think we can just leave these out, and if we need to find these later we can make the argument non-optional and use the compiler.

> Source/WebCore/css/MediaQueryExpression.cpp:123
> -static inline bool featureWithZeroOrOne(const String& mediaFeature, const CSSPrimitiveValue& value)
> +static inline bool featureWithZeroOrOne(const String& mediaFeature, const CSSPrimitiveValue& value, const MediaQueryParserContext&)

Plz undo these as mentioned

> Source/WebCore/css/StyleRuleImport.cpp:79
> +    m_mediaQueries = MediaQuerySet::create(String(), MediaQueryParserContext(*document)); // TODO: not sure what string this should be created with

What’s this?!

> Source/WebCore/css/parser/MediaQueryParser.h:43
> +struct CSSParserContext;
>  class MediaQuerySet;

Sort.
Comment 7 Megan Gardner 2018-03-15 15:55:36 PDT
Created attachment 335899 [details]
Patch
Comment 8 Megan Gardner 2018-03-15 16:16:34 PDT
Created attachment 335904 [details]
Patch
Comment 9 Tim Horton 2018-03-15 17:52:08 PDT
Comment on attachment 335904 [details]
Patch

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

> Source/WebCore/css/MediaQueryParserContext.h:29
> +
> +
> +

weird extra spaces

> Source/WebCore/css/MediaQueryParserContext.h:37
> +    MediaQueryParserContext() { };

no semicolon

> Source/WebCore/css/parser/MediaQueryParser.cpp:77
> +    m_mediaQueryData.setMediaQueryParserContext(context);

Could avoid this set method if we added this to the MediaQueryData constructor and passed it in in the initializer list above. Or do other people construct MediaQueryData?
Comment 10 Megan Gardner 2018-03-15 18:02:35 PDT
Comment on attachment 335904 [details]
Patch

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

>> Source/WebCore/css/parser/MediaQueryParser.cpp:77
>> +    m_mediaQueryData.setMediaQueryParserContext(context);
> 
> Could avoid this set method if we added this to the MediaQueryData constructor and passed it in in the initializer list above. Or do other people construct MediaQueryData?

No, but the initializer set everything to default, and then sets things after, so I was just following what was being done for the other variables. But I guess we have this at creation time, so we can stuff it in there immediately.
Comment 11 Megan Gardner 2018-03-15 18:45:54 PDT
https://trac.webkit.org/changeset/229654/webkit