Last Comment Bug 524925 - Avoid reflows for transform changes
: Avoid reflows for transform changes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- enhancement with 4 votes (vote)
: mozilla12
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on: 727357 771062 665597 720701 720987 724195 750115 764554 770645 785684 825999 1140486
Blocks: 641341 657893 670311 678226 689504 691645 713532 715037 719177 746705 157681 698297 705174 b2g-demo-phone
  Show dependency treegraph
 
Reported: 2009-10-28 02:21 PDT by Michael Ventnor
Modified: 2015-03-06 10:07 PST (History)
34 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.48 KB, patch)
2009-10-28 02:21 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
Snapping the sidebars should be animated (9.12 KB, patch)
2011-05-06 12:40 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Splinter Review
Snapping the sidebars should be animated (9.12 KB, patch)
2011-05-06 12:40 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Splinter Review
WIP with updating overflow rect (7.29 KB, patch)
2011-05-24 10:31 PDT, Benjamin Stover (:stechz)
dbaron: feedback-
Details | Diff | Splinter Review
WIP with updating overflow rect (14.90 KB, patch)
2011-05-24 14:49 PDT, Benjamin Stover (:stechz)
dbaron: feedback-
Details | Diff | Splinter Review
Recompute overflow without reflowing for transforms (11.98 KB, patch)
2011-06-09 11:54 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Splinter Review
Recompute overflow without reflowing for transforms (13.72 KB, patch)
2011-06-09 15:39 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Splinter Review
Recompute overflow without reflowing for transforms (14.11 KB, patch)
2011-06-14 11:05 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Splinter Review
Recompute overflow without reflowing for transforms (15.76 KB, patch)
2011-06-15 14:08 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Splinter Review
added transform hint, rebased (16.01 KB, patch)
2011-08-11 09:24 PDT, Florian Hänel [:heeen]
no flags Details | Diff | Splinter Review
Added special-case code to various frames, rebased (17.13 KB, patch)
2011-08-22 11:07 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Reflow MathML container frames, consider IsCollapsed, etc. (35.28 KB, patch)
2011-08-23 10:27 PDT, Chris Lord [:cwiiis]
roc: feedback+
Details | Diff | Splinter Review
Small fix-ups (35.47 KB, patch)
2011-08-24 02:43 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Small fix-ups (35.51 KB, patch)
2011-08-24 04:35 PDT, Chris Lord [:cwiiis]
roc: review+
roc: feedback+
Details | Diff | Splinter Review
Rebase with new childlist iteration (36.28 KB, patch)
2011-08-25 03:37 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Use FrameChildListIterator (35.62 KB, patch)
2011-08-26 03:26 PDT, Chris Lord [:cwiiis]
roc: review+
dbaron: review-
Details | Diff | Splinter Review
Address some of dbaron's review (39.12 KB, patch)
2011-10-04 13:50 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Address all(?) of dbaron's review (39.97 KB, patch)
2011-10-04 17:01 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Test if scrollable overflow is updated correctly after changing a CSS transform (813 bytes, text/html)
2011-10-04 17:05 PDT, Chris Lord [:cwiiis]
no flags Details
Address review, fix bad misunderstanding/typo (69.37 KB, patch)
2011-10-05 12:07 PDT, Chris Lord [:cwiiis]
dbaron: review+
Details | Diff | Splinter Review
part 6, add overflow handling test (2.05 KB, patch)
2011-10-05 12:08 PDT, Chris Lord [:cwiiis]
dbaron: review+
Details | Diff | Splinter Review
Rebase on changes to bug #665597 (69.91 KB, patch)
2011-10-10 12:28 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Rebase and address comment #78 (67.00 KB, patch)
2011-10-27 08:08 PDT, Chris Lord [:cwiiis]
chrislord.net: review+
romaxa: feedback+
Details | Diff | Splinter Review
Updated to trunk (64.13 KB, patch)
2011-12-13 07:26 PST, Mats Palmgren (:mats)
mats: review+
Details | Diff | Splinter Review
Bug fix and style nits (4.09 KB, patch)
2011-12-13 07:35 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Updated to trunk, on top of bug 665597 (64.34 KB, patch)
2011-12-14 17:26 PST, Mats Palmgren (:mats)
mats: review+
Details | Diff | Splinter Review
additional fixes (20.29 KB, patch)
2011-12-14 17:36 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 1, recompute overflow without reflowing for transforms (64.10 KB, patch)
2012-01-03 13:14 PST, Mats Palmgren (:mats)
mats: review+
Details | Diff | Splinter Review
part 2, additional fixes (17.14 KB, patch)
2012-01-03 13:17 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 4, Make outline style changes update the overflow and repaint instead of reflow (3.32 KB, patch)
2012-01-05 05:49 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 2, Update the overflow for all the affected frames, not just the primary frame. And the ancestors of those. (3.30 KB, patch)
2012-01-05 05:56 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 3, Make all transform style changes update the overflow and repaint instead of reflow (2.60 KB, patch)
2012-01-05 06:03 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 5, Consolidate overflow clipping checks to nsFrame::ApplyOverflowClipping(); and fix some code style nits (13.62 KB, patch)
2012-01-05 06:05 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 2, Update the overflow for all the affected frames, not just the primary frame. And the ancestors of those. (3.02 KB, patch)
2012-01-14 10:01 PST, Mats Palmgren (:mats)
mats: review+
Details | Diff | Splinter Review

Description Michael Ventnor 2009-10-28 02:21:56 PDT
Created attachment 408813 [details] [diff] [review]
Patch

While testing Fennec, I found the instant snapping of the sidebars to be a bit off-putting. With the arguable trend nowadays of applications making everything in the UI go whoosh, I found it easy to make an animation out of this snapping. Just apply it and partly drag out each sidebar to test it.

I think mfinkle is the best person to review, but I haven't done any Fennec-specific work until now.
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2009-11-25 23:01:18 PST
While this works ok on the desktop, it's not smooth on devices. We have a bit of work to do in gecko to make animating elements work better/faster.

I'm going to drop the review? flag for now. We can revisit animation after releasing Fennec 1.0

Thanks for the patch, I hope we can use it soon.
Comment 2 Benjamin Stover (:stechz) 2011-05-06 12:39:26 PDT
I'm working on getting sidebar snapping smooth on device now. I hope it's okay if I take this bug.

I've taken the approach of using CSS animations, which seems promising but it is going to need some platform changes.

Right now, we issue a reflow for any change in moz transition. This works smoothly on desktop but has issues on my Nexus S. I've managed to get the transition to work rather smoothly by removing the reflow (which I know breaks things) and adding a beginning 200msec delay.

Things to consider from here:
1) from my testing it looks like we will need to be smarter about when we reflow. Not reflowing during an animation might be something worth looking into. Perhaps dbaron has some ideas.
2) we need some instrumentation on what is causing the initial pause that makes the animation choppy if we don't delay the animation.
3) we can take this a different direction and use scrollTo on a timer like kinetic panning does.
Comment 3 Benjamin Stover (:stechz) 2011-05-06 12:40:33 PDT
Created attachment 530697 [details] [diff] [review]
Snapping the sidebars should be animated
Comment 4 Benjamin Stover (:stechz) 2011-05-06 12:40:46 PDT
Created attachment 530698 [details] [diff] [review]
Snapping the sidebars should be animated
Comment 5 Benjamin Stover (:stechz) 2011-05-06 12:42:22 PDT
A test build is available here: http://people.mozilla.org/~bstover/fennec-transition.apk
Comment 6 Benjamin Stover (:stechz) 2011-05-06 12:49:00 PDT
Comment on attachment 530698 [details] [diff] [review]
Snapping the sidebars should be animated

Hey David,

See comment above. Do you have any ideas on how we could reduce the reflows done on a moz-transform transition? I noticed a significant improvement on device by just commenting out the reflowframe hint for changing the transform.
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-08 09:57:35 PDT
We currently need to reflow to update overflow areas; I'd like to add a method for updating overflow areas separate from reflow to avoid this problem; I think we have bugs on it.
Comment 8 Benjamin Stover (:stechz) 2011-05-10 09:27:44 PDT
David, do you have the bug #? Is this something you will be tackling in the Firefox 6 timeframe? Is it something I could help with?
Comment 9 Benjamin Stover (:stechz) 2011-05-24 10:31:03 PDT
Created attachment 534827 [details] [diff] [review]
WIP with updating overflow rect
Comment 10 Benjamin Stover (:stechz) 2011-05-24 10:35:35 PDT
Comment on attachment 534827 [details] [diff] [review]
WIP with updating overflow rect

It looks like FinishAndStoreOverflow applies all the necessary transformations, so I just restored the pre-transformed overflows. This isn't working though... For my tests, I see gray after one sidebar transition.

Daniel Holbert suggested I look at how reflow changed the frames and make sure my shortcut did the same thing. As far as I can tell, this code does the same thing to the overflow rects as reflow does for my test case.

David, do you have any advice?
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-24 13:19:19 PDT
We discussed this in front of a whiteboard this morning.  A few notes, though somewhat revised from this morning (based on thinking more about it while I'm writing this, including some important bits about handling scroll frames that I forgot):

The key flaw in the patch is failure to propagate overflow areas up to ancestors.

I had been thinking I'd like to be able to do general updating of overflow areas in response to dynamic changes.  But for changes to transforms (and probably also changes to absolute positioning) we can add a hint that's a good bit less work to do, and probably all we'll ever need.  The hint, when given on a frame, would mean that its affect on its ancestor's overflow area (including the conversion of its pre-transform to post-transform overflow area) needs to be updated, but that its overflow area relative to its own bounds doesn't require any updating.

We could handle this hint by:
 1. recomputing the pre-transform to post-transform overflow area on the
    target
 2. walking up the ancestor chain, for each ancestor:
    a. walk all its kids and accumulate their overflow areas, and also add in
       the overflow area from the bounds and from any native appearance, as
       we do in reflow
    b. merge them together and finish the overflow area computation
       - if the overflow areas didn't change, break out of the loop
    c. if the ancestor in question is a scrolled frame, call some method on
       its scroll frame to handle the update and then break out of the loop

This presumes (which we'd need to check) that there aren't any special cases in how any *containers* compute overflow.  (There certainly are special cases for a bunch of leaves, e.g., for text frame.)


As bug 504247 points out, there's also a special case needed for blocks where we'd need to update the overflow areas for lines.
Comment 12 Benjamin Stover (:stechz) 2011-05-24 14:49:47 PDT
Created attachment 534898 [details] [diff] [review]
WIP with updating overflow rect
Comment 13 Benjamin Stover (:stechz) 2011-05-24 15:03:11 PDT
Comment on attachment 534898 [details] [diff] [review]
WIP with updating overflow rect

Hey David,

Thanks for all the help so far. This is another WIP patch, but this time it renders correctly for my case! So things I need to know before we begin to do a more appropriate patch:
1) Is it OK to do this overflow calculation in the CSS constructor or where should this live?
2) What is the difference between nsBox and nsBoxFrame? I was surprised to see the only class they had in common was nsIFrame.
3) I assume we'll need to do something similar for HTML frames. Where is the common-case code for that? Hopefully both HTML and XUL frames can use the same method, and then we can refactor the individual reflow paths to use that method for calculating overflow in the common case.
4) You mentioned there may be some container frame special cases for overflow rects. I'm not really sure how to go about finding those, but I think I'll understand a lot more once I know where to do this for HTML frames.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-24 21:26:09 PDT
(In reply to comment #13)
> 2) What is the difference between nsBox and nsBoxFrame? I was surprised to
> see the only class they had in common was nsIFrame.

Every frame is an nsBox. That's sort of a legacy thing :-(. nsBoxFrame is the frame type for XUL container frames.

(In reply to comment #11)
>     c. if the ancestor in question is a scrolled frame, call some method on
>        its scroll frame to handle the update and then break out of the loop

If the scrollable overflow area for a scrolled frame changes, then we can just do a FrameNeedsReflow on the scrolled frame to ensure that the scrollframe's scrollbars get updated ... except when the scrollbar styles returned by nsIScrollableFrame::GetScrollbarStyles() are HIDDEN and the horizontal and vertical scroll offsets are both zero; in that case we don't have to do anything since we know no scrollbar state needs to change, nor do we have to change the scroll position (sometimes when the scrollable overflow area decreases, we need to scroll up or left).
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-25 15:25:36 PDT
Comment on attachment 534898 [details] [diff] [review]
WIP with updating overflow rect

A few naming changes:
  nsChangeHint_NeedOverflow -> nsChangeHint_UpdateOverflow
  PreTransformBBoxScrollableProperty -> PreTransformScrollableOverflowProperty
  (in nsCSSFrameConstructor.cpp) overflowFrame -> ancestor

In nsCSSFrameConstructor.cpp, you should not be calling frame->SetRect().

I think you should just implement OverflowChanged on nsIFrame (put
the implementation in nsFrame.cpp) rather than making it pure virtual.
(It still needs to be virtual to implement the handling for scrollframes.)

In OverflowChanged, you should use the nsIFrame child list methods,
and you should iterate over all child lists.  (Search for callers
of GetAdditionalChildListName to see how.)

You also need to implement the handling of scrollframes, probably per
roc's comment.  You'll need to override the method on both nsHTMLScrollFrame and nsXULScrollFrame, probably forwarding both to a helper method on nsGfxScrollFrameInner (their mInner, for code shared between them).

I also think you should reverse the meaning of the boolean return value --
make it return whether its own overflow changed.  (And it should have
a comment saying that it should get called when a child's overflow changed.)

The |differentOverflow| check is wrong, since FinishAndStoreOverflow
changes the overflow areas in various ways.  You really need to compare
the old overflow areas to the new ones *after* FinishAndStoreOverflow has
done its thing.

You should also remove the ComputesOwnOverflowArea() check.  That's
just an ugly piece of glue between XUL box layout and non-box reflow, so
we don't compute overflow areas twice.

You should also remove the DoesClipChildren() check once you add an 
override of the method on both classes of scroll frame (in
nsGfxScrollFrame.cpp).
Comment 16 Benjamin Stover (:stechz) 2011-06-08 11:31:46 PDT
> If the scrollable overflow area for a scrolled frame changes, then we can
> just do a FrameNeedsReflow on the scrolled frame to ensure that the
> scrollframe's scrollbars get updated ... except when the scrollbar styles
> returned by nsIScrollableFrame::GetScrollbarStyles() are HIDDEN and the
> horizontal and vertical scroll offsets are both zero; in that case we don't
> have to do anything since we know no scrollbar state needs to change, nor do
> we have to change the scroll position (sometimes when the scrollable
> overflow area decreases, we need to scroll up or left).

If I understand this right, the whole point is to avoid FrameNeedsReflow :(
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-08 20:42:38 PDT
Well, you only need to do FrameNeedsReflow on the scrolled frame, not on the descendant(s) whose overflow areas changed. That's a win.

And the rest of comment #14 explains how to avoid doing FrameNeedsReflow for overflow:hidden elements.
Comment 18 Benjamin Stover (:stechz) 2011-06-09 11:53:10 PDT
> A few naming changes:
>   nsChangeHint_NeedOverflow -> nsChangeHint_UpdateOverflow

Done.

>   PreTransformBBoxScrollableProperty ->
> PreTransformScrollableOverflowProperty

Done.

>   (in nsCSSFrameConstructor.cpp) overflowFrame -> ancestor

Done.

> In nsCSSFrameConstructor.cpp, you should not be calling frame->SetRect().

Yep, done.

> I think you should just implement OverflowChanged on nsIFrame (put
> the implementation in nsFrame.cpp) rather than making it pure virtual.
> (It still needs to be virtual to implement the handling for scrollframes.)

Done.

> In OverflowChanged, you should use the nsIFrame child list methods,
> and you should iterate over all child lists.  (Search for callers
> of GetAdditionalChildListName to see how.)

Thanks. Done.

> You also need to implement the handling of scrollframes, probably per
> roc's comment.  You'll need to override the method on both nsHTMLScrollFrame
> and nsXULScrollFrame, probably forwarding both to a helper method on
> nsGfxScrollFrameInner (their mInner, for code shared between them).

Yes. Done.

> I also think you should reverse the meaning of the boolean return value --
> make it return whether its own overflow changed.  (And it should have
> a comment saying that it should get called when a child's overflow changed.)

Good idea. Done.

> The |differentOverflow| check is wrong, since FinishAndStoreOverflow
> changes the overflow areas in various ways.  You really need to compare
> the old overflow areas to the new ones *after* FinishAndStoreOverflow has
> done its thing.

Good point! Done.

> You should also remove the ComputesOwnOverflowArea() check.  That's
> just an ugly piece of glue between XUL box layout and non-box reflow, so
> we don't compute overflow areas twice.

Done.

> You should also remove the DoesClipChildren() check once you add an 
> override of the method on both classes of scroll frame (in
> nsGfxScrollFrame.cpp).

Done.

This patch is now (seemingly) working perfectly for me. I am now storing the entirety of the overflow areas before they are touched at all by FinishAndStoreOverflow, just to make sure I was getting the exact results through the "normal" path, aside from the updated transform.

For my purposes, I never see the scroll frame needing to reflow anyway (yay!), so I left out roc's optimization above for now, with a comment.
Comment 19 Benjamin Stover (:stechz) 2011-06-09 11:54:10 PDT
Created attachment 538309 [details] [diff] [review]
Recompute overflow without reflowing for transforms
Comment 20 Benjamin Stover (:stechz) 2011-06-09 12:14:57 PDT
Hmm, I seem to be missing some file changes I made... Going to lunch, but will look at it afterwards.
Comment 21 Benjamin Stover (:stechz) 2011-06-09 15:39:24 PDT
Created attachment 538368 [details] [diff] [review]
Recompute overflow without reflowing for transforms

Updated patch with missing file, but sadly it means that I wasn't properly
testing earlier. Now, scroll frames do end up needing to be reflowed for me. :(
Doing an early return in the scroll frame creates painting problems.

David, I could still use a review on what I've done so far. Maybe I'm doing
something wrong.
Comment 22 Benjamin Stover (:stechz) 2011-06-14 11:05:44 PDT
Created attachment 539251 [details] [diff] [review]
Recompute overflow without reflowing for transforms

Updated with gfx scroll frame optimization. Still hoping for a comment on how correct this patch looks.
Comment 23 Benjamin Stover (:stechz) 2011-06-15 14:08:57 PDT
Created attachment 539655 [details] [diff] [review]
Recompute overflow without reflowing for transforms

OK! The problem was that the overflow rect was already transformed by the time
we got to invalidating the old area. This looks good for Fennec now.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-15 15:06:32 PDT
Comment on attachment 539655 [details] [diff] [review]
Recompute overflow without reflowing for transforms

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

::: layout/generic/nsFrame.cpp
@@ +4589,5 @@
> +    nsIFrame* child = GetFirstChild(childList);
> +    while (child) {
> +      nsOverflowAreas childOverflow =
> +        child->GetOverflowAreas() + child->GetPosition();
> +      overflowAreas.UnionWith(childOverflow);

You should be calling ConsiderChildOverflow.

@@ +4596,5 @@
> +    childList = GetAdditionalChildListName(listIndex++);
> +  } while (childList);
> +
> +  nsOverflowAreas oldOverflow = GetOverflowAreas();
> +  FinishAndStoreOverflow(overflowAreas, GetSize());

There's a problem here, which is that some reflow implementations do something special to compute the overflow area, like nsBlockFrame::ComputeOverflowAreas, so their overflow areas could be set incorrectly here. I'm afraid some tedious auditing of callers to FinishAndStoreOverflow is needed, and you may need to factor out some code into another virtual function you can call.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +3277,5 @@
> +    return changed;
> +  }
> +
> +  nsIScrollableFrame* sf = do_QueryFrame(mOuter);
> +  ScrollbarStyles ss = sf->GetScrollbarStyles();

Just call this->GetScrollbarStyles() directly, that's all the outer frame does.

@@ +3281,5 @@
> +  ScrollbarStyles ss = sf->GetScrollbarStyles();
> +  if (ss.mVertical == NS_STYLE_OVERFLOW_HIDDEN &&
> +      ss.mHorizontal == NS_STYLE_OVERFLOW_HIDDEN &&
> +      GetScrollPosition() == nsPoint()) {
> +    // No need to overflow in this special case. There are no scrollbars and we

"No need to reflow"

@@ +3291,5 @@
> +  nsPresContext* presContext = mOuter->PresContext();
> +  presContext->PresShell()->FrameNeedsReflow(
> +    mOuter, nsIPresShell::eResize, NS_FRAME_IS_DIRTY);
> +
> +  return changed;

I think you can always return false here. The overflow

::: layout/generic/nsGfxScrollFrame.h
@@ +494,5 @@
>    }
>    virtual PRBool IsScrollingActive() {
>      return mInner.IsScrollingActive();
>    }
> +  PRBool OverflowChanged() {

virtual

@@ +732,5 @@
>    }
>    virtual PRBool IsScrollingActive() {
>      return mInner.IsScrollingActive();
>    }
> +  PRBool OverflowChanged() {

virtual

::: layout/generic/nsIFrame.h
@@ +1739,5 @@
>    /**
> +   * Indicates that an overflow rect of this frame's children has changed.
> +   * @return PR_TRUE iff the overflow of this frame has changed.
> +   */
> +  virtual PRBool OverflowChanged();

Call it UpdateOverflow to indicate that this overflow areas of this frame should be modified if necessary.

::: layout/style/nsStyleStruct.cpp
@@ +2149,3 @@
>       */
>      if (mTransform != aOther.mTransform)
> +      NS_UpdateHint(hint, nsChangeHint_UpdateOverflow);

Include nsChangeHint_UpdateTransformLayer, surely? How would this work otherwise?
Comment 25 Benjamin Stover (:stechz) 2011-06-16 09:36:17 PDT
(In reply to comment #24)
> ::: layout/generic/nsFrame.cpp
> @@ +4589,5 @@
> > +    nsIFrame* child = GetFirstChild(childList);
> > +    while (child) {
> > +      nsOverflowAreas childOverflow =
> > +        child->GetOverflowAreas() + child->GetPosition();
> > +      overflowAreas.UnionWith(childOverflow);
> 
> You should be calling ConsiderChildOverflow.

Moved ConsiderChildOverflow to nsIFrame to do this.

> 
> @@ +4596,5 @@
> > +    childList = GetAdditionalChildListName(listIndex++);
> > +  } while (childList);
> > +
> > +  nsOverflowAreas oldOverflow = GetOverflowAreas();
> > +  FinishAndStoreOverflow(overflowAreas, GetSize());
> 
> There's a problem here, which is that some reflow implementations do
> something special to compute the overflow area, like
> nsBlockFrame::ComputeOverflowAreas, so their overflow areas could be set
> incorrectly here. I'm afraid some tedious auditing of callers to
> FinishAndStoreOverflow is needed, and you may need to factor out some code
> into another virtual function you can call.

OK, I can definitely work on nsBlockFrame. I'm currently looking through everything that implements ::Reflow to do this audit; should I be looking for anything else? I'll have a follow-up comment with specific questions.

> 
> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +3277,5 @@
> > +    return changed;
> > +  }
> > +
> > +  nsIScrollableFrame* sf = do_QueryFrame(mOuter);
> > +  ScrollbarStyles ss = sf->GetScrollbarStyles();
> 
> Just call this->GetScrollbarStyles() directly, that's all the outer frame
> does.

Ah, I assume you mean GetScrollbarStylesFromFrame? Done.

> 
> @@ +3281,5 @@
> > +  ScrollbarStyles ss = sf->GetScrollbarStyles();
> > +  if (ss.mVertical == NS_STYLE_OVERFLOW_HIDDEN &&
> > +      ss.mHorizontal == NS_STYLE_OVERFLOW_HIDDEN &&
> > +      GetScrollPosition() == nsPoint()) {
> > +    // No need to overflow in this special case. There are no scrollbars and we
> 
> "No need to reflow"

Done.

> 
> @@ +3291,5 @@
> > +  nsPresContext* presContext = mOuter->PresContext();
> > +  presContext->PresShell()->FrameNeedsReflow(
> > +    mOuter, nsIPresShell::eResize, NS_FRAME_IS_DIRTY);
> > +
> > +  return changed;
> 
> I think you can always return false here. The overflow

Incomplete thought? But this makes sense. If we schedule a reflow we might as well stop going up the ancestor chain.

> 
> ::: layout/generic/nsGfxScrollFrame.h
> @@ +494,5 @@
> >    }
> >    virtual PRBool IsScrollingActive() {
> >      return mInner.IsScrollingActive();
> >    }
> > +  PRBool OverflowChanged() {
> 
> virtual
> 
> @@ +732,5 @@
> >    }
> >    virtual PRBool IsScrollingActive() {
> >      return mInner.IsScrollingActive();
> >    }
> > +  PRBool OverflowChanged() {
> 
> virtual

Done.

> 
> ::: layout/generic/nsIFrame.h
> @@ +1739,5 @@
> >    /**
> > +   * Indicates that an overflow rect of this frame's children has changed.
> > +   * @return PR_TRUE iff the overflow of this frame has changed.
> > +   */
> > +  virtual PRBool OverflowChanged();
> 
> Call it UpdateOverflow to indicate that this overflow areas of this frame
> should be modified if necessary.

Done.

> 
> ::: layout/style/nsStyleStruct.cpp
> @@ +2149,3 @@
> >       */
> >      if (mTransform != aOther.mTransform)
> > +      NS_UpdateHint(hint, nsChangeHint_UpdateOverflow);
> 
> Include nsChangeHint_UpdateTransformLayer, surely? How would this work
> otherwise?

Yes, this was a casualty from a code cleanup. Done.
Comment 26 Benjamin Stover (:stechz) 2011-06-16 10:23:02 PDT
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsLeafFrame.cpp#81

I'm assuming that anything that doesn't have children will never see an overflow change from its children. :) Does everything that is a leaf derive from this? If so, we can override to give a warning if this is ever called on a child frame.

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp#627

This is a leaf frame, yes? So I think the "no children" argument applies?

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFirstLetterFrame.cpp#185

It shouldn't be possible to transform the first letter of a frame or any of its children.

---

I'm going to pause here because I'm not entirely confident I'm going about this the right way. It's looking like for a lot of cases we can say "this will never get called", and perhaps just in case fire a warning and reflow. We can add a helper to do this in nsIFrame (um, UnexpectedUpdateOverflow?), and have these frames call this method specially.
Comment 27 Benjamin Stover (:stechz) 2011-06-16 10:45:22 PDT
Hm, it looks like any computation of nsOverflowAreas is a better place to start:
http://mxr.mozilla.org/mozilla-central/search?string=nsOverflowAreas&find=layout
Comment 28 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-06-16 15:44:58 PDT
So, for a start, I think the special-case code in nsBlockFrame is really a special-case fix for a bug that we should have fixed in the general case, and is probably much easier to fix in the general case now than it once was.  In particular, we should just consider all margins as part of scrollable overflow (but not visual overflow).  I think that's the intent of what nsBlockFrame does differently.

(This probably requires an audit to make sure that we're ok with scrollable overflow being larger than visual overflow.)


Second, I tend to think probably roc or I should do the audit for other similar things; I think asking somebody new to the layout code to do that probably isn't a great idea (both because it's a lot harder and because it's easier to get wrong).  I could probably do this soon, but probably not this week.


roc, does this seem reasonable to you?
Comment 29 Benjamin Stover (:stechz) 2011-06-16 16:42:17 PDT
The special exceptions I see for nsBlockFrame:
1. The bottom margin is not added in children overflow rects, so it must be done for the parent. Why doesn't this logic apply to other margins? Having this margin outside of the child overflow makes OverflowChange not work for us. The simplest solution is to cache this calculation and include it when overflows need updating.
2. Likewise, the bottom padding needs to be applied here. Where are the other paddings applied? In the lines overflow? We could cache this too.
3. Bullet overflows are sometimes not applied by the lines so it is done explicitly. I'm assuming bullets are normal children of the frame. This should be covered by UpdateOverflow() I think?
4. Of course, this function uses lines to calculate the overflow instead of iterating through children. I'm not sure what the difference is here, but I assume it takes things like margins and padding and borders into account? This isn't something UpdateOverflow() for the current frame.
5. ocBounds is unioned in if there is a previous frame in flow. I don't know what this means exactly, but it looks like it just iterates through a subset of the child frames which is already covered by UpdateOverflow().
6. fcBounds is unioned, which I believe is already covered by UpdateOverflow().
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-16 21:36:10 PDT
(In reply to comment #25)
> > @@ +3291,5 @@
> > > +  nsPresContext* presContext = mOuter->PresContext();
> > > +  presContext->PresShell()->FrameNeedsReflow(
> > > +    mOuter, nsIPresShell::eResize, NS_FRAME_IS_DIRTY);
> > > +
> > > +  return changed;
> > 
> > I think you can always return false here. The overflow
> 
> Incomplete thought? But this makes sense. If we schedule a reflow we might
> as well stop going up the ancestor chain.

I was going to observe that scrollframes clip their contents, so changes to the overflow of the scrolled content can't affect the overflow of the scrollframe. However, your observation is also correct.
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-16 21:48:11 PDT
(In reply to comment #28)
> So, for a start, I think the special-case code in nsBlockFrame is really a
> special-case fix for a bug that we should have fixed in the general case,
> and is probably much easier to fix in the general case now than it once was.
> In particular, we should just consider all margins as part of scrollable
> overflow (but not visual overflow).  I think that's the intent of what
> nsBlockFrame does differently.

I agree.

> Second, I tend to think probably roc or I should do the audit for other
> similar things; I think asking somebody new to the layout code to do that
> probably isn't a great idea (both because it's a lot harder and because it's
> easier to get wrong).  I could probably do this soon, but probably not this
> week.
> 
> roc, does this seem reasonable to you?

Yes, except that I'm not eager to do the audit myself :-)

(In reply to comment #29)
> 1. The bottom margin is not added in children overflow rects, so it must be
> done for the parent. Why doesn't this logic apply to other margins?

nsBlockFrame includes left and top margins when computing the positions of lines. Right margins may in fact be an existing bug.

> Having
> this margin outside of the child overflow makes OverflowChange not work for
> us. The simplest solution is to cache this calculation and include it when
> overflows need updating.

I think David's right that we should just include margins in the scrollable overflow area of a frame. Then the special code in block frames can go away.

> 2. Likewise, the bottom padding needs to be applied here. Where are the
> other paddings applied? In the lines overflow? We could cache this too.

The bottom padding is included in the border-box so we shouldn't need to add that into the scrollable overflow area explicitly, since we always include the border-box.

> 3. Bullet overflows are sometimes not applied by the lines so it is done
> explicitly. I'm assuming bullets are normal children of the frame. This
> should be covered by UpdateOverflow() I think?

Yes.

> 4. Of course, this function uses lines to calculate the overflow instead of
> iterating through children. I'm not sure what the difference is here, but I
> assume it takes things like margins and padding and borders into account?
> This isn't something UpdateOverflow() for the current frame.

I don't think the lines are contributing anything to the overflow areas that the frames don't know about (assuming you traverse the float and abs-pos child lists, which you do).

> 5. ocBounds is unioned in if there is a previous frame in flow. I don't know
> what this means exactly, but it looks like it just iterates through a subset
> of the child frames which is already covered by UpdateOverflow().

Yes.

> 6. fcBounds is unioned, which I believe is already covered by
> UpdateOverflow().

Yes.
Comment 32 Benjamin Stover (:stechz) 2011-06-17 09:25:45 PDT
Can someone point me in the direction I need to go? Where do margins get added to overflow rects?

Also, what is the difference between scrollable overflow and visual overflow? Why were margins not included in scrollable overflow before?
Comment 33 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-06-17 14:32:24 PDT
(In reply to comment #32)
> Can someone point me in the direction I need to go? Where do margins get
> added to overflow rects?

It should be a relatively straightforward change in ConsiderChildOverflow, but it probably belongs in a separate patch (ideally underneath this one).  I would expect a few tests would likely need adjusting.

> Also, what is the difference between scrollable overflow and visual
> overflow? Why were margins not included in scrollable overflow before?

Mainly because we only started distinguishing between the two relatively recently.
Comment 34 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-17 15:13:48 PDT
(In reply to comment #33)
> (In reply to comment #32)
> > Can someone point me in the direction I need to go? Where do margins get
> > added to overflow rects?
> 
> It should be a relatively straightforward change in ConsiderChildOverflow,
> but it probably belongs in a separate patch (ideally underneath this one). 
> I would expect a few tests would likely need adjusting.

Wouldn't we add the margin to the scrollable overflow area in FinishAndStoreOverflow, so ConsiderChildOverflow doesn't need to get it? The margins should be added before we transform the scrollable overflow, I think.

I assume the idea here is that the margin on the scrollable overflow area will be the computed margin, not taking into account margin collapsing. I think that should be OK.
Comment 35 Benjamin Stover (:stechz) 2011-06-17 16:48:21 PDT
> Wouldn't we add the margin to the scrollable overflow area in
> FinishAndStoreOverflow, so ConsiderChildOverflow doesn't need to get it? The
> margins should be added before we transform the scrollable overflow, I think.

Yes.

> I assume the idea here is that the margin on the scrollable overflow area
> will be the computed margin, not taking into account margin collapsing. I
> think that should be OK.

As we discussed, for non-negative margins this would work fine since we only union these overflows together. However, even the possibility of having negative margins means we cannot simply union scrollable overflow areas together in the parent frame.

Take for instance:
<div id="parent">
  <div id="modelChild" style="margin-bottom: 20px; height: 10px"></div>
  <div id="blackSheep" style="margin-top: -20px; height: 10px"></div>
</div>

During reflow overflow computation for /parent/, we can't simply decide exclude the margin during /modelChild/'s FinishAndStoreOverflow because we have no idea what /blackSheep/ may contain.

This is problematic. At /parent/, we basically need to calculate a new overflow rect for /modelChild/; one that either doesn't include margins (and then unioning that with the bottom edge of children like we do now) or one that now factors in /blackSheep/.

I have ideas to circumvent this, but I doubt this refactoring will come out any cleaner than it is now.
Comment 36 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-17 16:51:04 PDT
I think we should just add the computed margin to the scrollable overflow area and ignore margin-collapsing, unless there's a good example of where that causes problems.
Comment 37 Benjamin Stover (:stechz) 2011-06-17 16:57:50 PDT
(In reply to comment #36)
> I think we should just add the computed margin to the scrollable overflow
> area and ignore margin-collapsing, unless there's a good example of where
> that causes problems.

See above. We possibly can't union child overflow rects if there was a negative margin somewhere in the descendants. Am I missing something?
Comment 38 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-17 17:26:12 PDT
We would ignore negative margins when modifying the scrollable overflow area. So in your example, modelChild would have scrollable overflow area (0,0,w,30) at position 0,0, and blackSheep would have scrollable overflow area (0,0,w,10) at position 0,10. So the computed scrollable overflow area for 'parent' would be (0,0,w,30). What's the problem here, other than we can scroll a bit further than we really need to?
Comment 39 Benjamin Stover (:stechz) 2011-06-20 10:16:20 PDT
I thought it would be a good idea to continue this discussion on bug 665597, as it is a rather separate issue from snappy sidebars. :)

If we really think this is acceptable, great. Otherwise, one really simple solution is to just cache the bottom edge of children and re-use it during overflow calculations.
Comment 40 Benjamin Stover (:stechz) 2011-06-29 11:10:27 PDT
Could we move forward on this? What if HTML elements continued to reflow and XUL did this overflow update thing? That would be enough to move forward with this bug.
Comment 41 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-29 19:20:11 PDT
Run over to dbaron's desk and harass him :-)
Comment 42 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-07-07 14:06:05 PDT
I'm looking through the code for exceptions that we need to think about.  So far I've found:

* blocks need to set overflow areas on line boxes
* scrollframes (of course)
* visibility:collapse in tables (which is currently somewhat broken)
* nsBox::IsCollapsed
* things that cause clipping ('clip', nsBox::DoesClipChildren)

That was from looking through callers of FinishAndStoreOverflow.  Now I need to look through everything that touches nsHTMLReflowState::mOverflowAreas...
Comment 43 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-07-08 16:09:35 PDT
I did the remainder of the code auditing:

Audit of mOverflowAreas
=======================

nsBlockFrame::ComputeOverflowAreas has special code for text-overflow
and for shadows

nsTableCellFrame::VerticallyAlignChild considers collapsing borders
as overflow (call to GetBorderOverflow())

nsTableFrame::Reflow does the same for tables (call to
GetExcludedOuterBCBorder)

nsMathMLContainerFrame::GatherAndStoreOverflow does some fun stuff
(shadows and char stuff)

Auditing callers of nsHTMLReflowMetrics::ScrollableOverflow() found nothing.
Auditing callers of nsHTMLReflowMetrics::VisualOverflow() found nothing.


Need to remember to apply GetWidgetOverflow to visual overflow.


The other danger (that's hard to audit for) is children we intentionally
do not add to the overflow areas.
Comment 44 Florian Hänel [:heeen] 2011-08-11 09:24:18 PDT
Created attachment 552392 [details] [diff] [review]
added transform hint, rebased

With the help of this patch we can have scrollbars on fennec with close to zero repaints, see bug 657893. So I really want this :)

I rebased it and fixed an apparent bug about updating the transform property.
Comment 45 Benjamin Stover (:stechz) 2011-08-11 09:50:16 PDT
(In reply to Florian Hänel [:heeen] from comment #44)
> Created attachment 552392 [details] [diff] [review]
> added transform hint, rebased
> 
> With the help of this patch we can have scrollbars on fennec with close to
> zero repaints, see bug 657893. So I really want this :)
> 
> I rebased it and fixed an apparent bug about updating the transform property.

This is going to take some work, heeen. See the comments above; there are a lot of edge cases about this, and we also need something like the patch from bug 665597. Perhaps we could convince dbaron to only allow this new overflowchanged behavior for XUL elements, and HTML elements could still trigger a reflow like the old scrollboxes?

David, what do you think?

Heeen, would you be interested in picking this patch up?
Comment 46 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-08-11 12:59:05 PDT
(In reply to Benjamin Stover (:stechz) from comment #45)
> This is going to take some work, heeen. See the comments above; there are a
> lot of edge cases about this, and we also need something like the patch from
> bug 665597. Perhaps we could convince dbaron to only allow this new
> overflowchanged behavior for XUL elements, and HTML elements could still
> trigger a reflow like the old scrollboxes?
> 
> David, what do you think?

So, about a quarter of the exceptions above apply only to XUL elements, so there'd still be work to do.

Secondly, I'm not sure what the "for XUL elements" test is.  You'd need a test that applies to the element and all its ancestors, and probably (though I haven't done a code audit for *that*) all children (but not all descendants) of any ancestors.

So I'm not at all convinced that such a restriction would reduce the amount of work left.
Comment 47 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-11 16:08:38 PDT
Comment on attachment 552392 [details] [diff] [review]
added transform hint, rebased

This is not ready for review yet.
Comment 48 Benjamin Stover (:stechz) 2011-08-18 09:35:57 PDT
Taking myself off as owner on the bug due to lots of bugs on my plate. Chris and I have some plans to work on this together after Chris is more up to speed.
Comment 49 Chris Lord [:cwiiis] 2011-08-22 11:07:49 PDT
Created attachment 554908 [details] [diff] [review]
Added special-case code to various frames, rebased

Hopefully this takes most of the previous comments into account and adds code to nsBlockFrame, nsTableCellFrame and nsTableFrame to update their overflow areas correctly(?)

I haven't touched nsMathMLContainerFrame yet, as it seems a bit hairy - do we want to just reflow in the case of these frames for now, or is it not as hard as it looks?

Not sure if how I've gotten the nsStyleDisplay in nsBlockFrame is correct (same in nsTableFrame), or in fact if anything I've done is correct :)

Feedback very much appreciated.
Comment 50 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-22 16:45:02 PDT
Comment on attachment 554908 [details] [diff] [review]
Added special-case code to various frames, rebased

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

Good stuff!

We don't need to do anything for text-overflow, since it happens entirely at paint time. We should also have sufficient invalidation already.

I think the remaining stuff from David's list that needs to be addressed is just:
* nsBox::IsCollapsed
* things that cause clipping ('clip', nsBox::DoesClipChildren)
* For nsMathMLContainerFrame I think we should just force a reflow. Sometimes MathML alters layout based on the overflow area. We don't need to be adding complexity to optimize transforms inside MathML containers :-).

::: layout/generic/nsFrame.cpp
@@ +4506,5 @@
> +      overflowAreas.UnionWith(childOverflow);
> +      child = child->GetNextSibling();
> +    }
> +    childList = GetAdditionalChildListName(listIndex++);
> +  } while (childList);

Here I think you need to exclude popup-list children.

::: layout/generic/nsIFrame.h
@@ +1734,5 @@
>  
>    /**
> +   * Updates the overflow areas of the frame. This can be called if an
> +   * overflow area of the frame's children has changed without reflowing.
> +   * @return PR_TRUE if the overflow of this frame has changed.

to be clear "return true if either of the overflow areas for this frame have changed".

::: layout/tables/nsTableFrame.cpp
@@ +1893,5 @@
> +      overflowAreas.UnionWith(childOverflow);
> +      child = child->GetNextSibling();
> +    }
> +    childList = GetAdditionalChildListName(listIndex++);
> +  } while (childList);

Probably should stick this code in a helper function in nsLayoutUtils.

@@ +1906,5 @@
> +  overflowAreas.UnionAllWith(bounds);
> +
> +  nsOverflowAreas oldOverflow = GetOverflowAreas();
> +  FinishAndStoreOverflow(overflowAreas, GetSize());
> +  return GetOverflowAreas() != oldOverflow;

Maybe these three lines should be shared too. Maybe FinishAndStoreOverflow could just return a boolean if it changed the areas?
Comment 51 Oleg Romashin (:romaxa) 2011-08-22 21:20:55 PDT
Comment on attachment 554908 [details] [diff] [review]
Added special-case code to various frames, rebased


>   // Compute our final size
>   ComputeFinalSize(*reflowState, state, aMetrics);
>-  ComputeOverflowAreas(*reflowState, aMetrics);
I'm wondering where did you get these sources? according to this commit:http://hg.mozilla.org/mozilla-central/rev/abef302b61be#l1.14 it is 2008... ?

>+  nsRect areaBounds = nsRect(0, 0, aMetrics.width, aMetrics.height);
>+  ComputeOverflowAreas(areaBounds, aReflowState.mStyleDisplay, aMetrics.mOverflowAreas);
>   // Factor overflow container child bounds into the overflow area
Comment 52 Chris Lord [:cwiiis] 2011-08-23 02:12:53 PDT
(In reply to Oleg Romashin (:romaxa) from comment #51)
> Comment on attachment 554908 [details] [diff] [review]
> Added special-case code to various frames, rebased
> 
> 
> >   // Compute our final size
> >   ComputeFinalSize(*reflowState, state, aMetrics);
> >-  ComputeOverflowAreas(*reflowState, aMetrics);
> I'm wondering where did you get these sources? according to this
> commit:http://hg.mozilla.org/mozilla-central/rev/abef302b61be#l1.14 it is
> 2008... ?
> 
> >+  nsRect areaBounds = nsRect(0, 0, aMetrics.width, aMetrics.height);
> >+  ComputeOverflowAreas(areaBounds, aReflowState.mStyleDisplay, aMetrics.mOverflowAreas);
> >   // Factor overflow container child bounds into the overflow area

Sorry, I forgot to add that this depends on the patch found in bug #665597 - added the dependency now.
Comment 53 Chris Lord [:cwiiis] 2011-08-23 10:27:55 PDT
Created attachment 555130 [details] [diff] [review]
Reflow MathML container frames, consider IsCollapsed, etc.

I think this is doing the other things that have been asked - again, if I've misinterpreted anything, please do enlighten me so I can fix it :)

To consider IsCollapsed(), I've removed the nsBoxLayoutState parameter - nothing used it anyway, so I figured this was better than trying to cache it when it's available.

How far are we away from getting this, and the patch in bug #665597 landed do you think?
Comment 54 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-23 16:58:39 PDT
Comment on attachment 555130 [details] [diff] [review]
Reflow MathML container frames, consider IsCollapsed, etc.

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

CSS "clip" is handled by FinishAndStoreOverflow already so you don't have to do anything extra there.

The rest looks good. This is great work by you and Ben.

I'm almost r+ here, just want to see the suggested change in nsFrame.cpp.

dbaron should review this too.

::: layout/base/nsLayoutUtils.cpp
@@ +166,5 @@
> +  PRInt32 listIndex = 0;
> +  do {
> +    nsIFrame* child = aFrame->GetFirstChild(childList);
> +    while (child) {
> +      if (!IsPopup(child)) {

More efficient to compare childList to nsGkAtoms::popupList

::: layout/generic/nsFrame.cpp
@@ +6188,4 @@
>  nsIFrame::FinishAndStoreOverflow(nsOverflowAreas& aOverflowAreas,
>                                   nsSize aNewSize)
>  {
> +  nsOverflowAreas oldOverflow = GetOverflowAreas();

Instead of doing this we can be slightly more efficient and have SetOverflowAreas and ClearOverflowRects return whether they changed the overflow area. It's pretty easy for the implementations of those methods to determine that.

Maybe it doesn't matter, but this code is hot.

::: layout/mathml/nsMathMLContainerFrame.cpp
@@ +880,5 @@
> +  // Our overflow areas may have changed, reflow the frame
> +  PresContext()->PresShell()->FrameNeedsReflow(
> +    this, nsIPresShell::eResize, NS_FRAME_IS_DIRTY);
> +
> +  return PR_TRUE;

I think when we force a reflow we can return false here since we can be sure things will be updated correctly, we don't need to bother propagating the change up further.

::: layout/mathml/nsMathMLContainerFrame.h
@@ +171,5 @@
>                                const nsRect&           aDirtyRect,
>                                const nsDisplayListSet& aLists);
>  
> +  virtual PRBool
> +  UpdateOverflow();

One line

::: layout/xul/base/src/nsBox.cpp
@@ +638,5 @@
> +  nsRect bounds(nsPoint(0,0), GetSize());
> +  nsOverflowAreas overflowAreas(bounds, bounds);
> +
> +  if (!DoesClipChildren() && !IsCollapsed())
> +    UnionChildOverflow(overflowAreas);

{}
Comment 55 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-23 17:01:45 PDT
(In reply to Chris Lord [:cwiiis] from comment #53)
> How far are we away from getting this, and the patch in bug #665597 landed
> do you think?

I think we're getting pretty close!
Comment 56 Chris Lord [:cwiiis] 2011-08-24 02:43:39 PDT
Created attachment 555350 [details] [diff] [review]
Small fix-ups

This does most of what you said - I didn't change SetOverflowAreas/ClearOverflowRects in the end - it's easy for ClearOverflowRects to know if something has changed, but SetOverflowAreas ends up doing about as much work as just doing the comparison anyway.

We could add an extra check below the visualOverflowChanged check, but I decided to keep the current check, but to short-circuit if there's no change in either area, and that ought to save us even more work anyway, I think?

If this looks ok, I'll add the r? flags for you and dbaron.
Comment 57 Chris Lord [:cwiiis] 2011-08-24 04:35:57 PDT
Created attachment 555360 [details] [diff] [review]
Small fix-ups

Realised that you couldn't short-circuit the function like that, so just added the extra scrollableOverflowChanged check below the visualOverflowChanged check.
Comment 58 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-24 17:41:25 PDT
Comment on attachment 555360 [details] [diff] [review]
Small fix-ups

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

r+ from me!

::: layout/base/nsLayoutUtils.cpp
@@ +174,5 @@
> +      }
> +      child = child->GetNextSibling();
> +    }
> +    childList = aFrame->GetAdditionalChildListName(listIndex++);
> +  } while (childList);

Stuff has landed on mozilla-inbound that changes the way we iterate through child lists, so you'll need to pull that and rebase on top of it.
Comment 59 Chris Lord [:cwiiis] 2011-08-25 03:37:32 PDT
Created attachment 555691 [details] [diff] [review]
Rebase with new childlist iteration

I'm re-adding the r? as I'm not sure I picked the correct list of children to iterate (all except pop-ups)

I've also not finished a build (had to clobber), but if this doesn't build I'll update it as soon as my build finishes with a fixed version (wanted to get it up ASAP for review/feedback).
Comment 60 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-25 17:51:38 PDT
Comment on attachment 555691 [details] [diff] [review]
Rebase with new childlist iteration

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

::: layout/base/nsLayoutUtils.cpp
@@ +182,5 @@
> +      nsOverflowAreas childOverflow =
> +        child->GetOverflowAreas() + child->GetPosition();
> +      aOverflowAreas.UnionWith(childOverflow);
> +    }
> +  }

Don't do this! Use FrameChildListIterator.
Comment 61 Chris Lord [:cwiiis] 2011-08-26 03:26:29 PDT
Created attachment 555988 [details] [diff] [review]
Use FrameChildListIterator

Used FrameChildListIterator.
Comment 62 Oleg Romashin (:romaxa) 2011-09-21 00:03:10 PDT
Any progress here? this is quite important for QGL rendering performance...
Comment 63 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-09-26 11:53:53 PDT
I have about 115 lines of review comments so far; still not through the patch yet, and I haven't started reviewing my previous comments.  Hopefully I'll finish the review within a day or two.
Comment 64 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-09-27 12:56:37 PDT
Comment on attachment 555988 [details] [diff] [review]
Use FrameChildListIterator

In the future, please do changes like the removal of the parameter to
IsCollapsed() as a separate patch rather than mixing it in to a
substantive patch.

In the IsCollapsed changes, you missed changing the implementation
of IsCollapsed in layout/forms/nsTextControlFrame.{h,cpp}
Please change the signature there too.

So the way that nsIFrame::PreTransformScrollableOverflowProperty() is
set is pretty odd -- I suggested that name because the existing
PreTransformBBoxProperty is essentially the pre-transform visual
overflow.  However, I didn't intend that the scrollable overflow should
have both overflow areas.  You should either:
 * merge the two into a single property, containing nsOverflowAreas, or
 * have two separate properties, each containing nsRect
rather than the current situation where we store the visual overflow
twice.  (It doesn't much matter where in FinishAndStoreOverflow the
*scrollable* overflow gets set, since FinishAndStoreOverflow should be
idempotent.  But where the visual overflow is stored may well matter for
the existing users of the PreTransformBBoxProperty.  Renaming
PreTransformBBoxProperty to PreTransformVisualOverflowProperty would be
entirely reasonable.)

In nsChangeHint.h, please add a comment above
nsChangeHint_UpdateOverflow saying:
  // The frame's effect on its ancestors overflow areas has changed,
  // either through a change in its transform or change in its position.

Given the current code in nsFrame::ConsiderChildOverflow, you're
breaking the IsTableClip case by not explicitly calling
ConsiderChildOverflow from nsLayoutUtils::UnionChildOverflow.  However,
the easiest way to fix that is to just fix that issue.  In other words,
remove the IsTableClip() check in ConsiderChildOverflow and add it in
nsFrame::FinishAndStoreOverflow as a third reason to enter this:
>  if (disp->mOverflowX == NS_STYLE_OVERFLOW_CLIP ||
>      nsFrame::ApplyPaginatedOverflowClipping(this)) {
>    // The contents are actually clipped to the padding area·
>    aOverflowAreas.SetAllTo(bounds);
>  }
The bug in your patch that this (I think) fixes is that if you
dynamically change a transform on something inside of a table cell (or
other table part) that has overflow:hidden, the overflow:hidden will
suddenly stop working.

nsBlockFrame::UpdateOverflow is incorrect.  This is because lines
have cached overflow areas (block frames group their children into
lines, but lines aren't strictly speaking part of the frame tree in
terms of the frame tree traversal apis), and
nsBlockFrame::UpdateOverflow simply digs through the existing cached
overflow areas.  I think the most straightforward fix here is to either
add code either:
 * directly in nsBlockFrame::UpdateOverflow
 * in nsBlockFrame::ComputeOverflowAreas, triggered by a boolean passed
   as true from UpdateOverflow and as false from the other callers
that recomputes all of the lines' overflow areas.  (Probably a little
better to put it in UpdateOverflow since then you can only walk the
lines once, and have better memory locality.  Er, actually, scratch
that... see my next review comment.)  This is probably easiest to do
using nsFrame::ConsiderChildOverflow.  Beware that iterating the
children of a line requires using line->GetChildCount(); if you don't
use that you'll just keep walking into all later lines and end up with
an O(N^2) algorithm.  The bug this causes is that any overflow changes
from transform changes simply don't propagate through blocks.  It would
be nice to add a reftest for this, and make sure it fails with your
current patch (probably easiest to do with scrollbars rather than trying
to do a repainting-based test).

Additionally, nsBlockFrame::UpdateOverflow needs to consider the other
child lists that are added to overflow areas after the call to
ComputeOverflowAreas in Reflow.  These are:
 * overflow containers (these occur only when paginating)
 * pushed floats (these occur only when paginating)
 * absolutely positioned elements
I'm inclined to suggest that you give up on the attempts to adapt
nsBlockFrame::ComputeOverflowAreas to be usable from
nsBlockFrame::UpdateOverflow (i.e., undo the changes to
ComputeOverflowAreas and IsClippingChildren), and then change
nsBlockFrame::UpdateOverflow to:
 * do the loop over the lines I described in my previous comment to
   update (since they'll need to be updated for the next time we Reflow)
 * then call nsLayoutUtils::UnionChildOverflow to rebuild the overflow
   areas, which will go through all child lists correctly

In nsIFrame::UpdateOverflow, I think the IsCollapsed() check is
incorrect.  IsCollapsed is a XUL layout method; though the XUL layout
methods have been folded into nsIFrame, its result is still only
meaningful for XUL boxes, and your check makes UpdateOverflow
inconsistent with the original (non-update) codepath.  The code that
checks IsCollapsed in the normal codepath is in nsBox::SyncLayout,
called from nsBox::Layout, called from nsIFrame::Layout, which is called
only when we're using the box layout protocol.  I believe the easiest
fix is to change:
>+  if (!IsCollapsed()) {
to:
+  bool collapsed = (IsBoxFrame() || IsBoxWrapped() && IsCollapsed();
+  if (!collapsed) {

nsGfxScrollFrameInner::UpdateOverflow should not be virtual; this adds
a vtable pointer to a class that does not currently have one and should
never need one.  (nsHTMLScrollFrame:: and
nsXULScrollFrame::UpdateOverflow should still be virtual.)

In nsIFrame.h, |friend class nsCSSFrameConstructor| shouldn't be needed.
Why is it needed?  Please get rid of it, and make what you need public
instead or use appropriate existing public methods.

nsGfxScrollFrameInner::UpdateOverflow has two serious problems.  Scroll
frames never have overflow area, since they always clip their children.
This means that calling the base class's UpdateOverflow method is
incorrect, since it means that it will set an inappropriate overflow
area.  Likewise, returning true is always incorrect.  You should revise
it to:
 * remove the chunk at the beginning that calls the base class's
   UpdateOverflow method (whose return value is pretty meaningless here)
 * always return false
It's probably best to invert the condition and move the FrameNeedsReflow
call inside it, and thus just have one return statement at the end of
the function.

In nsMathMLContainerFrame::UpdateOverflow, please put a "so" after the
first comma and a period at the end of both sentences.

In nsStyleDisplay::MaxDifference, could you at least wrap your revision
at less that 80 characters (and maybe the existing overlong line too)?

nsTableCellFrame::UpdateOverflow should use
nsLayoutUtils::UnionChildOverflow rather than calling
ConsiderChildOverflow directly.  There's other work in progress (for
absolute positioning inside any relatively positioned element) that's
likely to lead to table cells having other child lists, I think.

nsTableFrame::UpdateOverflow would be cleaner if you inflate the bounds
up front, before constructing overflowAreas.

In nsTableFrame::UpdateOverflow:
>+  const nsStyleDisplay* display = mContent->GetPrimaryFrame()->GetStyleDisplay();

mContent->GetPrimaryFrame() returns |this|, but very slowly.

>+  if (display->IsTableClip()) {

You're missing a ! here.

Combine the two, and make it just:

>if (!GetStyleDisplay()->IsTableClip()) {

(and, as I said above, earlier in the function).


nsBox::UpdateOverflow is wrong for a bunch of reasons.  It needs a
little background to explain, though.  XUL display types have a rather
distinct layout model:  most of the methods they use for position
calculation are different.  The class inheritance hierarchy used to
involve those XUL objects inheriting from both nsIFrame and nsBox.
However, to (a) simplify the interaction between XUL and non-XUL layout
and (b) reduce the memory consumption of XUL layout objects (reduce the
number of vtable pointers), nsBox was stuck into the inheritance
hierarchy for all frames; thus nsFrame now inherits from nsBox.
However, only frame classes that inherit from nsBoxFrame and
nsLeafBoxFrame (which are much further down in the hierarchy) actually
*use* the layout logic from nsBox.

What you've done in nsBox::UpdateOverflow is implemented an
UpdateOverflow method that actually applies to *all* frame classes.
Essentially, nsIFrame::UpdateOverflow is never used because
nsBox::UpdateOverflow is always called instead, for all concrete frame
types.  This is a problem in three ways:
 * there's code in nsBox::UpdateOverflow that should only apply to real
   XUL boxes and not to other frame types.  What you've done makes
   certain XUL concepts apply to other frame types in an inconsistent
   way (i.e., only after certain dynamic changes have taken place)
 * it's confusing -- you have an nsIFrame::UpdateOverflow implementation
   that's never used
 * what you've done actually *doesn't work* on non-XUL-box frames,
   because it uses GetChildBox() for frame tree iteration (which there's
   actually no need to do).  So as far as I can tell this makes the
   patch simply not work for most cases.
There's also code in nsBox::UpdateOverflow -- in particular, the view
update code, that really does need to apply to all frame classes.

There shouldn't be an nsBox::UpdateOverflow.  Everything from it that's
needed should be merged into nsIFrame::UpdateOverflow -- this includes
the DoesClipChildren() check (which, like the IsCollapsed check, should
only be tested on box frames) and the view updating code.


Now, to look through the parts of comment 42 and comment 43 to make sure
they're all addressed (either in the patch or in comments above made
while reviewing it):  the following points seem not to be addressed:

from comment 42:
>* visibility:collapse in tables (which is currently somewhat broken)

from comment 43:
>nsBlockFrame::ComputeOverflowAreas has special code for text-overflow
>and for shadows
(addressed in the current version, but needs to be readdressed given my
nsBlockFrame comments above)

Given the current broken state of visibility:collapse in tables, I
supposed I'd be ok postponing dealing with that to a followup bug.



Given the errors I *found*, I suspect there are some that I didn't find
and thus I suspect this patch is likely to break Web content due to
errors in the patch.  I think it would probably be a good idea to try
writing some automated tests that test this patch's effect on Web
content before landing it, given that most of the code in the patch
didn't actually achieve what it was intending to do.

Testing that scrollable overflow areas have been dynamically updated
correctly is relatively straightforward:  you can examine the extents of
scrollbars.  Testing that visual overflow areas are updated correctly is
a little harder, since it involves tests involving repainting -- and you
need to hit a relevant repainting codepath without doing something that
triggers reflow of the subtree.  Testing that you've found such a
codepath probably may require poking at things in the debugger or with
printfs.

Marking review- because I think I do need to look at another revision of
this one.
Comment 65 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-09-27 12:58:50 PDT
(In reply to David Baron [:dbaron] from comment #64)
> Testing that scrollable overflow areas have been dynamically updated
> correctly is relatively straightforward:  you can examine the extents of
> scrollbars.  Testing that visual overflow areas are updated correctly is
> a little harder, since it involves tests involving repainting -- and you
> need to hit a relevant repainting codepath without doing something that
> triggers reflow of the subtree.  Testing that you've found such a
> codepath probably may require poking at things in the debugger or with
> printfs.

However, I should also note:  given the similarity of the two codepaths,
it may well be sufficient to test only scrollable overflow for now.
Comment 66 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-09-27 13:12:50 PDT
(In reply to David Baron [:dbaron] from comment #64)
> So the way that nsIFrame::PreTransformScrollableOverflowProperty() is
> set is pretty odd -- I suggested that name because the existing
> PreTransformBBoxProperty is essentially the pre-transform visual
> overflow.  However, I didn't intend that the scrollable overflow should
> have both overflow areas.  You should either:
>  * merge the two into a single property, containing nsOverflowAreas, or
>  * have two separate properties, each containing nsRect

And, on second thought, I think a single property (say,
PreTransformOverflowProperty) is clearly better.  It would need to be
set where the PreTransformBBoxProperty is currently set.
Comment 67 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-27 15:11:28 PDT
Wow, my review was very feeble. Thanks David!
Comment 68 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-10-03 17:18:03 PDT
(In reply to David Baron [:dbaron] from comment #43)
> I did the remainder of the code auditing:
> 
> Audit of mOverflowAreas
> =======================
> 
> nsBlockFrame::ComputeOverflowAreas has special code for text-overflow
> and for shadows

The shadows bit was removed in https://hg.mozilla.org/mozilla-central/rev/1d2b61ed8e44 ; I'm not sure where I got the text-overflow bit from, since I can't find anything now.
Comment 69 Chris Lord [:cwiiis] 2011-10-04 13:50:45 PDT
Created attachment 564660 [details] [diff] [review]
Address some of dbaron's review

ok, here's a patch with most (but not all) of the comments addressed, hopefully. 

Things I've not done:
* Combine PreTransformBBoxProperty / PreTransformScrollableOverflowProperty properties
* Write tests

Any suggestions for tests would be good - would animating a CSS translate transform and checking the scrollHeight DOM property of a containing element with overflow: auto/scroll be enough?

Asking for feedback to make sure I've interpreted the comments correctly, but not review yet as I realise even if I have, it isn't quite ready yet.
Comment 70 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-10-04 13:55:49 PDT
(In reply to Chris Lord [:cwiiis] from comment #69)
> Any suggestions for tests would be good - would animating a CSS translate
> transform and checking the scrollHeight DOM property of a containing element
> with overflow: auto/scroll be enough?

If by animating, you mean dynamically changing (just need to change it once), yes.  Note that you want to change from translate(0) to translate(something else) (rather than changing from 'none'), that the thing you check scrollHeight on has to be scrollable (overflow:hidden may be easiest), and that the interesting thing about the tests is what is an ancestor of the element with the transform but still a descendant of the scrollable element -- since what you want to test is how overflow propagates through different things.
Comment 71 Chris Lord [:cwiiis] 2011-10-04 17:01:53 PDT
Created attachment 564707 [details] [diff] [review]
Address all(?) of dbaron's review

I've now combined the PreTransformBBoxProperty and PreTransformScrollableOverflowProperty into a single PreTransformOverflowAreasProperty.

I've also written a small test (which I'll attach afterwards) which shows that this isn't working correctly... I'll go through debugging it, but any feedback would be greatly appreciated.
Comment 72 Chris Lord [:cwiiis] 2011-10-04 17:05:12 PDT
Created attachment 564708 [details]
Test if scrollable overflow is updated correctly after changing a CSS transform

The attached test works in current Firefox/fennec but breaks when this patch and the patch from bug #665597 are applied. I'm in the process of debugging this.
Comment 73 Chris Lord [:cwiiis] 2011-10-04 18:20:18 PDT
ok, after looking over this a bit, I think the logic might be wrong... This test fails because the overflow of container2 doesn't change (because it's overflow: visible). So, either this is a bug that the outer container becomes scrollable (given the intermediate overflow:visible, should it?), or the logic of breaking when overflow areas don't change isn't quite right.

If it's a bug, that's interesting, if not, should the logic be changed so that if our overflow hasn't changed because we're clipping (or non-scrollable), we should return true from UpdateOverflow?
Comment 74 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-10-04 18:31:15 PDT
(In reply to Chris Lord [:cwiiis] from comment #73)
> ok, after looking over this a bit, I think the logic might be wrong... This
> test fails because the overflow of container2 doesn't change (because it's
> overflow: visible). So, either this is a bug that the outer container
> becomes scrollable (given the intermediate overflow:visible, should it?), or
> the logic of breaking when overflow areas don't change isn't quite right.

The overflow of container2 should change.
Comment 75 Chris Lord [:cwiiis] 2011-10-05 12:07:28 PDT
Created attachment 564948 [details] [diff] [review]
Address review, fix bad misunderstanding/typo

Either I misunderstood something dbaron said on IRC, or I made a typo interpreting it, but that's what caused the test to fail. Here's the revised patch for review.
Comment 76 Chris Lord [:cwiiis] 2011-10-05 12:08:56 PDT
Created attachment 564950 [details] [diff] [review]
part 6, add overflow handling test

This patch adds a test to the layout tests that checks whether dynamic transform changes result in the correct scrollable overflow being propagated through parent frames.
Comment 77 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-10-10 10:15:58 PDT
Comment on attachment 564950 [details] [diff] [review]
part 6, add overflow handling test

This is good, though it would probably also be a good idea to test overflow propagation through a few other types of things (e.g., some things that should clip the overflow like scroll frames ('overflow'), 'clip', collapsed XUL boxes, and perhaps also some things that should propagate the overflow).

r=dbaron on this
Comment 78 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-10-10 11:33:14 PDT
Comment on attachment 564948 [details] [diff] [review]
Address review, fix bad misunderstanding/typo

Comments based on reviewing the diff between the previous patch I
reviewed and this one:

>+  // The frame's effect on its ancestors overflow areas has changed,
>+  // either through a change in its transform or a change in its position.

s/ancestors/ancestors'/
(my fault!)

nsBlockFrame::UpdateOverflow should, at the end, call through to
its immediate base class, i.e.:
  return nsBlockFrameSuper::UpdateOverflow();
instead of:
  return nsFrame::UpdateOverflow();
so that if a new override is added in between, nsBlockFrame will pick
it up.

nsBlockFrame::UpdateOverflow should not have this bit:
>+  const nsStyleDisplay* display = mContent->GetPrimaryFrame()->GetStyleDisplay();
>+  if (IsClippingChildren(this, display))
>+    return PR_FALSE;
It still needs to update the overflow in its line boxes, and nsFrame's
UpdateOverflow has the appropriate checks.  Just remove these lines.

Still in nsBlockFrame::UpdateOverflow:
>+      ConsiderChildOverflow(lineAreas, lineFrame);
>+      lineAreas.UnionWith(lineFrame->GetOverflowAreas());
The first line should be sufficient.  The second line is incorrect, since
it unions in a rectangle that's in a different coordinate system
(compare it to what nsFrame::ConsiderChildOverflow does).  Remove the
second of these lines.

You shouldn't need to move GetLayoutFlags and DoesClipChildren from
nsBox to nsIFrame now that the base UpdateOverflow is on nsFrame
rather than nsIFrame.  Put them back where they were.

In nsFrame::UpdateOverflow:
>+    PRUint32 flags = 0;
>+    GetLayoutFlags(flags);
>+
>+    nsIView* view = GetView();
>+    if (view && (flags & NS_FRAME_NO_SIZE_VIEW) == 0) {

Could you null-check |view| before calling the virtual and rarely-needed
|GetLayoutFlags| function?

Also, please run the boolean updating scripts from mwu's blog to
convert PRBool->bool, PR_FALSE->false, and PR_TRUE->true.

>+  bool hasTransform = IsTransformed();
>+  if (hasTransform) {
>+    Properties().Set(nsIFrame::PreTransformOverflowAreasProperty(),
>+                     new nsOverflowAreas(aOverflowAreas));
>+  } else {
>+    Properties().Delete(nsIFrame::PreTransformOverflowAreasProperty());
>+  }

You can remove this; setting it below is sufficient, except that you'll
need to add the |Delete| bit to the part below.

In nsFrame::ConsiderChildOverflow, also remove |disp|.

In nsGfxScrollFrameInner::UpdateOverflow, the brace-indent should be
2 spaces, not 4 (so it shouldn't line up with the multiline if
condition).

Also, I don't think you really need the |presContext| temporary; may
as well just use one line.



Comments based on going through my previous review comments (comment 64)
in relation to this patch:

Still need to file a followup bug on 'visibility: collapse' in tables.

r=dbaron with those things fixed, though I think some additional tests
would also be a good idea
Comment 79 Chris Lord [:cwiiis] 2011-10-10 12:28:30 PDT
Created attachment 565991 [details] [diff] [review]
Rebase on changes to bug #665597
Comment 80 Oleg Romashin (:romaxa) 2011-10-10 13:22:33 PDT
Tested latest reviewed patch with CSS3D transforms and still see the problem with rendering some Text objects...
http://romaxa.bolshe.net/css3d/morphing/morphing-cubes.html
http://romaxa.bolshe.net/css3d/poster/poster-circle.html

with these patches "Text frames" are disappearing, with previous patches I've seen also whole morphing planes disappeared too, now is only text
Comment 81 Chris Lord [:cwiiis] 2011-10-27 08:08:59 PDT
Created attachment 569978 [details] [diff] [review]
Rebase and address comment #78

This is a rebased patch with all issues from comment #78 addressed. I also tested the two links in comment #80 and both appear fine for me, but I'd appreciate feedback to see if they work ok for you.

The last time I pushed this to try, some tests failed, so I won't check this in until I have positive feedback and all tests pass. This patch should not change any rendering behaviour.
Comment 82 Oleg Romashin (:romaxa) 2011-10-27 13:50:41 PDT
Comment on attachment 569978 [details] [diff] [review]
Rebase and address comment #78

Checked this patch and it seems to work fine for me
Comment 83 Mardeg 2011-11-20 15:28:28 PST
Should bug 157681 depend on this one?
Comment 84 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-20 15:35:49 PST
Yes.
Comment 85 Mats Palmgren (:mats) 2011-12-13 07:26:45 PST
Created attachment 581267 [details] [diff] [review]
Updated to trunk

Minor edits to make it compile.
Comment 86 Mats Palmgren (:mats) 2011-12-13 07:35:19 PST
Created attachment 581270 [details] [diff] [review]
Bug fix and style nits

The failed test was layout/reftests/bugs/609272-1.html
The bug was that IsTableClip() was used on all types of frames,
it's only valid for table frames.
Comment 87 Mats Palmgren (:mats) 2011-12-13 07:40:59 PST
BTW, do we still need to reflow for a transform-origin change? or could we use
nsChangeHint_UpdateOverflow for the other transform properties as well?  
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#2220
Comment 88 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-13 13:26:19 PST
Comment on attachment 581270 [details] [diff] [review]
Bug fix and style nits

+      (GetType() == nsGkAtoms::tableFrame && disp->IsTableClip()) ||

Reverse the order of the && parts. disp->IsTableClip will be fast, but GetType() is a new virtual call.

Seems to me, looking at ApplyOverflowHiddenClipping, we should be checking or tableCellFrame/bcTableCellframe too? Maybe we should have a new IsFrameOfType bit instead of hardcoding frame types?

Seems to me your assertion will fail in calls to IsTableClip from nsFrame::ConsiderChildOverflow. How about we pass in the frame and have IsTableClip check the frame type and return false if the frame is not a suitable table part? Then ApplyOverflowHiddenClipping would call IsTableClip instead of checking the frame type itself.
Comment 89 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-13 13:29:40 PST
(In reply to Mats Palmgren [:mats] from comment #87)
> BTW, do we still need to reflow for a transform-origin change? or could we
> use
> nsChangeHint_UpdateOverflow for the other transform properties as well?  
> http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.
> cpp#2220

I think we could avoid reflowing for all those property changes, yes.
Comment 90 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-13 13:31:15 PST
We could also do this for 'outline' changes, but that should probably be a separate patch or bug.
Comment 91 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-13 22:52:42 PST
(In reply to Mats Palmgren [:mats] from comment #85)
> Created attachment 581267 [details] [diff] [review]
> Updated to trunk
> 
> Minor edits to make it compile.

It looks like you also updated it to no longer be on top of bug 665597 -- which means that I think it will now cause incremental reflow bugs related to bottom margins.  The reason for bug 665597 was to avoid that -- though we might end up having to take the alternative approach of never including margins in overflow rather than (as bug 665597 does) always including them -- to replace the current state of sometimes including them that's hard to maintain with this new code.
Comment 92 Mats Palmgren (:mats) 2011-12-14 17:26:20 PST
Created attachment 581847 [details] [diff] [review]
Updated to trunk, on top of bug 665597

This is on top of the patches in bug 665597.
Comment 93 Mats Palmgren (:mats) 2011-12-14 17:36:45 PST
Created attachment 581849 [details] [diff] [review]
additional fixes

> Seems to me, looking at ApplyOverflowHiddenClipping, we should be checking
> or tableCellFrame/bcTableCellframe too?

Yes.  We have quite a few of these test functions so I replaced them
with just one.  While we cannot possibly match ::blockFrame for the
calls inside nsTableFrame and vice-versa for the calls in nsBlockFrame,
I think it's worth it to consolidate these tests into one function.

> Maybe we should have a new
> IsFrameOfType bit instead of hardcoding frame types?

It's now only in one place again, so it didn't seem worth abstracting
for just one call site.

So the changes are:
1. consolidate ApplyOverflowHiddenClipping/ApplyOverflowClipping/
ApplyPaginatedOverflowClipping/IsTableClip into one function -
  static bool nsFrame::ApplyOverflowClipping

2. use nsChangeHint_UpdateOverflow for all transform style changes
   (except for BackfaceVisibility as before)

3. fix a bug in nsBlockFrame::ComputeOverflowAreas() that the original
   patch introduced when changing 'aReflowState.mComputedBorderPadding'
   to 'GetUsedPadding()'.

(Bug 665597 causes plenty of reftest failures though, with or without
these patches.)
Comment 94 Mats Palmgren (:mats) 2012-01-03 13:14:17 PST
Created attachment 585533 [details] [diff] [review]
part 1, recompute overflow without reflowing for transforms

Updated to trunk, on top of new set of patches in bug 665597.
No changes from last version.
Comment 95 Mats Palmgren (:mats) 2012-01-03 13:17:20 PST
Created attachment 585535 [details] [diff] [review]
part 2, additional fixes

See comment 93 for a description of the changes.  This new version is just
an update over the new patches in bug 665597, no new changes.
Comment 96 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-03 13:38:31 PST
Comment on attachment 585535 [details] [diff] [review]
part 2, additional fixes

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

Let's land the nsStyleStruct.cpp changes as a separate patch for better bisection.

::: layout/generic/nsBlockFrame.cpp
@@ +950,5 @@
>    // If we have non-auto height, we're clipping our kids and we fit,
>    // make sure our kids fit too.
>    if (aReflowState.availableHeight != NS_UNCONSTRAINEDSIZE &&
>        aReflowState.ComputedHeight() != NS_AUTOHEIGHT &&
> +      nsFrame::ApplyOverflowClipping(this, aReflowState.mStyleDisplay)) {

why is the nsFrame:: qualifier needed, here and a lot of other places below in nsFrame subclasses? Get rid of it :-)

::: layout/generic/nsFrame.cpp
@@ +1395,5 @@
>    // Other overflow clipping is applied by nsHTML/XULScrollFrame.
> +  // We allow -moz-hidden-unscrollable to apply to any kind of frame. This
> +  // is required by comboboxes which make their display text (an inline frame)
> +  // have clipping.
> +  if (nsFrame::ApplyOverflowClipping(aFrame, aDisp)) {

Write it as if (!...) return false;

::: layout/generic/nsFrame.h
@@ +600,5 @@
> +      // Actually there were also table rows and the inner table frame
> +      // doing this, but 'overflow' isn't applicable to them according to CSS
> +      // 2.1 so I (roc) removed them. Also, we used to clip at tableOuterFrame
> +      // but we should actually clip at tableFrame (as per discussion with Hixie
> +      // and bz).

Just remove this comment.
Comment 97 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-03 13:41:06 PST
Also, I'd like to have a patch in this bug that makes 'outline' changes use the UpdateOverflow hint. The reason I'd like that in this bug is that should produce a good performance improvement in a lot of situations (changing focus rings should no longer require a reflow). It may also expose bugs in the infrastructure here.
Comment 98 Mats Palmgren (:mats) 2012-01-04 14:39:02 PST
For the record, the latest patch set does fail a reftest on Android:
https://tbpl.mozilla.org/?tree=Try&rev=f3b461d774ed
The test is layout/reftests/transform/dynamic-inherit-1.html
It's a dynamic -moz-transform-origin change so I suspect the patches in this bug
rather than bug 665597.
Comment 99 Mats Palmgren (:mats) 2012-01-05 05:49:11 PST
Created attachment 586048 [details] [diff] [review]
part 4, Make outline style changes update the overflow and repaint instead of reflow
Comment 100 Mats Palmgren (:mats) 2012-01-05 05:56:16 PST
Created attachment 586049 [details] [diff] [review]
part 2, Update the overflow for all the affected frames, not just the primary frame. And the ancestors of those.

> Also, I'd like to have a patch in this bug that makes 'outline' changes use
> the UpdateOverflow hint.
> It may also expose bugs in the infrastructure here.

Yes, it did - we need to update the overflow for all continuations
and their ancestors (but not the ancestors' continuations, right?).
Comment 101 Mats Palmgren (:mats) 2012-01-05 06:03:33 PST
Created attachment 586050 [details] [diff] [review]
part 3, Make all transform style changes update the overflow and repaint instead of reflow

> Let's land the nsStyleStruct.cpp changes as a separate patch
> for better bisection.

Ok, this is that patch (with a fix for the Android failure - I mistakenly
changed Repaint to UpdateTransformLayer hint in the last version)
Comment 102 Mats Palmgren (:mats) 2012-01-05 06:05:52 PST
Created attachment 586051 [details] [diff] [review]
part 5, Consolidate overflow clipping checks to nsFrame::ApplyOverflowClipping(); and fix some code style nits

With nits in comment 96 addressed as suggested.
Comment 104 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-05 12:29:25 PST
Comment on attachment 586049 [details] [diff] [review]
part 2, Update the overflow for all the affected frames, not just the primary frame. And the ancestors of those.

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

The code to only update the parent when we hit the last continuation with that parent is great, but the hashtable optimization doesn't seem right. If you have two continuations, A and B, whose parents AP and BP are different, but their parent C is the same, then updating overflow on A, then AP, then C, then B and BP but not C again could be incorrect. The contribution of BP's updated overflow area to C would not be recorded.

I'd just rip out the hashtable optimization. I doubt it will help very much in practice, since at some point up the ancestor chain, probably not very far, the overflow area will stop changing as we process each continuation, at which point we stop walking the chain, so this should not be expensive even though we have to visit some frames multiple times. And updating overflow only on frames with many continuations is likely to be very rare. And fixing the optimization to be correct would add a lot of complexity, it would have to become some kind of bottom-first traversal of the frame tree.
Comment 105 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-05 12:48:53 PST
Comment on attachment 586049 [details] [diff] [review]
part 2, Update the overflow for all the affected frames, not just the primary frame. And the ancestors of those.

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

r+ if you remove the hashtable optimization.
Comment 106 Mats Palmgren (:mats) 2012-01-14 10:01:16 PST
Created attachment 588661 [details] [diff] [review]
part 2, Update the overflow for all the affected frames, not just the primary frame. And the ancestors of those.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #104)
Yes, good points, thanks.  I removed the hashtable optimization.
I'll push this bug + bug 665597 when the tree reopens.
Comment 107 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-14 12:18:20 PST
I suggest letting bug 665597 go into a nightly before landing this bug, to make regression hunting easier.

I also suggest filing a followup bug to have a good look at CalcDifference implementations and see which reflow hints can be converted to UpdateOverflow. I think there's quite a few that can be converted...
Comment 110 Timothy Nikkel (:tnikkel) 2015-02-09 16:58:12 PST
Part 4 was backed out in bug 724432.

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