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

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit 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 166775: Patch
https://bugs.webkit.org/attachment.cgi?id=166775&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=166775&action=review


I have a lot of questions.

> Source/WebCore/ChangeLog:41
> +	   (WebCore::ExclusionPolygon::ExclusionPolygon): see above for a
description of this type
> +	   (WebCore::computeXIntersection): find and classify the X intercept
of a polygon edge for some Y value, if any
> +	   (WebCore::ExclusionPolygon::rightVertexY): used to decide if a
horizontal line "crosses" a vertex
> +	   (WebCore::appendIntervalX): append the start or end X coordinates of
an ExclusionInterval
> +	   (WebCore::ExclusionPolygon::computeXIntersections): all of the
intersections of a horizontal line and the polygon's edges
> +	   (WebCore::ExclusionPolygon::computeEdgeIntersections): the X
projections of the edges that overlap a horizonal rectangle
> +	   (WebCore::ExclusionPolygon::getExcludedIntervals): X intervals
within a horizontal rectangle that overlap the polygon
> +	   (WebCore::ExclusionPolygon::getIncludedIntervals): X intervals
within a horizontal rectangle that fit inside the polygon

Even small comments need phrases. This is the common style in WebKit that
applies to Changelogs as welll.

> Source/WebCore/rendering/ExclusionPolygon.cpp:41
> +    Normal = 0,
> +    VertexMinY = 1,
> +    VertexMaxY = 2,
> +    VertexYBoth = 3

The integer representation seems unnecessary. You start with 0 anyway.

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

No FloatPoint here? :)

> Source/WebCore/rendering/ExclusionPolygon.cpp:62
> +    // FIXME: handle special cases: less than 3 non-colinear vertices
> +    // FIXME: assuming open polygons for now

Sentences please.

> Source/WebCore/rendering/ExclusionPolygon.cpp:69
> +    float minX = getXAt(0);
> +    float minY = getYAt(0);

You call getXAt most of the time in combination with getYAt. Can't you use
getPointAt() and also use x,y as FloatPoint representation in ExclusionPolygon?
I would just agree, that FloatPoint has no good functionality to get min/max :)


> Source/WebCore/rendering/ExclusionPolygon.cpp:92
> +    m_boundingBox.setX(minX);
> +    m_boundingBox.setY(minY);
> +    m_boundingBox.setWidth(maxX - minX);
> +    m_boundingBox.setHeight(maxY - minY);

It doesn't seem to be a bad idea to add a definition to FloatRect with two
FloatPoints top-left, bottom-right.

> Source/WebCore/rendering/ExclusionPolygon.cpp:104
> +static bool computeXIntersection(const ExclusionPolygonEdge* ep, float y,
EdgeIntersection& result)
> +{
> +    const ExclusionPolygonEdge& e = *ep;

I know you are a programmer, but can you find some nice names instead of just
one or two letters please? :)

> Source/WebCore/rendering/ExclusionPolygon.cpp:177
> +    std::sort(intersections.begin(), intersections.end(),
WebCore::compareEdgeIntersectionX);

Can you use using namespace std; at the beginning of the file please? We
usually don't use namespaces in the code itself.

> Source/WebCore/rendering/ExclusionPolygon.cpp:190
> +		       // skip VertexMaxY,VertexMaxY and VertexMinY,VertexMinY

A sentence please.

> Source/WebCore/rendering/ExclusionPolygon.cpp:196
> +		       index += 1;

index++?

> Source/WebCore/rendering/ExclusionPolygon.cpp:220
> +	   index += 1;

Ditto?

> Source/WebCore/rendering/ExclusionPolygon.h:48
> +// Used by PODIntervalTree for debugging

Dot at the end. :)

> Source/WebCore/rendering/ExclusionPolygon.h:59
> +    float getXAt(unsigned index) const { return (*m_vertices)[index].x(); }
> +    float getYAt(unsigned index) const { return (*m_vertices)[index].y(); }

I would still like to see a returning FloatPoint& instead. Can also be const.

> Source/WebCore/rendering/ExclusionPolygon.h:75
> +    OwnPtr<Vector<FloatPoint> > m_vertices;

Why does it need to be OwnPtr<...>?

> Source/WebCore/rendering/ExclusionPolygon.h:82
> +struct ExclusionPolygonEdge {

Why is the definition of this struct not on the top? Dependency reasons?

> Source/WebCore/rendering/ExclusionPolygon.h:86
> +    float x1() const { ASSERT(polygon); return polygon->getXAt(index1); }
> +    float y1() const { ASSERT(polygon); return polygon->getYAt(index1); }
> +    float x2() const { ASSERT(polygon); return polygon->getXAt(index2); }
> +    float y2() const { ASSERT(polygon); return polygon->getYAt(index2); }

Please FloatPoint& :(.

> Source/WebCore/rendering/ExclusionPolygon.h:91
> +    float minX() const { return std::min(x1(), x2()); }
> +    float minY() const { return std::min(y1(), y2()); }
> +    float maxX() const { return std::max(x1(), x2()); }
> +    float maxY() const { return std::max(y1(), y2()); }

How often is minX(), minY(), ... called? Might it make sense to store them?
Also, point1() point2() would look so much nicer :).

> Source/WebCore/rendering/ExclusionPolygon.h:95
> +    const ExclusionPolygon* polygon;
> +    unsigned index1;
> +    unsigned index2;

I wonder if you don't want to create a real class. Do all these information
need to be exposed?

> Source/WebCore/rendering/ExclusionShape.cpp:133
> +	       float vertexX = floatValueForLength(values.at(i), boxWidth);
> +	       float vertexY = floatValueForLength(values.at(i + 1),
boxHeight);

Use FloatPoint directly and pass the FloatPoint instead.

> Source/WebCore/rendering/ExclusionShape.cpp:137
> +		   vertices->at(vertexIndex).setX(vertexX);
> +		   vertices->at(vertexIndex).setY(vertexY);

As I said. Use FloatPoint directly. But even set(float, float) would be better.


> Source/WebCore/rendering/ExclusionShape.cpp:140
> +		   vertices->at(vertexIndex).setY(vertexX);
> +		   vertices->at(vertexIndex).setX(vertexY);

Ditto.