[webkit-reviews] review canceled: [Bug 107880] [CSS Regions] Region boxes should respect -shape-inside CSS property : [Attachment 189079] Added support for additional cases as well
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Feb 19 08:54:44 PST 2013
Zoltan Horvath <zoltan at webkit.org> has canceled Zoltan Horvath
<zoltan at webkit.org>'s request for review:
Bug 107880: [CSS Regions] Region boxes should respect -shape-inside CSS
property
https://bugs.webkit.org/show_bug.cgi?id=107880
Attachment 189079: Added support for additional cases as well
https://bugs.webkit.org/attachment.cgi?id=189079&action=review
------- Additional Comments from Zoltan Horvath <zoltan at webkit.org>
(In reply to comment #25)
> (From update of attachment 189071 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=189071&action=review
>
> > LayoutTests/fast/regions/shape-inside-on-regions.html:14
> > + width: 300px;
>
> All regions, except region1 are defining width. You should add the width to
region1 and remove it from the class definition.
Fixed.
> > LayoutTests/fast/regions/shape-inside-on-regions.html:35
> > + <div id="rectangle">
>
> I would like to see a mention about the bug that is tested, like <p> Bug <a
href="http://webkit.org/b/XXXX">XXXX</a>: Regions should respect
shape-inside</p>
I added a link to the bug.
> > Source/WebCore/ChangeLog:9
> > +
>
> I would like to see more in description about this bug. Can you put a link to
shape-inside property from the spec? Can you give more details about what
"basic support" means in this case? What is the general approach when solving
it?
I added reference to the spec and explanation to shape-inside.
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:85
> > + if (renderFlowThread) {
>
> Why do you need this test?
We don't. I removed it.
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1542
> > + exclusionShapeInsideInfo = layoutExclusionShapeInsideInfo(this);
>
> How was exclusionShapeInsideInfo computed here before since you are testing
it below?
I added explanation to the changelog.
> > Source/WebCore/rendering/RenderRegion.cpp:270
> > }
>
> Why do you need this here? I thought we had code to compute exclusion shape
for blocks and region is a render block.
Good catch. It's no longer needed since the region-block refactoring is landed.
> > Source/WebCore/rendering/ExclusionShapeInfo.cpp:63
> > + logicalTopOffset +=
toRenderRegion(m_renderer)->logicalTopForFlowThreadContent();
>
> I would like to see an explanation about what this code is trying to achieve.
I added a comment to the source to make it clear.
More information about the webkit-reviews
mailing list