[webkit-reviews] review denied: [Bug 45250] Add cubic texture coordinate computation : [Attachment 66609] 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 45250: Add cubic texture coordinate computation
https://bugs.webkit.org/show_bug.cgi?id=45250

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
r- for nits again, looks good overall.

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

> WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:36
> +using namespace LoopBlinnMathUtils;
Check if this is being used any more and remove if not.

> WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:38
> +void LoopBlinnTextureCoords::Compute(const LoopBlinnClassifier::Result& c,
Should be 'compute'.  Use a more descriptive name than 'c'

> WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:51
> +    const float OneThird = 1.0f / 3.0f;
> +    const float TwoThirds = 2.0f / 3.0f;
These should be static

> WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:76
> +	   result->coords[1] = FloatPoint3D(OneThird * (3.0f * ls * ms -
> +							ls * mt -
> +							lt * ms),
> +					    ls * ls * (ls - lt),
> +					    ms * ms * (ms - mt));
> +	   result->coords[2] = FloatPoint3D(OneThird * (lt * (mt - 2.0f * ms) +

> +							ls * (3.0f * ms - 2.0f
* mt)),
Try to avoid line wrapping in the middle of an expression.

> WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:101
> +	       // TODO(kbr): may require more work?
I think this TODO is no longer needed.	Looks like we should return immediately
here, or have a more descriptive FIXME.

> WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:108
> +	       // TODO(kbr): may require more work?
Same as above.

> WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:134
> +	   reverseOrientation =
> +		   ((c.d1() > 0.0f && result->coords[0].x() < 0.0f)
> +		    || (c.d1() < 0.0f && result->coords[0].x() > 0.0f));
nit: this wraps oddly

> WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:183
> +	       result->coords[i].setX(-result->coords[i].x());
> +	       result->coords[i].setY(-result->coords[i].y());
Add a comment or rename the type so it's clearer that the X and Y here are
actually the K and L coordinates from the paper.

> WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.h:71
> +    static void Compute(const LoopBlinnClassifier::Result& classification,
Should be compute (lowercase).

> WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.h:72
> +			   bool fillRightSide,
Can this be an enum to make callsites clearer?

> WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.h:73
> +			   Result* result);
This should return a Result.