[webkit-reviews] review denied: [Bug 16662] CSS3: Implement calc() : [Attachment 105581] added new files to EFL compile

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 30 15:12:40 PDT 2011


Eric Seidel <eric at webkit.org> has denied Mike Lawther
<mikelawther at chromium.org>'s request for review:
Bug 16662: CSS3: Implement calc()
https://bugs.webkit.org/show_bug.cgi?id=16662

Attachment 105581: added new files to EFL compile
https://bugs.webkit.org/attachment.cgi?id=105581&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=105581&action=review


I don't feel like I gave you a great review here.  Most of this has been purged
from my memory by now.	There are a few style issues, and some possible leaks. 
I'm not sure who is your best hope for reviewer here.  Your'e in sydney, right?
 You might pick on Luke to get him to sanity check your CSS changes as he's
been in that file as of late.  Ideally beth dakin would look at this, but it
may be hard to get some of her time.

> Source/WebCore/css/CSSCalcValue.cpp:52
> +static CalcCategory unitCategory(CSSPrimitiveValue::UnitTypes type)

Don't we have a header with all these conversion functions to/from
CSSPrimativeValue somewhere?

> Source/WebCore/css/CSSCalcValue.cpp:71
> +    default:
> +	   return CalcOther;

Do we need this default?  Normally we try and avoid them to let the compiler
warn when we forget a case.

> Source/WebCore/css/CSSCalcValue.cpp:79
> +    static PassRefPtr<CSSCalcPrimitiveValue> create(CSSPrimitiveValue*
value, bool isInt)

Ick.  Is isInt normally part of the signature?	That really should be an enum.

> Source/WebCore/css/CSSCalcValue.cpp:86
> +	   return !m_value->getDoubleValue();

Normally we don't have "get" in method names, but I suspect CSSPrimativeValue
is just old (not your fault here, obviously).

Will this do the right thing for NaN?

> Source/WebCore/css/CSSCalcValue.cpp:94
> +    PassOwnPtr<CalcValue> toCalcValue(RenderStyle* style, RenderStyle*
rootStyle, double zoom) const

Should this be a create function on CalcValue?	to* functions are normally
static conversions that don't return Pass* pointers, but this is OK too.

> Source/WebCore/css/CSSCalcValue.cpp:110
> +	   default:
> +	       return nullptr;

Please no default:

> Source/WebCore/css/CSSCalcValue.cpp:120
> +    explicit CSSCalcPrimitiveValue(CSSPrimitiveValue* value, bool isInt)
> +	   : m_value(value)		    
> +    {
> +	   m_category =
unitCategory((CSSPrimitiveValue::UnitTypes)value->primitiveType());
> +	   m_isInt = isInt;
> +    }

Ah, so this is your cusomt signature.  Please make this isInt thing an enum.

> Source/WebCore/css/CSSCalcValue.cpp:163
> +	   switch (op) {
> +	   case '+':
> +	   case '-':		    
> +	       if (leftCategory == rightCategory)    
> +		   newCategory = leftCategory;
> +	       else if (leftCategory != CalcNumber && rightCategory !=
CalcNumber)
> +		   newCategory = CalcPercent;
> +	       else    
> +		   return 0;
> +	       break;
> +		   
> +	   case '*':
> +	       if (leftCategory != CalcNumber && rightCategory != CalcNumber) 
> +		   return 0;
> +	       
> +	       if (leftCategory == CalcNumber)
> +		   newCategory = rightCategory;
> +	       else 
> +		   newCategory = leftCategory;
> +	       break;
> +
> +	   case '/':
> +	   case '%':
> +	       if (rightCategory != CalcNumber || rightSide->isZero())
> +		   return 0;
> +	       newCategory = leftCategory;
> +	       break;
> +
> +	   default:
> +	       ASSERT_NOT_REACHED();
> +	       return 0;

This feels like a newCatagoryForOperator helper function, which you call, and
then check that CalcCategory is a valid value?

> Source/WebCore/css/CSSCalcValue.cpp:177
> +	   return "(" + m_leftSide->cssText() + m_operator +
m_rightSide->cssText() + ")";

return makeString(foo, bar, baz) is better.

> Source/WebCore/css/CSSCalcValue.cpp:183
> +	   if (!left.get())

You don't need .get(), there is a bool operator on OwnPtr already.

> Source/WebCore/css/CSSCalcValue.cpp:197
> +	   ASSERT(op == '+' || op == '-' || op == '*' || op == '/' || op ==
'%');  

Should we make op an Enum instad?  WE could even give that enum the same values
as these strings?  Unclear if that woudl be better or not. Certainly would make
some of these signatures cleaner.

> Source/WebCore/css/CSSCalcValue.cpp:293
> +    struct Value {
> +	   RefPtr<CSSCalcValue> value;
> +    };

I'm confused by why we have this struct.  It could also just be a type-def if
you wanted to save typing RefPtr everywhere.

> Source/WebCore/css/CSSCalcValue.cpp:298
> +	       return 0;

As an Enum this could return nice semantic enums, like InvalidOperator (which
could equal 0).

> Source/WebCore/css/CSSCalcValue.h:73
> +    void setIsInt(bool isInt) { m_isInt = isInt; }

It's not immediately clear to me why this is externally mutable, but OK.

> Source/WebCore/css/CSSParser.cpp:702
> +	   break;	     

typo.

> Source/WebCore/css/CSSPrimitiveValue.cpp:105
> +    default:
> +	   return CSSPrimitiveValue::CSS_UNKNOWN;

Can we move this out of the default case?  Again, we tend to avoid default: in
WebCore where possible.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:335
> +		     return;

indent.

> Source/WebCore/css/CSSStyleSelector.cpp:5223
> +    return *new Length(CSSPrimitiveValue::create(mm,
CSSPrimitiveValue::CSS_MM)->computeLength<Length>(style(),
m_rootElementStyle));

How does this ever get deleted?

> Source/WebCore/css/CSSStyleSelector.cpp:5228
> +    return *new Length(CSSPrimitiveValue::create(inch,
CSSPrimitiveValue::CSS_IN)->computeLength<Length>(style(),
m_rootElementStyle));

Leak?

> Source/WebCore/platform/CalcValue.cpp:59
> +    switch (op) {
> +    case '+':
> +	   m_operator = CalcAdd;
> +	   return;
> +    case '-':
> +	   m_operator = CalcSubtract;
> +	   return;
> +    case '*':
> +	   m_operator = CalcMultiply;
> +	   return;
> +    case '/':
> +	   m_operator = CalcDivide;
> +	   return;
> +    case '%':
> +	   m_operator = CalcMod;
> +	   return;
> +    }
> +    ASSERT_NOT_REACHED();

You want a helper function here.  m_operator =

> Source/WebCore/platform/Length.cpp:169
> +    : m_quirk(false), m_type(Calculated), m_isFloat(false)

one per line.


More information about the webkit-reviews mailing list