[Webkit-unassigned] [Bug 112986] Incorrect error handling for Media Queries

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=112986





--- Comment #15 from Rune Lillesveen <rune at opera.com>  2013-04-03 04:38:04 PST ---
(From update of attachment 194516)
View in context: https://bugs.webkit.org/attachment.cgi?id=194516&action=review

>> Source/WebCore/ChangeLog:52
>> +        * css/MediaList.h:
> 
> You could add some comments here

Do you mean inside css/MediaList.h? Or anything missing in the Changelog?

>> Source/WebCore/css/CSSParser.cpp:1490
>> +    // can't use { because tokenizer state switches from mediaquery to initial
> 
> Now you are fixing it, please use uppercase C, as comments should start with uppercase.

Will fix.

>> Source/WebCore/css/CSSParser.h:507
>> +    // token.
> 
> just keep on one line

Will fix.

>> Source/WebCore/css/CSSParser.h:670
>> +    };
> 
> Why not ReEmissionState { DontEmit, ReEmit, DidReEmit }?

Will fix.

>> Source/WebCore/css/CSSParser.h:821
>> +{
> 
> This method makes it sounds as it is going to re-emit, but that is not really what it is doing

How about "markCurrentTokenForReEmission()"?

>> Source/WebCore/css/CSSParser.h:825
>> +    ASSERT(m_reEmitToken == NoEmit);
> 
> shouldnt this be != ?

No, this assert triggers if the a token has just been re-emitted. It would mean that one of these situations occurred, given a production of e.g. "error ','" that caused the first re-emission:

1. The re-emitted ',' matches a ',' token in a parent rule which also contains an error token.
2. The re-emitted ',' matches the same production containing the error token causing an infinite re-emission.

The assert is there to catch such problematic constructs in the grammar. The if-test below the assert is there to prevent infinite re-emissions, should it occur.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.