Bug 57082

Created attachment 86890 [details] Patch

This is another patch sliced off the master bug 16662. See the latest patch on that bug to see this patch is its full context. This implements the parsing stage of calc, where the CSSParserValues are turned into data structures. calc() still doesn't 'work' with this patch, so no test outputs have changed. The functional change to WebKit is that the parsing happens, but nothing happens with the data.

Comment on attachment 86890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86890&action=review I think this is good. I'm glad we sat down and went through it. I look forward to reviewing the next round. > Source/WebCore/ChangeLog:12 > + No new tests. (OOPS!) the cq will get mad at us for OOPS. > Source/WebCore/css/CSSCalcValue.cpp:48 > + return CalcValue::UNumber; Should this just be a constructor for CalcValue to allow transparent construction? (Like how we do for CSSPrimativeValues?) Or at least be a member on CalcValue so to avoid the copy/paste of CalcValue:: > Source/WebCore/css/CSSCalcValue.cpp:62 > + default: > + return CalcValue::UOther; We generally avoid defaults if we're going to use switch statements, so the compiler can help us. :) Otherwise we'd just use ifs. > Source/WebCore/css/CSSCalcValue.cpp:67 > +class CSSCalcPrimitiveValue : public CSSCalcValue { You should re-evaluate whether you want your classes to actually be subclasses of CSSValue. You really only need one CSSValue subclass, your CSSCalcValue, which to be teh resulting parsedValue from CSSParser::parseValue(). But the implementation details of the resulting calc-subtree, need not use the CSSValue system at all (although they can, up to you!). But if you don't ever to expose these to teh web, you should document such. It's also unclear to me what value this CSSCalcPrimativeValue class adds over CSSCalcValue. > Source/WebCore/css/CSSCalcValue.cpp:112 > + else Generally we don't else after return. > Source/WebCore/css/CSSCalcValue.cpp:154 > + String result = "("; > + result.append(m_leftSide->cssText()); > + result.append(m_operator); > + result.append(m_rightSide->cssText()); > + result.append(')'); > + return result; I believe makeString is the new hotness. > Source/WebCore/css/CSSCalcValue.cpp:179 > + , m_list() Not needed (implicit). > Source/WebCore/css/CSSCalcValue.cpp:213 > +#define CHECK_DEPTH_AND_INDEX \ Macros are always sad times. > Source/WebCore/css/CSSCalcValue.cpp:240 > + CSSCalcMinMaxValueList* result = new CSSCalcMinMaxValueList(type); Should be in an RefPtr immediately and then later released. > Source/WebCore/css/CSSCalcValue.cpp:254 > + char operatorValue(CSSParserValueList* tokens, unsigned index) Really? Not an enum? Can we cast to one? > Source/WebCore/css/CSSCalcValue.cpp:275 > + CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value.get()); static_ptr_cast I think is the function you want. > Source/WebCore/css/CSSCalcValue.h:54 > + CalcValue::UnitCategory category() const { return m_category; } I think we want to get rid of the CalcValue class "namespace". > Source/WebCore/css/CSSCalcValue.h:58 > + bool isInt() const { return m_isInt; } > + void setIsInt(bool isInt) { m_isInt = isInt; } How is this different from the Units type? > Source/WebCore/css/CSSParser.cpp:132 > +static bool isCalc(CSSParserValue* value) Slightly deceptive that this also does min/max. > Source/WebCore/css/CSSParser.cpp:483 > + if (isCalc(value) && parseCalc(value)) { This seems like the right way to go, but we need to explain what we're doing here. Also, if the parseCalc fails, I don't think we want to fall through to the other type checks, so that could be moved inside the if, no? We just need to explain here, that calc() intentionally mascarades as a primative value during parse time, and note that we have special code in the validPrimitive case at the end of parseValue() which knows how to create the CSSCalcValue instead of CSSPrimativeValue correctly. An alternate (perhaps better?) solution would be to check isCalc at the top of parseValue() and pre-parse the calc then. Then here, we can check the m_parsedCalc in a smaller if... > Source/WebCore/css/CSSParser.cpp:499 > + b = (unitflags & FLength); > + break; > + case CalcValue::UPercent: > + b = (unitflags & FPercent); > + break; > + case CalcValue::UNumber: > + b = (unitflags & FNumber); > + if (!b && (unitflags & FInteger) && m_parsedCalc->isInt()) > + b = true; > + break; > + default: > + break; > + } > + } else { I would just have done this as an early return to avoid the indent. It's unclear to me why calc units are special here... > Source/WebCore/css/CSSParser.cpp:500 > + switch (value->unit) { How do other function-based values work so that they avoid having to change validUnit like this? Like url() or counter() > Source/WebCore/css/CSSParser.cpp:632 > + ASSERT(!m_parsedCalc.get()); No need for .get() I don't think. RefPtr will transparently convert to bools like normal pointers do. We should document here why we're doing this. That we have to use a member so that isValidUnit can do the right thing for calc. // Note: m_parsedCalc is used to pass the calc value to isValidUnit and then cleared at the end of this function. // FIXME: This is to avoid having to pass parsedCalc to all isValidUnit callers. > Source/WebCore/css/CSSParser.cpp:1898 > + m_parsedCalc.release(); A bit odd to ignore the return value of m_parsedCalc, but I understand why you need to, you might want to comment here to. > Source/WebCore/css/CSSParser.cpp:1901 > } We could even ASSERT m_parsedCalc is empty before we return? > Source/WebCore/css/CSSParser.h:357 > - static bool validUnit(CSSParserValue*, Units, bool strict); > + bool validUnit(CSSParserValue*, Units, bool strict); ? > Source/WebCore/platform/CalcValue.h:39 > +class CalcValue { > +public: > + enum UnitCategory { > + UNumber, Normally we just have headers with Enums, like TextDirection.h, or UnicodeBidi.h, etc.

Created attachment 120404 [details] Patch

Comment on attachment 120404 [details] Patch Hi Eric - thanks for your review. I've addressed your comments, and calc has grown to cover a lot more cases in the meantime :) The only functional change in this patch is that RGB and HSL values can now be computed using calc. This is because they must evaluate to simple integers at parse time, and this also demonstrates the expression evaluation working.

Created attachment 120414 [details] Patch

Comment on attachment 120414 [details] Patch Attachment 120414 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10939152 New failing tests: css3/calc/padding.html css3/calc/table-calcs.html css3/calc/margin.html css3/calc/border.html

Created attachment 120912 [details] Updated to patch against tip

Comment on attachment 120912 [details] Updated to patch against tip Attachment 120912 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11075149 New failing tests: css3/calc/padding.html css3/calc/table-calcs.html css3/calc/margin.html css3/calc/border.html

Created attachment 121063 [details] modified some tests to pass on cr-linux

Created attachment 124071 [details] Patch

Hi Hyatt - this is the parsing stage for CSS calc. This is a slice from the full patch (at http://webkit.org/b/16662). Can you please take a look? Thanks!

Created attachment 124130 [details] Patch removed min/max parsing to trim patch down

Comment on attachment 124130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124130&action=review My high level comment (although I don't feel super strongly about it) is that perhaps "Calc" should become "Calculation" to disambiguate it from the specific case of "-webkit-calc." I am assuming calc expressions, calc values, etc. all apply to min/max as well, so maybe it would be better terminology to use the more generic "Calculation" when dealing with a class that applies to all three. Save the abbreviated form, "calc", for when you're talking specifically about -webkit-calc. The values and units draft refers to calc, min, and max as "Calculations" e.g., "9.1. Calculations: ...", so it seems like this might be good terminology to use for referring generically to all future calculations as well. > Source/WebCore/css/CSSCalcValue.cpp:307 > + RefPtr<CSSCalcExpressionNode> expression = 0; You don't need to say " = 0" here. RefPtr will start off null. > Source/WebCore/css/CSSCalcValue.cpp:316 > + else if (equalIgnoringCase(name, "-webkit-min(")) > + // FIXME calc (http://webkit.org/b/16662) Add parsing for min here > + expression = 0; > + else if (equalIgnoringCase(name, "-webkit-max(")) > + // FIXME calc (http://webkit.org/b/16662) Add parsing for max here > + expression = 0; I would just remove all this code and replace with a single FIXME. Seems silly to redundantly re-initialize to 0. > Source/WebCore/css/CSSParser.cpp:732 > + if (!parseCalc(value)) > + return false; > + > + switch (m_parsedCalc->category()) { > + case CalcLength: > + b = (unitflags & FLength); > + break; > + case CalcPercent: > + b = (unitflags & FPercent); > + // FIXME calc (http://webkit.org/b/16662): test FNonNeg here, eg > + // if (b && (unitflags & FNonNeg) && m_parsedCalc->doubleValue() < 0) > + // b = false; > + break; > + case CalcNumber: > + b = (unitflags & FNumber); > + if (!b && (unitflags & FInteger) && m_parsedCalc->isInt()) > + b = true; > + // FIXME calc (http://webkit.org/b/16662): test FNonNeg here, eg > + // if (b && (unitflags & FNonNeg) && m_parsedCalc->doubleValue() < 0) > + // b = false; > + break; > + case CalcPercentLength: > + b = (unitflags & FPercent) && (unitflags & FLength); > + break; > + case CalcPercentNumber: > + b = (unitflags & FPercent) && (unitflags & FNumber); > + break; > + case CalcOther: > + break; > + } > + // TODO: handle FNonNeg > + if (!b) > + m_parsedCalc.release(); > + return b; Put this in a separate function please, e.g., validCalcUnit or something. > Source/WebCore/css/CSSParser.cpp:744 > + break; Extra whitespace it looks like? > Source/WebCore/css/CSSParser.cpp:4939 > +bool CSSParser::isCalcMinMax(CSSParserValue* value) I don't like the name "isCalcMinMax" because it sounds like you're testing just for webkit-min and webkit-max calc values instead of for all three. The CSS spec refers to this section (covering calc, min, max) as "calculations" so perhaps that could be our terminology for covering all three cases. So how about "isCalculationValue" instead? > Source/WebCore/css/CSSValue.h:79 > + bool isCalcValue() const {return m_classType == CalcClass; } I think I'd prefer "isCalculationValue" as I mentioned above, since isCalc sounds more like it only covers -webkit-calc and not -webkit-min/max. > Source/WebCore/css/CSSValue.h:130 > + CalcClass, CalculationClass

Created attachment 124270 [details] Patch

Comment on attachment 124130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124130&action=review Thanks for the review Hyatt. I've done your suggestions, plus I also renamed the files using Calc->Calculation in line with your suggestion that 'Calculation' be used to cover all three of calc,min,max. Those files will ultimately contain code to deal with all three. >> Source/WebCore/css/CSSCalcValue.cpp:307 >> + RefPtr<CSSCalcExpressionNode> expression = 0; > > You don't need to say " = 0" here. RefPtr will start off null. Done. >> Source/WebCore/css/CSSCalcValue.cpp:316 >> + expression = 0; > > I would just remove all this code and replace with a single FIXME. Seems silly to redundantly re-initialize to 0. Done. >> Source/WebCore/css/CSSParser.cpp:732 >> + return b; > > Put this in a separate function please, e.g., validCalcUnit or something. Done. >> Source/WebCore/css/CSSParser.cpp:744 >> + break; > > Extra whitespace it looks like? Oops. Fixed. >> Source/WebCore/css/CSSParser.cpp:4939 >> +bool CSSParser::isCalcMinMax(CSSParserValue* value) > > I don't like the name "isCalcMinMax" because it sounds like you're testing just for webkit-min and webkit-max calc values instead of for all three. The CSS spec refers to this section (covering calc, min, max) as "calculations" so perhaps that could be our terminology for covering all three cases. So how about > > "isCalculationValue" > > instead? Sounds good. Done. >> Source/WebCore/css/CSSValue.h:79 >> + bool isCalcValue() const {return m_classType == CalcClass; } > > I think I'd prefer "isCalculationValue" as I mentioned above, since isCalc sounds more like it only covers -webkit-calc and not -webkit-min/max. Done. >> Source/WebCore/css/CSSValue.h:130 >> + CalcClass, > > CalculationClass Done.

Comment on attachment 124270 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124270&action=review r=me, with a few renaming suggestions. Not going to cq+ as a result of that. > Source/WebCore/css/CSSCalculationValue.cpp:87 > + static PassRefPtr<CSSCalcPrimitiveValue> create(CSSPrimitiveValue* value, bool isInt) isInteger would read better. > Source/WebCore/css/CSSCalculationValue.cpp:99 > + explicit CSSCalcPrimitiveValue(CSSPrimitiveValue* value, bool isInt) Ditto. > Source/WebCore/css/CSSCalculationValue.cpp:113 > + {CalcNumber, CalcOther, CalcPercentNumber, CalcPercentNumber, CalcOther}, > + {CalcOther, CalcLength, CalcPercentLength, CalcOther, CalcPercentLength}, > + {CalcPercentNumber, CalcPercentLength, CalcPercent, CalcPercentNumber, CalcPercentLength}, > + {CalcPercentNumber, CalcOther, CalcPercentNumber, CalcPercentNumber, CalcOther}, > + {CalcOther, CalcPercentLength, CalcPercentLength, CalcOther, CalcPercentLength}, Minor style nit: I think you should have spaces before and after the { } on each line, e.g.,: { CalcNumber, CalcOther, CalcPercentNumber, CalcPercentNumber, CalcOther }, > Source/WebCore/css/CSSCalculationValue.h:64 > + bool isInt() const { return m_isInt; } I think isInteger would read better. > Source/WebCore/css/CSSCalculationValue.h:75 > + bool m_isInt; Ditto. > Source/WebCore/css/CSSParser.cpp:5213 > + CSSParser* m_parent; I don't like using the terminology "parent" here. Why not just m_parser? > Source/WebCore/css/CSSParser.cpp:5602 > + : m_parent(parent) Same here. Would prefer m_parser. > Source/WebCore/css/CSSParser.cpp:5720 > + : m_parent(parent) Same here.

Comment on attachment 124270 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124270&action=review Thanks for the review Hyatt. The 'isInt' naming was carried over from CSSParserValue, but I'm happy to change it. >> Source/WebCore/css/CSSCalculationValue.cpp:87 >> + static PassRefPtr<CSSCalcPrimitiveValue> create(CSSPrimitiveValue* value, bool isInt) > > isInteger would read better. Done. >> Source/WebCore/css/CSSCalculationValue.cpp:99 >> + explicit CSSCalcPrimitiveValue(CSSPrimitiveValue* value, bool isInt) > > Ditto. Done. >> Source/WebCore/css/CSSCalculationValue.cpp:113 >> + {CalcOther, CalcPercentLength, CalcPercentLength, CalcOther, CalcPercentLength}, > > Minor style nit: I think you should have spaces before and after the { } on each line, e.g.,: > > { CalcNumber, CalcOther, CalcPercentNumber, CalcPercentNumber, CalcOther }, Done. >> Source/WebCore/css/CSSCalculationValue.h:64 >> + bool isInt() const { return m_isInt; } > > I think isInteger would read better. Done. >> Source/WebCore/css/CSSCalculationValue.h:75 >> + bool m_isInt; > > Ditto. Done. >> Source/WebCore/css/CSSParser.cpp:5213 >> + CSSParser* m_parent; > > I don't like using the terminology "parent" here. Why not just m_parser? Yep, that sounds better. Done. >> Source/WebCore/css/CSSParser.cpp:5602 >> + : m_parent(parent) > > Same here. Would prefer m_parser. Done. >> Source/WebCore/css/CSSParser.cpp:5720 >> + : m_parent(parent) > > Same here. Done.

Created attachment 124376 [details] Patch for landing

Comment on attachment 124376 [details] Patch for landing Clearing flags on attachment: 124376 Committed r106166: <http://trac.webkit.org/changeset/106166>

All reviewed patches have been landed. Closing bug.