Closed
Bug 1169880
Opened 9 years ago
Closed 9 years ago
Recompute image visibility on a timer if layout or style flushes have occurred
Categories
(Core :: Layout: Images, Video, and HTML Frames, defect)
Core
Layout: Images, Video, and HTML Frames
Tracking
()
People
(Reporter: seth, Assigned: seth)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jocheng
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
Layout or style changes can cause images that were previously visible to stop being visible. This can happen, for example, because new elements are inserted into the document that push the element containing the image out of the visible region, or because of changes to CSS properties like |visibility|.
Unfortunately, I'm not aware of a general way in which we can precisely detect when changes like these will change an image's visibility. To make sure that our image visibility data doesn't become too far out of date, then, we need to periodically recompute it if there's a reasonable chance that something could have changed.
Assignee | ||
Comment 1•9 years ago
|
||
Here's the patch. We schedule an image visibility update in the refresh driver if:
- A layout or style flush has occurred.
- Enough time has passed since the last time we did it. (The default is 100ms.)
- The refresh driver isn't throttled. (i.e., we don't do this for background
tabs, in which all images should already be unlocked.)
- We're not still in paint supression.
This patch fixes the testcase in bug 1168985, but it's more general. It helps a
great deal with memory usage of the Gallery app during the scanning process,
which is what the testcase in bug 1168985 was meant to simulate.
Attachment #8613200 -
Flags: review?(tnikkel)
Assignee | ||
Comment 2•9 years ago
|
||
Comment 3•9 years ago
|
||
Comment on attachment 8613200 [details] [diff] [review]
Recompute image visibility on a timer if layout or style flushes have occurred
100ms feels a little low to me. I base this on nothing in particular :) If we are doing ticks more than every 100ms anyway we might want to back off on image visibility updates (and let other work happen) more so than just limiting them to 100ms.
Attachment #8613200 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 4•9 years ago
|
||
Agreed on the timer interval. I tested that 1000ms was fine both for the desktop
testcase and for Gallery scanning on the device, and it passed with flying
colors, so this version of the patch changes the default interval to 1000ms.
Assignee | ||
Updated•9 years ago
|
Attachment #8613200 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Comment 6•9 years ago
|
||
It occured to me that this rate limiting doesn't take into account other image visibility updates that are triggered via other means (scrolling, display port margins changing). We should probably do that.
Updated•9 years ago
|
Flags: needinfo?(seth)
Comment 7•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Timothy Nikkel (:tn) from comment #6)
> It occured to me that this rate limiting doesn't take into account other
> image visibility updates that are triggered via other means (scrolling,
> display port margins changing). We should probably do that.
Yep, I'll file a followup to clear mNeedToRecomputeVisibility if something else calls ScheduleImageVisibilityUpdate(). Should be straightforward.
Flags: needinfo?(seth)
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8613205 [details] [diff] [review]
Recompute image visibility on a timer if layout or style flushes have occurred
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not sure, but it was exposed by the fact that we enabled image locking on B2G in bug 1148696.
User impact if declined: Images may fail to decode due to low memory, causing blank thumbnails in the gallery which may persist across reboots (making this effectively a data loss issue). This patch is one of several that together fix bug 1166136, which is nominated for blocking 2.2, and bug 1164164, which is a 2.2 blocker.
Testing completed: Extensively tested locally. On mozilla-central.
Risk to taking this patch (and alternatives if risky): This is fairly low-risk patch.
String or UUID changes made by this patch: None.
Attachment #8613205 -
Flags: approval-mozilla-b2g37?
Comment 10•9 years ago
|
||
2.2+ as this causing function broken for Gallery app.
Updated•9 years ago
|
Attachment #8613205 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Comment 11•9 years ago
|
||
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•