Bug 183677 - Create MediaQueryParserContext to provide additional context for the evaluation of media queries
Summary: Create MediaQueryParserContext to provide additional context for the evaluati...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-15 14:28 PDT by Megan Gardner
Modified: 2018-03-15 18:45 PDT (History)
6 users (show)

See Also:


Attachments
Patch (48.68 KB, patch)
2018-03-15 14:29 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.46 MB, application/zip)
2018-03-15 15:29 PDT, EWS Watchlist
no flags Details
Patch (43.73 KB, patch)
2018-03-15 15:55 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (43.74 KB, patch)
2018-03-15 16:16 PDT, Megan Gardner
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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