Closed Bug 1079139 Opened 5 years ago Closed 5 years ago

Make nsFlowAreaRect logical

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox36 --- fixed
firefox37 --- wontfix
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: smontagu, Assigned: smontagu)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → smontagu
Attachment #8503985 - Flags: review?(jfkthame)
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() ...
         + ....;

?
Attached patch Patch v2Splinter Review
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.
Attachment #8505513 - Flags: review?(jfkthame)
Attachment #8503985 - Attachment is obsolete: true
Attachment #8503985 - Flags: review?(jfkthame)
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.
Attachment #8505513 - Flags: review?(jfkthame) → review+
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).
https://hg.mozilla.org/mozilla-central/rev/3b63378162d7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
This was backed out from Fx37 as part of addressing bug 1114329.
You need to log in before you can comment on or make changes to this bug.