Bug 62050

I'm going to start smaller with just parsing flex() for width and height. I'll add parsing of flex() for margin/padding/border in a follow up patch.

Created attachment 97026 [details] Patch

Comment on attachment 97026 [details] Patch Attachment 97026 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8837287

Created attachment 97029 [details] Patch

Comment on attachment 97026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97026&action=review Nice test case! Code looks good to me except for the readability nits. > LayoutTests/css3/flexbox/flex-parsing-expected.txt:11 > +PASS testFlex("-webkit-flex(1)", "width") is "-webkit-flex(1 0 auto)" > +PASS testFlex("-webkit-flex(0)", "width") is "-webkit-flex(0 0 auto)" > +PASS testFlex("-webkit-flex(2.4)", "width") is "-webkit-flex(2.4 0 auto)" > +PASS testFlex("-webkit-flex(auto)", "width") is "-webkit-flex(1 0 auto)" Here and below it's not clear to me whether we should return the normalized value or the value you set. For other properties (e.g. color) we seem inconsistent (e.g. if you set #CCC we return the rgb value, if you set "Blue" we return the lowercased string value and FireFox returns the case-sensitive string value). I don't think we need to block this patch on figuring that out, but we should get the spec to be explicit. > LayoutTests/css3/flexbox/flex-parsing-expected.txt:40 > +PASS testFlex("-webkit-flex(1 3px 2)", "width") is "-webkit-flex(1 2 3px)" > +PASS testFlex("-webkit-flex(3px 1 2)", "width") is "-webkit-flex(1 2 3px)" > +PASS testFlex("-webkit-flex(auto 0 0)", "width") is "-webkit-flex(0 0 auto)" > +PASS testFlex("-webkit-flex(0 0px 0)", "width") is "-webkit-flex(0 0 0px)" > +PASS testFlex("-webkit-flex(1 2 3)", "width") is "" In quirks mode we usually allow non-suffixed pixel values. Allowing that would complicate the case one line 36 though. Again, it would be good for the spec to be explicit about this. > Source/WebCore/css/CSSParser.cpp:4803 > + else if (arg2IsNonNegativeNumber && (arg1->id == CSSValueAuto || validUnit(arg1, FLength | FPercent | FNonNeg, m_strict))) Moving "arg1->id == CSSValueAuto || validUnit(arg1, FLength | FPercent | FNonNeg, m_strict))" into a helper function would make this more readable. > Source/WebCore/css/CSSParser.cpp:4814 > + else if (arg1IsNonNegativeNumber && arg2IsNonNegativeNumber && !validUnit(arg3, FNumber, m_strict) && (arg3->id == CSSValueAuto || validUnit(arg3, FLength | FPercent | FNonNeg, m_strict))) Moving "!validUnit(arg3, FNumber, m_strict) && (arg3->id == CSSValueAuto || validUnit(arg3, FLength | FPercent | FNonNeg, m_strict))" into a helper function would make this a lot more readable.

Tab, there's a few spec questions in comment 5. I'm not sure they need to be addressed in the Flex spec since they're generic CSS spec questions, but there should be some spec that addresses this and, in the meantime, the Flex spec might want to call it out explicitly.

Created attachment 97041 [details] Patch

Also, Tab, if you have some other cases we should test, let me know.

(In reply to comment #5) > > LayoutTests/css3/flexbox/flex-parsing-expected.txt:11 > > +PASS testFlex("-webkit-flex(1)", "width") is "-webkit-flex(1 0 auto)" > > +PASS testFlex("-webkit-flex(0)", "width") is "-webkit-flex(0 0 auto)" > > +PASS testFlex("-webkit-flex(2.4)", "width") is "-webkit-flex(2.4 0 auto)" > > +PASS testFlex("-webkit-flex(auto)", "width") is "-webkit-flex(1 0 auto)" > > Here and below it's not clear to me whether we should return the normalized value or the value you set. For other properties (e.g. color) we seem inconsistent (e.g. if you set #CCC we return the rgb value, if you set "Blue" we return the lowercased string value and FireFox returns the case-sensitive string value). I'll be adding an explicit section to the spec in the very near future, similar to the section in Image Values. Hint: It will specify that we should normalize the value. > > LayoutTests/css3/flexbox/flex-parsing-expected.txt:40 > > +PASS testFlex("-webkit-flex(1 3px 2)", "width") is "-webkit-flex(1 2 3px)" > > +PASS testFlex("-webkit-flex(3px 1 2)", "width") is "-webkit-flex(1 2 3px)" > > +PASS testFlex("-webkit-flex(auto 0 0)", "width") is "-webkit-flex(0 0 auto)" > > +PASS testFlex("-webkit-flex(0 0px 0)", "width") is "-webkit-flex(0 0 0px)" > > +PASS testFlex("-webkit-flex(1 2 3)", "width") is "" > > In quirks mode we usually allow non-suffixed pixel values. Allowing that would complicate the case one line 36 though. Again, it would be good for the spec to be explicit about this. CSS doesn't say anything about this, because it's not a CSS issue - it's a private quirk of HTML that is getting carried through to CSS. There's no way to resolve the ambiguity that this would introduce; I recommend not trying to do anything with the quirk here, and in general not spreading the quirk to new CSS things.

Created attachment 97504 [details] Patch

The only change from the previous patch is to use StringBuilder in CSSFlexValue.cpp. Can someone comfortable with CSSParser changes review this?

Maybe Simon can review?

Comment on attachment 97504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97504&action=review In general this looks fine. > Source/WebCore/css/CSSParser.cpp:792 > + return primitiveValueCache()->createValue(value->fValue, (CSSPrimitiveValue::UnitTypes) value->unit); We don't generally c-style cast. Can we use a helper here to avoid these long lines? > Source/WebCore/css/CSSParser.cpp:4787 > +bool CSSParser::parseFlex(int propId, bool important) I suspect we could write this shorter with a differnet algorithm. If this is the exact algorithm from the spec, we should add a comment to note that. http://dev.w3.org/csswg/css3-flexbox/#flex-function Here is an alternate algorithm for your consideration: CSSParserValue* value = m_valueList->current(); CSSParserValueList* args = value->function->args.get(); if (!equalIgnoringCase(value->function->name, "-webkit-flex(") || !args) return false; RefPtr<CSSFlexValue> flex; if (args->size() > 3) return false; double positive = -1; double negative = -1; CSSPrimativeValue* size; while (CSSParserValue* arg = args->current()) { // First check to see if we have a raw number (no units). if (validUnit(arg, FNumber | FNonNeg, m_strict)) { if (positive == -1) positive = arg->fValue; else if (negative == -1) negative = arg->fValue; else { // flex() contains at most 2 non-size values. return false; } } else if (arg->id == CSSValueAuto || validUnit(arg, FLength | FPercent | FNonNeg, m_strict)) size = parseValidPrimitive(arg->id, arg); } else { // This was not a valid arg for flex() we just bail. return false; } arg->next(); } RefPtr<CSSFlexValue> flex = CSSFlexValue::create(arg2->fValue, 0, parseValidPrimitive(arg1->id, arg1)); addProperty(propId, flex.release(), important); m_valueList->next(); // Tell the parser we're done with flex()? return true;

Created attachment 98064 [details] Patch

Created attachment 98068 [details] Patch

Updated to use the alternate algorithm proposed by Eric since it's a bit shorter and easier to read. I also added some more test cases (0 args and 4 args) and added a helper method for the static cast (not sure if I understood that properly).

Comment on attachment 98068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98068&action=review Asside from the question of 0 0 0 parsing, I think this is ready to go. I feel like I sent you on a bit of a yak-shave, for which I apologize. :( > Source/WebCore/css/CSSParser.cpp:786 > +inline CSSPrimitiveValue::UnitTypes parserUnitToCSSPrimitiveValueUnit(int unit) > +{ > + return static_cast<CSSPrimitiveValue::UnitTypes>(unit); > +} Nah, what I was going for here was that we should just have a createPrimativeValue() helper on CSSParser, which does the whole shebang: return primitiveValueCache()->createValue(value->string, parserUnitToCSSPrimitiveValueUnit(value->unit)); Maybe takes an enum? CSSParser::createPrimativeValue(value, String) CSSParser::createPrimativeValue(value, Float) Maybe just is overloaded and is fed the two value->string and value->unit values? CSSParser::createPrimativeValue(value->string, value->unit) CSSParser::createPrimativeValue(value->fValue, value->unit) Not sure. In either case, it's sorta out of scope for this patch, so you can go back to your non-inline version if you prefer. Thanks for the attempt. > Source/WebCore/css/CSSParser.cpp:4790 > + double positiveFlex = -1; Were I thinking about it when I wrote up the example, I might have defined a const double invalidValue = -1, or unsetValue, or whatever nice name, and then used that in place of -1 throught the rest of the function. > Source/WebCore/css/CSSParser.cpp:4800 > + else if (!arg->fValue) { I know webkit shies away from == 0, but I wonder if that's apprpriate here? (I can never remember if we have to worry about 0 vs. -0 for floats... > Source/WebCore/css/CSSParser.cpp:4801 > + // flex() only allows a size of 0 if the positive and negative flex values have already been set. Does this really work then? It seems that (0 0 0) will be valid with this logic? Since the earlier checks don't check arg->fValue? > Source/WebCore/css/CSSParser.cpp:4804 > + } else > + return false; We should probably document why this returns false here. Specifically it seems that when we see a number and we've already seen two other numbers that's an invalid flex. e.g. flex(1 1 1). > Source/WebCore/css/CSSParser.cpp:4805 > + } else if (!preferredSize.get() && (arg->id == CSSValueAuto || validUnit(arg, FLength | FPercent | FNonNeg, m_strict))) No need for the .get(), RefPtr is smart enough to overload operator bool. > Source/WebCore/css/CSSParser.cpp:4818 > + if (!preferredSize.get()) No .get() is necessary. > Source/WebCore/css/CSSParser.cpp:4823 > + m_valueList->next(); We should probably add a comment here.

Created attachment 98077 [details] Patch

Comment on attachment 98077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98077&action=review LGTM! > Source/WebCore/css/CSSParser.cpp:788 > + return primitiveValueCache()->createValue(value->string, (CSSPrimitiveValue::UnitTypes) value->unit); We don't generally use C-style casts. But you can land this as-is, especially if you have any plans to clean up this primativeValueCache()->createValue repetative nonsense throughout this file. :)

You reviewed quickly :) For those who care, the spec says: "If the first two values are non-negative numbers and the third value is ‘0’, the positive flexibility is set to the first value, the negative flexibility is set to the second value, and the preferred size is set to ‘0px’." So 0 without units is valid as the third value.

Created attachment 98082 [details] Patch for landing

Comment on attachment 98082 [details] Patch for landing Clearing flags on attachment: 98082 Committed r89396: <http://trac.webkit.org/changeset/89396>

All reviewed patches have been landed. Closing bug.