903173 - Merge ComputeHeightDependentValue into ComputeCBDependentValue

Status: NEW

(Core :: Layout, defect)

Description

[Corey Ford [:coyotebush]]

nsLayoutUtils::ComputeWidthDependentValue was changed to ComputeCBDependentValue in bug 851379. It's essentially identical to ComputeHeightDependentValue, which is still used in several places.

Uses of ComputeHeightDependentValue should be replaced with ComputeCBDependentValue. We can probably also move over the stricter precondition that ComputeHeightDependentValue currently has.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Corey Ford [:corey] from comment #0)
> We can probably also move over the stricter
> precondition that ComputeHeightDependentValue currently has.

(Note: it's stricter in that it's an assertion instead of a warning, but its condition is slightly less-strict -- it only complains about nscoord_MAX as the CB size if we're resolving a percentage.  In any case, per IRC discussion: it's probably worth co-opting that precondition, but replacing NS_AUTOHEIGHT with NS_UNCONSTRAINEDSIZE in order to be less axis-specific.)

Comment 2

[Corey Ford [:coyotebush]]

Attachment: Replace ComputeHeightDependentValue with ComputeCBDependentValue.

I kept the XXXldb comments -- at least the first one looks still accurate with regard to nsHTMLReflowState::ComputeRelativeOffsets.

A try push just to check (though the OS X test slaves look a bit overwhelmed right now):
https://tbpl.mozilla.org/?tree=Try&rev=21ae4aa311e6

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 787837 [details] [diff] [review]
Replace ComputeHeightDependentValue with ComputeCBDependentValue.

> /* static */ nscoord
> nsLayoutUtils::ComputeCBDependentValue(nscoord aPercentBasis,
>                                        const nsStyleCoord& aCoord)
> {
[...]
>+  // XXXldb Some callers explicitly check aPercentBasis
>+  // against NS_UNCONSTRAINEDSIZE *and* unit against eStyleUnit_Percent or
>+  // calc()s containing percents before calling this function.
>+  // However, it would be much more likely to catch problems without
>+  // the unit conditions.

I see you converted NS_AUTOHEIGHT to NS_UNCONSTRAINEDSIZE when moving the comment there. Technically, we should leave it as NS_AUTOHEIGHT in the comment (since we're talking about what callers do, and the callers in question do use NS_AUTOHEIGHT). Though maybe you could make add a parenthetical about (== NS_UNCONSTRAINEDSIZE).

Also: This assertion may end up giving us some trouble -- we might have to update some crashtest/reftest "asserts()" annotations (as you'll probably find in your try run).  It actually looks like dbaron *downgraded* ComputeWidthDependentValue's check from an assertion to a warning in in bug 525100, due to trouble with spurious assertions from testcases with extremely large widths.  Probably worth running this by him, since you'll be effectively reverting that.

Anyway -- r=me with the comment-tweak noted above, and with assertions annotated as-necessary. (assuming there aren't too many annotations required)

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 787837 [details] [diff] [review]
Replace ComputeHeightDependentValue with ComputeCBDependentValue.

Tagging dbaron for feedback, per prev comment. dbaron, this patch sort of reverts your assertion-softening in bug 525100 comment 2; is that OK with you? 

In other words: assuming you agree with the premise of the patch -- that it'd be worth merging these functions, for a code-simplification/cleanup win -- our options are to either:
 (a) keep the assertion (which lets us strictly check that we never resolve a percentage against NS_AUTOHEIGHT, but may have some false-positives on silly-huge testcases)
...or...
 (b) keep the NS_WARN_IF_FALSE, which would mean we lose assertion-coverage for NS_AUTOHEIGHT mishandling but would be less noisy about tests with huge containing blocks.

Comment 5

[Daniel Holbert [:dholbert]]

(for reference: the motivation for removing the distinction was in bug 886646 comment 45 -- my "ResolveStickyOffset" suggestion there.  That bug doesn't strictly depend on this patch, but it inspired this patch.)

Comment 6

[Corey Ford [:coyotebush]]

The only interesting test I see in the Try results is gfx/tests/crashtests/423110-1.xhtml, which now results in lots of assertions. (The mochitest-2 failure looks like a known intermittent thing, and the SyntaxError is bug 902239).

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 787837 [details] [diff] [review]
Replace ComputeHeightDependentValue with ComputeCBDependentValue.

*Maybe* ok if it passes try, I guess.

Though I guess the assertion changes here make me a little queasy -- it would be nice if we could split into two cases:
 * callers that promise *never* to pass a percentage value
    - ignore aPercentBasis
    - assert that they're not passing any percentages or calcs-with-percentages
 * callers that can't make that promise
    - assert that aPercentBasis is good

Otherwise we get the problem that the assertion isn't really helping us catch problems that we don't already have a testcase for.

If it's reasonably possible to do that split somehow, I'd probably prefer that.

Comment 8

[Daniel Holbert [:dholbert]]

So, all of the existing ComputeHeightDependentValue callers are invoking it on a component of a "nsMargin".

What if we created something like:
  nscoord ComputeCBDependentValueForSide(nscoord aPercentBasis,
                                         nsMargin aMargin,
                                         Side aSide)
...and then assert the appropriate things, based on whether aSide is top/bottom vs. left/right, and then call ComputeCBDependentValue?

(For aSide=top/bottom, we'd have an assertion like the one from ComputeHeightDependentValue; for aSide=left/right, we could pull out the existing ComputeCBDependentValue warning and upgrade it to an assertion, with annotations as-necessary for the 423110-1.xhtml crashtest)

Comment 9

[Corey Ford [:coyotebush]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #7)
>  * callers that promise *never* to pass a percentage value
>     - ignore aPercentBasis
>     - assert that they're not passing any percentages or
> calcs-with-percentages

The closest thing I see to an explicit check against percentage values is the part of ComputeRelativeOffsets that avoids passing percent with an auto basis. (But maybe we can reason some other way that percentages won't show up?)
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#787

(In reply to Daniel Holbert [:dholbert] from comment #8)
> So, all of the existing ComputeHeightDependentValue callers are invoking it
> on a component of a "nsMargin".

More precisely, all the callers (of either method, actually) are invoking it on a component of an nsStyleSides (stylePosition->mOffset, styleMargin->mMargin, or stylePadding->mPadding).

> What if we created something like:
>   nscoord ComputeCBDependentValueForSide(nscoord aPercentBasis,
>                                          nsMargin aMargin,
>                                          Side aSide)
> ...and then assert the appropriate things, based on whether aSide is
> top/bottom vs. left/right, and then call ComputeCBDependentValue?
> 
> (For aSide=top/bottom, we'd have an assertion like the one from
> ComputeHeightDependentValue; for aSide=left/right, we could pull out the
> existing ComputeCBDependentValue warning and upgrade it to an assertion,
> with annotations as-necessary for the 423110-1.xhtml crashtest)

That sounds pretty reasonable.