1243610 - Refactor UpdateOverflow to separate out local overflow from descendants

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

This was initially discussed in bug 1198135 comment 9.

To correctly compute the overflow areas of preserve-3d children, we need to do custom processing of the descendants.

To do this we want to split UpdateOverflow into two pieces, one that computes the overflow for the frame itself, and one that accumulates the overflow areas of children.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Split UpdateOverflow

Comment 2

[Matt Woodrow (:mattwoodrow)]

One thing I noticed is that nsBlockFrame::UpdateOverflow (now nsBlockFrame::UnionChildOverflow) has special handing for line children.

I suspect this means that RecomputePreserve3DChildrenOverflow is potentially incorrect for block frames since it just walks the child lists instead.

I don't really understand the lines code, so I'm not sure why we need to do this.

I think the best fix (as a follow-up) would be to make nsIFrame::UnionChildOverflow (along with nsLayoutUtils::UnionChildOverflow) 'preserve-3d aware' and then have RecomputePreserve3DChildrenOverflow use that instead of having it's own implementation of unioning child overflow areas.

This is an existing bug (not affected by these changes), but still something we'd want to fix before bug 1198135.

Comment 3

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

Comment on attachment 8737011 [details] [diff] [review]
Split UpdateOverflow

It would have been nice to have the refactoring of GetLayoutFlags
in a separate patch underneath.

Given that you're promoting GetLayoutFlags from nsBox to nsIFrame,
please rename it to GetXULLayoutFlags, and put it in the part of
the nsIFrame.h header with the other XUL methods, rather than
next to the overflow methods.  (Search for "GetXUL"; you'll find a
whole bunch together.)

Please remove the:
   // XXX Maybe these three should be a separate interface?
right above UpdateOverflow in nsIFrame.h.  Looking at the changeset
where it was added:
https://github.com/mozilla/gecko-dev/commit/a12c3d6c5d1fa18ef018f40a2738bc2e8295416b
it's not clear if the comment referred to the 3 before the comment or
the three after, but either way, we don't want to do it and the comment
has become very unclear.

I'm not convinced that DidUpdateOverflow and ComputeCustomOverflow need
to be two separate things.  Why can't the callbacks just happen in
ComputeCustomOverflow?  (In the case of ScrollFrameHelper, after it
returns false.)  I'm OK with fixing that in a followup bug if you think
it's risky, though.

I also think that it's not OK to skip DidUpdateOverflow if
FinishAndStoreOverflow returns false, since a number of the things in
DidUpdateOverflow callbacks are about things that need to be cleared
when overflow of children change.

I think it's also probably cleaner for all of the derived
ComputeCustomOverflow and (if it stays) DidUpdateOverflow methods to end
with a call to the base class method.  Otherwise things seem risky for
future introductions of new overrides.


r=dbaron with those changes

Sorry for the massive delay getting to this.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35b6bedcbb2e

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35b6bedcbb2e