1148833 - Remove nsIFrame::WillReflow

Status: RESOLVED FIXED

(Core :: Layout, defect, P5)

Description

[Mats Palmgren (inactive)]

nsIFrame::WillReflow is a virtual method called as a separate step
before Reflow is called.  It has two uses in the entire tree:

1. nsMathMLContainerFrame uses it to reset the "NS_MATHML_ERROR" bit
http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLContainerFrame.h#118

2. nsFrame sets NS_FRAME_IN_REFLOW
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#4407

It's seems unnecessary to have a separate step for such simple
tasks.  I think we can do both at the start of Reflow instead.

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 5 - Fix indentation of some Reflow params (white-space changes only)

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 8585147 [details] [diff] [review]
part 5 - Fix indentation of some Reflow params (white-space changes only)

Sorry about that, bzexport didn't quite work as I expected.

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 1 - Remove nsMathMLContainerFrame::WillReflow, reset the NS_MATHML_ERROR bit at the start of Reflow instead

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 2 - Remove nsIFrame::WillReflow

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 3 - Makes sure gLogModule is initialized by calling GetLogModuleInfo()

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 4 - Add a MarkInReflow method that sets NS_FRAME_IN_REFLOW. Call it at the start of Reflow()

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 5 - Fix indentation of some Reflow params (white-space changes only)

Comment 8

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4396dcd53c0

Comment 9

[Mats Palmgren (inactive)]

FYI, part 3 is a bug fix: the tracing functions used in Reflow was depending
on the tracing statement in WillReflow to have initialized gLogModule.

Comment 10

[Mats Palmgren (inactive)]

At some point we should probably standardize the Reflow preamble.  We now have:

   MarkInReflow();
   DO_GLOBAL_REFLOW_COUNT("nsVideoFrame");
   DISPLAY_REFLOW(aPresContext, this, aReflowState, aMetrics, aStatus);
   NS_FRAME_TRACE(...)

which is getting a bit silly.
Also, some methods currently leave out one part or another.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8585149 [details] [diff] [review]
part 2 - Remove nsIFrame::WillReflow

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

Looks OK but I'd prefer to have patches ordered so we pass tests after each patch. Please squash or reorder to get that --- after this patch, no-one's setting the IN_REFLOW bit.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mats Palmgren (:mats) from comment #10)
> At some point we should probably standardize the Reflow preamble.  We now
> have:
> 
>    MarkInReflow();
>    DO_GLOBAL_REFLOW_COUNT("nsVideoFrame");
>    DISPLAY_REFLOW(aPresContext, this, aReflowState, aMetrics, aStatus);
>    NS_FRAME_TRACE(...)
> 
> which is getting a bit silly.
> Also, some methods currently leave out one part or another.

This would be a really good thing to fix!

We should probably just remove NS_FRAME_TRACE in favour of DISPLAY_REFLOW.

We already have DEBUG-only nsIFrame::GetFrameName. If we make that a method that just returns a char*, then the current callers of GetFrameName could call another method that does the work nsFrame::MakeFrameName currently does. Then DO_GLOBAL_REFLOW_COUNT could call GetFrameName and not be a macro, so we could fold the entire preamble into an inline non-virtual BeginReflow function.

Comment 13

[Mats Palmgren (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> Looks OK but I'd prefer to have patches ordered so we pass tests after each
> patch. Please squash or reorder to get that --- after this patch, no-one's
> setting the IN_REFLOW bit.

OK, I'll fold part 2 into 4.

Comment 14

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54a080c7d385
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfd12c2cb5cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/41c005e9398e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7b8787283ce

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/54a080c7d385
https://hg.mozilla.org/mozilla-central/rev/cfd12c2cb5cb
https://hg.mozilla.org/mozilla-central/rev/41c005e9398e
https://hg.mozilla.org/mozilla-central/rev/d7b8787283ce