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.
Created attachment 214591 [details] Patch
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?
Created attachment 214597 [details] Patch
(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.
(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
Comment on attachment 214597 [details] Patch r=me
Committed r157820: <http://trac.webkit.org/changeset/157820>