Bug 123033 – [CSS Shapes] Match adjustLogicalLineTopAndLogicalHeightIfNeeded's implementation with Blink's

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Zoltan Horvath]

In blink I made this function in a bit different way. This change modifies it to be identical, which helps a lot in the future cross-merging patches.

[Zoltan Horvath]

Created attachment 214591 [details]
Patch

[Darin Adler]

Comment on attachment 214591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214591&action=review

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1472
> -    if (shapeInsideInfo && !wordMeasurements.size() && containsFloats()) {
> -        lastFloatFromPreviousLine = m_floatingObjects->set().last().get();
> -        LayoutUnit floatLogicalTopOffset = shapeInsideInfo->computeFirstFitPositionForFloat(logicalSizeForFloat(lastFloatFromPreviousLine));
> -        if (logicalHeight() < floatLogicalTopOffset)
> -            adjustedLogicalLineTop = floatLogicalTopOffset;
> +    if (shapeInsideInfo) {
> +        lastFloatFromPreviousLine = (containsFloats()) ? m_floatingObjects->set().last().get() : 0;
> +        if (!wordMeasurements.size() && lastFloatFromPreviousLine) {
> +            LayoutUnit floatLogicalTopOffset = shapeInsideInfo->computeFirstFitPositionForFloat(logicalSizeForFloat(lastFloatFromPreviousLine));
> +            if (logicalHeight() < floatLogicalTopOffset)
> +                adjustedLogicalLineTop = floatLogicalTopOffset;
> +        }
>      }

I like the old version better than the new version. The use of ? : and 0 is not as nice as regular if statement. Could you merge in the other direction instead?

[Zoltan Horvath]

Created attachment 214597 [details]
Patch

[Zoltan Horvath]

(In reply to comment #2)
> (From update of attachment 214591 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214591&action=review
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1472
> > -    if (shapeInsideInfo && !wordMeasurements.size() && containsFloats()) {
> > -        lastFloatFromPreviousLine = m_floatingObjects->set().last().get();
> > -        LayoutUnit floatLogicalTopOffset = shapeInsideInfo->computeFirstFitPositionForFloat(logicalSizeForFloat(lastFloatFromPreviousLine));
> > -        if (logicalHeight() < floatLogicalTopOffset)
> > -            adjustedLogicalLineTop = floatLogicalTopOffset;
> > +    if (shapeInsideInfo) {
> > +        lastFloatFromPreviousLine = (containsFloats()) ? m_floatingObjects->set().last().get() : 0;
> > +        if (!wordMeasurements.size() && lastFloatFromPreviousLine) {
> > +            LayoutUnit floatLogicalTopOffset = shapeInsideInfo->computeFirstFitPositionForFloat(logicalSizeForFloat(lastFloatFromPreviousLine));
> > +            if (logicalHeight() < floatLogicalTopOffset)
> > +                adjustedLogicalLineTop = floatLogicalTopOffset;
> > +        }
> >      }
> 
> I like the old version better than the new version. The use of ? : and 0 is not as nice as regular if statement. Could you merge in the other direction instead?

I updated the patch. What do you think about the new version?
I'm working on the refactoring of the RenderBlockLineLayout's nextSegmentBreak method, and because of that change I need to update the lastFloatFromPreviousLine variable in the shapeInsideInfo case. That's the reason because I want to do the merge this direction. This one looks better now, I think.

[Zoltan Horvath]

(In reply to comment #4)
> I'm working on the refactoring of the RenderBlockLineLayout's nextSegmentBreak method, and because of that change I need to update the lastFloatFromPreviousLine variable in the shapeInsideInfo case. That's the reason because I want to do the merge this direction. This one looks better now, I think.
patch preview on bug #123038

[Dave Hyatt]

Comment on attachment 214597 [details]
Patch

r=me

[Zoltan Horvath]

Committed r157820: <http://trac.webkit.org/changeset/157820>