Bug 103429

Created attachment 184022 [details] Patch

Created attachment 184027 [details] Patch Removed an unused ExclusionPolygon field.

Comment on attachment 184027 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184027&action=review Only made it through the intersection code so far, but looks good. I've commented on some small tweaks you may consider. > LayoutTests/fast/exclusions/shape-inside/shape-inside-first-fit-001-expected.html:6 > + if (window.internals) > + window.internals.settings.setCSSExclusionsEnabled(true); This isn't necessary in the expectation file. > LayoutTests/fast/exclusions/shape-inside/shape-inside-first-fit-001-expected.html:44 > + </br/>&nbsp;&nbsp;&nbsp;X<br/>&nbsp;&nbsp;<span id="blue">XX</span><br/>&nbsp;XXX Would it be cleaner to wrap this in a pre tag rather than use the nbsp entity? > LayoutTests/fast/exclusions/shape-inside/shape-inside-first-fit-001.html:37 > + X <span id="blue">XX</span> XXX You probably want class for 'blue', 'shape-inside', and 'shape-outline' as ids should only be used once per document. > Source/WebCore/rendering/ExclusionPolygon.cpp:65 > +static inline bool isPointOnLineSegment(const FloatPoint& p0, const FloatPoint& p1, const FloatPoint& p2) It would help if the argument names distinguished which point you were testing to be on the line segment. > Source/WebCore/rendering/ExclusionPolygon.cpp:384 > +static inline float leftSide(const FloatPoint& vertex1, const FloatPoint& vertex2, const FloatPoint& point) Returning > 0 seems a little odd, since I typically think of numbers increasing from left to right. But if it's a common polygon geometry term, don't sweat it. > Source/WebCore/rendering/ExclusionPolygon.cpp:389 > +bool ExclusionPolygon::pointInPolygon(const FloatPoint& point) const Where does this algorithm come from? If you can just include a reference in your changelog, that would be helpful. > Source/WebCore/rendering/ExclusionPolygon.cpp:427 > + if (!leftSideValues[i]) Does a collinear vertex not count as overlapping? > Source/WebCore/rendering/ExclusionPolygon.cpp:445 > + float den = determinant(thisDelta, otherDelta); the jump from the mentioned paper to determinants was non-obvious to me and took a little head-scratching. Might be worth noting in the code or in the changelog. > Source/WebCore/rendering/ExclusionPolygon.cpp:446 > + if (!den) what if the two lines are collinear?

(In reply to comment #3) > (From update of attachment 184027 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184027&action=review > > > LayoutTests/fast/exclusions/shape-inside/shape-inside-first-fit-001-expected.html:6 > > + if (window.internals) > > + window.internals.settings.setCSSExclusionsEnabled(true); > > This isn't necessary in the expectation file. Thanks, I've removed all occurrences. > > LayoutTests/fast/exclusions/shape-inside/shape-inside-first-fit-001-expected.html:44 > > + </br/>&nbsp;&nbsp;&nbsp;X<br/>&nbsp;&nbsp;<span id="blue">XX</span><br/>&nbsp;XXX > > Would it be cleaner to wrap this in a pre tag rather than use the nbsp entity? The pre tag is confusing (at least to me) in the way it handles leading white space, and spaces are rather critical here. I was able to use it in two of the tests, and it definitely improves readability. > > LayoutTests/fast/exclusions/shape-inside/shape-inside-first-fit-001.html:37 > > + X <span id="blue">XX</span> XXX > > You probably want class for 'blue', 'shape-inside', and 'shape-outline' as ids should only be used once per document. Yes, I've fixed that. And I've removed the extraneous "blue" styles from the files that don't use them. > > Source/WebCore/rendering/ExclusionPolygon.cpp:65 > > +static inline bool isPointOnLineSegment(const FloatPoint& p0, const FloatPoint& p1, const FloatPoint& p2) > > It would help if the argument names distinguished which point you were testing to be on the line segment. I've renamed the line line segment endpoint parameters vertex1 and vertex2. Although names like lineSegmentEndpoint1,2 would be more meaningful, they kill the readability of the point arithmetic expressions. > > Source/WebCore/rendering/ExclusionPolygon.cpp:384 > > +static inline float leftSide(const FloatPoint& vertex1, const FloatPoint& vertex2, const FloatPoint& point) > > Returning > 0 seems a little odd, since I typically think of numbers increasing from left to right. But if it's a common polygon geometry term, don't sweat it. There's no secret geometer's convention at work here although I have seen this function coded similarly elsewhere. > > Source/WebCore/rendering/ExclusionPolygon.cpp:389 > > +bool ExclusionPolygon::pointInPolygon(const FloatPoint& point) const > > Where does this algorithm come from? If you can just include a reference in your changelog, that would be helpful. There are a million variations on this simple point in polygon algorithm. I don't know a definitive reference for exactly this variant, but http://paulbourke.net/geometry/polygonmesh/ is a good source for point in polygon algorithms in general. > > Source/WebCore/rendering/ExclusionPolygon.cpp:427 > > + if (!leftSideValues[i]) > > Does a collinear vertex not count as overlapping? Yes. In other words if the first fit rectangle touches a polygon edge (it's guaranteed to for at least one edge) that's OK. > > Source/WebCore/rendering/ExclusionPolygon.cpp:446 > > + if (!den) > > what if the two lines are collinear? In the abstract I think that's OK, since there's no intersection "point" in that case. It may be necessary to deal with this when reflex vertex detection is included, since one could create a shape that pinned the first fit location between a pair of opposing reflex vertices. As a practical matter, this is unlikely to come up very often.

Created attachment 184068 [details] Patch Incorporated Bear's feedback.

Comment on attachment 184068 [details] Patch 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

Created attachment 184277 [details] Patch Made the changes that Dirk recommended.

Attachment 184277 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/exclusions/shape-inside/shape-inside-first-fit-001-expected.html', u'LayoutTests/fast/exclusions/shape-inside/shape-inside-first-fit-001.html', u'LayoutTests/fast/exclusions/shape-inside/shape-inside-first-fit-002-expected.html', u'LayoutTests/fast/exclusions/shape-inside/shape-inside-first-fit-002.html', u'LayoutTests/fast/exclusions/shape-inside/shape-inside-first-fit-003-expected.html', u'LayoutTests/fast/exclusions/shape-inside/shape-inside-first-fit-003.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/FloatSize.h', u'Source/WebCore/rendering/ExclusionPolygon.cpp', u'Source/WebCore/rendering/ExclusionPolygon.h']" exit_code: 1 Source/WebCore/rendering/ExclusionPolygon.cpp:452: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 184277 [details] Patch Please fix the style issue before landing. r=me.

Created attachment 184303 [details] Patch

Comment on attachment 184303 [details] Patch Clearing flags on attachment: 184303 Committed r140606: <http://trac.webkit.org/changeset/140606>

All reviewed patches have been landed. Closing bug.