Closed
Bug 1148833
Opened 9 years ago
Closed 9 years ago
Remove nsIFrame::WillReflow
Categories
(Core :: Layout, defect, P5)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
Details
(Keywords: perf)
Attachments
(5 files, 1 obsolete file)
7.27 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
19.03 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
1.87 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
46.57 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
8.42 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Attachment #8585147 -
Flags: review?(roc)
Assignee | ||
Comment 2•9 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•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8585149 -
Flags: review?(roc)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8585150 -
Flags: review?(roc)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8585151 -
Flags: review?(roc)
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8585148 -
Flags: review?(roc)
Assignee | ||
Comment 8•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4396dcd53c0
Assignee | ||
Comment 9•9 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•9 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.
Attachment #8585148 -
Flags: review?(roc) → review+
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+
Attachment #8585150 -
Flags: review?(roc) → review+
Attachment #8585151 -
Flags: review?(roc) → review+
Attachment #8585152 -
Flags: 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•9 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.
Assignee | ||
Comment 14•9 years ago
|
||
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
Flags: in-testsuite-
Comment 15•9 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•