399495 - nsCSSParser/Scanner: include '(' within the function token

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Alfred Kayser]

Attachment: V1: Simplify the parser logic by letting the scanner eat the '(' of the function

In GatherIdent in nsCSSParser.cpp, functions are detected by the following of a '(' after the ident. But then it is pushed back, so that nsCSSScanner needs to always do GetToken to get the '(' symbol.

By changing GatherIdent to eat the '(' when it tokenizes a function, nsCSSScanner then doesn't need to check for '(' again.

Comment 1

[Alfred Kayser]

Attachment: Whitespace ignorant version for easier review

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Alfred, would it be worth verifying that this patch still passes the reftest suite?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think this patch is wrong because it causes asymmetry in skipping code; we now need to pair function tokens with their closing (, and any code that eats a function token and then calls SkipUntil now needs to push that function token back.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 284516 [details] [diff] [review]
V1: Simplify the parser logic by letting the scanner eat the '(' of the function

... plus the skipping code needs to be updated.

I haven't looked at the patch much beyond that, though, but marking review-.

Comment 5

[Zack Weinberg (:zwol)]

Attachment: revised patch, skipping should work correctly

Here's a revision that, I think, hits all the places where skipping now needs to pair function tokens with close parentheses.

I doubt this makes any measurable performance difference, and frankly I wish the spec didn't have this stupid wart.  I did it because it'll be easier to validate a hypothetical machine-generated scanner if it tokenizes exactly the same as our existing scanner *and* exactly the same as the rules in css2.1 section 4.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 401341 [details] [diff] [review]
revised patch, skipping should work correctly

I think there are still tons of places that this patch misses.

The first few examples in the parser:

CSSParserImpl::ParseMediaQueryExpression needs an UngetToken before its first SkipUntil call, the SkipUntil call after !mToken.IsSymbol(':')

CSSParserImpl::ParseTreePseudoElement needs the same before its SkipUntil (which was introduced since this patch was written)

CSSParserImpl::ParseCounter also needs an UngetToken before its first three SkipUntil calls.

CSSParserImpl::ParseImageRect needs an UngetToken before a number of its break statements.

And I'm not all the way through the parser yet (although I am most of the way).


Given that, I tend to think that having to use UngetToken in many cases before SkipUntil is an even worse model than the current one (since the bugs it would lead to are hard-to-notice error-handling bugs rather than code-obviously-doesn't-work bugs).  So I think my current inclination is that we don't want to take this approach at all; sorry for not realizing how painful it would be earlier (though sometimes you just can't tell until you see it).


I think we could make things a good bit simpler without combining the tokens.  In particular, we could have a *void* function named something like ConsumeFunctionStartParen that would:
 * NS_ABORT_IF_FALSE that the current token is a function token
 * consume the parenthesis following it

rather than using ExpectSymbol as we currently do.


What do you and Boris think of that approach instead?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, all those places are currently broken and they need the UngetToken even in the current model.  So maybe this is ok.  Let me re-review.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And I think that's the complete list of UngetTokens that are needed given the added UngetToken already in the patch (in CSSParserImpl::ParseFontSrcFormat).

We should add tests that would catch those, though, and fix them.

(Do you remember if you added the one in ParseFontSrcFormat because of a test that failed?)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, I missed a missing UngetToken in:
    case nsMediaFeature::eResolution:
in CSSParserImpl::ParseMediaQueryExpression.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 401341 [details] [diff] [review]
revised patch, skipping should work correctly

OK, this looks good.  Sorry for the confusion, and the delay in getting to it.  r=dbaron


Do you want to do the followup on those missing UngetToken calls, or should I?

Comment 11

[Zack Weinberg (:zwol)]

I have four or five things ahead of this on my queue.  You probably have more, so I'd be happy to do it -- when I get around to it. :)

Comment 12

[Boris Zbarsky [:bzbarsky]]

David, do you still want an answer to the end of comment 6?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #10)
> Do you want to do the followup on those missing UngetToken calls, or should I?

I filed bug 528096 on doing this; feel free to take if you want or let me know otherwise.  Please don't do it in a revised patch for this bug, though.

(In reply to comment #12)
> David, do you still want an answer to the end of comment 6?

No.

Comment 14

[Zack Weinberg (:zwol)]

Attachment: rediffed

Patch needed rediffing for dbaron's changes in bug 541434.  I'm waiting to land this until bug 528096 can go too, by the way.

Comment 15

[Alfred Kayser]

Nit: that patch chunk: if (Peek() == '(') { Read(); ... can be replaced with: if (LookAhead('(')) { ...

Comment 16

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/d7081443284b

Without Alfred's suggestion, because LookAhead() is going to go away in the near future.