[webkit-reviews] review requested: [Bug 107880] [CSS Regions] Region boxes should respect -shape-inside CSS property : [Attachment 189098] Updated patch, based on Mihnea's review

[bugzilla-daemon at webkit.org]

Zoltan Horvath <zoltan at webkit.org> has asked  for review:
Bug 107880: [CSS Regions] Region boxes should respect -shape-inside CSS
property
https://bugs.webkit.org/show_bug.cgi?id=107880

Attachment 189098: Updated patch, based on Mihnea's review
https://bugs.webkit.org/attachment.cgi?id=189098&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.