Moved MQ parsing block tracking to tokenizer

Moved MQ parsing block tracking to tokenizer

For better spec compliance of the tokenizer itself, and to enable the sizes
parser to be aware of blocks, I've moved the block tracking logic from the 
MQ parser to the tokenizer.

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

[eseidel, 2014-04-08 16:58:48]

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

https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryParser.cpp:175: ++m_blockLevel;
Should the tokenizer just have a blockLevel() accessor?  In general it's fine
for Parsers to read from the Tokenizer just not the other way around. :)

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

https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryParser.h:84: void
observeBlocks(TokenIterator&);
nit: I'm really not a fan of "observe" for this name.  "handle" would be more
inline with how the HTML parser names similar functions.. .I Think.

https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
File Source/core/css/parser/MediaQueryToken.h (right):

https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryToken.h:43: enum BlockToken {
Maybe BlockType?  BlockToken makes me think this is a subclass of Token for
blocks or something?

https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryToken.h:52:
MediaQueryToken(MediaQueryTokenType, String, BlockToken);
What does this String do?

[Yoav Weiss, 2014-04-08 17:40:41]

On 2014/04/08 16:58:48, eseidel wrote:
>
https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
> File Source/core/css/parser/MediaQueryParser.cpp (right):
> 
>
https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryParser.cpp:175: ++m_blockLevel;
> Should the tokenizer just have a blockLevel() accessor?  In general it's fine
> for Parsers to read from the Tokenizer just not the other way around. :)

I started by doing that, but it required adding an int to each one of the
Tokens. Probably premature optimization.
I'll add the counter to the tokenizer then.

> 
>
https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
> File Source/core/css/parser/MediaQueryParser.h (right):
> 
>
https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryParser.h:84: void
> observeBlocks(TokenIterator&);
> nit: I'm really not a fan of "observe" for this name.  "handle" would be more
> inline with how the HTML parser names similar functions.. .I Think.
I'll change that.

> 
>
https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
> File Source/core/css/parser/MediaQueryToken.h (right):
> 
>
https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryToken.h:43: enum BlockToken {
> Maybe BlockType?  BlockToken makes me think this is a subclass of Token for
> blocks or something?

BlockType it is.
> 
>
https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryToken.h:52:
> MediaQueryToken(MediaQueryTokenType, String, BlockToken);
> What does this String do?

That's the token's value. I'll add the name here.

[Yoav Weiss, 2014-04-08 22:43:19]

On 2014/04/08 17:40:41, Yoav Weiss wrote:
> On 2014/04/08 16:58:48, eseidel wrote:
> >
>
https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
> > File Source/core/css/parser/MediaQueryParser.cpp (right):
> > 
> >
>
https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
> > Source/core/css/parser/MediaQueryParser.cpp:175: ++m_blockLevel;
> > Should the tokenizer just have a blockLevel() accessor?  In general it's
fine
> > for Parsers to read from the Tokenizer just not the other way around. :)
> 
> I started by doing that, but it required adding an int to each one of the
> Tokens. Probably premature optimization.
> I'll add the counter to the tokenizer then.
> 
> > 
> >
>
https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
> > File Source/core/css/parser/MediaQueryParser.h (right):
> > 
> >
>
https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
> > Source/core/css/parser/MediaQueryParser.h:84: void
> > observeBlocks(TokenIterator&);
> > nit: I'm really not a fan of "observe" for this name.  "handle" would be
more
> > inline with how the HTML parser names similar functions.. .I Think.
> I'll change that.
> 
> > 
> >
>
https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
> > File Source/core/css/parser/MediaQueryToken.h (right):
> > 
> >
>
https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
> > Source/core/css/parser/MediaQueryToken.h:43: enum BlockToken {
> > Maybe BlockType?  BlockToken makes me think this is a subclass of Token for
> > blocks or something?
> 
> BlockType it is.
> > 
> >
>
https://codereview.chromium.org/225293006/diff/20001/Source/core/css/parser/M...
> > Source/core/css/parser/MediaQueryToken.h:52:
> > MediaQueryToken(MediaQueryTokenType, String, BlockToken);
> > What does this String do?
> 
> That's the token's value. I'll add the name here.

Fixed all of the above.

[eseidel, 2014-04-08 22:58:51]

I'm not sure I fully understand yet. May try to chat more with you this evneing.

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

https://codereview.chromium.org/225293006/diff/40001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryParser.cpp:163: if (token->blockType() ==
MediaQueryToken::BlockEnd && token->blockLevel() <= 1)
Why <= 1?

https://codereview.chromium.org/225293006/diff/40001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryParser.cpp:172: && (token->type() !=
LeftParenthesisToken || token->blockLevel() > 1))
Why > 1?

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

https://codereview.chromium.org/225293006/diff/40001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryToken.cpp:58:
MediaQueryToken::MediaQueryToken(MediaQueryTokenType type, String value,
BlockType blockType)
It's really error-prone to have this many conctructors.  Very easy to fail to
initialze one member.

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

https://codereview.chromium.org/225293006/diff/40001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryTokenizer.cpp:220:
token.setBlockLevel(tokenizer.m_blockStack.size());
I see.  Because the parser doesn't know how to maintain the block level anymore,
we have to set the block level on every token, ouch.

this is all to share code between two possible parsers who both want to watch
block level?  Should we instead do this by having a parser superclass, or a
tokenizer driver which both parser use to drive the tokenizer?

[Yoav Weiss, 2014-04-09 04:56:40]

On 2014/04/08 22:58:51, eseidel wrote:
> I'm not sure I fully understand yet. May try to chat more with you this
evneing.
> 
>
https://codereview.chromium.org/225293006/diff/40001/Source/core/css/parser/M...
> File Source/core/css/parser/MediaQueryParser.cpp (right):
> 
>
https://codereview.chromium.org/225293006/diff/40001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryParser.cpp:163: if (token->blockType() ==
> MediaQueryToken::BlockEnd && token->blockLevel() <= 1)
> Why <= 1?
Oh, wanted to turn that to a const, and missed it before uploading.
This "1" should probably be EndOfBlocks.
> 
>
https://codereview.chromium.org/225293006/diff/40001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryParser.cpp:172: && (token->type() !=
> LeftParenthesisToken || token->blockLevel() > 1))
> Why > 1?
This "1" should be PermittedLevelsForParenteses
> 
>
https://codereview.chromium.org/225293006/diff/40001/Source/core/css/parser/M...
> File Source/core/css/parser/MediaQueryToken.cpp (right):
> 
>
https://codereview.chromium.org/225293006/diff/40001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryToken.cpp:58:
> MediaQueryToken::MediaQueryToken(MediaQueryTokenType type, String value,
> BlockType blockType)
> It's really error-prone to have this many conctructors.  Very easy to fail to
> initialze one member.
OK. I'll converge them.
> 
>
https://codereview.chromium.org/225293006/diff/40001/Source/core/css/parser/M...
> File Source/core/css/parser/MediaQueryTokenizer.cpp (right):
> 
>
https://codereview.chromium.org/225293006/diff/40001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryTokenizer.cpp:220:
> token.setBlockLevel(tokenizer.m_blockStack.size());
> I see.  Because the parser doesn't know how to maintain the block level
anymore,
> we have to set the block level on every token, ouch.

This is why I prefered counting blocks at a higher level in the previous
patchset.
> 
> this is all to share code between two possible parsers who both want to watch
> block level?  
The purpose is not really code sharing, since at a higher level, each parser's
minimal block counting needs are different.

> Should we instead do this by having a parser superclass, or a
> tokenizer driver which both parser use to drive the tokenizer?

Maybe extend TokenIterator to do that? It'd still be required to maintain that
info on a per token (or per iterator) basis.

[Yoav Weiss, 2014-04-09 08:58:17]

After our IRC chat, and after thinking about this some more, here's where I'm at
right now.
The "BlockStart","BlockEnd" indication on the MQToken are essential in order for
the parser to be aware of "component values" and were they start.
The Sizes parser algorithm requires that. (which AFAIU means that the tokenizer
must maintain state, unless we want it to be recursive)

So, assuming that is acceptable, the question here is "Who should do the level
counting?"
I'll add a blockWatcher class that would do that, but it'd probably be pretty
thin.

[eseidel, 2014-04-10 06:45:37]

lgtm

The poor yak is bare.  lets move on.

[commit-bot: I haz the power, 2014-04-10 06:45:47]

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

[commit-bot: I haz the power, 2014-04-10 06:54:54]

Change committed as 171237