Seth Fowler [:seth] [:s2h] Reporter
	Description • 9 years ago

We should only lock CSS images (background-image, border-image, etc) when they're visible. After bug 1157546, this can be implemented by locking CSS images in nsIFrame::OnBecameVisible() and unlocking them in nsIFrame::OnBecameNonvisible(). We'll also need to remove the existing code that locks them forever and throws away the key.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 1 • 9 years ago

checking in on this since it blocks a performance regression!

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 2 • 9 years ago

(In reply to Joel Maher (:jmaher) from comment #1)
> checking in on this since it blocks a performance regression!

Patches coming right up. Unfortunately I had to rewrite this one a few times to get an implementation I was really happy with.

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 3 • 9 years ago

Here's the patch! Again, there's only one part, but I've separated the patch in
two for easier reviewing.

Part 1a takes care of removing the old tracking code for CSS images that's no
longer needed given the changes in part 1b. In particular, the style structs
that contain images no longer need to support TrackImage() and UntrackImage()
calls, and we can just delete the code that calls those methods in most cases,
as in part 1b we start relying on the image visibility mechanism instead.

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 4 • 9 years ago

This part (the final part) actually implements visibility tracking for CSS
images.

It turns out to be pretty simple:

 - In ImageLoader, AssociateRequestToFrame() now registers the image with the
   document (locking it) if the frame is visible. DisassociateRequestFromFrame()
   similarly unregisters the image if the frame is visible.

 - We also need to consider dynamic changes in visibility, so ImageLoader gains
   two new methods: RegisterImagesForFrame() and UnregisterImagesForFrame().
   These methods are called from nsIFrame::OnBecameVisible() and
   nsIFrame::OnBecameNonvisible() respectively.

That's really about it! Since AssociateRequestToFrame() and
DisassociateRequestFromFrame() are already called in the appropriate places
(e.g. when a CSS background-image is added or removed for a given frame), these
changes are pretty much all we need to make things work. Easy!

(Though I have to admit, it took me a couple of passes to realize things could
be this simple!)

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 5 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68bf3fc2e2cf

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 6 • 9 years ago

Looks like there are a few failures in that try job:

image/test/mochitest/test_background_image_anim.html (in oth)
layout/base/tests/browser_onbeforeunload_only_after_interaction_in_frame.js (in bc3)
layout/reftests/bugs/1114526-1.html (in R1 or R2)

Surprisingly few considering what a fundamental change this is.

I'll investigate those and upload an updated version of the patch.

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 7 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70b3fc619b25

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 8 • 9 years ago

^^^ This updated try job includes bug 1223747, bug 1223751, and bug 1223753 , which should fix all of the failures above. It's worth noting that these were all existing issues that were just exposed by accurately tracking visibility of CSS images.

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 9 • 9 years ago

A minor rebase was required because of changes in the blocking bugs.

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 10 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eef0a1dbe766

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 11 • 9 years ago

Updated this patch to use IsVisible() when possible.

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 12 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4d170394823

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 13 • 9 years ago

Based upon the try results above, it looks like this entire patch stack is now ready to land module review.

	Timothy Nikkel (:tnikkel)
	Comment 14 • 9 years ago

Comment on attachment 8687052 [details] [diff] [review]
(Part 1b) - Register CSS images with the document when their associated frame is visible.

>@@ -88,16 +91,47 @@ ImageLoader::AssociateRequestToFrame(imgIRequest* aRequest,
>+    if (aFrame->IsVisible()) {
>+      nsIDocument* doc = aFrame->PresContext()->Document();
>+      if (doc) {
>+        doc->AddImage(aRequest);
>+      }
>+    }
>+  }
>+}

You can count me as the first victim of the naming of IsVisible, because as I would reading this I was thinking "why is the css style 'visibility: visible' all we need to check here??".

	Timothy Nikkel (:tnikkel)
	Comment 15 • 9 years ago

Comment on attachment 8687052 [details] [diff] [review]
(Part 1b) - Register CSS images with the document when their associated frame is visible.

Are you planning to respond to UnlockedDraw notifications? Because I think you need to (for animation purposes at least).

	Eric Rahm [:erahm]
	Comment 16 • 7 years ago

Jet, is there someone who can pick this work back up again or should we deprioritize it?

	Jet Villegas (inactive)
	Comment 17 • 7 years ago

tn: is this something we should still try to get landed? If yes, can you help summarize the remaining pieces needed? Thx!

	Timothy Nikkel (:tnikkel)
	Comment 18 • 7 years ago

It doesn't seem like it's that high of priority. It doesn't cause us problems in common cases most of the time.

The work remaining is similar to the two patches that are posted in this bug, although they have surely bitrot.

	Eric Rahm [:erahm]
	Comment 19 • 6 years ago

P2 per comment 18.