svgfeimage frame's attempt to ensure it is "visible" doesn't work

RESOLVED FIXED in Firefox 22

Status

()

defect
RESOLVED FIXED
6 years ago
8 months ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

Trunk
mozilla23
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox22+ fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Posted patch patchSplinter Review
In SVGFEImageFrame::Init we call FrameCreated and then IncrementVisibleCount on the associated nsImageLoadingContent. However in Init the primary frame pointer hasn't been setup yet. FrameCreated correctly works around this by passing the SKIP_FRAME_CHECK to TrackImage. TrackImage won't track unless the visible count is positive and there is a primary frame.

So if we call FrameCreated first the TrackImage call won't track because the visible count is 0 (even though we correctly skip the primary frame check). We want to call IncrementVisibleCount first so the visible count is positive when FrameCreated calls TrackImage.

This is kind of fragile, I'm open to a better solution.

This came up while testing my patches for 847223.
Attachment #742131 - Flags: review?(matspal)
Comment on attachment 742131 [details] [diff] [review]
patch

Looks good, r=mats

I find the comment is a little confusing though; couldn't you just say:

+    // Increment the visible count before calling FrameCreated so that
+    // tracks the image.

I think the primary frame / SKIP_FRAME_CHECK part would be clearer as
a separate comment before the FrameCreated() call, if needed.
(it's already documented in FrameCreated)
Attachment #742131 - Flags: review?(matspal) → review+
(Assignee)

Comment 2

6 years ago
Ok, I changed the comment to be basically what you suggested.
(Assignee)

Comment 4

6 years ago
Comment on attachment 742131 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 689623
User impact if declined: dynamically added SVGFEImage's won't animate
Testing completed (on m-c, etc.): just landed on inbound now
Risk to taking this patch (and alternatives if risky): this should be a safe patch, I think the risk of not taking it is higher than the risk of taking it
String or IDL/UUID changes made by this patch: none
Attachment #742131 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/e185401101a8
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Attachment #742131 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

8 months ago
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.