[webkit-reviews] review denied: [Bug 45252] Add local triangulation of cubic curve control points : [Attachment 66611] 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 45252: Add local triangulation of cubic curve control points
https://bugs.webkit.org/show_bug.cgi?id=45252

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=66611&action=prettypatch

> WebCore/platform/graphics/gpu/LoopBlinnLocalTriangulator.cpp:35
> +using namespace LoopBlinnMathUtils;
explicitly 'using' the functions used in this cpp

> WebCore/platform/graphics/gpu/LoopBlinnLocalTriangulator.cpp:39
> +    for (int i = 0; i < 3; i++)
nit: could use indexForVertex()

> WebCore/platform/graphics/gpu/LoopBlinnLocalTriangulator.cpp:45
> +LoopBlinnLocalTriangulator::Vertex*
LoopBlinnLocalTriangulator::Triangle::nextVertex(LoopBlinnLocalTriangulator::Ve
rtex* cur, bool ccw)
rename params as in header

> WebCore/platform/graphics/gpu/LoopBlinnLocalTriangulator.cpp:101
> +    bool done = false;
instead of having a done flag, could the work in the first half of this
function be a helper that early-returns?

> WebCore/platform/graphics/gpu/LoopBlinnLocalTriangulator.cpp:107
> +    for (int i = 0; i < 4 && !done; i++)
> +	   for (int j = i + 1; j < 4 && !done; j++)
these need braces

> WebCore/platform/graphics/gpu/LoopBlinnLocalTriangulator.cpp:231
> +		       if (!next->marked()
> +			   && !isSharedEdge(v, next)
> +			   && (!next->interior() || next->end())) {
intentation looks funny. I think the predicate can go one line

> WebCore/platform/graphics/gpu/LoopBlinnLocalTriangulator.cpp:242
> +	       // Something went wrong with the above algorithm; add the last
add a FIXME?

> WebCore/platform/graphics/gpu/LoopBlinnLocalTriangulator.h:46
> +	   Vertex()
initialize the flags in here

> WebCore/platform/graphics/gpu/LoopBlinnLocalTriangulator.h:141
> +	   Vertex* nextVertex(Vertex* cur, bool ccw);
cur -> current

ccw -> counterClockWise, or an enum { ClockWise, CounterClockWise }

> WebCore/platform/graphics/gpu/LoopBlinnLocalTriangulator.h:165
> +	   Vertex* m_vertices[3];
Add a comment indicating why these are raw pointers (that they point to data
from the arena or something of that sort).

> WebCore/platform/graphics/gpu/LoopBlinnLocalTriangulator.h:196
> +    void triangulate(bool computeInsideEdges,
> +			bool fillRightSide);
these bool params should be enums so callsites look more like
triangulage(ComputeInsideEdges, FillLeftSide);