[webkit-reviews] review granted: [Bug 79621] CSS3 calc: mixed absolute/percentages work for width, height, margin and padding : [Attachment 130543] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has granted Mike Lawther
<mikelawther at chromium.org>'s request for review:
Bug 79621: CSS3 calc: mixed absolute/percentages work for width, height, margin
and padding
https://bugs.webkit.org/show_bug.cgi?id=79621

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

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=130543&action=review


r=me based on prior partial approval from others, and the changes looking good
to me.

It's great that you've sprinkled FIXMEs. Makes it much easier to understand the
patch.

> Source/WebCore/css/CSSCalculationValue.cpp:129
> +	       {
> +		   Length length =
CSSStyleSelector::convertToFloatLength(m_value.get(), style, rootStyle, zoom);
> +		   return adoptPtr(new CalcExpressionLength(length));
> +	       }

I'd write this as a single line.

> Source/WebCore/platform/Length.h:246
>      bool isZero() const 
>      {
>	   ASSERT(!isUndefined());
> -	   return m_isFloat ? !m_floatValue : !m_intValue;
> +	   return isCalculated() ? false : m_isFloat ? !m_floatValue :
!m_intValue;
> +    }
> +    bool isPositive() const
> +    {
> +	   return isUndefined() ? false : isCalculated() ? true :
getFloatValue() > 0;
> +    }
> +    bool isNegative() const
> +    {
> +	   return isUndefined() ? false : isCalculated() ? false :
getFloatValue() < 0;
>      }

I feel like we can do this without abusing ternary operators.

> Source/WebCore/platform/Length.h:315
> +    void incrementCalculatedRef() const;
> +    void decrementCalculatedRef() const;    

I don't understand why these are const.