|Summary:||[CSS Exclusions] Refactor the ExclusionPolygon class to enable storing multiple boundaries|
|Product:||WebKit||Reporter:||Hans Muller <giles_joplin>|
|Component:||CSS||Assignee:||Hans Muller <giles_joplin>|
|Severity:||Normal||CC:||eric, esprehn+autocc, gyuyoung.kim, ojan.autocc, rakuco, webkit-ews, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
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 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 <firstname.lastname@example.org> > + > + [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.