A discussion on www-style clarified that webkits behaviour in recognizing @mediaall (without space) as equivalent to @media all was incorrect. In general this is true of all @-rules in webkit -- it needs to be a long greedy match for an identifier.

Created attachment 22190[details]
Patch that fixes @-rules from being recognized too early
Fix the bug by adding a rule to tokenizer.flex:
"@"{ident}
And have this rule return the '@' token -- this means that CSSGrammar.y does not have to change.

(In reply to comment #0)
> A discussion on www-style clarified that webkits behaviour in recognizing
> @mediaall (without space) as equivalent to @media all was incorrect. In general
> this is true of all @-rules in webkit -- it needs to be a long greedy match for
> an identifier.
>
Working on a better patch (that also follows the patch guidelines :-)

Created attachment 22245[details]
Updated patch to use "@"{ident}, updated ChangeLog and added test case
Changed the fix to use "@"{ident} to return ATKEYWORD to be closer to the CSS2.1 spec. The '@' by itself (without a following identifier) should per CSS2.1 not be used in error recovery rules the same place that the ATKEYWORD is used.
In practice this doesn't change error recovery as the invalid_block and invalid_block_list are used in all productions that need error recovery.

Comment on attachment 22245[details]
Updated patch to use "@"{ident}, updated ChangeLog and added test case
You need to include layout test ChangeLog and results too. Maybe you can use dumpAsText to avoid the pixel test.
Are there no regressions?
Why the yellow background? Can't you just use white or not set the background?

(In reply to comment #5)
> (From update of attachment 22245[details] [edit])
> You need to include layout test ChangeLog and results too. Maybe you can use
> dumpAsText to avoid the pixel test.
>
> Are there no regressions?
>
Not sure I follow -- atrule_longest_match.html render correctly before; now it will. I see that I am missing the expected text -- is that what you were referring to?
> Why the yellow background? Can't you just use white or not set the background?
>
I suppose I can -- I just needed a check to ensure that the parser recovered after the incorrect @-rule.
I'll work on updating the patch.
Thanks for the input,
- Jacob

Comment on attachment 22253[details]
Fixed tabbing and added expected.txt
'ealier' is mentioned in the ChangeLog. Email address needs to be set.
You need to rerun the layout ChangeLog so it lists the .txt file too.
Does the patch cause no regressions on the tests?
If you can fix the minor niggles, put up the new patch and have assured there are no regressions with the patch, then I'll okay it.

(In reply to comment #6)
> > Are there no regressions?
> >
> Not sure I follow -- atrule_longest_match.html render correctly before; now it
> will. I see that I am missing the expected text -- is that what you were
> referring to?
Just to be clear, all patches need to go through the run-webkit-tests run
to check for regressions. See 'Regression tests' here:
https://www.webkit.org/coding/contributing.html
What basically is required is that you run the whole suite without your patch,
note the numer of pass/fails, then apply and build with your patch, rerun the suite and note whether anything changed with the pass/fail with the patch. Only when the fail count is the same can the patch be accepted.
Cheers,
Rob.

(In reply to comment #11)
> (In reply to comment #6)
> > > Are there no regressions?
> > >
> > Not sure I follow -- atrule_longest_match.html render correctly before; now it
> > will. I see that I am missing the expected text -- is that what you were
> > referring to?
>
> Just to be clear, all patches need to go through the run-webkit-tests run
> to check for regressions. See 'Regression tests' here:
>
> https://www.webkit.org/coding/contributing.html
>
> What basically is required is that you run the whole suite without your patch,
> note the numer of pass/fails, then apply and build with your patch, rerun the
> suite and note whether anything changed with the pass/fail with the patch. Only
> when the fail count is the same can the patch be accepted.
> Cheers,
>
> Rob.
>
Thanks for the followup Rob -- it does pass in the sense no new or different errors were reported in regression against Qt and Gtk.
- Jacob

Hi,
(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #6)
> > > > Are there no regressions?
> > > >
> > > Not sure I follow -- atrule_longest_match.html render correctly before; now it
> > > will. I see that I am missing the expected text -- is that what you were
> > > referring to?
> >
> > Just to be clear, all patches need to go through the run-webkit-tests run
> > to check for regressions. See 'Regression tests' here:
> >
> > https://www.webkit.org/coding/contributing.html
> >
> > What basically is required is that you run the whole suite without your patch,
> > note the numer of pass/fails, then apply and build with your patch, rerun the
> > suite and note whether anything changed with the pass/fail with the patch. Only
> > when the fail count is the same can the patch be accepted.
> > Cheers,
> >
> > Rob.
> >
>
> Thanks for the followup Rob -- it does pass in the sense no new or different
> errors were reported in regression against Qt and Gtk.
>
> - Jacob
Nice! I tested on OS X tiger and could see no regressions either.
Cheers,
Rob.

Comment on attachment 22307[details]
Final patch - fixed changelog
Minor niggles, there should be a newline after the Reviewed line in the WebCore ChangeLog and also a newline between LayoutTests ChangeLog entries.
In general check the existing entries for the right formatting.
r=me

(In reply to comment #15)
> (From update of attachment 22307[details] [edit])
> Minor niggles, there should be a newline after the Reviewed line in the WebCore
> ChangeLog and also a newline between LayoutTests ChangeLog entries.
>
> In general check the existing entries for the right formatting.
>
> r=me
>
Okay - thx. Fixed the newline.
- Jacob