[webkit-reviews] review denied: [Bug 57082] CSS calc parsing stage : [Attachment 86890] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Mike Lawther
<mikelawther at chromium.org>'s request for review:
Bug 57082: CSS calc parsing stage
https://bugs.webkit.org/show_bug.cgi?id=57082

Attachment 86890: Patch
https://bugs.webkit.org/attachment.cgi?id=86890&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.