Bug 111766

Summary: [CSS Exclusions] Refactor the ExclusionPolygon class to enable storing multiple boundaries
Product: WebKit Reporter: Hans Muller <giles_joplin>
Component: CSSAssignee: Hans Muller <giles_joplin>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, esprehn+autocc, gyuyoung.kim, ojan.autocc, rakuco, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 111080    
Attachments:
Description Flags
Patch
none
Patch
webkit-ews: commit-queue-
Patch
none
Patch
none
Patch
krit: review+, krit: commit-queue-
Patch none

Description Hans Muller 2013-03-07 12:43:40 PST
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.
Comment 1 Hans Muller 2013-03-07 12:59:33 PST
Created attachment 192072 [details]
Patch
Comment 2 Dirk Schulze 2013-03-07 16:27:31 PST
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?
Comment 3 Hans Muller 2013-03-07 16:45:47 PST
(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".
Comment 4 Hans Muller 2013-03-08 12:17:46 PST
Created attachment 192268 [details]
Patch

Refactored platform/graphics/FloatPolygon class out of rendering/ExclusionPolygon per Dirk's feedback.
Comment 5 Early Warning System Bot 2013-03-08 12:29:40 PST
Comment on attachment 192268 [details]
Patch

Attachment 192268 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17113152
Comment 6 Hans Muller 2013-03-08 12:45:29 PST
Created attachment 192275 [details]
Patch

Updated Qt Target.pri build file.
Comment 7 Dirk Schulze 2013-03-08 13:46:53 PST
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):
>  
> +2013-03-07  Hans Muller  <hmuller@adobe.com>
> +
> +        [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.
Comment 8 Hans Muller 2013-03-08 14:37:58 PST
Created attachment 192284 [details]
Patch

Removed duplicate ChangeLog entry.
Comment 9 Hans Muller 2013-03-11 09:00:18 PDT
Created attachment 192480 [details]
Patch

Resync'd with latest build file changes.  Hopefully that will make the recalcitrant EWS bots happier.
Comment 10 Dirk Schulze 2013-03-11 12:39:09 PDT
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.
Comment 11 Hans Muller 2013-03-11 13:42:50 PDT
Created attachment 192546 [details]
Patch

Made the suggested changes.
Comment 12 WebKit Review Bot 2013-03-11 15:14:31 PDT
Comment on attachment 192546 [details]
Patch

Clearing flags on attachment: 192546

Committed r145411: <http://trac.webkit.org/changeset/145411>
Comment 13 WebKit Review Bot 2013-03-11 15:14:37 PDT
All reviewed patches have been landed.  Closing bug.