Get Media Query parser to handle parens, brackets and braces blocks correctly

Get Media Query parser to handle parens, brackets and braces blocks correctly

According to the specification, parenthesis, brackets and braces blocks inside media queries should be observed and ignored as a malformed media query.
This CL fixes the previous behavior that didn't take that into account.

BUG=358041
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170633

[Yoav Weiss, 2014-03-30 22:56:54]

According to the spec at http://www.w3.org/TR/css3-mediaqueries/#error-handling
blocks should be handled while observing "the rules of matching pairs".
The initial implementation didn't include that, but this fixes that for parens,
brackets and braces.
Since quote blocks should be handled as part of the tokenizer, I'll fix them in
a separate CL.

[rune, 2014-03-31 09:23:55]

Doesn't look like this is handling multiple levels of parentheses/braces
correctly.

Does this work as expected (should yield "not all", not "print")?:

  media="[(), print"

[Yoav Weiss, 2014-03-31 09:30:07]

On 2014/03/31 09:23:55, rune - CET wrote:
> Doesn't look like this is handling multiple levels of parentheses/braces
> correctly.
> 
> Does this work as expected (should yield "not all", not "print")?:
> 
>   media="[(), print"

I don't think it would do that, which probably means I misunderstood the spec.
Shouldn't the first bracket cause the entire expression (including the comma) to
be ignored?
I'd highly appreciate a set of test cases like that, that I can integrate into
MediaQuerySetTest and make sure this is doing what it should be.

[Yoav Weiss, 2014-03-31 09:44:55]

On 2014/03/31 09:30:07, Yoav Weiss wrote:
> On 2014/03/31 09:23:55, rune - CET wrote:
> > Doesn't look like this is handling multiple levels of parentheses/braces
> > correctly.
> > 
> > Does this work as expected (should yield "not all", not "print")?:
> > 
> >   media="[(), print"
> 
> I don't think it would do that, which probably means I misunderstood the spec.
> Shouldn't the first bracket cause the entire expression (including the comma)
to
> be ignored?
> I'd highly appreciate a set of test cases like that, that I can integrate into
> MediaQuerySetTest and make sure this is doing what it should be.

After a chat on IRC, my understanding of the spec is OK, but I misread the
example.
In fact this example should work fine. However, media="[{}, print" wouldn't.
It's a bug and I'll fix that, while adding this example and others in that
spirit to the unit test.

[Yoav Weiss, 2014-03-31 15:43:14]

On 2014/03/31 09:44:55, Yoav Weiss wrote:
> On 2014/03/31 09:30:07, Yoav Weiss wrote:
> > On 2014/03/31 09:23:55, rune - CET wrote:
> > > Doesn't look like this is handling multiple levels of parentheses/braces
> > > correctly.
> > > 
> > > Does this work as expected (should yield "not all", not "print")?:
> > > 
> > >   media="[(), print"
> > 
> > I don't think it would do that, which probably means I misunderstood the
spec.
> > Shouldn't the first bracket cause the entire expression (including the
comma)
> to
> > be ignored?
> > I'd highly appreciate a set of test cases like that, that I can integrate
into
> > MediaQuerySetTest and make sure this is doing what it should be.
> 
> After a chat on IRC, my understanding of the spec is OK, but I misread the
> example.
> In fact this example should work fine. However, media="[{}, print" wouldn't.
> It's a bug and I'll fix that, while adding this example and others in that
> spirit to the unit test.

Fixed the algorithm and added more tests according to my chat with Rune.

[eseidel, 2014-03-31 16:05:34]

https://codereview.chromium.org/217423005/diff/20001/Source/core/css/parser/M...
File Source/core/css/parser/MediaQueryParser.cpp (right):

https://codereview.chromium.org/217423005/diff/20001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryParser.cpp:165: if (!m_blockStack.isEmpty() &&
m_blockStack.last() == ParenthesisBlock)
These ifs are the same for each block type.  I would probably ahve used a helper
to make thsi more generic.

switch(tokenType)
{
case leftParen:
    return push(parenBlock);
case rightParen:
    return pop(parenBlock);
...
default:
ASSERT_NOT_REACHED();
}

Or:

if (type == LeftParen)
   push(ParenethsisBlock)
else if (type == RightParen)
     pop(ParenethsisBlock)
...

Or you could even just use something like this:
BlockType blockType; // paren, braces, brackets.
BlockDirection direction;  // push, pop?  or left/right?
blockTypeAndDirection(tokenType, blockType, blockDirection);
if (blockDirection == left)
    push(blockType)
else
   pop(blockType).


Any of those are fine, so whatever you chose.  The current code is rather
verbose.

https://codereview.chromium.org/217423005/diff/20001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryParser.cpp:186: observeBlocks(type);
Why have it be a separate function?  Why not just have it be part of your normal
token-type switch?

[Yoav Weiss, 2014-04-01 12:42:11]

On 2014/03/31 16:05:34, eseidel wrote:
>
https://codereview.chromium.org/217423005/diff/20001/Source/core/css/parser/M...
> File Source/core/css/parser/MediaQueryParser.cpp (right):
> 
>
https://codereview.chromium.org/217423005/diff/20001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryParser.cpp:165: if (!m_blockStack.isEmpty()
&&
> m_blockStack.last() == ParenthesisBlock)
> These ifs are the same for each block type.  I would probably ahve used a
helper
> to make thsi more generic.
> 
> switch(tokenType)
> {
> case leftParen:
>     return push(parenBlock);
> case rightParen:
>     return pop(parenBlock);
> ...
> default:
> ASSERT_NOT_REACHED();
> }
> 
> Or:
> 
> if (type == LeftParen)
>    push(ParenethsisBlock)
> else if (type == RightParen)
>      pop(ParenethsisBlock)
> ...
> 
> Or you could even just use something like this:
> BlockType blockType; // paren, braces, brackets.
> BlockDirection direction;  // push, pop?  or left/right?
> blockTypeAndDirection(tokenType, blockType, blockDirection);
> if (blockDirection == left)
>     push(blockType)
> else
>    pop(blockType).
> 
> 
> Any of those are fine, so whatever you chose.  The current code is rather
> verbose.

You're right. It was unnecessarily verbose. I've now changed it to something
more generic.
> 
>
https://codereview.chromium.org/217423005/diff/20001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryParser.cpp:186: observeBlocks(type);
> Why have it be a separate function?  Why not just have it be part of your
normal
> token-type switch?

My normal switch is based on the State, not on TokenType. I need to check for
blocks regardless of the state the parser is currently in, so I thought that a
separate function called before the state function is called is appropriate.

[eseidel, 2014-04-01 21:15:08]

https://codereview.chromium.org/217423005/diff/40001/Source/core/css/parser/M...
File Source/core/css/parser/MediaQueryParser.cpp (right):

https://codereview.chromium.org/217423005/diff/40001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryParser.cpp:154: if (m_blockStack.isEmpty())
I'm not sure I understand this.  I guess it depends on how it's called?  What
states can cause us to start skipping? do tdhey all want this comma behavior? 
Maybe this comma behavior should be explicit in the callsites?

https://codereview.chromium.org/217423005/diff/40001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryParser.cpp:185: { LeftBracketToken,
RightBracketToken, BracketsBlock, ModifyState },
What does "modify state" mean?  It appears to me "colapse verything between
start and end into a single token?"

https://codereview.chromium.org/217423005/diff/40001/Source/core/css/parser/M...
File Source/core/css/parser/MediaQueryParser.h (right):

https://codereview.chromium.org/217423005/diff/40001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryParser.h:73: MediaQueryTokenType lefttoken;
leftToken?

[eseidel, 2014-04-01 21:26:41]

I think this is OK.  I would like to see more testing of skipping (possibly
otherwise invalid) content inside [] and {} but not ().

[eseidel, 2014-04-01 21:28:17]

Nevermind, I better understand what { and [ do now that you've explained it to
me over irc.

[Yoav Weiss, 2014-04-01 22:11:41]

I've added more tests and fixed the typo.

[eseidel, 2014-04-01 22:11:43]

lgtm

[commit-bot: I haz the power, 2014-04-01 22:42:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/217423005/50001

[commit-bot: I haz the power, 2014-04-01 22:54:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 22:54:46]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[Yoav Weiss, 2014-04-02 03:47:49]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-04-02 03:47:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/217423005/50001

[commit-bot: I haz the power, 2014-04-02 04:15:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 04:15:17]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[Yoav Weiss, 2014-04-02 04:37:22]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-04-02 04:37:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/217423005/50001

[commit-bot: I haz the power, 2014-04-02 05:18:20]

Change committed as 170633