Closed
Bug 785333
Opened 12 years ago
Closed 12 years ago
Keep track of display-item data with merged frames
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla18
Tracking | Status | |
---|---|---|
firefox17 | --- | fixed |
People
(Reporter: cwiiis, Assigned: cwiiis)
References
Details
Attachments
(6 files, 6 obsolete files)
4.54 KB,
patch
|
roc
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
10.34 KB,
patch
|
roc
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
14.18 KB,
patch
|
bas.schouten
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
3.57 KB,
patch
|
roc
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
32.74 KB,
patch
|
cwiiis
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
7.35 KB,
patch
|
cwiiis
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Sometimes we redraw more than is necessary because the underlying frame of an item has changed. We mitigate against this in nsDisplayScroll, for example, by swapping the underlying frame when merging. Rather than doing it this way, we could keep track of display-item data for merged frames and when a layer isn't found, check for layers against merged frames as well. This would protect against the underlying frame changing to another merged frame, and from the underlying frame changing to a new frame (but keeping other merged frames). This would also allow us to remove the aforementioned code in nsDisplayScroll (I've tested this). I'll split this patch up between doing this for container items and then non-container items.
Assignee | ||
Comment 2•12 years ago
|
||
And this applies over the last one and stores layers against *all* items with merged frames.
Attachment #654970 -
Flags: review?(roc)
Attachment #654962 -
Flags: review?(roc) → review+
Attachment #654970 -
Flags: review?(roc) → review+
Assignee | ||
Comment 3•12 years ago
|
||
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/79345542f853 https://hg.mozilla.org/integration/mozilla-inbound/rev/a1756976e61d
Comment 4•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/79345542f853 https://hg.mozilla.org/mozilla-central/rev/a1756976e61d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 5•12 years ago
|
||
Backed out due to bug 785626: http://hg.mozilla.org/mozilla-central/rev/ee5484eb9f4a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla17 → ---
Comment 6•12 years ago
|
||
(also backed out on Aurora, post-merge, so this is backed out everywhere: https://hg.mozilla.org/releases/mozilla-aurora/rev/b766865d2857)
Assignee | ||
Comment 7•12 years ago
|
||
This caused crashes due to merged items separating into multiple layers (due to things becoming visible between merged items). Without this code, no layers are retained in this situation, so I think it's worthwhile to fix it, even if the retaining we do in this situation is limited (at the very least, it reduces layer creation/destruction churn). I have a somewhat lengthy fix incoming (lengthy as it required layer additions to change how ContainerState::Finish worked efficiently - the underlying fix is actually pretty small).
Assignee | ||
Comment 8•12 years ago
|
||
I needed to change some layer stuff and it made sense to do this at the same time. Currently, D3D/GL layers have inefficient insert/remove functions, this just synchronises them with the implementation in BasicContainerLayer.
Attachment #656101 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 9•12 years ago
|
||
Add a reposition function to ContainerLayer to save from having to remove/add.
Attachment #656102 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 10•12 years ago
|
||
This changes ContainerState::Finish from assuming that layer ordering is always unchanged to allowing it to change, by using ContainerLayer::Reposition instead of its current iterate+remove loop.
Attachment #656104 -
Flags: review?(roc)
Assignee | ||
Comment 11•12 years ago
|
||
This applies on top of attachment #654962 [details] [diff] [review] and attachment #654970 [details] [diff] [review], and adds the necessary code to avoid the assertion in ProcessDisplayItems. This users LayerUserData to track when a layer has been associated with a frame during construction, to prevent it being found again by another frame. This is necessary for the situation where frames that were previously merged are now represented on separate layers due to items becoming visible in-between (this is what caused the crash in bug #785626).
Attachment #656106 -
Flags: review?(roc)
Assignee | ||
Comment 12•12 years ago
|
||
Here's a version that should actually build on Windows.
Attachment #656102 -
Attachment is obsolete: true
Attachment #656102 -
Flags: review?(bas.schouten)
Attachment #656140 -
Flags: review?(bas.schouten)
Updated•12 years ago
|
Attachment #656101 -
Flags: review?(bas.schouten) → review+
Assignee | ||
Comment 13•12 years ago
|
||
Sorry for churn, here's a version that should be less crashy :p If this still has problems, I promise I'll sleep on it and wait for try before pushing another...
Attachment #656140 -
Attachment is obsolete: true
Attachment #656140 -
Flags: review?(bas.schouten)
Attachment #656151 -
Flags: review?(bas.schouten)
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 656151 [details] [diff] [review] Part 2 - Add ContainerLayer::RepositionChild Replacing r=bas, didn't notice the r+ between adding revisions (confirmed on IRC).
Attachment #656151 -
Flags: review?(bas.schouten) → review+
Comment on attachment 656104 [details] [diff] [review] Part 3 - Change how old layers are removed in ContainerState::Finish Review of attachment 656104 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/FrameLayerBuilder.cpp @@ +2051,5 @@ > > + // Remove layers that have disappeared > + if (prevChild) { > + Layer* layer; > + while((layer = prevChild->GetNextSibling())) { I think we can move this block out of the loop now instead of having an iteration with i==mNewChildLayers.Length()
Attachment #656106 -
Flags: review?(roc) → review+
Assignee | ||
Comment 16•12 years ago
|
||
This makes the suggested change, but I'm still seeing an orange + crash on M2, so looking at that. Possibly something I've done wrong with RepositionChild. Would still care for review, I won't push until everything is green of course.
Attachment #656104 -
Attachment is obsolete: true
Attachment #656104 -
Flags: review?(roc)
Attachment #656362 -
Flags: review?(roc)
Assignee | ||
Comment 17•12 years ago
|
||
Carrying r+. This just moves the userdata removal from ContainerState::Finish to FrameLayerBuilder::DidEndTransaction. The previous patch only removed the data of container children, so could conceivably have still broken, I think. This is more reliable and clearer, anyway.
Attachment #656106 -
Attachment is obsolete: true
Attachment #656364 -
Flags: review+
Attachment #656362 -
Flags: review?(roc) → review+
Assignee | ||
Comment 18•12 years ago
|
||
Carrying r=bas - This fixes the Shadow operation implementation that was causing the oranges (you can't pass a NULL argument over ipdl - added RaiseToTopChild to compensate). Tests are looking green up to this point: https://tbpl.mozilla.org/?tree=Try&rev=12e22dc7a118
Attachment #656151 -
Attachment is obsolete: true
Attachment #656379 -
Flags: review+
Assignee | ||
Comment 19•12 years ago
|
||
Heh, so this is now all green, but no longer fixes the changing frames in scroll-layer bug. Hopefully something small.
Assignee | ||
Comment 20•12 years ago
|
||
It was something simple - I didn't notice the outer if statement on the call in DidEndTransaction. Modified the function to take flags and always be called, fixes the issue and still works, carrying r=roc.
Attachment #656364 -
Attachment is obsolete: true
Attachment #656391 -
Flags: review+
Assignee | ||
Comment 21•12 years ago
|
||
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/c06b09e067d7 https://hg.mozilla.org/integration/mozilla-inbound/rev/a9023cfa7721 https://hg.mozilla.org/integration/mozilla-inbound/rev/2466826796a9 https://hg.mozilla.org/integration/mozilla-inbound/rev/7d39a94c966a https://hg.mozilla.org/integration/mozilla-inbound/rev/d1ab93cb1541 https://hg.mozilla.org/integration/mozilla-inbound/rev/a36606002731
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c06b09e067d7 https://hg.mozilla.org/mozilla-central/rev/a9023cfa7721 https://hg.mozilla.org/mozilla-central/rev/2466826796a9 https://hg.mozilla.org/mozilla-central/rev/7d39a94c966a https://hg.mozilla.org/mozilla-central/rev/d1ab93cb1541 https://hg.mozilla.org/mozilla-central/rev/a36606002731
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment 23•12 years ago
|
||
These patches fixed the issue reported in bug 788193, but the problem is still present in Aurora. As per comment 5 in that bug, does it make sense to port this one to Aurora? Or should that be tracked separately/untracked?
Assignee | ||
Comment 24•12 years ago
|
||
(In reply to Virgil Dicu [:virgil] [QA] from comment #23) > These patches fixed the issue reported in bug 788193, but the problem is > still present in Aurora. As per comment 5 in that bug, does it make sense to > port this one to Aurora? Or should that be tracked separately/untracked? I think it makes sense to uplift these patches. They only missed the merge by a few days... I didn't request because these weren't solving a solid problem, just a problem I perceived could happen, and I was worried about the risk. It's had time to bake and I haven't seen any reports directly related to this, so I'll request aurora approval.
Assignee | ||
Comment 25•12 years ago
|
||
Comment on attachment 656101 [details] [diff] [review] Part 1 (bonus) - Synchronise Layer Insert/Remove [Approval Request Comment] Bug caused by (feature/regressing bug #): Inefficient layer insertion and removal. User impact if declined: None, except the other patches in this bug are based on top of this one. Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints. Risk to taking this patch (and alternatives if risky): Practically no risk (the code is already used in the basic layers case). String or UUID changes made by this patch: None
Attachment #656101 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 26•12 years ago
|
||
Comment on attachment 656379 [details] [diff] [review] Part 2 - Add ContainerLayer::RepositionChild [Approval Request Comment] Bug caused by (feature/regressing bug #): Inability to reposition layers (only remove/add, which involves more work) User impact if declined: Other patches in this bug require this one. Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints. Risk to taking this patch (and alternatives if risky): No risk, these functions are unused except by the other patches in this bug. String or UUID changes made by this patch: None.
Attachment #656379 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 27•12 years ago
|
||
Comment on attachment 656362 [details] [diff] [review] Part 3 - Change how old layers are removed in ContainerState::Finish [Approval Request Comment] Bug caused by (feature/regressing bug #): If layers somehow change order, a crash will likely occur (there is an assertion that will trigger in this situation) User impact if declined: None, except the other patches in this bug are based on top of this one. Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints. Risk to taking this patch (and alternatives if risky): Low risk. Slightly less efficient than the old code in some situations, but more versatile/safer. String or UUID changes made by this patch: None
Attachment #656362 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 28•12 years ago
|
||
Comment on attachment 654962 [details] [diff] [review] Store container layers against merged frames of container items [Approval Request Comment] Bug caused by (feature/regressing bug #): Full-layer invalidation may occur when container layers are associated with different frames (performance issue). Also possible bad layer association when invalidating areas (bug 788193) User impact if declined: Possible performance regression, more visible on mobile. Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints. Risk to taking this patch (and alternatives if risky): Requires part 6 of the patches in this bug. With part 6, small risk of exposed invalidation issues (without part 6, high risk of various elements disappearing + assertions) String or UUID changes made by this patch: None
Attachment #654962 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 29•12 years ago
|
||
Comment on attachment 654970 [details] [diff] [review] Track merged frames of all display items [Approval Request Comment] Bug caused by (feature/regressing bug #): Full-layer invalidation may occur when non-container layers are associated with different frames (performance issue). User impact if declined: Possible performance regression, more visible on mobile. Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints. Risk to taking this patch (and alternatives if risky): Small risk of exposed invalidation issues. String or UUID changes made by this patch: None
Attachment #654970 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 30•12 years ago
|
||
Comment on attachment 656391 [details] [diff] [review] Part 6 - Track layer ownership during construction [Approval Request Comment] Bug caused by (feature/regressing bug #): Layers associating with multiple frames during layer-building User impact if declined: Attachment #654962 [details] [diff] may cause elements to disappear, or possibly other bad side-effects Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints. Risk to taking this patch (and alternatives if risky): Low risk. String or UUID changes made by this patch: None
Attachment #656391 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 31•12 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #28) > Comment on attachment 654962 [details] [diff] [review] > Store container layers against merged frames of container items > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): Full-layer invalidation may occur > when container layers are associated with different frames (performance > issue). Also possible bad layer association when invalidating areas (bug > 788193) > User impact if declined: Possible performance regression, more visible on > mobile. > Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints. > Risk to taking this patch (and alternatives if risky): Requires part 6 of > the patches in this bug. With part 6, small risk of exposed invalidation > issues (without part 6, high risk of various elements disappearing + > assertions) > String or UUID changes made by this patch: None Meant to say for the user impact here that you may end up with invalidation-related issues, like those in bug 788193.
Updated•12 years ago
|
Attachment #654962 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #654970 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #656101 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #656362 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #656379 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #656391 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 32•12 years ago
|
||
Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/dc1cd7420bf4 https://hg.mozilla.org/releases/mozilla-aurora/rev/9b6753834713 https://hg.mozilla.org/releases/mozilla-aurora/rev/9ab54a081255 https://hg.mozilla.org/releases/mozilla-aurora/rev/c587c7a7c669 https://hg.mozilla.org/releases/mozilla-aurora/rev/8192ce427a5d https://hg.mozilla.org/releases/mozilla-aurora/rev/2753c3553b4a
Updated•12 years ago
|
status-firefox17:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•