[webkit-reviews] review denied: [Bug 96811] [CSS Exclusions] Add support for polygonal shapes : [Attachment 166032] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 96811: [CSS Exclusions] Add support for polygonal shapes
https://bugs.webkit.org/show_bug.cgi?id=96811

Attachment 166032: Patch
https://bugs.webkit.org/attachment.cgi?id=166032&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=166032&action=review


I gave the patch a cursory look. 39k is really too big to be landed with one
test, LayoutTests are only one type of tests you can add to validate your new
data structure (see the unit tests in Tools/TestWebKitAPI/Tests/).

I would advise re-using existing structures as much as possible to reduce the
size and also because they usually have good APIs.

> Source/WebCore/rendering/ExclusionPolygon.cpp:47
> +    float x;
> +    float y;

FloatPoint

> Source/WebCore/rendering/ExclusionPolygon.cpp:60
> +    bool operator() (const ExclusionPolygonEdge* e, float y) const
> +    {
> +	   return e->minY() < y;
> +    }

This doesn't look like it's used?

I find the comparator object to be an overkill compared to using a simple
comparator function.

> Source/WebCore/rendering/ExclusionPolygon.cpp:167
> +	   m_edges[i].i2 = (i+1) % nVertices;

Style violation.

> Source/WebCore/rendering/ExclusionPolygon.h:52
> +    float getXAt(size_t i) const { return m_coordinates->at(i*2); }
> +    float getYAt(size_t i) const { return m_coordinates->at(i*2 + 1); }

Coding style violation.

> Source/WebCore/rendering/ExclusionPolygon.h:75
> +    size_t i1;
> +    size_t i2;

'i' for 'index'? We usually prefer unsigned instead of size_t.

Do we really need this struct which is a glorified std::pair<FloatPoint>?

> Source/WebCore/rendering/ExclusionPolygon.h:87
> +    ExclusionPolygonEdge()
> +	   : polygon(0)
> +	   , i1(0)
> +	   , i2(0)
> +    {
> +    }
> +
> +    float x1() const { return polygon->getXAt(i1); }
> +    float y1() const { return polygon->getYAt(i1); }
> +    float x2() const { return polygon->getXAt(i2); }
> +    float y2() const { return polygon->getYAt(i2); }

This API makes no sense:
* you are allowing polygon to be 0 but don't check it here.
* an edge is supposed to be between two adjacent vertex, yet you don't ASSERT
that.

> Source/WebCore/rendering/ExclusionPolygon.h:95
> +// an "interval tree"

We already have an interval tree in WebCore/platform/PODIntervalTree.h, why
can't we reuse it here?

> Source/WebCore/rendering/ExclusionPolygon.h:101
> +    float center;
> +    Vector<ExclusionPolygonEdge*> overlapCenter;
> +    OwnPtr<ExclusionPolygonEdgeTree> aboveCenter;
> +    OwnPtr<ExclusionPolygonEdgeTree> belowCenter;

Any reason why all your data structures are 'struct' even if you end up
defining a constructor, without prefixing all the private data members with m_
and allow the copy constructor or assignment operator?

This is also backwards, we usually put the members last.

> Source/WebCore/rendering/ExclusionShape.cpp:67
> +static PassOwnPtr<ExclusionShape>
createExclusionPolygon(PassOwnPtr<Vector<float> > coordinates, WindRule
fillRule)
> +{
> +    ASSERT(!(coordinates->size() % 2));
> +    return adoptPtr(new ExclusionPolygon(coordinates, fillRule));
> +}

If you are going to pass ExclusionShape in OwnPtr, ExclusionShape should
probably be WTF_MAKENONCOPYABLE.

> Source/WebCore/rendering/ExclusionShapeInsideInfo.h:58
> +    LayoutUnit shapeLogicalTop() const
> +    {

Usually whitespace changes occurs on touched code to avoid confusing svn / git
blame. On top of that the last discussion on the topic was a while ago and
didn't bear any consensus
(https://lists.webkit.org/pipermail/webkit-dev/2009-August/009692.html). I
didn't repeat this comment to the other whitespace changes in your patch.