[webkit-reviews] review denied: [Bug 103429] [CSS Exclusions] Add support for computing first included interval position for polygons : [Attachment 184068] 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 103429: [CSS Exclusions] Add support for computing first included interval
position for polygons
https://bugs.webkit.org/show_bug.cgi?id=103429

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

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


Some style comments. I am not so familiar with the algorithms and need to trust
you here.

> Source/WebCore/rendering/ExclusionPolygon.cpp:397
> +    for (unsigned i = 0; i < numberOfEdges(); i++) {

++i

> Source/WebCore/rendering/ExclusionPolygon.cpp:404
> +		   windingNumber += 1;

why not windingNumber++ and

> Source/WebCore/rendering/ExclusionPolygon.cpp:407
> +		   windingNumber -= 1;

windingNumber-- ? Use the main advantage of C++ over C :D

> Source/WebCore/rendering/ExclusionPolygon.cpp:427
> +    int s = 0;

other name for s please.

> Source/WebCore/rendering/ExclusionPolygon.cpp:431
> +	   int sgn = leftSideValues[i] > 0 ? 1 : -1;

We don't have a competition for finding the shortest name :D More descriptive
name please.

> Source/WebCore/rendering/ExclusionPolygon.cpp:447
> +    float den = determinant(thisDelta, otherDelta);

Why den? det would make more sense (used in math anyway). I would even prefer
float determinant = this->determinant() but don't know where it is specified.

> Source/WebCore/rendering/ExclusionPolygon.cpp:453
> +    float ua = determinant(otherDelta, vertex1Delta) / den;
> +    float ub = determinant(thisDelta, vertex1Delta) / den;

can you choose more descriptive names here please?

> Source/WebCore/rendering/ExclusionPolygon.cpp:467
> +    for (unsigned i = 0; i < overlappingEdges.size(); i++) {

++i

> Source/WebCore/rendering/ExclusionPolygon.cpp:500
> +    for (unsigned i = 0; i < numberOfEdges(); i++) {

++i

> Source/WebCore/rendering/ExclusionPolygon.cpp:514
> +	   for (unsigned j = 0; j < offsetEdgePair.size(); j++)

++j

> Source/WebCore/rendering/ExclusionPolygon.cpp:525
> +    for (unsigned i = 0; i < offsetEdges.size() - 1; i++) {

++i

> Source/WebCore/rendering/ExclusionPolygon.cpp:526
> +	   for (unsigned j = i + 1; j < offsetEdges.size(); j++) {

++j