Closed Bug 1148833 Opened 5 years ago Closed 5 years ago

Remove nsIFrame::WillReflow

Categories

(Core :: Layout, defect, P5, minor)

defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: mats, Assigned: mats)

Details

(Keywords: perf)

Attachments

(5 files, 1 obsolete file)

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 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.
Attachment #8585147 - Attachment is obsolete: true
Attachment #8585147 - Flags: review?(roc)
Attachment #8585149 - Flags: review?(roc)
Attachment #8585148 - Flags: review?(roc)
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.
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 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.
Attachment #8585149 - Flags: review?(roc) → review+
(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.
(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.
You need to log in before you can comment on or make changes to this bug.