Last Comment Bug 197581 - need to recover overflow area properly during incremental reflow
: need to recover overflow area properly during incremental reflow
Status: RESOLVED FIXED
[a=rjesup,sspitzer for 1.4 final]
: fixed1.4
Product: Core
Classification: Components
Component: Layout: Misc Code (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.4final
Assigned To: David Baron :dbaron: ⌚️UTC-10
: Nobody; OK to take it and work on it
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 96670 173277 188153 197311 201066 205165
  Show dependency treegraph
 
Reported: 2003-03-15 11:00 PST by David Baron :dbaron: ⌚️UTC-10
Modified: 2008-01-13 13:28 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.99 KB, patch)
2003-05-07 11:44 PDT, Bernd
no flags Details | Diff | Splinter Review
revised patch (11.13 KB, patch)
2003-05-15 21:39 PDT, Bernd
kinmoz: review+
dbaron: superreview+
rjesup: approval1.4+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-10 2003-03-15 11:00:32 PST
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) 2003-04-08 22:37:29 PDT
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] 2003-04-08 22:57:40 PDT
"re-reflow" -- *shudder*.

;-)

/be
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2003-04-09 00:11:05 PDT
> 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-10 2003-04-09 06:23:37 PDT
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 2003-04-30 21:49:29 PDT
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) 2003-05-01 05:15:02 PDT
Yeah, we could use that.
Comment 7 Bernd 2003-05-07 11:44:32 PDT
Created attachment 122683 [details] [diff] [review]
patch
Comment 8 Bernd 2003-05-11 10:27:16 PDT
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 2003-05-11 10:38:59 PDT
stealing the bug
Comment 10 Bernd 2003-05-15 21:39:14 PDT
Created attachment 123477 [details] [diff] [review]
revised patch
Comment 11 kinmoz 2003-05-22 22:15:48 PDT
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 2003-05-28 13:17:09 PDT
Comment on attachment 123477 [details] [diff] [review]
revised patch

These changes look ok to me, r=kin@netscape.com, 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 2003-05-28 14:23:12 PDT
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) 2003-05-29 12:13:22 PDT
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) 2003-05-29 12:43:45 PDT
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 2003-05-29 13:10:51 PDT
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 2003-05-31 06:03:24 PDT
fix checked in
Comment 18 Bernd 2003-05-31 12:27:57 PDT
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) 2003-06-02 10:21:40 PDT
We should aim this patch at 1.4 after it bakes a few days ... bug 205165 depends
on it.
Comment 20 Samir Gehani 2003-06-06 10:57:34 PDT
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] 2003-06-09 11:57:18 PDT
Comment on attachment 123477 [details] [diff] [review]
revised patch

Please mark with fixed1.4 when checked into 1.4 branch.
Comment 22 kinmoz 2003-06-09 15:48:41 PDT
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 2003-08-10 10:29:42 PDT
Can't we mark this one fixed?
Comment 24 David Baron :dbaron: ⌚️UTC-10 2003-08-10 12:02:53 PDT
No.  It's not fixed.
Comment 25 Markus Hübner 2004-08-12 01:47:07 PDT
How does this interact with bug 170330 ?
Comment 26 Bernd 2008-01-13 08:20:41 PST
David, isn't this bug obsolete after the landing of the reflow branch?
Comment 27 David Baron :dbaron: ⌚️UTC-10 2008-01-13 10:03:25 PST
Not reflow branch, but I think roc fixed it at a different point... I should find the bug.

Note You need to log in before you can comment on or make changes to this bug.