Bug 111766 – [CSS Exclusions] Refactor the ExclusionPolygon class to enable storing multiple boundaries

• Component: CSS
• Status: RESOLVED FIXED

[Hans Muller]

To support the shape-margin and shape-padding CSS properties, the ExclusionPolygon class will need to lazily compute a padded (inset) version of its polygonal boundary, and one that's expanded by the margin.

A FloatPolygon class should be factored out of the ExclusionPolygon class so that its internal state can be defined with FloatPolygon valued fields for each of the padded/margin/original boundaries.

[Hans Muller]

Created attachment 192072 [details]
Patch

[Dirk Schulze]

Comment on attachment 192072 [details]
Patch

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

> Source/WebCore/rendering/ExclusionPolygon.cpp:77
> +FloatPolygon::FloatPolygon(PassOwnPtr<Vector<FloatPoint> > vertices, WindRule fillRule)

New classes usually get new files. I would like to have this in a separate file as well. Is it generic enough to move it to WebCore/platform? If not, what would make it generic?

[Hans Muller]

(In reply to comment #2)
> (From update of attachment 192072 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192072&action=review
> 
> > Source/WebCore/rendering/ExclusionPolygon.cpp:77
> > +FloatPolygon::FloatPolygon(PassOwnPtr<Vector<FloatPoint> > vertices, WindRule fillRule)
> 
> New classes usually get new files. I would like to have this in a separate file as well. Is it generic enough to move it to WebCore/platform? If not, what would make it generic?

To make this class generic, I'd need to remove the following two methods, since they're rather specific to Exclusion processing:

    void computeXIntersections(float y, bool isMinY, Vector<ExclusionInterval>&) const;
    void computeOverlappingEdgeXProjections(float minY, float maxY, Vector<ExclusionInterval>&) const;

To move FloatPolygon to its own pair of files, I'd also move/include VertexPair and FloatPolygonEdge. 

And perhaps VertexPair should be called FloatVertexPair.  I'd call it FloatPointPair, but it exists to provide access to a pair of points called "vertices".

[Hans Muller]

Created attachment 192268 [details]
Patch

Refactored platform/graphics/FloatPolygon class out of rendering/ExclusionPolygon per Dirk's feedback.

[Early Warning System Bot]

Comment on attachment 192268 [details]
Patch

Attachment 192268 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17113152

[Hans Muller]

Created attachment 192275 [details]
Patch

Updated Qt Target.pri build file.

[Dirk Schulze]

Comment on attachment 192275 [details]
Patch

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

I would like to wait for the bots before giving it my final review.

> Source/WebCore/ChangeLog:1295
>          (WebCore::FormData::appendKeyValuePairItems):
>  
> [phone]  Hans Muller  <[e-mail]>
> +
> +        [CSS Exclusions] Refactor the ExclusionPolygon class to enable storing multiple boundaries
> +        https://bugs.webkit.org/show_bug.cgi?id=111766
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Refactored the ExclusionPolygon class to enable adding support for shape-margin and shape-padding.
> +        Extracted a new FloatPolygon class which is now used by ExclusionPolygon to represent the shape's
> +        boundary. It will be used to add m_paddedPolygon and m_marginPolygon members to ExclusionPolygon

Merge problem here.

[Hans Muller]

Created attachment 192284 [details]
Patch

Removed duplicate ChangeLog entry.

[Hans Muller]

Created attachment 192480 [details]
Patch

Resync'd with latest build file changes.  Hopefully that will make the recalcitrant EWS bots happier.

[Dirk Schulze]

Comment on attachment 192480 [details]
Patch

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

I am sorry. It took me a bit longer to review it. The patch looks great to me. Just some snippets before landing.

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

I would keep the braces here, since the content of the for loop is more than a line.

> Source/WebCore/rendering/ExclusionPolygon.h:40
>  #include "FloatPoint.h"
> +#include "FloatPolygon.h"
>  #include "FloatRect.h"
> -#include "PODIntervalTree.h"
>  #include "WindRule.h"
> -#include <wtf/MathExtras.h>
>  #include <wtf/OwnPtr.h>
>  #include <wtf/PassOwnPtr.h>

Can't your remove FloatPoint, FloatRect, WindRule, OwnPtr etc. dependencies here? Should be included in FloatPolygon already.

[Hans Muller]

Created attachment 192546 [details]
Patch

Made the suggested changes.

[WebKit Review Bot]

Comment on attachment 192546 [details]
Patch

Clearing flags on attachment: 192546

Committed r145411: <http://trac.webkit.org/changeset/145411>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.