806001 - nsListControlFrame passes the wrong type of status to DidReflow (a nsReflowStatus instead of a nsDidReflowStatus)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Quoting nsListControlFrame.cpp:
> 349 NS_IMETHODIMP 
> 350 nsListControlFrame::Reflow(nsPresContext*           aPresContext, 
> 351                            nsHTMLReflowMetrics&     aDesiredSize,
> 352                            const nsHTMLReflowState& aReflowState, 
> 353                            nsReflowStatus&          aStatus)
> 354 {
[...]
> 468   nsHTMLScrollFrame::DidReflow(aPresContext, &state, aStatus);
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#468

That "aStatus" variable that we're passing to DidReflow() is a nsReflowStatus (a bit-field), but DidReflow doesn't take a nsReflowStatus -- it takes a nsDidReflowStatus (note the "Did" in the name there), which is typedef'd to bool.

(If we were using enums instead of typedefs-to-basic-types, compilers would help us with this -- about to file a bug on turning nsDidReflowStatus into an enum, though we can't do that until this bug here is fixed)

Comment 1

[Daniel Holbert [:dholbert]]

So the question is, should we be passing in NOT_FINISHED or FINISHED?

From looking at nsFrame::DidReflow, the main difference is that FINISHED makes us clear a bunch of flags (like e.g. NS_FRAME_IN_REFLOW), and those flags won't get set again until the next call to WillReflow().

From the nsIFrame docs, it looks like NOT_FINISHED is for cases where we do something like this:
  WillReflow()
    Reflow()
    DidReflow(NOT_FINISHED)
    Reflow()
    DidReflow(NOT_FINISHED)
    Reflow()
  DidReflow(FINISHED)
...with many Reflow/DidReflow calls before the next WillReflow call.

That's not the case here -- we explicitly call WillReflow() before our next Reflow() call -- so I think the correct thing to do is to pass FINISHED, and let the existing WillReflow() call re-toggle all those flags for us.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1a (with more lines of context)

(In reply to Daniel Holbert [:dholbert] from comment #1)
> we explicitly call WillReflow() before our next
> Reflow() call -- so I think the correct thing to do is to pass FINISHED, and
> let the existing WillReflow() call re-toggle all those flags for us.

Re-attaching the patch w/ more lines of context, to demonstrate that we're calling "WillReflow/Reflow" after the modified DidReflow call, in the first patch-chunk.  (It's true of the second patch-chunk, too, but there are ~50 lines of code in between, so that doesn't fit in patch-context. Here's an MXR link instead -- you might have to scroll down to see the marked WillReflow call:
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp?rev=c0c98a831804&mark=549-549,596-597#544
)

Comment 4

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

Comment on attachment 675743 [details] [diff] [review]
fix v1a (with more lines of context)

r=dbaron

Comment 5

[Daniel Holbert [:dholbert]]

Green try run, aside from a few known-intermittent-oranges:
  https://tbpl.mozilla.org/?tree=Try&rev=f08b1abd0d3d

Landed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/fcdb39332b7c

Comment 6

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/fcdb39332b7c