Bug 164821 - [CSS Parser] Add @supports, @keyframe and media query parsing options
Summary: [CSS Parser] Add @supports, @keyframe and media query parsing options
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: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-16 08:47 PST by Dave Hyatt
Modified: 2016-11-17 09:29 PST (History)
2 users (show)

See Also:


Attachments
Patch (55.83 KB, patch)
2016-11-16 08:59 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (1.47 MB, application/zip)
2016-11-16 09:59 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.28 MB, application/zip)
2016-11-16 10:05 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (1.84 MB, application/zip)
2016-11-16 10:20 PST, Build Bot
no flags Details
Patch (67.94 KB, patch)
2016-11-16 13:00 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (69.26 KB, patch)
2016-11-16 13:38 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (69.27 KB, patch)
2016-11-16 14:11 PST, Dave Hyatt
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2016-11-16 08:47:56 PST
[CSS Parser] Add @supports, @keyframe and media query parsing otions
Comment 1 Dave Hyatt 2016-11-16 08:59:26 PST
Created attachment 294943 [details]
Patch
Comment 2 zalan 2016-11-16 09:23:34 PST
Comment on attachment 294943 [details]
Patch

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

> Source/WebCore/css/CSSKeyframeRule.cpp:77
> +    std::unique_ptr<Vector<double>> keys = CSSParser::parseKeyframeKeyList(keyText);

auto?

> Source/WebCore/css/MediaList.cpp:138
> +            --i;

What happens when i == 0?
Comment 3 Build Bot 2016-11-16 09:59:15 PST
Comment on attachment 294943 [details]
Patch

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

New failing tests:
fast/media/mq-js-media-except-03.html
fast/media/mq-invalid-syntax-02.html
fast/media/mq-js-stylesheet-media-01.html
fast/media/mq-invalid-syntax-05.html
fast/media/mq-invalid-syntax-01.html
fast/media/mq-js-stylesheet-media-03.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/update-the-source-set.html
fast/media/mq-js-media-except-02.html
fast/media/mq-js-stylesheet-media-02.html
fast/media/mq-js-media-forward-syntax.html
Comment 4 Build Bot 2016-11-16 09:59:18 PST
Created attachment 294947 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 5 Build Bot 2016-11-16 10:05:44 PST
Comment on attachment 294943 [details]
Patch

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

New failing tests:
fast/media/mq-js-media-except-03.html
fast/media/mq-invalid-syntax-02.html
fast/media/mq-js-stylesheet-media-01.html
fast/media/mq-invalid-syntax-05.html
fast/media/mq-js-stylesheet-media-02.html
fast/media/mq-invalid-syntax-01.html
fast/media/mq-js-stylesheet-media-03.html
fast/media/mq-js-media-except-02.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/update-the-source-set.html
fast/media/mq-js-media-forward-syntax.html
Comment 6 Build Bot 2016-11-16 10:05:47 PST
Created attachment 294949 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 7 Build Bot 2016-11-16 10:20:32 PST
Comment on attachment 294943 [details]
Patch

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

New failing tests:
fast/media/mq-js-media-except-03.html
fast/media/mq-invalid-syntax-02.html
fast/media/mq-js-stylesheet-media-01.html
fast/media/mq-invalid-syntax-05.html
fast/media/mq-invalid-syntax-01.html
fast/media/mq-js-stylesheet-media-03.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/update-the-source-set.html
fast/media/mq-js-media-except-02.html
fast/media/mq-js-stylesheet-media-02.html
fast/media/mq-js-media-forward-syntax.html
Comment 8 Build Bot 2016-11-16 10:20:35 PST
Created attachment 294950 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 Dave Hyatt 2016-11-16 13:00:43 PST
Created attachment 294966 [details]
Patch
Comment 10 Dave Hyatt 2016-11-16 13:38:34 PST
Created attachment 294971 [details]
Patch
Comment 11 Dave Hyatt 2016-11-16 14:11:28 PST
Created attachment 294975 [details]
Patch
Comment 12 Sam Weinig 2016-11-16 15:57:26 PST
Comment on attachment 294975 [details]
Patch

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

> Source/WebCore/css/CSSKeyframesRule.cpp:77
> +    std::unique_ptr<Vector<double>> keys = CSSParser::parseKeyframeKeyList(key);

Could use auto here to avoid this mouthful of a type name.

> Source/WebCore/css/MediaList.cpp:99
>  bool MediaQuerySet::add(const String& queryString)

If this and set always return true, can we get rid of the return value? Or should we still be returning false in some cases, like if the media query is invalid?

> Source/WebCore/css/MediaList.cpp:193
> +    if (!m_mediaQueries->set(value))
>          return Exception { SYNTAX_ERR };

Set always returns true, no need for this condition.

> Source/WebCore/css/StyleMedia.cpp:70
> +    if (!media->set(query))
>          return false;

Set always returns true, no need for this condition.
Comment 13 Dave Hyatt 2016-11-17 09:29:42 PST
Landed in r208847.