Closed Bug 399495 Opened 14 years ago Closed 12 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: alfredkayser, Assigned: zwol)

References

Details

(Keywords: perf)

Attachments

(1 file, 3 obsolete files)

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.
Attachment #284516 - Flags: review?(dbaron)
Alfred, would it be worth verifying that this patch still passes the reftest suite?
Version: unspecified → Trunk
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 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-.
Attachment #284516 - Flags: review?(dbaron) → review-
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.
Assignee: alfredkayser → zweinberg
Attachment #284516 - Attachment is obsolete: true
Attachment #284518 - Attachment is obsolete: true
Attachment #401341 - Flags: review?(dbaron)
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?
Attachment #401341 - Flags: review?(dbaron) → review-
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.
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?)
Oh, I missed a missing UngetToken in:
    case nsMediaFeature::eResolution:
in CSSParserImpl::ParseMediaQueryExpression.
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?
Attachment #401341 - Flags: review- → review+
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. :)
David, do you still want an answer to the end of comment 6?
(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.
Attached patch rediffedSplinter Review
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.
Attachment #401341 - Attachment is obsolete: true
Attachment #423460 - Flags: review+
Nit: that patch chunk: if (Peek() == '(') { Read(); ... can be replaced with: if (LookAhead('(')) { ...
Blocks: 528096
http://hg.mozilla.org/mozilla-central/rev/d7081443284b

Without Alfred's suggestion, because LookAhead() is going to go away in the near future.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.