[webkit-reviews] review canceled: [Bug 16662] CSS3: Implement calc() : [Attachment 86745] version after lexer/grammar patch has landed

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has canceled 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 86745: version after lexer/grammar patch has landed
https://bugs.webkit.org/attachment.cgi?id=86745&action=review

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

We're going to deal with this patch in pieces on other bugs.

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

What is this used by?  Seems odd.

> Source/WebCore/css/CSSStyleSelector.cpp:4345
> +		   OwnPtr<CalcValue> calcValue =
primitiveValue->getCalcValue()->toCalcValue(style(), m_rootElementStyle,
zoomFactor);

WebCore avoids "get" in function names, normally.

> Source/WebCore/platform/CalcValue.cpp:43
> +    ASSERT(op == '+' || op == '-' || op == '*' || op == '/' || op == '%');

This goes away with an enum.

> Source/WebCore/platform/CalcValue.cpp:50
> +    switch (m_operator) {

Yeah, just an enum for m_operator and then the compiler helps you! :)

> Source/WebCore/platform/CalcValue.cpp:103
> +    if (m_type == MIN) {
> +	   retval = m_list[0]->evaluate(maxValue);
> +	   for (unsigned i = 1; i < m_list.size(); ++i) {
> +	       float current = m_list[i]->evaluate(maxValue);
> +	       if (current < retval)
> +		   retval = current;
> +	   }
> +    } else {
> +	   retval = m_list[0]->evaluate(maxValue);
> +	   for (unsigned i = 1; i < m_list.size(); ++i) {
> +	       float current = m_list[i]->evaluate(maxValue);
> +	       if (current > retval)
> +		   retval = current;
> +	   }
> +    }

I'm not sure what this copy/paste code is trying to do.

> Source/WebCore/platform/CalcValue.h:48
> +    enum UnitCategory {
> +	   UNumber,
> +	   UPercent,
> +	   ULength,
> +	   UOther
> +    };

I might still pull this enum out, even though there is a bunch of other things
in this class.

> Source/WebCore/platform/CalcValue.h:80
> +    virtual float evaluate(float maxValue) const

What's maxValues?

> Source/WebCore/platform/CalcValue.h:101
> +class CalcMinMaxValueList : public CalcValue {

It's possible that we coudl avoid having to expose all this CalcValue stuff to
platform, if we could design a system whereby we pre-process a calc tree down
to a Length in the form: ax + by + c where x and y are containing block width
and font size, respecitively.  Not sure if such is possible, but might be worth
thinking about.

> Source/WebCore/platform/Length.cpp:255
> +float Length::calculatedCalcFloatValue(int maxValue) const

If you go down this hash road, I might put i in a separate class.  A class
which Length can have a static member of.  Or some function in length.cpp has
it as a static function local.