Following on bug 1062963, we need to make the nsFlowAreaRect used by all the AvailableSpace methods in nsBlockReflowState.cpp contain a LogicalRect instead of an nsRect.

Presumably all of the rects in a single float manager will have the same writing mode?

Yes, at least at any given moment.

Comment on attachment 8503985 [details] [diff] [review] Patch Review of attachment 8503985 [details] [diff] [review]: ----------------------------------------------------------------- Looking good! One question, and a few minor nits while we're here. ::: layout/generic/nsBlockFrame.cpp @@ +1848,5 @@ > // layout. > nsRect overflow = aLine->GetOverflowArea(eScrollableOverflow); > + nscoord lineBCoordCombinedBefore = overflow.y + aDeltaBCoord; > + nscoord lineBCoordCombinedAfter = lineBCoordCombinedBefore + > + overflow.height; |overflow| here is still a physical rect, so we have an invalid phys/log mixture; should you convert it to logical, and then use BStart/BSize here? Or is that coming in a followup? ::: layout/generic/nsBlockFrame.h @@ +613,2 @@ > nscoord& aAvailableSpaceHeight, /* in-out */ > bool* aKeepReflowGoing); While you're here, maybe tidy up the uneven indentation of the params? @@ +672,5 @@ > + // Compute the available inline size for a float. > + mozilla::LogicalRect AdjustFloatAvailableSpace( > + nsBlockReflowState& aState, > + const mozilla::LogicalRect& aFloatAvailableSpace, > + nsIFrame* aFloatFrame); Align params, like the following case. ::: layout/generic/nsBlockReflowState.cpp @@ +140,5 @@ > // only give it free space. An example is a table frame - the > // tables do not flow around floats. > // However, we can let its margins intersect floats. > + NS_ASSERTION(aFloatAvailableSpace.IStart(wm) >= mContentArea.IStart(wm), > + "bad avail space rect x"); s/x/inline-coord/ @@ +147,1 @@ > "bad avail space rect width"); s/width/inline-size/ @@ +159,2 @@ > > + nscoord iStartFloatXOffset = s/XOffset/IOffset/, yes? (I don't much like the names we end up with in cases like this, but consistency is probably a good thing.) @@ +161,5 @@ > + aFloatAvailableSpace.IStart(wm) - mContentArea.IStart(wm); > + iStartOffset = std::max(iStartFloatXOffset, frameMargin.IStart(wm)) - > + frameMargin.IStart(wm); > + iStartOffset = std::max(iStartOffset, 0); // in case of negative margin > + nscoord iEndFloatXOffset = ditto @@ +299,5 @@ > blockSize, mContentArea, aState, > mContainerWidth); > + // Keep the inline size >= 0 for compatibility with nsSpaceManager. > + if (result.mRect.ISize(wm) < 0) > + result.mRect.ISize(wm) = 0; Add the missing braces. @@ +335,5 @@ > mFloatManager->GetFlowArea(wm, aBCoord, nsFloatManager::WIDTH_WITHIN_HEIGHT, > aBSize, mContentArea, aState, mContainerWidth); > // Keep the width >= 0 for compatibility with nsSpaceManager. > + if (result.mRect.ISize(wm) < 0) > + result.mRect.ISize(wm) = 0; Braces. @@ +634,5 @@ > aFloatOffsetState.ComputedLogicalPadding().Size(fosWM), > aFloatOffsetState.ComputedLogicalPadding().Size(fosWM), > + true).ISize(fosWM) + > + aFloatOffsetState.ComputedLogicalMargin().IStartEnd(fosWM) + > + aFloatOffsetState.ComputedLogicalBorderPadding().IStartEnd(fosWM); Can we make this a bit more readable somehow? Maybe split into two statements, as nscoord floatISize = aFloat->ComputeSize(... ... ...).ISize(fosWM); return floatISize + aFloatOffsetState.ComputedLogicalMargin() ... + ....; ?

Addressed review comments and added some changes in nsLineLayout.cpp which I had as a separate patch originally and forgot to include before. I made FloatMarginISize more readable slightly differently from how you suggested, but I hope you like this way too.

Comment on attachment 8505513 [details] [diff] [review] Patch v2 Review of attachment 8505513 [details] [diff] [review]: ----------------------------------------------------------------- LGTM - with one small slip as noted below! ::: layout/generic/nsBlockReflowState.cpp @@ +628,5 @@ > + nscoord floatSize = > + aFloat->ComputeSize( > + aCBReflowState.rendContext, > + wm, > + aCBReflowState.ComputedSize(wm), Looks like floatSize needs to be declared as a LogicalSize here.

Good catch! Me: "This is such a trivial change, no need to make sure that it still builds". I'll make sure to do another try run of the patch queue from bug 1062963 and here before checking in.

Backed out both this and bug 1062963 in https://hg.mozilla.org/integration/mozilla-inbound/rev/2c490d1c97b0 because I didn't know which caused the failures in 427129-table-caption.html (permaorange in b2g reftest-6, intermittent in Android 2.3 reftest-5).

This was backed out from Fx37 as part of addressing bug 1114329.