Parse media attributes and CSSOM media text as media_query_list.

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

[kenneth.r.christiansen, 2013-06-10 10:34:15]

lgtm, but someone needs to look at the parser code

[rune, 2013-06-10 11:29:38]

Note to reviewers:

This is the first time I'm changing anything with OwnPtr/RefPtr, so you might
want to check those specifically.

[SeRya, 2013-06-10 12:11:47]

https://codereview.chromium.org/15679021/diff/2001/Source/core/css/CSSGrammar...
File Source/core/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/15679021/diff/2001/Source/core/css/CSSGrammar...
Source/core/css/CSSGrammar.y.in:366: INTERNAL_MEDIALIST_SYM maybe_space
maybe_media_list TOKEN_EOF {
Apparently syntax change used to support autoclosing all sorts of blocks. I've
seen the spec proposal regarding autoclosing in stylesheets. But I still
uncertain about other places. For instance it doesn't work with querySelectors
(document.querySelectorAll('[hidden'); throws an exception). Are you sure the
spec requires it in this context?

https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList.cpp
File Source/core/css/MediaList.cpp (right):

https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList....
Source/core/css/MediaList.cpp:81: PassRefPtr<MediaQuerySet>
MediaQuerySet::parseMediaQueryList(const String& mediaString)
It could be static. And MediaQuerySet::create could be like this:

return parseMediaQueryList(mediaString);

And MediaQuerySet::MediaQuerySet(const String& mediaString) could be removed.

https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList....
Source/core/css/MediaList.cpp:93: ASSERT(result);
I think this ASSERT says nothing once next line wold cause core dump.

https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList....
Source/core/css/MediaList.cpp:104: ASSERT(result);
ditto

https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList....
Source/core/css/MediaList.cpp:131: ASSERT(result);
ditto

[kenneth.r.christiansen, 2013-06-10 12:32:17]

>
https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList....
> Source/core/css/MediaList.cpp:81: PassRefPtr<MediaQuerySet>
> MediaQuerySet::parseMediaQueryList(const String& mediaString)
> It could be static. And MediaQuerySet::create could be like this:
> 
> return parseMediaQueryList(mediaString);
> 
> And MediaQuerySet::MediaQuerySet(const String& mediaString) could be removed.

Then maybe it should turn into a create() method ? I mean maybe it could be
merged with create then if not use in any other places.

>
https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList....
> Source/core/css/MediaList.cpp:93: ASSERT(result);
> I think this ASSERT says nothing once next line wold cause core dump.

It makes the intent pretty clear. If not, people wonder if that case should be
handled or whether the method is being called wrongly. With the assert that is
clear

[rune, 2013-06-10 19:52:59]

On 2013/06/10 12:32:17, kenneth.r.christiansen wrote:
> >
>
https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList....
> > Source/core/css/MediaList.cpp:81: PassRefPtr<MediaQuerySet>
> > MediaQuerySet::parseMediaQueryList(const String& mediaString)
> > It could be static. And MediaQuerySet::create could be like this:
> > 
> > return parseMediaQueryList(mediaString);
> > 
> > And MediaQuerySet::MediaQuerySet(const String& mediaString) could be
removed.
> 
> Then maybe it should turn into a create() method ? I mean maybe it could be
> merged with create then if not use in any other places.

Did so.

[rune, 2013-06-10 19:53:07]

https://codereview.chromium.org/15679021/diff/2001/Source/core/css/CSSGrammar...
File Source/core/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/15679021/diff/2001/Source/core/css/CSSGrammar...
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 wrote:
> Apparently syntax change used to support autoclosing all sorts of blocks. I've
> seen the spec proposal regarding autoclosing in stylesheets. But I still
> uncertain about other places. For instance it doesn't work with querySelectors
> (document.querySelectorAll('[hidden'); throws an exception). Are you sure the
> spec requires it in this context?

Not explicit as far as I can see, but it seems it will be more explicit the way
it's written in CSS3 Syntax. The official w3c test suite contains test cases
where auto-closing is expected and it uses setAttribute on the media attribute
for the style element. According to the implementation report this passes in
Gecko, Presto, and Trident.

When it comes to querySelector, both Gecko and Presto do auto-close for
querySelector. I tried to contain media query changes in this patch, so I didn't
address auto-close for other rules.

https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList.cpp
File Source/core/css/MediaList.cpp (right):

https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList....
Source/core/css/MediaList.cpp:81: PassRefPtr<MediaQuerySet>
MediaQuerySet::parseMediaQueryList(const String& mediaString)
On 2013/06/10 12:11:47, SeRya wrote:
> It could be static. And MediaQuerySet::create could be like this:
> 
> return parseMediaQueryList(mediaString);
> 
> And MediaQuerySet::MediaQuerySet(const String& mediaString) could be removed.

Fixed. Made a "create" method out of parseMediaQueryList as Kenneth proposed.

https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList....
Source/core/css/MediaList.cpp:93: ASSERT(result);
On 2013/06/10 12:11:47, SeRya wrote:
> I think this ASSERT says nothing once next line wold cause core dump.

This is way it's done across Source/core. ASSERT followed by accessing a pointer
without a null check when the invariant is that it should never be null.

I have removed them, though since I made a create method instead, as Kenneth
suggested. A create method normally has the convention that it doesn't return
null (unless there's an out-of-memory situation). Also, there is already an
ASSERT in CSSParser::parseMediaQueryList that covers the invariant.

https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList....
Source/core/css/MediaList.cpp:104: ASSERT(result);
On 2013/06/10 12:11:47, SeRya wrote:
> ditto

Done.

https://codereview.chromium.org/15679021/diff/2001/Source/core/css/MediaList....
Source/core/css/MediaList.cpp:131: ASSERT(result);
On 2013/06/10 12:11:47, SeRya wrote:
> ditto

Done.

[SeRya, 2013-06-11 08:49:18]

lgtm

[apavlov, 2013-06-11 09:05:47]

lgtm

[commit-bot: I haz the power, 2013-06-11 09:06:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/15679021/14001

[commit-bot: I haz the power, 2013-06-11 10:46:10]

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...

[commit-bot: I haz the power, 2013-06-11 10:55:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/15679021/14001

[commit-bot: I haz the power, 2013-06-11 12:27:58]

Change committed as 152218