[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
bugzilla-daemon at webkit.org
Wed Jan 23 10:49:30 PST 2013
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
More information about the webkit-reviews
mailing list