197581 - need to recover overflow area properly during incremental reflow

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

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

We have problems where we don't recover the overflow area properly during
incremental reflow.  (Bernd mentioned problems with tables, and I'd be surprised
if we don't have others.)

roc had some progress on fixing this in bug 170330, but that's not a complete
fix.  We really need exact recovery of the overflow area so that 'overflow:
auto' and 'overflow: scroll' work correctly.  One possibility is storing the
overflow area on some frames.  Another possibility is recovering it with the
help of the NS_FRAME_OUTSIDE_CHILDREN bit, which should be effectively as fast
for the normal case where most frames don't overflow.

Comment 1

[John Keiser (jkeiser)]

Since even an ordinary div could overflow if it is set to a fixed width bigger
than its parent div (so that pretty much all frames *could* overflow), we'd have
to store these 4 nscoords in every frame.  However, we might be able to avoid
that with a hashtable in presshell.  If < 5% of frames have overflow areas that
would avoid most of the bloat that could arise from such a plan.

The problem with tables as bernd explained it is that they don't re-reflow their
children all the time (an optimization) and so miss setting the overflow area
bit sometimes since that information only comes down through reflow.

Comment 2

[Brendan Eich [:brendan]]

"re-reflow" -- *shudder*.

;-)

/be

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> However, we might be able to avoid that with a hashtable in presshell

The frame manager lets you get and set frame properties which are implemented
this way, so this part is easy.

I think exact recovery of overflow area in incremental reflow is just a matter
of fixing bugs here and there, although it may be that by just storing it, we
would simplify our code significantly.

Comment 4

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

We may not need to store anything that we're not storing now.  Having overflow
is an unusual case, and we can just descend into any frames that have
NS_FRAME_OUTSIDE_CHILDREN set.

Comment 5

[Bernd]

I looked at
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsAbsoluteContainingBlock.cpp#255
Couldn't we use that mechanism and copy the GetOverflowAreaProperty routine to
nsFrame.cpp. Is there anything wrong with that? If not, I would use this
mechanism to propagate the overflow area inside the table frames to verify that
it works.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, we could use that.

Comment 7

[Bernd]

Attachment: patch

Comment 8

[Bernd]

Comment on attachment 122683 [details] [diff] [review]
patch

I believe this must work for a nsIFrame*. The implementation in
nsAbsoluteContainingBlock needs to removed otherwise there will be two
destructor functions for the same property.

Comment 9

[Bernd]

stealing the bug

Comment 10

[Bernd]

Attachment: revised patch

Comment 11

[kinmoz]

Comment on attachment 123477 [details] [diff] [review]
revised patch

FYI, bernd's patch here allows us to fix bug 205165 rather easily. You can see
the small tweak I made in bug 205165 comment 31.

Comment 12

[kinmoz]

Comment on attachment 123477 [details] [diff] [review]
revised patch

These changes look ok to me, r=[e-mail], just a few nit-picks.


====  Capitalize the 'c' in "create" to be consistent with other comments in
the  file:


+  /** create or retrieve the previously stored overflow area, if the frame
does 
+    * not overflow and no creation is required return nsnull.


==== Not a big deal, but I'll ask anyways, in addition to unsetting the
NS_FRAME_OUTSIDE_CHILDREN bit, should |nsFrame::StoreOverflow()| also remove
any existing overflowAreaProperty so it can delete it's associated rect? I know
it will eventually get deleted, but I'm just thinking about dhtml and composer.


==== Capitalize the 'r' in "return", get rid of one of the spaces between
"overflow  and" and put a '.' at the end of the comment:


+  // return the previously stored overflow area, if the frame does not 
+  // overflow	and a creation is not requested it will return nsnull


==== I'd probably rework this comment:


+  // if necessary set the NS_FRAME_OUTSIDE_CHILDREN flag and store the
overflow
+  // area via the frame manager, by this it can be retrieved later without 
+  // reflowing the frame


Maybe something like this:


+  // Set/unset the NS_FRAME_OUTSIDE_CHILDREN flag and store the overflow area
+  // as a frame property in the frame manager so that it can be retrieved
later
+  // without reflowing the frame.

Comment 13

[kinmoz]

Comment on attachment 123477 [details] [diff] [review]
revised patch

Oh yeah, and remove the 2 blank lines this change leaves at the end of this
function:


@@ -1542,17 +1526,7 @@

   ComputeCombinedArea(aReflowState, aMetrics);

-  // If the combined area of our children exceeds our bounding box
-  // then set the NS_FRAME_OUTSIDE_CHILDREN flag, otherwise clear it.
-  if ((aMetrics.mOverflowArea.x < 0) ||
-      (aMetrics.mOverflowArea.y < 0) ||
-      (aMetrics.mOverflowArea.XMost() > aMetrics.width) ||
-      (aMetrics.mOverflowArea.YMost() > aMetrics.height)) {
-    mState |= NS_FRAME_OUTSIDE_CHILDREN;
-  }
-  else {
-    mState &= ~NS_FRAME_OUTSIDE_CHILDREN;
-  }
+  
 }

Comment 14

[John Keiser (jkeiser)]

Comment on attachment 123477 [details] [diff] [review]
revised patch

dbaron, could you sr?  We need this for bug 205165 and all Kin's comments
except one were purely syntactical.

Comment 15

[John Keiser (jkeiser)]

Comment on attachment 123477 [details] [diff] [review]
revised patch

I spoke too soon, bernd says there *may* be a problem with the patch causing
empty rect invalidates--unsure whether this patch is the cause yet.

Comment 16

[kinmoz]

I just used one of my debug builds, without bernd's patch, to go to the test
page he was using and I see the same empty invalidate rect warnings in my
console, so they happen even without his patch.

Comment 17

[Bernd]

fix checked in

Comment 18

[Bernd]

giving the bug back to David. The patch that I checked in is only the
infrastructure for the real fix. Bug 173277 covers the overflow area recovery
for table frames. I don't know block and line layout good enough to fix the
incr. reflow problems there.

Comment 19

[John Keiser (jkeiser)]

We should aim this patch at 1.4 after it bakes a few days ... bug 205165 depends
on it.

Comment 20

[Samir Gehani]

Comment on attachment 123477 [details] [diff] [review]
revised patch

Seeking approval from drivers for kin to land this on the 1.4 branch since
Netscape is very interested in taking bug 205165, an nsbeta1+/adt1 which
depends on this fix landing on the 1.4 branch.	Thanks.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 123477 [details] [diff] [review]
revised patch

Please mark with fixed1.4 when checked into 1.4 branch.

Comment 22

[kinmoz]

Fix checked into the MOZILLA_1_4_BRANCH:

  mozilla/layout/html/base/src/nsFrame.h                      revision 3.188.2.1
  mozilla/layout/html/base/src/nsFrame.cpp                    revision 3.429.2.1
  mozilla/layout/html/base/src/nsBlockFrame.cpp               revision 3.572.2.2
  mozilla/layout/html/base/src/nsAbsoluteContainingBlock.cpp  revision 1.44.16.1
  mozilla/layout/base/public/nsIFrame.h                       revision 3.219.2.1

Comment 23

[Markus Hübner]

Can't we mark this one fixed?

Comment 24

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

No.  It's not fixed.

Comment 25

[Markus Hübner]

How does this interact with bug 170330 ?

Comment 26

[Bernd]

David, isn't this bug obsolete after the landing of the reflow branch?

Comment 27

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

Not reflow branch, but I think roc fixed it at a different point... I should find the bug.