Make nsFlowAreaRect logical

RESOLVED FIXED in Firefox 36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: smontagu, Assigned: smontagu)

Tracking

(Blocks: 1 bug)

unspecified
mozilla36
Points:
---

Firefox Tracking Flags

(firefox36 fixed, firefox37 wontfix, firefox38 fixed, firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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?
(Assignee)

Comment 2

4 years ago
Yes, at least at any given moment.
(Assignee)

Comment 3

4 years ago
Created attachment 8503985 [details] [diff] [review]
Patch
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() ...
         + ....;

?
(Assignee)

Comment 5

4 years ago
Created attachment 8505513 [details] [diff] [review]
Patch v2

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)
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 7

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
This was backed out from Fx37 as part of addressing bug 1114329.
status-firefox36: --- → fixed
status-firefox37: --- → wontfix
status-firefox38: --- → fixed
status-firefox39: --- → fixed
You need to log in before you can comment on or make changes to this bug.