[webkit-reviews] review denied: [Bug 120211] [CSS Shapes] Improve the performance of image valued shapes : [Attachment 210651] Patch

[bugzilla-daemon at webkit.org]

Alexandru Chiculita <achicu at adobe.com> has denied Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 120211: [CSS Shapes] Improve the performance of image valued shapes
https://bugs.webkit.org/show_bug.cgi?id=120211

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

------- Additional Comments from Alexandru Chiculita <achicu at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=210651&action=review


> Source/WebCore/rendering/shapes/RasterShape.cpp:95
>  bool RasterShapeIntervals::firstIncludedIntervalY(int minY, const IntSize&
minSize, LayoutUnit& result) const

There's a bunch of code that lives in
RasterShape::firstIncludedIntervalLogicalTop that checks that minY fits in the
bounds of the intervals, but I would rather have that here instead. It seems
like RasterShape knows too much about RasterShapeIntervals.

> Source/WebCore/rendering/shapes/RasterShape.cpp:104
> +	   if (!getIntervalX1Values(lineY, lineY + minSize.height(),
intervalX1Values))

Is there any reason to consider the intervals that are smaller than the
minSize.width() ?

> Source/WebCore/rendering/shapes/RasterShape.cpp:113
> +	       if (contains(IntRect(lineX, lineY, minSize.width(),
minSize.height()))) {

Keep a reference to the IntRect on the stack and just update the x() / y()
members when needed. The size is never changing.

> Source/WebCore/rendering/shapes/RasterShape.cpp:123
> +void RasterShapeIntervals::getIncludedIntervals(int y1, int y2,
IntShapeIntervals& result) const

ditto, I would move the checks from RasterShape::getIncludedIntervals in here
instead.

> Source/WebCore/rendering/shapes/RasterShape.cpp:135
> +	   IntShapeInterval::intersectShapeIntervals(result, getIntervals(y),
intervals);

I think you can check if you have any remaining intervals and just break early.


> Source/WebCore/rendering/shapes/RasterShape.cpp:136
> +	   result = intervals;

You can do a vector swap instead as you don't need to preserve previous vector.
(There's a function in Vector.h for that)

> Source/WebCore/rendering/shapes/RasterShape.cpp:153
> +	   result = intervals;

ditto. Swap vectors instead.

> Source/WebCore/rendering/shapes/RasterShape.cpp:193
> +    for (unsigned i = 0; i < excludedIntervals.size(); ++i)
> +	   result.append(LineSegment(excludedIntervals[i].x1(),
excludedIntervals[i].x2() + 1));

It seems like this could be a function as you do it twice.

> Source/WebCore/rendering/shapes/RasterShape.h:53
> +    bool getIntervalX1Values(int minY, int maxY, Vector<int>& result) const;


All the methods that are not supposed to be used by other classes should be
private.