[webkit-reviews] review denied: [Bug 121020] Crash on shape-outside when using calc() : [Attachment 212805] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 121020: Crash on shape-outside when using calc()
https://bugs.webkit.org/show_bug.cgi?id=121020

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=212805&action=review


Thanks for all the work Hans! Great back porting patch. And thanks for cleaning
up the source code as well. Just some snippets inside. Really just minor things
but I would like to look over the patch again.

> LayoutTests/css3/calc/cssom-expected.txt:12
> -100px * (1 + 2 * 3 - 4 / 5) => calc(100px * ((1 + (2 * 3)) - (4 / 5)))
> -(100px) + 200px => calc(100px + 200px)
> +100px * (1 + 2 * 3 - 4 / 5) => calc(620px)
> +(100px) + 200px => calc(300px)

Do we have nested calc's as well? This is important if you animate from one
calc function to a real value on CSS Shapes. So we need a test for that. I
believe a small test calc(5px + calc(3px * 5%)) is sufficient enough.

> Source/WebCore/css/BasicShapeFunctions.cpp:41
> +static PassRefPtr<CSSPrimitiveValue> convertToCSSPrimitiveValue(const
RenderStyle* style, const Length& length)

This function actually makes the calls longer. Compare the both:
 
pool.createValue(rectangle->x(), style);
convertToCSSPrimitiveValue(style, rectangle->x())

It doesn't look like it is worth it and you have to look at another place to
see what is going on as well. I suggest removing this function again.

> Source/WebCore/css/BasicShapeFunctions.cpp:121
> +    return value->convertToLength<CalculatedConversion |
FixedIntegerConversion | FixedFloatConversion | PercentConversion |
ViewportPercentageConversion>(style, rootStyle, style->effectiveZoom());

I think CalculatedConversion is lined up after PercentConversion every where
else. Just a snippet, but makes search and replace easier later when we
refactor the code :D

> Source/WebCore/css/CSSCalculationValue.cpp:273
> +

One newline too many.

> Source/WebCore/css/CSSCalculationValue.cpp:292
> +	   ASSERT_NOT_REACHED();

We already have the assertion in the switch, you'll never reach it.

> Source/WebCore/css/CSSCalculationValue.cpp:326
> +/* CalcNumber */	   { CalcNumber,	CalcOther,	  
CalcPercentNumber, CalcPercentNumber, CalcOther },
> +/* CalcLength */	   { CalcOther, 	CalcLength,	  
CalcPercentLength, CalcOther,	      CalcPercentLength },
> +/* CalcPercent */	   { CalcPercentNumber, CalcPercentLength, CalcPercent,
      CalcPercentNumber, CalcPercentLength },
> +/* CalcPercentNumber */ { CalcPercentNumber, CalcOther,	  
CalcPercentNumber, CalcPercentNumber, CalcOther },
> +/* CalcPercentLength */ { CalcOther, 	CalcPercentLength,
CalcPercentLength, CalcOther,	      CalcPercentLength },

Do you mind if you add these comments at the end of each line?

> Source/WebCore/css/CSSCalculationValue.cpp:362
> +    // Not testing for actual integer values.

I don't understand this comment.

> Source/WebCore/css/CSSCalculationValue.cpp:383
> +    static PassRefPtr<CSSCalcExpressionNode>
createSimplified(PassRefPtr<CSSCalcExpressionNode> leftSide,
PassRefPtr<CSSCalcExpressionNode> rightSide, CalcOperator op)

Wow! This function is massive! Do we have tests for special cases like calc(a +
b / calc(d * calc(e + f))) ?

> Source/WebCore/css/CSSCalculationValue.cpp:748
> +	   // FIXME(crbug.com/269320): Create a CSSCalcExpressionNode
equivalent of CalcExpressionBlendLength.

Hmmm. I am not sure if we should leave that in WebKit source code. I think we
have a WebKit bug for it already. Can you search it and replace the comment
with the webkit bug please?

> Source/WebCore/css/CSSCalculationValue.cpp:795
> +    ASSERT_NOT_REACHED();

I am unsure if we should leave these assertions. But maybe we should if someone
forgets to add an return. But then we should remove the assertion and return in
case Undefined:. (Same with the other switches)