1655856 - nsPageContentFrame's Reflow methods has suspicious reuse of its ReflowOutput parameter

Status: RESOLVED FIXED

(Core :: Printing: Output, task, P3)

Description

[Daniel Holbert [:dholbert]]

In bug 1655608, I'm noticing that nsPageContentFrame's and nsPageFrame's Reflow methods reuse the passed-in ReflowOutput parameter when reflowing their child frame(s).

They generate a ReflowInput for their child frame, but they don't generate a ReflowOutput object for the child frame to use in child reflow -- instead, they pass in the ReflowOutput that they themselves received as an arg.

You can see this via our passing through aDesiredSize to ReflowChild in these two lines:
https://searchfox.org/mozilla-central/rev/d9f92154813fbd4a528453c33886dc3a74f27abb/layout/generic/nsPageFrame.cpp#137
https://searchfox.org/mozilla-central/rev/d9f92154813fbd4a528453c33886dc3a74f27abb/layout/generic/nsPageContentFrame.cpp#60

(Note: I'll be renaming these aDesiredSize vars to aReflowOutput in bug 1655608, but the problem will still be present.)

The current setup may technically work (it probably does, partially or entirely), but it's a bit suspicious and doesn't match the way we normally set things up for reflow, and it could cause trouble in edge cases.

Comment 1

[Daniel Holbert [:dholbert]]

Looks like Emilio fixed the nsPageFrame version of this in https://phabricator.services.mozilla.com/D85273 - thanks emilio!

So, the only ReflowOutput fishiness that remains (for this bug here) is in nsPageContentFrame::Reflow. (Maybe/hopefully that one is also relatively trivial to address.)

Comment 2

[Daniel Holbert [:dholbert]]

Relatedly: these same classes reuse their ReflowStatus aStatus parameter.

This is sloppy, and it can produce the odd result of these classes reporting themselves as overflow-incomplete[1], though arguably that makes no sense for these classes. (This creates more cognitive load & possibilities that need to be considered in these classes' containers' reflow methods.)

Unlike most frames, page frames & page-content-frames don't need to have a meaningful distinction between fragments that are needed for the content box vs. fragments that are needed for overflowing pieces of children, so it doesn't make sense for them to be overflow-incomplete.

[1] I had a WIP patch on bug 1652278 which tried to assert that the nsPageFrame reflow status is never overflow-incomplete, but it failed for several reftests including e.g. abspos-breaking-001.xhtml . For posterity (in case we address this here), here's the snippet that I had used to add that assertion:

    // Sanity-check: page frames should only be complete or incomplete.
    // Overflow-incomplete is not a meaningful state for them to be in, because
    // they can always generate an additional continuation if needed. (They're
    // not like frames for regular web content, which can reach their
    // author-specified BSize and then need to call themselves complete and
    // throw all overflowing content into an overflow continuation.)
    NS_ASSERTION(!status.IsOverflowIncomplete(),
                 "it makes no sense to have an overflow-incomplete page frame");

Comment 3

[Mats Palmgren (inactive)]

Attachment: Bug 1655856 - Make nsPageContentFrame include its own border-box in its reported overflow-areas.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4fbf99122bab
Make nsPageContentFrame include its own border-box in its reported overflow-areas. r=emilio

Comment 5

[Mats Palmgren (inactive)]

Attachment: Bug 1655856 - Don't push all child frames, just 'kidFrame'. r=TYLin

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 9191527 [details]
Bug 1655856 - Don't push all child frames, just 'kidFrame'. r=TYLin

Wrong bug number.

Comment 7

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/4fbf99122bab