[webkit-reviews] review denied: [Bug 45249] Add cubic curve classifier : [Attachment 66608] 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 45249: Add cubic curve classifier
https://bugs.webkit.org/show_bug.cgi?id=45249

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
Looks good overall, r- for the nits below.

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

> WebCore/platform/graphics/gpu/LoopBlinnClassifier.cpp:42
> +LoopBlinnClassifier::Result LoopBlinnClassifier::classify(const FloatPoint&
c0,
> +							     const FloatPoint&
c1,
> +							     const FloatPoint&
c2,
> +							     const FloatPoint&
c3)
> +{
> +    // Consult the chapter for the definitions of the following
> +    // (terse) variable names.
Could we add a link to the algorithm itself somewhere?

> WebCore/platform/graphics/gpu/LoopBlinnClassifier.cpp:74
> +    float discr = d1 * d1 * term0;
This should be discriminant.

> WebCore/platform/graphics/gpu/LoopBlinnClassifier.cpp:84
> +    d1 = roundToZero(d1);
> +    d2 = roundToZero(d2);
> +    d3 = roundToZero(d3);
> +    discr = roundToZero(discr);
Have these declare the namespace explicitly.

> WebCore/platform/graphics/gpu/LoopBlinnClassifier.h:63
> +	   float d1() const { return m_d1; }
> +	   float d2() const { return m_d2; }
> +	   float d3() const { return m_d3; }
Please add a comment indicating what these mean.