[webkit-reviews] review denied: [Bug 121619] [CSS Shapes] add shape-margin support for image valued shapes : [Attachment 212398] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Sep 26 09:49:45 PDT 2013
Alexandru Chiculita <achicu at adobe.com> has denied Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 121619: [CSS Shapes] add shape-margin support for image valued shapes
https://bugs.webkit.org/show_bug.cgi?id=121619
Attachment 212398: Patch
https://bugs.webkit.org/attachment.cgi?id=212398&action=review
------- Additional Comments from Alexandru Chiculita <achicu at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=212398&action=review
> Source/WebCore/ChangeLog:9
> + value is an image. The current implementation is correct but
suboptimal.
Is there a bug that will improve it? Why not just do the optimal version now :)
?
Is it correct to ignore the empty intervals inside the shape?
> Source/WebCore/rendering/shapes/RasterShape.cpp:56
> + m_xIntercepts.resize(radius + 1);
I'm not sure if it is worth caching these values. And if you don't need the
cache, then you don't really need the Generator class at all.
> Source/WebCore/rendering/shapes/RasterShape.cpp:87
> + ASSERT(m_intervalLists[y].size() <= 1);
It seems like you are going to need more than one interval. Seems like
ShapeInterval::uniteShapeIntervals does very similar things.
> Source/WebCore/rendering/shapes/RasterShape.cpp:233
> + intervalGenerator.set(y, intervalsAtY[0].x1(),
intervalsAtY.last().x2());
Isn't this going to ignore the intervals inside? Why are you not doing the
traversal for each interval?
> Source/WebCore/rendering/shapes/RasterShape.cpp:234
> + for (int marginY = marginY0; marginY <= marginY1; ++marginY)
You can generate all the intervals for one line and then use
ShapeInterval::uniteShapeIntervals to unite it.
More information about the webkit-reviews
mailing list