Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(47)

Issue 15679021: Parse media attributes and CSSOM media text as media_query_list. (Closed)

Created:
4 years, 6 months ago by rune
Modified:
4 years, 6 months ago
CC:
blink-reviews, apavlov+blink_chromium.org, dglazkov+blink, eae+blinkwatch, kenneth.christiansen, darktears
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Parse media attributes and CSSOM media text as media_query_list. This is per HTML5 and CSSOM specifications. This change makes the remaining fails in the W3C Media Queries test suite pass. Some resulting changes: - Brackets/parentheses balanced correctly. - Queries split between correct commas. - Spaces is an empty media query list like the empty string. - Unexpected EOF like missing parentheses makes the query valid as per CSS21 spec. - Syntax errors become "not all" as per CSSMQ spec. - Renamed @-webkit-mediaquery to @-internal-mediaquery to be consistent with other internal @-rules. - Removed MediaQueryParserMode which is now the same for all MQ parsing. BUG=247657 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152218

Patch Set 1 #

Patch Set 2 : Rebased onto master #

Total comments: 10

Patch Set 3 : Fixed review issues. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+55 lines, -113 lines) Patch
M LayoutTests/fast/media/matchmedium-query-api.html View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/media/matchmedium-query-api-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/media/media-descriptor-syntax-06.html View 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/fast/media/media-descriptor-syntax-06-expected.html View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/media/media-query-list-01.html View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/media/media-query-list-01-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/media/mq-append-delete.html View 4 chunks +4 lines, -4 lines 0 comments Download
M LayoutTests/fast/media/mq-js-media-except-01.html View 1 chunk +8 lines, -4 lines 0 comments Download
M LayoutTests/fast/media/mq-parsing-expected.txt View 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/css/CSSGrammar.y.in View 1 3 chunks +6 lines, -6 lines 0 comments Download
M Source/core/css/CSSParser.h View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/css/CSSParser.cpp View 1 2 chunks +10 lines, -12 lines 0 comments Download
M Source/core/css/MediaList.h View 1 2 2 chunks +2 lines, -17 lines 0 comments Download
M Source/core/css/MediaList.cpp View 1 2 4 chunks +14 lines, -59 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
rune
4 years, 6 months ago (2013-06-10 09:58:53 UTC) #1
kenneth.r.christiansen
lgtm, but someone needs to look at the parser code
4 years, 6 months ago (2013-06-10 10:34:15 UTC) #2
rune
Note to reviewers: This is the first time I'm changing anything with OwnPtr/RefPtr, so you ...
4 years, 6 months ago (2013-06-10 11:29:38 UTC) #3
SeRya
https://codereview.chromium.org/15679021/diff/2001/Source/core/css/CSSGrammar.y.in File Source/core/css/CSSGrammar.y.in (right): https://codereview.chromium.org/15679021/diff/2001/Source/core/css/CSSGrammar.y.in#newcode366 Source/core/css/CSSGrammar.y.in:366: INTERNAL_MEDIALIST_SYM maybe_space maybe_media_list TOKEN_EOF { Apparently syntax change used ...
4 years, 6 months ago (2013-06-10 12:11:47 UTC) #4
kenneth.r.christiansen
> https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList.cpp#newcode81 > Source/core/css/MediaList.cpp:81: PassRefPtr<MediaQuerySet> > MediaQuerySet::parseMediaQueryList(const String& mediaString) > It could be static. And ...
4 years, 6 months ago (2013-06-10 12:32:17 UTC) #5
rune
On 2013/06/10 12:32:17, kenneth.r.christiansen wrote: > > > https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList.cpp#newcode81 > > Source/core/css/MediaList.cpp:81: PassRefPtr<MediaQuerySet> > > ...
4 years, 6 months ago (2013-06-10 19:52:59 UTC) #6
rune
https://codereview.chromium.org/15679021/diff/2001/Source/core/css/CSSGrammar.y.in File Source/core/css/CSSGrammar.y.in (right): https://codereview.chromium.org/15679021/diff/2001/Source/core/css/CSSGrammar.y.in#newcode366 Source/core/css/CSSGrammar.y.in:366: INTERNAL_MEDIALIST_SYM maybe_space maybe_media_list TOKEN_EOF { On 2013/06/10 12:11:47, SeRya ...
4 years, 6 months ago (2013-06-10 19:53:07 UTC) #7
SeRya
lgtm
4 years, 6 months ago (2013-06-11 08:49:18 UTC) #8
apavlov
lgtm
4 years, 6 months ago (2013-06-11 09:05:47 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rune@opera.com/15679021/14001
4 years, 6 months ago (2013-06-11 09:06:05 UTC) #10
commit-bot: I haz the power
Retried try job too often on win_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout_rel&number=9509
4 years, 6 months ago (2013-06-11 10:46:10 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rune@opera.com/15679021/14001
4 years, 6 months ago (2013-06-11 10:55:49 UTC) #12
commit-bot: I haz the power
4 years, 6 months ago (2013-06-11 12:27:58 UTC) #13
Message was sent while issue was closed.
Change committed as 152218

Powered by Google App Engine
This is Rietveld 0eb02b776