Summary: | Use enumeration for CSS parser mode | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Schulze <krit> | ||||||||||||||
Component: | CSS | Assignee: | 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
Dirk Schulze
2012-03-23 08:30:18 PDT
Created attachment 133499 [details]
Patch
Comment on attachment 133499 [details]
Patch
This is not for review.
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 on attachment 133499 [details] Patch Attachment 133499 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12123627 Comment on attachment 133499 [details] Patch Attachment 133499 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12116699 Comment on attachment 133499 [details] Patch Attachment 133499 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12120699 Comment on attachment 133499 [details] Patch Attachment 133499 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12117689 Comment on attachment 133499 [details] Patch Attachment 133499 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12116702 Comment on attachment 133499 [details] Patch Attachment 133499 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12120701 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.
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 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 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
Created attachment 133516 [details]
Patch
Patch. Fixed a bug in the enum.
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 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 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 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? (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). 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. (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. (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). Created attachment 133571 [details]
Patch
Patch. Replaced the bit mask with an enum. Added two helper functions inStrictMode() and inQuirksMode() for a better readability.
(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 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. (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. 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.
(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 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. (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. 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. Sorry, wrong links. Patch was committed in: http://trac.webkit.org/changeset/112310 |