Make sure we notify for invalidations in VectorImage even for non-animated images

RESOLVED FIXED in mozilla28

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Depends on: 1 bug)

Trunk
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Bug 911862 makes us only send out VectorImage invalidation notifications in RequestRefresh and OnSVGDocumentLoaded. For animated images, this is fine, but it's not clear that this won't fail in rare cases for non-animated images that may not be 100% ready to paint when OnSVGDocumentLoaded is called.

This needs more investigation, but it seems very possible that we may need to duplicate this code in another place. For example, if |mHaveAnimations| is false, we may want to send out invalidations directly in InvalidateObserversOnNextRefreshDriverTick.
(Assignee)

Updated

5 years ago
Blocks: 764299
(In reply to Seth Fowler [:seth] from comment #0)
> For example, if |mHaveAnimations|
> is false, we may want to send out invalidations directly in
> InvalidateObserversOnNextRefreshDriverTick.

That sounds reasonable. (In which case maybe we should change that back to being called InvalidateObservers, and it just happens to delay its invalidation until the next refresh tick some of the time.)
Whiteboard: [Australis:P-]
(Assignee)

Comment 2

5 years ago
Created attachment 8334947 [details] [diff] [review]
Fix VectorImage invalidation for non-animated images.

This patch implements the change mentioned in comment 0.

Try job here:
https://tbpl.mozilla.org/?tree=Try&rev=9b1f9dedce20
Attachment #8334947 - Flags: review?(dholbert)
(Assignee)

Updated

5 years ago
Assignee: nobody → seth
Status: NEW → ASSIGNED
Comment on attachment 8334947 [details] [diff] [review]
Fix VectorImage invalidation for non-animated images.

[side note: did we end up actually hitting this anywhere, or is this still a theoretical issue?]

>+    // ready to paint when OnSVGDocumentLoaded is called. When this happens we
>+    // need to send out these notifications later, when the image is actually
>+    // ready, but without animation RequestRefresh will never get called. In

Nit: The "...but" clause is a bit of a non sequitor, at first glance.

Let's add a bit more explanation there -- e.g. replace that last line and everything after it with something like:
      [...]
      // ready. For animated images, this isn't a problem, because clients will
      // call RequestRefresh, which flushes these invalidations. But non-animated
      // images won't receive any such RequestRefresh calls, so we send the
      // notifications here instead.

One other thought:
>+    // this case we send those notifications here instead.
>+    if (mStatusTracker) {
>+      SurfaceCache::Discard(this);
>+      mStatusTracker->FrameChanged(&nsIntRect::GetMaxSizedIntRect());
>+      mStatusTracker->OnStopFrame();

So those last 3 lines are now duplicated (here and in RequestRefresh). Maybe we should refactor them into a helper-method?  (FlushInvalidation() or DiscardAndInvalidate() or something like that?) I guess it's not much code, and it's only duplicated twice now, so it's also probably fine as-is.

r=me with the comment clarification, anyway.
Attachment #8334947 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #3)
[er, s/flushes these invalidations/flushes these notifications/, I guess, in my suggested comment-tweak]
(Assignee)

Comment 5

5 years ago
Thanks for the review, Daniel!

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Comment on attachment 8334947 [details] [diff] [review]
> Fix VectorImage invalidation for non-animated images.
> 
> [side note: did we end up actually hitting this anywhere, or is this still a
> theoretical issue?]

I don't think we have ever actually seen an issue caused by this in the wild.

> Nit: The "...but" clause is a bit of a non sequitor, at first glance.

Agreed.

> So those last 3 lines are now duplicated (here and in RequestRefresh).

Yeah, I was kinda on the fence about that. They're almost duplicated in OnSVGDocumentLoaded, too, except that we don't call SurfaceCache::Discard there. (We don't need to because we refuse to draw before mIsFullyLoaded is set to true.) I decided against making a helper method yesterday, but looking at it again I'm leaning towards folding the |if (mStatusTracker)| check into the method, which probably makes it beefy enough to justify. I'll go ahead and do this.
Sounds good!
(Assignee)

Comment 7

5 years ago
Created attachment 8335727 [details] [diff] [review]
Fix VectorImage invalidation for non-animated images.

Addressed review comments. Added a new helper method and moved all of the (rewritten) documentation there to avoid spreading it over multiple methods.
(Assignee)

Updated

5 years ago
Attachment #8334947 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0df2441272fb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
Depends on: 1013574
You need to log in before you can comment on or make changes to this bug.