1232194 - [writing-mode] GetConsumedBSize() should return block-size, not physical height

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P3)

Description

[Mats Palmgren (inactive)]

This doesn't look right to me:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSplittableFrame.cpp#210
GetContentRectRelativeToSelf().height; is the physical height, right?

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - [writing-mode] Drop "Get" from GetConsumedBSize() and add a WritingMode param.

I think we should drop the "Get" since it always returns a valid result,
to follow the naming convention.  This patch also adds a WritingMode
param but does not change the behavior yet.  That's the next patch...

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - [writing-mode] Make ConsumedBSize() return the block-axis size, not the physical height.

I will add reftests in an upcoming bug, since we have more bugs
in this area...

Comment 3

[Mats Palmgren (inactive)]

Sadly, these patches actually pass all current reftests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf28d386b5f41310652429fef26faf5d44119334&exclusion_profile=false

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Mats Palmgren (:mats) from comment #0)
> This doesn't look right to me:
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/
> nsSplittableFrame.cpp#210
> GetContentRectRelativeToSelf().height; is the physical height, right?

Indeed it is, so this must be wrong, and our tests are inadequate. Thanks for picking this up!

Comment 5

[Mats Palmgren (inactive)]

(FYI, I'm adding a reftest in bug 1328095 that will fail without these patches.)

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8823022 [details] [diff] [review]
part 1 - [writing-mode] Drop "Get" from GetConsumedBSize() and add a WritingMode param.

Review of attachment 8823022 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/BlockReflowInput.cpp
@@ +153,3 @@
>    }
>  
>    return mConsumedBSize;

This caching of |mConsumedBSize| means that you're implicitly expecting |aWM| to always be the same thing, in every call to this function. (If aWM varied from one call to the next, then the cached value would be bogus!!)

This expectation is worth explicitly asserting & documenting, since it's different from how nsSplittableFrame::ConsumedBSize behaves. (That one is more forgiving/flexible, I think.)  Or, we should just avoid introducing this variability.

So -- please do one of the following -- EITHER:
 (1) Please add an assertion that aWM == mReflowInput.GetWritingMode()  (I suspect that should be valid).  And please mention this aWM expectation in the documentation in BlockReflowInput.h.

...OR, perhaps better:
 (2) Don't add the aWM arg to this particular function-signature, and instead just document that this function uses mReflowInput's WM. And adjust the mBlock->ConsumedBSize() function-call to pass in mReflowState->GetWritingMode().


Personally I lean towards option #2, particularly since this only has one caller:
https://dxr.mozilla.org/mozilla-central/rev/c91249f41e3766274131a84f9157a4d9d9949520/layout/generic/nsBlockFrame.cpp#1630-1631
...and it's indeed correct to use the BlockReflowInput's mReflowState.GetWritingMode() as its writing mode, for that caller.  Your patch currently adjusts that caller to pass "wm", which is declared here as aState.mReflowInput.GetWritingMode():
https://dxr.mozilla.org/mozilla-central/rev/c91249f41e3766274131a84f9157a4d9d9949520/layout/generic/nsBlockFrame.cpp#1549

::: layout/generic/nsBlockFrame.cpp
@@ +1622,5 @@
>        finalSize.BSize(wm) = std::max(aReflowInput.AvailableBSize(),
>                                       aState.mBCoord + nonCarriedOutBDirMargin);
>        // ... but don't take up more block size than is available
>        nscoord effectiveComputedBSize =
> +        GetEffectiveComputedBSize(aReflowInput, aState.ConsumedBSize(wm));

(If you choose option #2 for my note BlockReflowInput notes above, then your patch won't need to modify this line.)

Comment 7

[Daniel Holbert [:dholbert]]

To be clear (since Splinter doesn't show enough context), the previous comment is entirely about BlockReflowInput::ConsumedBSize(), which is completely separate from the nsSplittableFrame function of the same name.

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 1 - [writing-mode] Drop "Get" from GetConsumedBSize() and add a WritingMode param.

I agree option #2 seems better; no need for flexibility here, the purpose
of this method is just to coalesce calls that should yield the same value.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8823404 [details] [diff] [review]
part 1 - [writing-mode] Drop "Get" from GetConsumedBSize() and add a WritingMode param.

Review of attachment 8823404 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! r=me

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8823023 [details] [diff] [review]
part 2 - [writing-mode] Make ConsumedBSize() return the block-axis size, not the physical height.

Review of attachment 8823023 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcbf5734ae38
part 1 - [writing-mode] Drop "Get" from GetConsumedBSize() and add a WritingMode param.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/efad45adb8c4
part 2 - [writing-mode] Make ConsumedBSize() return the block-axis size, not the physical height.  r=dholbert

Comment 12

[Mats Palmgren (inactive)]

(the reftest in bug 1328095 covers this bug too)

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bcbf5734ae38
https://hg.mozilla.org/mozilla-central/rev/efad45adb8c4