Remove nsIFrame::WillReflow

RESOLVED FIXED in Firefox 39

Status

()

P5
minor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({perf})

Trunk
mozilla39
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8585147 [details] [diff] [review]
part 5 - Fix indentation of some Reflow params (white-space changes only)
Attachment #8585147 - Flags: review?(roc)
(Assignee)

Comment 2

4 years ago
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)
(Assignee)

Comment 3

4 years ago
Created attachment 8585148 [details] [diff] [review]
part 1 - Remove nsMathMLContainerFrame::WillReflow, reset the NS_MATHML_ERROR bit at the start of Reflow instead
(Assignee)

Comment 4

4 years ago
Created attachment 8585149 [details] [diff] [review]
part 2 - Remove nsIFrame::WillReflow
Attachment #8585149 - Flags: review?(roc)
(Assignee)

Comment 5

4 years ago
Created attachment 8585150 [details] [diff] [review]
part 3 - Makes sure gLogModule is initialized by calling GetLogModuleInfo()
Attachment #8585150 - Flags: review?(roc)
(Assignee)

Comment 6

4 years ago
Created attachment 8585151 [details] [diff] [review]
part 4 - Add a MarkInReflow method that sets NS_FRAME_IN_REFLOW.  Call it at the start of Reflow()
Attachment #8585151 - Flags: review?(roc)
(Assignee)

Comment 7

4 years ago
Created attachment 8585152 [details] [diff] [review]
part 5 - Fix indentation of some Reflow params (white-space changes only)
(Assignee)

Updated

4 years ago
Attachment #8585148 - Flags: review?(roc)
(Assignee)

Comment 9

4 years ago
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.
(Assignee)

Comment 10

4 years ago
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.
(Assignee)

Comment 13

4 years ago
(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.