Closed
Bug 989897
Opened 11 years ago
Closed 11 years ago
b2g OOM zooming https://etherpad.mozilla.org
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: tnikkel, Assigned: tnikkel)
References
Details
(Keywords: perf, Whiteboard: [MemShrink:P2])
Attachments
(1 file)
(deleted),
patch
|
kats
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
See bug 957668 comment 87 for str.
Assignee | ||
Comment 1•11 years ago
|
||
I think there are two problems here. The main one is that the root composition bounds aren't calculated correctly. The other one is that the baserect we use for the iframe's display port incorporates the display port for the root scroll frame of the root content document, this means the display port for the iframe essentially gets expanded twice. We should be using what is actually visible, not what is visible in the display port.
The reason the root composition bounds are incorrect is this line
CSSSize size =
ParentLayerSize(rootCompositionSize) / (parentResolution * aMetrics.mDevPixelsPerCSSPixel);
This converts from parent layer pixels to CSS pixels, but the CSS pixels are of the parent layer. One CSS pixel in the child layer will take up |resolution| CSS pixels in the parent (where resolution is the resolution of the child). It would make sense to store the root composition bounds in parent layer pixels, except the parent layer in question might be the grand parent or higher. Maybe we need root layer pixels or something.
Comment 2•11 years ago
|
||
Is this a child process OOM (browser tab crashed with "well this is embarrassing") or a parent process OOM (phone restart)?
Comment 3•11 years ago
|
||
this is child process OOM (browser tab crashed with "well this is embarrassing")
Assignee | ||
Comment 4•11 years ago
|
||
The calculation was just wrong.
The key here is that we want to view the root composition size as layer pixels in the current layer (because that is what we want to use it to limit, the actual number of layer pixels in the current layer) and then convert it to CSS pixels. There is no magic conversion to do what we want, we just want plop the root composition size down in any layer and use it as is.
Attachment #8400069 -
Flags: review?(bugmail.mozilla)
Updated•11 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Comment 6•11 years ago
|
||
Comment on attachment 8400069 [details] [diff] [review]
fix root composition size calculation
Review of attachment 8400069 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/base/nsDisplayList.cpp
@@ +677,5 @@
> } else {
> + LayoutDeviceToParentLayerScale parentResolution(1.0f);
> + parentResolution.scale =
> + rootPresShell->GetCumulativeResolution().width
> + / rootPresShell->GetResolution().width;
Combine this assignment into the parentResolution declaration
LayoutDeviceToParentLayerScale parentResolution(
blah.width / blah.width);
Attachment #8400069 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 7•11 years ago
|
||
Assignee | ||
Comment 8•11 years ago
|
||
Filed bug 991488 for the other problem mentioned in comment 1, the one not addressed by the patch.
Comment 9•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee | ||
Comment 10•11 years ago
|
||
Comment on attachment 8400069 [details] [diff] [review]
fix root composition size calculation
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 957668, bug 986413
User impact if declined: OOM due to asking for too large a displayport on webpages, eg etherpad
Testing completed (on m-c, etc.): been on m-c for several days now
Risk to taking this patch (and alternatives if risky): relatively low, should affect b2g only
String or IDL/UUID changes made by this patch: none
Attachment #8400069 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Attachment #8400069 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 11•11 years ago
|
||
Updated•11 years ago
|
status-firefox31:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•