1191185 - Remove redundant fields in nsHypotheticalBox [was: nsHypotheticalBox::mIEndIsExact is unused and should perhaps be removed]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

In bug 1079151, we changed the (debug-only) variable "mRightIsExact" to "mIEndIsExact":
http://hg.mozilla.org/mozilla-central/rev/e641f522e5ae#l1.56

BUT, we also removed its one usage (in an assertion):
-      NS_ASSERTION(hypotheticalBox.mRightIsExact, "should always have "
-                   "exact value on containing block's start side");
http://hg.mozilla.org/mozilla-central/rev/e641f522e5ae#l1.517

So, the variable isn't much use these days. We should probably remove it?  Or should we still be asserting about it?

(Its counterpart "mIStartIsExact" is still used in an assertion, so that one's worth keeping.)

Comment 1

[Daniel Holbert [:dholbert]]

(For the record, these variables were made debug-only [only used in assertions] way back in bug 419076.]

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Simplify nsHypotheticalBox, eliminating obsolete/redundant fields, and rename to nsHypotheticalPosition

Comment 3

[Jonathan Kew [:jfkthame]]

We can do better than that: we can eliminate mIEnd, too, as the current code calculates it but never actually makes use of it. (This is a result of bidi now being handled via logical coordinates in InitAbsoluteConstraints.)

And mIStartIsExact is obsolete, too: it's being unconditionally set to true, and later we assert that it's true; but with mIEnd gone there's no longer an alternative codepath whereby mIEnd would be exact and mIStart not-exact. Really, nsHypotheticalBox needs only mIStart, mBStart and mWritingMode; and perhaps it should be renamed nsHypotheticalPosition.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8644542 [details] [diff] [review]
Simplify nsHypotheticalBox, eliminating obsolete/redundant fields, and rename to nsHypotheticalPosition

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

Nice! r=me, with the following addressed as you see fit

::: layout/generic/nsHTMLReflowState.cpp
@@ +1038,5 @@
>  }
>  
> +struct nsHypotheticalPosition {
> +  // offset from inline-start edge of containing block (which is a padding edge)
> +  nscoord       mIStart;

Hmm, so nsHypotheticalPosition looks a lot like a LogicalPoint now -- the only difference seems to be that nsHypotheticalPosition keeps track of its WritingMode even in opt builds, whereas LogicalPoint only keeps track of it in debug builds.

Can we just use LogicalPoint, and keep track of the point's WritingMode in a separate variable (e.g. separate outparam from CalculateHypotheticalBox), if we even need it? (If we do this, it probably belongs in a followup.  If you agree that this is a good idea, maybe drop an XXX comment here pointing to that followup bug, to make it clearer that this struct is deprecated-ish?)

@@ +1180,5 @@
>  // In the code below, |cbrs->frame| is the absolute containing block, while
>  // |containingBlock| is the nearest block container of the placeholder frame,
>  // which may be different from the absolute containing block.
>  void
> +nsHTMLReflowState::CalculateHypotheticalBox

Sine the outparam here is now "nsHypotheticalPosition" (no longer nsHypotheticalBox), should this method be renamed with s/Box/Pos/?

(Also: The documentation above this method-definition starts out "Calculate the hypothetical box" -- that probably also wants s/box/position/?)

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Comment on attachment 8644542 [details] [diff] [review]
> Simplify nsHypotheticalBox, eliminating obsolete/redundant fields, and
> rename to nsHypotheticalPosition
> 
> Review of attachment 8644542 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice! r=me, with the following addressed as you see fit
> 
> ::: layout/generic/nsHTMLReflowState.cpp
> @@ +1038,5 @@
> >  }
> >  
> > +struct nsHypotheticalPosition {
> > +  // offset from inline-start edge of containing block (which is a padding edge)
> > +  nscoord       mIStart;
> 
> Hmm, so nsHypotheticalPosition looks a lot like a LogicalPoint now -- the
> only difference seems to be that nsHypotheticalPosition keeps track of its
> WritingMode even in opt builds, whereas LogicalPoint only keeps track of it
> in debug builds.
> 
> Can we just use LogicalPoint, and keep track of the point's WritingMode in a
> separate variable (e.g. separate outparam from CalculateHypotheticalBox), if
> we even need it? (If we do this, it probably belongs in a followup.  If you
> agree that this is a good idea, maybe drop an XXX comment here pointing to
> that followup bug, to make it clearer that this struct is deprecated-ish?)

It's true that this is now much like a LogicalPoint, but we do still need to keep track of its WritingMode as well (even in opt builds); InitAbsoluteConstraints depends on checking its bidi-dir against the containing block's. I'm inclined to leave it as is at this point, so that it's clear that we are returning a complete record of coords+mode here, unlike a normal LogicalPoint which relies on the code using it to know its mode.

We could split it into separate outparams, but I don't see any benefit to that... the struct serves to group together the values that are dependent on each other for meaning.

Maybe at some point we'll rework this further such that CalculateHypotheticalBox always returns a result in the containing block's mode, passed in from the caller; in that case it could just return a LogicalPoint. I'd have no objection to such a change, but neither do I think it's important to pursue it for now. (I experimented with it a bit while working on this patch, but it runs into problems in the case where knowBoxISize is false... the box-properties reftests are good at pointing this out.)

Comment 6

[Jonathan Kew [:jfkthame]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/98ef84304afd474107ac41466dd334762f0ce3b1
changeset:  98ef84304afd474107ac41466dd334762f0ce3b1
user:       Jonathan Kew <[e-mail]>
date:       Wed Aug 12 11:02:02 2015 +0100
description:
Bug 1191185 - Simplify nsHypotheticalBox, eliminating obsolete/redundant fields, and rename to nsHypotheticalPosition. r=dholbert

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/98ef84304afd