Bug 82056

Summary: Use enumeration for CSS parser mode
Product: WebKit Reporter: Dirk Schulze <krit>
Component: CSSAssignee: Dirk Schulze <krit>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, gustavo, koivisto, macpherson, menard, pnormand, webkit.review.bot, xan.lopez, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 46112, 82335    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch
none
Patch
none
Patch koivisto: review+

Description Dirk Schulze 2012-03-23 08:30:18 PDT
Use enumeration for CSS parser mode. This way we can not only differ between strict and quirks mode, but also have different rules for SVG attributes. In the long term we will support strict rules for SVG with specialized parsing rules like scientific numbers and optional units.
Comment 1 Dirk Schulze 2012-03-23 09:16:39 PDT
Created attachment 133499 [details]
Patch
Comment 2 Dirk Schulze 2012-03-23 09:20:01 PDT
Comment on attachment 133499 [details]
Patch

This is not for review.
Comment 3 WebKit Review Bot 2012-03-23 09:20:29 PDT
Attachment 133499 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:13:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Philippe Normand 2012-03-23 09:22:09 PDT
Comment on attachment 133499 [details]
Patch

Attachment 133499 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12123627
Comment 5 Build Bot 2012-03-23 09:23:04 PDT
Comment on attachment 133499 [details]
Patch

Attachment 133499 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12116699
Comment 6 Early Warning System Bot 2012-03-23 09:31:06 PDT
Comment on attachment 133499 [details]
Patch

Attachment 133499 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12120699
Comment 7 Build Bot 2012-03-23 09:32:14 PDT
Comment on attachment 133499 [details]
Patch

Attachment 133499 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12117689
Comment 8 WebKit Review Bot 2012-03-23 09:32:42 PDT
Comment on attachment 133499 [details]
Patch

Attachment 133499 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12116702
Comment 9 Early Warning System Bot 2012-03-23 09:35:11 PDT
Comment on attachment 133499 [details]
Patch

Attachment 133499 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12120701
Comment 10 Dirk Schulze 2012-03-23 09:36:09 PDT
Created attachment 133502 [details]
Patch

The style issue of the bot is a false negative in my eyes. Please wait till all bots pass before reviewing the patch.
Comment 11 WebKit Review Bot 2012-03-23 09:39:32 PDT
Attachment 133502 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/css/CSSParserMode.h:40:  Missing spaces around |  [whitespace/operators] [3]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 WebKit Review Bot 2012-03-23 09:59:37 PDT
Comment on attachment 133502 [details]
Patch

Attachment 133502 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12113724

New failing tests:
editing/pasteboard/4631972.html
http/tests/navigation/postredirect-basic.html
fast/backgrounds/animated-svg-as-mask.html
css2.1/20110323/replaced-intrinsic-ratio-001.htm
http/tests/navigation/postredirect-goback1.html
css3/images/cross-fade-overflow-position.html
fast/block/float/4145535Crash.html
animations/animation-add-events-in-handler.html
fast/backgrounds/size/backgroundSize09.html
css1/box_properties/border_bottom.html
http/tests/inspector/inspect-element.html
accessibility/aria-disabled.html
fast/backgrounds/size/parsing-inherit.html
css1/box_properties/border.html
fast/backgrounds/size/backgroundSize05.html
editing/execCommand/forward-delete-no-scroll.html
css1/box_properties/border_left.html
http/tests/navigation/javascriptlink-frames.html
css1/box_properties/border_right_inline.html
fast/backgrounds/size/backgroundSize11.html
compositing/culling/tile-occlusion-boundaries.html
fast/backgrounds/size/parsing-background-size-values.html
css1/box_properties/border_top.html
editing/selection/click-start-of-line.html
editing/execCommand/insert-line-break-no-scroll.html
fast/backgrounds/size/backgroundSize10.html
fast/backgrounds/size/backgroundSize12.html
css3/images/cross-fade-invalidation.html
fast/backgrounds/repeat/parsing-background-repeat.html
http/tests/navigation/error404-subframeload.html
Comment 13 WebKit Review Bot 2012-03-23 09:59:48 PDT
Created attachment 133505 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 14 Dirk Schulze 2012-03-23 11:17:55 PDT
Created attachment 133516 [details]
Patch

Patch. Fixed a bug in the enum.
Comment 15 WebKit Review Bot 2012-03-23 11:23:31 PDT
Attachment 133516 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/css/CSSParserMode.h:40:  Missing spaces around |  [whitespace/operators] [3]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Ryosuke Niwa 2012-03-23 12:04:42 PDT
Comment on attachment 133516 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133516&action=review

> Source/WebCore/css/CSSParser.cpp:1339
> +    if (m_cssParserMode & CSSStrictMode || inShorthand())

Maybe we should add isStrict() helper member function?

> Source/WebCore/css/CSSParser.cpp:2288
> -            || (id >= CSSValueWebkitFocusRingColor && id < CSSValueWebkitText && !m_strict)) {
> +            || (id >= CSSValueWebkitFocusRingColor && id < CSSValueWebkitText && m_cssParserMode & CSSQuirksMode)) {

Add isQuirksMode() helper function?

> Source/WebCore/css/CSSParser.cpp:9083
> -        rule->styleRule()->setProperties(StylePropertySet::create(m_parsedProperties.data(), m_parsedProperties.size(), m_strict));
> +        rule->styleRule()->setProperties(StylePropertySet::create(m_parsedProperties.data(), m_parsedProperties.size(), m_cssParserMode & CSSStrictMode));

Do you plan to change the argument of StylePropertySet::create as a follow up?
Comment 17 Simon Fraser (smfr) 2012-03-23 12:09:01 PDT
Comment on attachment 133516 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133516&action=review

> Source/WebCore/css/CSSParser.cpp:5232
> +    if (m_cssParserMode & CSSQuirksMode && value->unit == CSSPrimitiveValue::CSS_NUMBER

I'd prefer parens around the m_cssParserMode & CSSQuirksMode

> Source/WebCore/css/CSSParser.cpp:5240
> +                (m_cssParserMode & CSSQuirksMode && value->unit == CSSPrimitiveValue::CSS_DIMENSION)) {

Ditto. You could also make this easier to read by adding CSSParser::inQuirksMode() const.

> Source/WebCore/css/CSSParserMode.h:33
> +enum CSSParserMode {

Why does CSSParserMode needs its own header? Why not put this in CSSParser.h?

> Source/WebCore/css/CSSParserMode.h:37
> +    CSSQuirksMode = 0x1,
> +    CSSStrictMode = 0x2,
> +    // SVG should always be in strict mode. For SVG attributes, the rules differ to strict sometimes.
> +    SVGAttributeMode = 0x4

Are these intended to be ORed together? If so, would be clearer to use 1<<0, 1 << 1 etc.

Also, you should then define a type for the bitfield.
Comment 18 Antti Koivisto 2012-03-23 12:12:10 PDT
Comment on attachment 133516 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133516&action=review

r- for clarification.

> Source/WebCore/css/CSSParser.h:288
> +    CSSParserMode m_cssParserMode;

Since CSSParserMode now defines a mask of bits, it doesn't make sense to use it as the type for the field. Enum field should have values defined by the enum only.

> Source/WebCore/css/CSSParserMode.h:38
> +enum CSSParserMode {
> +    CSSQuirksMode = 0x1,
> +    CSSStrictMode = 0x2,
> +    // SVG should always be in strict mode. For SVG attributes, the rules differ to strict sometimes.
> +    SVGAttributeMode = 0x4
> +};

I thought we would have three modes. Instead this is now a bit mask. Why? What it the legal combination of these bits?
Comment 19 Dirk Schulze 2012-03-23 12:57:50 PDT
(In reply to comment #18)
> (From update of attachment 133516 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133516&action=review
> 
> r- for clarification.
> 
> > Source/WebCore/css/CSSParser.h:288
> > +    CSSParserMode m_cssParserMode;
> 
> Since CSSParserMode now defines a mask of bits, it doesn't make sense to use it as the type for the field. Enum field should have values defined by the enum only.
I'm not sure if I understand your concern. It is a bit mask, yes. But I use it the same way like for instances Units (FNumber, Flength, ...).

> 
> > Source/WebCore/css/CSSParserMode.h:38
> > +enum CSSParserMode {
> > +    CSSQuirksMode = 0x1,
> > +    CSSStrictMode = 0x2,
> > +    // SVG should always be in strict mode. For SVG attributes, the rules differ to strict sometimes.
> > +    SVGAttributeMode = 0x4
> > +};
> 
> I thought we would have three modes. Instead this is now a bit mask. Why? What it the legal combination of these bits?

Like the comment says. SVG should get parsed in strict mode. Even the attributes. But we still use some extra logic that differs from strict mode. Some of these cases are scientific numbers (1e2px) or unit less values.
But for instance for color parsing we don't want to have quirks mode (like we do at the moment).
Comment 20 Antti Koivisto 2012-03-23 13:23:59 PDT
You are giving CSSQuirksMode, CSSStrictMode and SVGAttributeMode each a bit of their own as if they were independent and testing them using bit operations. However some or all of these (quirks and strict...) are clearly mutually exclusive. I don't understand what the logic is here.

As far as I see you have exactly three mutually exclusive modes, something that should implemented in terms of simple three state enum.
Comment 21 Dirk Schulze 2012-03-23 14:02:14 PDT
(In reply to comment #20)
> You are giving CSSQuirksMode, CSSStrictMode and SVGAttributeMode each a bit of their own as if they were independent and testing them using bit operations. However some or all of these (quirks and strict...) are clearly mutually exclusive. I don't understand what the logic is here.
> 
> As far as I see you have exactly three mutually exclusive modes, something that should implemented in terms of simple three state enum.

I think I got your point. Yes, quirks and strict are exclusive. It is not expected that both are true (checked by an assert in the patch). I'll rethink the concept based on the comments above

PS: sorry, missed the first comments on reading the last review. I'll also look on that.
Comment 22 Dirk Schulze 2012-03-23 14:23:37 PDT
(In reply to comment #16)
> Do you plan to change the argument of StylePropertySet::create as a follow up?

Yes. This should get a parameter about the current parsing mode (strict, quirks, svgattribute).
Comment 23 Dirk Schulze 2012-03-23 15:22:43 PDT
Created attachment 133571 [details]
Patch

Patch. Replaced the bit mask with an enum. Added two helper functions inStrictMode() and inQuirksMode() for a better readability.
Comment 24 Dirk Schulze 2012-03-23 16:02:19 PDT
(In reply to comment #17)
> (From update of attachment 133516 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133516&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:5232
> > +    if (m_cssParserMode & CSSQuirksMode && value->unit == CSSPrimitiveValue::CSS_NUMBER
> 
> I'd prefer parens around the m_cssParserMode & CSSQuirksMode
> 
> > Source/WebCore/css/CSSParser.cpp:5240
> > +                (m_cssParserMode & CSSQuirksMode && value->unit == CSSPrimitiveValue::CSS_DIMENSION)) {
> 
> Ditto. You could also make this easier to read by adding CSSParser::inQuirksMode() const.
> 
> > Source/WebCore/css/CSSParserMode.h:33
> > +enum CSSParserMode {
> 
> Why does CSSParserMode needs its own header? Why not put this in CSSParser.h?
I didn't want to include CSSParser.h everywhere where I need the enum.
Comment 25 Nikolas Zimmermann 2012-03-24 02:49:32 PDT
Comment on attachment 133571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133571&action=review

> Source/WebCore/ChangeLog:8
> +        Introduce a new enumeration CSSParserMode to differ between strict, quirks and svg presentation

Introduce a new CSSParserMode enum to differ between strict / quirks and SVG presentation attribute parsing modes.

> Source/WebCore/ChangeLog:10
> +        The next step will be updating StylePropertySet and CSSStyleSheet to use this enumeration as well.

What does that mean?

> Source/WebCore/ChangeLog:12
> +        After that it will be easier possible to reuse the CSS parser in SVG as much as possible and
> +        introduce SVG only functionality.

s/SVG only/SVG specific/

> Source/WebCore/css/CSSParser.cpp:189
> +inline bool CSSParser::inStrictMode()
> +{
> +    return m_cssParserMode == CSSStrictMode;
> +}

Why not move this in the header? And I'd make it const.

> Source/WebCore/css/CSSParser.cpp:191
> +inline bool CSSParser::inQuirksMode()

Ditto.

> Source/WebCore/css/CSSParser.cpp:193
> +    // FIXME: Move SVGAttributeMod to inStrictMode() once StylePropertySet and CSSStyleSheet

typo: Mode

> Source/WebCore/css/CSSParser.cpp:198
>  CSSParser::CSSParser(bool strictParsing)

Hrm, why do you need a new constructor for this? I'd avoid this, even if its an intermediate step.
CSSParser myParser(0) is now ambiguous.

> Source/WebCore/css/CSSParser.cpp:260
> +    ASSERT(cssParserMode & CSSStrictMode ^ cssParserMode & CSSQuirksMode);

This is pointless, now you moved away from the bitfield.

> Source/WebCore/css/CSSParser.cpp:1244
> +        // Qirks mode and svg presentation attributes accept unit less values.
> +        if (!b && ((unitflags & (FLength | FAngle | FTime)) && (!value->fValue || cssParserMode == CSSQuirksMode || cssParserMode == SVGAttributeMode))) {

So why not introduce a "static inline bool shouldAcceptUnitLessValues(CSSParserMode)" ?
This avoids the extra-comment and makes the code more readable.

> Source/WebCore/css/CSSParser.cpp:1599
> -             (id >= CSSValueWebkitFocusRingColor && id < CSSValueWebkitText && !m_strict)) {
> +             (id >= CSSValueWebkitFocusRingColor && id < CSSValueWebkitText && inQuirksMode())) {

Shall we allow these color values for SVG as well? If not I'd make that !inStrictMode().

> Source/WebCore/css/CSSParser.cpp:1646
> -            } else if (!m_strict && value->id == CSSValueHand) // MSIE 5 compatibility :/
> +            } else if (inQuirksMode() && value->id == CSSValueHand) // MSIE 5 compatibility :/

This for sure won't be needed for SVG, so this should be !inStructMode() no?

> Source/WebCore/css/CSSParser.cpp:1655
> -        if (!m_strict && value->id == CSSValueHand) { // MSIE 5 compatibility :/
> +        if (inQuirksMode() && value->id == CSSValueHand) { // MSIE 5 compatibility :/

Ditto.

> Source/WebCore/css/CSSParser.cpp:1765
> -        validPrimitive = (!id && validUnit(value, FLength | FPercent, m_strict));
> +        validPrimitive = (!id && validUnit(value, FLength | FPercent, m_cssParserMode));

I would have created a new private validUnit() function that does NOT accept a CSSParserMode or bool argument, and just forwards to validUnit(param1, param1, .. m_cssParserMode) to avoid having to pass it around everywhere.
If you'd land that first as separate patch the size of this patch would shrink a lot!

> Source/WebCore/css/CSSParser.cpp:2236
> -            validPrimitive = validUnit(value, FLength, true);
> +            validPrimitive = validUnit(value, FLength, CSSStrictMode);

Ditto

> Source/WebCore/css/CSSParser.cpp:2245
> +        validPrimitive = (!id && (value->unit == CSSPrimitiveValue::CSS_PERCENTAGE || value->fValue) && validUnit(value, FInteger | FPercent | FNonNeg, CSSQuirksMode));

Ditto.

> Source/WebCore/css/CSSParser.cpp:2264
> +        if (id == CSSValueAuto || validUnit(value, FInteger | FNonNeg, CSSStrictMode))

Ditto.

> Source/WebCore/css/CSSParser.cpp:2269
> +        if (id == CSSValueNoLimit || validUnit(value, FInteger | FNonNeg, CSSStrictMode))

Ditto.

> Source/WebCore/css/CSSParser.cpp:2300
> -            || (id >= CSSValueWebkitFocusRingColor && id < CSSValueWebkitText && !m_strict)) {
> +            || (id >= CSSValueWebkitFocusRingColor && id < CSSValueWebkitText && inQuirksMode())) {

Should be irrelevant for SVG: so use !inStrictMode() here.

> Source/WebCore/css/CSSParser.cpp:3165
> +        (id >= CSSValueGrey && id < CSSValueWebkitText && inQuirksMode()))

Ditto.
Comment 26 Dirk Schulze 2012-03-26 09:16:04 PDT
(In reply to comment #25)
> (From update of attachment 133571 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133571&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Introduce a new enumeration CSSParserMode to differ between strict, quirks and svg presentation
> 
> Introduce a new CSSParserMode enum to differ between strict / quirks and SVG presentation attribute parsing modes.
> 
> > Source/WebCore/ChangeLog:10
> > +        The next step will be updating StylePropertySet and CSSStyleSheet to use this enumeration as well.
> 
> What does that mean?
> 
> > Source/WebCore/ChangeLog:12
> > +        After that it will be easier possible to reuse the CSS parser in SVG as much as possible and
> > +        introduce SVG only functionality.
> 
> s/SVG only/SVG specific/
> 
> > Source/WebCore/css/CSSParser.cpp:189
> > +inline bool CSSParser::inStrictMode()
> > +{
> > +    return m_cssParserMode == CSSStrictMode;
> > +}
> 
> Why not move this in the header? And I'd make it const.
> 
> > Source/WebCore/css/CSSParser.cpp:191
> > +inline bool CSSParser::inQuirksMode()
> 
> Ditto.
> 
> > Source/WebCore/css/CSSParser.cpp:193
> > +    // FIXME: Move SVGAttributeMod to inStrictMode() once StylePropertySet and CSSStyleSheet
> 
> typo: Mode
> 
> > Source/WebCore/css/CSSParser.cpp:198
> >  CSSParser::CSSParser(bool strictParsing)
> 
> Hrm, why do you need a new constructor for this? I'd avoid this, even if its an intermediate step.
> CSSParser myParser(0) is now ambiguous.
This would make the patch bigger, I just want to limit this patch to CSSParser, otherwise the patch gets less reviewable. But it is already in my followup patch.

> 
> > Source/WebCore/css/CSSParser.cpp:260
> > +    ASSERT(cssParserMode & CSSStrictMode ^ cssParserMode & CSSQuirksMode);
True.

> 
> This is pointless, now you moved away from the bitfield.
> 
> > Source/WebCore/css/CSSParser.cpp:1244
> > +        // Qirks mode and svg presentation attributes accept unit less values.
> > +        if (!b && ((unitflags & (FLength | FAngle | FTime)) && (!value->fValue || cssParserMode == CSSQuirksMode || cssParserMode == SVGAttributeMode))) {
> 
> So why not introduce a "static inline bool shouldAcceptUnitLessValues(CSSParserMode)" ?
> This avoids the extra-comment and makes the code more readable.
I doubt that the code gets more readable when we move everything in new functions. Nevertheless, I can do this. I just doubt that it makes the code more readable.

> 
> > Source/WebCore/css/CSSParser.cpp:1599
> > -             (id >= CSSValueWebkitFocusRingColor && id < CSSValueWebkitText && !m_strict)) {
> > +             (id >= CSSValueWebkitFocusRingColor && id < CSSValueWebkitText && inQuirksMode())) {
> 
> Shall we allow these color values for SVG as well? If not I'd make that !inStrictMode().
This patch focus on introducing the enum, I won't add any functional changes into this patch. That said, I won't fix any bugs on SVG with this patch. This is in the scope of the master patch.

> 
> > Source/WebCore/css/CSSParser.cpp:1646
> > -            } else if (!m_strict && value->id == CSSValueHand) // MSIE 5 compatibility :/
> > +            } else if (inQuirksMode() && value->id == CSSValueHand) // MSIE 5 compatibility :/
> 
> This for sure won't be needed for SVG, so this should be !inStructMode() no?
Ditto.

> 
> > Source/WebCore/css/CSSParser.cpp:1655
> > -        if (!m_strict && value->id == CSSValueHand) { // MSIE 5 compatibility :/
> > +        if (inQuirksMode() && value->id == CSSValueHand) { // MSIE 5 compatibility :/
> 
> Ditto.
Ditto.

> 
> > Source/WebCore/css/CSSParser.cpp:1765
> > -        validPrimitive = (!id && validUnit(value, FLength | FPercent, m_strict));
> > +        validPrimitive = (!id && validUnit(value, FLength | FPercent, m_cssParserMode));
> 
> I would have created a new private validUnit() function that does NOT accept a CSSParserMode or bool argument, and just forwards to validUnit(param1, param1, .. m_cssParserMode) to avoid having to pass it around everywhere.
> If you'd land that first as separate patch the size of this patch would shrink a lot!
I can introduce this new function. But I would still like to add it in one patch.

> 
> > Source/WebCore/css/CSSParser.cpp:2236
> > -            validPrimitive = validUnit(value, FLength, true);
> > +            validPrimitive = validUnit(value, FLength, CSSStrictMode);
> 
> Ditto
> 
> > Source/WebCore/css/CSSParser.cpp:2245
> > +        validPrimitive = (!id && (value->unit == CSSPrimitiveValue::CSS_PERCENTAGE || value->fValue) && validUnit(value, FInteger | FPercent | FNonNeg, CSSQuirksMode));
> 
> Ditto.
> 
> > Source/WebCore/css/CSSParser.cpp:2264
> > +        if (id == CSSValueAuto || validUnit(value, FInteger | FNonNeg, CSSStrictMode))
> 
> Ditto.
> 
> > Source/WebCore/css/CSSParser.cpp:2269
> > +        if (id == CSSValueNoLimit || validUnit(value, FInteger | FNonNeg, CSSStrictMode))
> 
> Ditto.
> 
> > Source/WebCore/css/CSSParser.cpp:2300
> > -            || (id >= CSSValueWebkitFocusRingColor && id < CSSValueWebkitText && !m_strict)) {
> > +            || (id >= CSSValueWebkitFocusRingColor && id < CSSValueWebkitText && inQuirksMode())) {
> 
> Should be irrelevant for SVG: so use !inStrictMode() here.
Not in this patch.

> 
> > Source/WebCore/css/CSSParser.cpp:3165
> > +        (id >= CSSValueGrey && id < CSSValueWebkitText && inQuirksMode()))
> 
> Ditto.
Ditto.
Comment 27 Dirk Schulze 2012-03-26 11:58:25 PDT
Created attachment 133857 [details]
Patch

Patch with fixes based on the reviewer feedback. Still no changes on functionality since this patch is just a preparation for the SVG fixes.
Comment 28 Dirk Schulze 2012-03-26 12:00:13 PDT
(In reply to comment #25)
> > Source/WebCore/css/CSSParser.cpp:1244
> > +        // Qirks mode and svg presentation attributes accept unit less values.
> > +        if (!b && ((unitflags & (FLength | FAngle | FTime)) && (!value->fValue || cssParserMode == CSSQuirksMode || cssParserMode == SVGAttributeMode))) {
> 
> So why not introduce a "static inline bool shouldAcceptUnitLessValues(CSSParserMode)" ?
> This avoids the extra-comment and makes the code more readable.
Made it just inline, since Unit (FLength, FAngle) is a private enum in CSSParser.
Comment 29 Antti Koivisto 2012-03-27 09:18:10 PDT
Comment on attachment 133857 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133857&action=review

> Source/WebCore/css/CSSGrammar.y:1026
> -        if (!p->m_strict)
> +        if (p->m_cssParserMode != CSSStrictMode)

It would be better to be explicit and write these in terms of modes that do get this behavior.
Comment 30 Dirk Schulze 2012-03-27 09:20:50 PDT
(In reply to comment #29)
> (From update of attachment 133857 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133857&action=review
> 
> > Source/WebCore/css/CSSGrammar.y:1026
> > -        if (!p->m_strict)
> > +        if (p->m_cssParserMode != CSSStrictMode)
> 
> It would be better to be explicit and write these in terms of modes that do get this behavior.

Yes, I'll do this change before landing. Thanks.
Comment 31 Dirk Schulze 2012-03-30 13:16:00 PDT
Patch was committed with:
http://trac.webkit.org/changeset/112537

Chromium build fix was committed with:
http://trac.webkit.org/changeset/112542

Close this bug now.
Comment 32 Dirk Schulze 2012-03-30 13:17:03 PDT
Sorry, wrong links. Patch was committed in:
http://trac.webkit.org/changeset/112310