Closed Bug 1174507 Opened 5 years ago Closed 5 years ago

replace the frame flag CONTAINS_RELATIVE_HEIGHT with CONTAINS_RELATIVE_BSIZE

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

We're currently inconsistent in how this bit is used; some call sites are still using it in relation to physical height, and others in relation to block-size. It's block-size we really care about, so we should rename the flag accordingly, and fix those places where code still pretends it's physical.
This renames the flags, and fixes most callsites except for a number of places in table code that I'll handle as part of the table-layout-conversion patches. I don't think this should break any current behavior, but we'll see what tryserver thinks...
Attachment #8622049 - Flags: review?(dholbert)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8622049 [details] [diff] [review]
Replace the frame flag CONTAINS_RELATIVE_HEIGHT with CONTAINS_RELATIVE_BSIZE, and adjust callsites appropriately.  try: -b do -p linux64 -u all

Review feedback, part 1 of 2:
=============================

>Bug 1174507 - Replace the frame flag CONTAINS_RELATIVE_HEIGHT with CONTAINS_RELATIVE_BSIZE, and adjust callsites appropriately. r?dholbert try: -b do -p linux64 -u all

Remember to drop the try syntax when landing.

>+++ b/layout/generic/nsColumnSetFrame.cpp
>-  if (aReflowState.ComputedHeight() != NS_AUTOHEIGHT) {
>-    NS_ASSERTION(aReflowState.ComputedHeight() != NS_INTRINSICSIZE,
[...]
>+  // Our children depend on our block-size if we have a fixed block-size.
>+  if (aReflowState.ComputedBSize() != NS_AUTOHEIGHT) {
>+    NS_ASSERTION(aReflowState.ComputedBSize() != NS_INTRINSICSIZE,

Side note: this assertion (which predates your patch) doesn't make sense, since NS_AUTOHEIGHT and NS_INTRINSICSIZE are #defined to the same value[1].

So the assertion will necessarily/trivially always pass.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsIFrame.h#143

>+++ b/layout/generic/nsFlexContainerFrame.cpp
>@@ -3857,17 +3858,17 @@ nsFlexContainerFrame::DoFlexLayout(nsPre
>         if (finalFlexItemCBSize ==
>             LogicalSize(flexWM,
>                         item->Frame()->GetContentRectRelativeToSelf().Size())) {
>           // Even if our size hasn't changed, some of our descendants might
>           // care that our height is now considered "definite" (whereas it
>           // wasn't in our previous "measuring" reflow), if they have a
>           // relative height.
>           if (!(item->Frame()->GetStateBits() &
>-                NS_FRAME_CONTAINS_RELATIVE_HEIGHT)) {
>+                NS_FRAME_CONTAINS_RELATIVE_BSIZE)) {

s/height/bsize/ in the comment there.

>+++ b/layout/generic/nsGfxScrollFrame.cpp
> bool
> nsHTMLScrollFrame::ScrolledContentDependsOnHeight(ScrollReflowState* aState)
> {
>   // Return true if ReflowScrolledFrame is going to do something different
>   // based on the presence of a horizontal scrollbar.
>-  return (mHelper.mScrolledFrame->GetStateBits() & NS_FRAME_CONTAINS_RELATIVE_HEIGHT) ||
>-    aState->mReflowState.ComputedHeight() != NS_UNCONSTRAINEDSIZE ||
>-    aState->mReflowState.ComputedMinHeight() > 0 ||
>-    aState->mReflowState.ComputedMaxHeight() != NS_UNCONSTRAINEDSIZE;
>+  return (mHelper.mScrolledFrame->GetStateBits() & NS_FRAME_CONTAINS_RELATIVE_BSIZE) ||
>+    aState->mReflowState.ComputedBSize() != NS_UNCONSTRAINEDSIZE ||
>+    aState->mReflowState.ComputedMinBSize() > 0 ||
>+    aState->mReflowState.ComputedMaxBSize() != NS_UNCONSTRAINEDSIZE;
> }

This function's (one) caller does actually care about physical dimensions (not just block-size), for now; it calls this function with a complex condition involving aAssumeVScroll and mReflowedContentsWithVScrollbar and equivalent "H" values. (for horizontal/vertical)

I don't know offhand what the significance of this mismatch is; could you file a followup on logicalizing ScrolledContentDependsOnHeight and its caller, and reference that bug with an XXX comment here?

>+++ b/layout/generic/nsHTMLReflowState.cpp
>   if (mStyleText->mLineHeight.GetUnit() == eStyleUnit_Enumerated) {
>     NS_ASSERTION(mStyleText->mLineHeight.GetIntValue() ==
>                  NS_STYLE_LINE_HEIGHT_BLOCK_HEIGHT,
>                  "bad line-height value");
> 
>     // line-height depends on block height
>-    frame->AddStateBits(NS_FRAME_CONTAINS_RELATIVE_HEIGHT);
>+    frame->AddStateBits(NS_FRAME_CONTAINS_RELATIVE_BSIZE);

Comment needs tweak: "depends on block-size"

>   // If we're the descendant of a table cell that performs special height
>   // reflows and we could be the child that requires them, always set
>   // the vertical resize in case this is the first pass before the
>   // special height reflow.  However, don't do this if it actually is
>   // the special height reflow, since in that case it will already be
>   // set correctly above if we need it set.
>-  if (!IsVResize() && mCBReflowState &&
>+  if (!IsBResize() && mCBReflowState &&

Comment needs tweak: s/vertical resize/block-axis resize/, or something?

>-      if (rs->frame->GetStateBits() & NS_FRAME_CONTAINS_RELATIVE_HEIGHT)
>+      if (rs->frame->GetStateBits() & NS_FRAME_CONTAINS_RELATIVE_BSIZE)
>         break; // no need to go further

So this is inside of a loop that's basically walking up the frame tree and setting this bit on all ancestor frames. Is it problematic that "relative bsize" means something different for our ancestors?

e.g. if I have a frame with a vertical writing-mode and a percent width (b-size), nested deeply inside of a bunch of horizontal writing-mode frames, does it make sense for all of those horizontal ancestors to have this flag set?  I'm not sure it does make sense...  It'd mean that we think we need to do a bunch of work whenever we adjust the height of some ancestor-div, but really we don't have to do that work, because none of the children care about that height having changed, because nobody's size depends on that changed height.

(Maybe it'd make sense to set this flag on all ancestors whose writing-mode matches this frame's writing-mode, or something? I'm not sure)
Comment on attachment 8622049 [details] [diff] [review]
Replace the frame flag CONTAINS_RELATIVE_HEIGHT with CONTAINS_RELATIVE_BSIZE, and adjust callsites appropriately.  try: -b do -p linux64 -u all

Review feedback, part 2 of 2:
=============================

>+++ b/layout/generic/nsViewportFrame.cpp
>@@ -178,19 +178,19 @@ ViewportFrame::Reflow(nsPresContext*    
>-  // Because |Reflow| sets mComputedHeight on the child to
>-  // availableHeight.
>-  AddStateBits(NS_FRAME_CONTAINS_RELATIVE_HEIGHT);
>+  // Because |Reflow| sets ComputedBSize() on the child to
>+  // AvailableBSize().
>+  AddStateBits(NS_FRAME_CONTAINS_RELATIVE_BSIZE);

This comment is not true (not true in the current codebase, and not true after your change).

It *was* true at the time it was added:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsViewportFrame.cpp&rev=1.88#260

At that time we had:
  kidReflowState.mComputedHeight = aReflowState.availableHeight;
which matches what the comment says.

But that was changed in bug 467881, in the second-to-last chunk of the patch there. The code now looks like this:
  kidReflowState.SetComputedBSize(aReflowState.ComputedBSize());

Anyway -- please correct the comment; I think it should end with "to our ComputedBSize()."

>+++ b/layout/tables/nsTableCellFrame.cpp
>@@ -134,17 +134,17 @@ nsTableCellFrame::NotifyPercentHeight(co
>     if (nsTableFrame::AncestorsHaveStyleHeight(*cellRS) ||
[...]
>-        rs->frame->AddStateBits(NS_FRAME_CONTAINS_RELATIVE_HEIGHT);
>+        rs->frame->AddStateBits(NS_FRAME_CONTAINS_RELATIVE_BSIZE);

(I assume this is one of the table callsites that isn't fixed yet, due to the dependence on "AncestorsHaveStyleHeight".)


Holding off on r+ at the moment, because I'm not sure how this flag is supposed to work w.r.t. being set on the ancestor chain, per end of previous comment.
Filed bug 1175509 regarding the ScrolledContentDependsOnHeight issue.

As for the ancestor chain: you're quite right, it doesn't really make sense to go setting this bit all the way up across orthogonal-flow boundaries. But I think we should tackle that as a separate followup, too. Filed bug 1175517 for this.
Updated to address review comments above, with the exception of the issues filed as separate followups.
Attachment #8623672 - Flags: review?(dholbert)
Attachment #8622049 - Attachment is obsolete: true
Attachment #8622049 - Flags: review?(dholbert)
Comment on attachment 8623672 [details] [diff] [review]
Replace the frame flag CONTAINS_RELATIVE_HEIGHT with CONTAINS_RELATIVE_BSIZE, and adjust callsites appropriately

>+++ b/layout/generic/nsHTMLReflowState.cpp
>-  // If we're the descendant of a table cell that performs special height
>+  // If we're the descendant of a table cell that performs special bsize
>   // reflows and we could be the child that requires them, always set
>-  // the vertical resize in case this is the first pass before the
>-  // special height reflow.  However, don't do this if it actually is
>-  // the special height reflow, since in that case it will already be
>+  // the block-axis resize in case this is the first pass before the
>+  // special bsize reflow.  However, don't do this if it actually is
>+  // the special bsize reflow, since in that case it will already be
>   // set correctly above if we need it set.
>-  if (!IsVResize() && mCBReflowState &&
>+  if (!IsBResize() && mCBReflowState &&

Looks like you renamed "special height reflow" to "special bsize reflow" here.  I think we should revert that change, until we've actually logicalized the "special height reflow" code-path. (which I don't think we've done yet; at least not in this patch).  Right now, "special bsize reflow" is meaningless (and we have lots of mentions of "special height reflow" elsewhere).

My review-comment on this section was just about keeping the "vertical resize" in-sync with the code (since you're converting IsVResize to IsBResize here).

(This will leave this comment in a somewhat inconsistent state where we're talking about "height" & "bsize" in the same comment. I think that's fine for now; it's better than using an undefined term "special bsize reflow", which doesn't actually correspond to something our code knows about yet.)

>-  // It would be nice to check that |mComputedHeight != NS_AUTOHEIGHT|
>-  // &&ed with the percentage height check.  However, this doesn't get
>-  // along with table special height reflows, since a special height
[...]
>+  // It would be nice to check that |ComputedBSize != NS_AUTOHEIGHT|
>+  // &&ed with the percentage bsize check.  However, this doesn't get
>+  // along with table special bsize reflows, since a special bsize

Same here.

r=me with the "special bsize reflow" reverted per above.
Attachment #8623672 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #7)
> r=me with the "special bsize reflow" reverted per above.

OK, will do. That change is in one of the upcoming patches; I'll move the comment changes over there.
Sounds good, thanks!
https://hg.mozilla.org/mozilla-central/rev/2f789a4def0a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1272997
You need to log in before you can comment on or make changes to this bug.