[webkit-reviews] review denied: [Bug 45251] Add math utilities for cubic curve processing : [Attachment 67128] Revised patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Kenneth Russell
<kbr at google.com>'s request for review:
Bug 45251: Add math utilities for cubic curve processing
https://bugs.webkit.org/show_bug.cgi?id=45251

Attachment 67128: Revised patch
https://bugs.webkit.org/attachment.cgi?id=67128&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
Almost there!

View in context:
https://bugs.webkit.org/attachment.cgi?id=67128&action=prettypatch

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:80
> +bool edgeAgainstTriEdges(const FloatPoint& v0,
nit: edgeAgainstTriEdges -> edgeAgainstTriangleEdges() or something else
without an abbreviation.

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:335
> +    if (numerator < 0) {
> +	   // Make the "numerator >= denominator" check below work.
> +	   numerator = -numerator;
> +	   denominator = -denominator;
> +    }
> +    if (!numerator || !denominator || numerator >= denominator)
nit: maybe change the check to fabs(numerator) >= fabs(denominator) instead of
flipping?

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:378
> +    float* r = roots;
> +    if (safeUnitDivide(q, a, *r))
> +	   ++r;
> +    if (safeUnitDivide(c, q, *r))
> +	   ++r;
> +    if (r - roots == 2) {
> +	   // Seemingly have two roots. Check for equality and sort.
> +	   if (roots[0] == roots[1])
> +	       return 1;
> +	   if (roots[0] > roots[1])
> +	       std::swap(roots[0], roots[1]);
> +    }
> +    return r - roots;
This section looks worlds better now - thanks for tackling it.	One more thing,
thought: instead of having r be a float* and using pointer math, this would be
done better by keeping a numRoots count and using that to index into roots[]. 
The pointer math logic is tricky and the result of (float* - float*) will not
necessarily fit into an int on a system with 64 bit pointers and 32 bit ints,
so this may generate warnings.