Closed
Bug 745485
Opened 13 years ago
Closed 11 years ago
Avoid reflows when a position change does not result in an actual change to the dimensions of a frame (top/left/right/bottom changes)
Categories
(Core :: Layout, defect, P3)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: ehsan.akhgari, Assigned: roc)
References
(Blocks 1 open bug)
Details
(Keywords: perf, Whiteboard: [qa-])
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
Bug 157681's optimization is a little bit more conservative than it could be for the abs-pos case. To avoid all unnecessary reflows, nsCSSFrameConstructor::RecomputePosition should be modified to compute the new dimensions where we suspect there to be a dimension change, and only fall back to reflow when the dimensions really change.
Updated•12 years ago
|
tracking-firefox16:
--- → ?
Keywords: regression
Reporter | ||
Comment 2•12 years ago
|
||
This is not a regression, it's an idea for further optimization. Definitely not needed to be tracked.
tracking-firefox16:
? → ---
Keywords: regression
Comment 3•12 years ago
|
||
But bug 729597 is a regression, which has now regressed even further.
Reporter | ||
Comment 4•12 years ago
|
||
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #3)
> But bug 729597 is a regression, which has now regressed even further.
It is not a regression, it's just a test case which used to do nothing because of bug 10209, and now does something, and the thing it does is expensive, and has always been so.
Updated•12 years ago
|
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Updated•11 years ago
|
Summary: Avoid reflows when a position change does not result in an actual change to the dimensions of a frame → Avoid reflows when a position change does not result in an actual change to the dimensions of a frame (top/left/right/bottom changes)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → roc
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Comment 6•11 years ago
|
||
Comment on attachment 810216 [details] [diff] [review]
fix
Not quite right yet.
Attachment #810216 -
Attachment is obsolete: true
Assignee | ||
Comment 7•11 years ago
|
||
Assignee | ||
Comment 8•11 years ago
|
||
This turned out to be pretty simple, I think!
Attachment #810249 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #810249 -
Flags: review?(dbaron) → review?(dholbert)
Comment 9•11 years ago
|
||
Comment on attachment 810249 [details] [diff] [review]
diff -w
Looks good! Just some comment-clarifying nits:
>+++ b/layout/base/RestyleManager.cpp
>@@ -379,27 +379,16 @@ RestyleManager::RecomputePosition(nsIFra
> // For the absolute positioning case, set up a fake HTML reflow state for
> // the frame, and then get the offsets from it.
This contextual comment needs adjusting - s/offsets/offsets and size/
(due to the reflowState.ComputedWidth() & ComputedHeight() accesses added in this patch)
BUT, while you're at it: it might be good to extend that comment slightly to explain the high-level idea of this chunk a little better now, along the lines of the comment higher up that says "For relative positioning, we can simply update the frame rect".
Maybe something like:
// For the absolute positioning case: if the frame's size hasn't changed, we
// can simply update the frame rect (and otherwise, fall back to a reflow).
// To determine whether the size has changed, and the frame's new position,
// set up a fake reflow state and get the size/offsets from it.
>+++ b/layout/style/nsStyleStruct.cpp
>+ // Don't try to handle changes between "auto" and non-auto efficiently.
Maybe add:
... "because such changes are likely to change the size and require reflow"
(right?)
r=me
Attachment #810249 -
Flags: review?(dholbert) → review+
Comment 10•11 years ago
|
||
(er, s/and otherwise/or otherwise/, probably)
Assignee | ||
Comment 11•11 years ago
|
||
Comment 12•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Does this mean that we do off-main-thread animations when top/left/bottom/right is animated? Except when reflow is actually needed of course.
Reporter | ||
Comment 14•11 years ago
|
||
(In reply to comment #13)
> Does this mean that we do off-main-thread animations when top/left/bottom/right
> is animated? Except when reflow is actually needed of course.
No, we just avoid reflowing, and move the frame directly, but this all happens on the main thread.
Blocks: 940073
Updated•11 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•