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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: patch

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.

Comment 1

[Mats Palmgren (inactive)]

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)

Comment 2

[Timothy Nikkel (:tnikkel)]

Ok, I changed the comment to be basically what you suggested.

Comment 3

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e185401101a8

Comment 4

[Timothy Nikkel (:tnikkel)]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e185401101a8

Comment 6

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b8946cce720