[webkit-reviews] review requested: [Bug 128693] [CSS Shapes] Adjust lineTop position to the next available wrapping location at shape-outsides : [Attachment 224912] Patch

[bugzilla-daemon at webkit.org]

Zoltan Horvath <zoltan at webkit.org> has asked  for review:
Bug 128693: [CSS Shapes] Adjust lineTop position to the next available wrapping
location at shape-outsides
https://bugs.webkit.org/show_bug.cgi?id=128693

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

------- Additional Comments from Zoltan Horvath <zoltan at webkit.org>
(In reply to comment #8)
> (From update of attachment 224790 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=224790&action=review
> 
> In general, I like this much better than the last attempt.
> 
> Why didn't you change all of the calls to fitBelowFloats to pass in the
height argument when shape outside is enabled?

I changed only the the sites, where I also had tests as well. - In this patch I
updated the 2 other places, but I don't know any use case for those with
shape-outside at this point.

> Also, I think it would be good if Hyatt had a look at this.
> 
> > Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:787
> > +		     m_width.fitBelowFloats(lineHeight);
> 
> Make fitBelowFloats take m_block and isFirstLine, and then do the lineHeight
computation in fitBelowFloats. That way, you don't need all of this nastiness
in the call.

LineWidth has got an m_block reference. However, I updated to pass isFirstLine,
I don't think it's less/more nastier than passing LineHeight. LineHeight is
going to be more interesting once we support variable line-heights.

> > Source/WebCore/rendering/line/LineWidth.cpp:169
> > +void LineWidth::updateLineDimension(LayoutUnit newLineTop, LayoutUnit
newLineWidth)
> 
> This should not be in the compile guard.

Good catch. I moved it out, thanks!