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

We need to track the visibility of CSS images, just as we do for content images.

	Hema Koka [:hema]
	Comment 1 • 9 years ago

Seth,

As you know, there a few 2.5 gallery blockers dependent on this bug. Based on your earlier comment https://bugzilla.mozilla.org/show_bug.cgi?id=1181290#c5, if these are significant changes, we need to try to get them in sooner in the 2.5 release cycle so we iron out any bugs coming out of the changes early in the cycle. Could you take this on and wrap it up by end of August/early september? Please let us know, so we can plan accordingly. 

Thanks
Hema

	Hema Koka [:hema]
	Comment 2 • 9 years ago

Seth: Could you please update us on whether this will be done for 2.5?

Thanks
Hema

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

(In reply to Hema Koka [:hema] from comment #2)
> Seth: Could you please update us on whether this will be done for 2.5?

It won't. We'll try to get this into the next release after 2.5.

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

OK, I've implemented this now. All of the parts have to be folded together to
land, but I've split things out a bit for easier reviewing.

Part 1 mostly just changes the terminology in nsPresShell to refer to frame
visibility tracking rather than image visibility tracking. The key functional
change is that we're now storing a set of raw pointers instead of a set of
RefPtr's, since frames don't have AddRef() and Release(). This means that we
have to be sure to remove frames from the visible frame set in nsFrame's
destructor or else we could end up with dangling pointers; we'll do that in part
1b.

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

This part essentially just moves nsIImageLoadingContent's API for visibility
tracking (IncrementVisibleCount(), DecrementVisibleCount(), GetVisibleCount())
onto nsIFrame. While we're at it, the uint32_t parameter that indicated what
action to take if the image became nonvisible has been replaced by a strongly
typed enum in OnNonvisible.h.

Frames that want to do something more than what nsIFrame does for them
automatically (which is nothing, yet, though followup bugs will handle CSS
images there) can override OnBecameVisible() and OnBecameNonvisible(). Notifying
nsImageLoadingContent about visibility changes, for example, can be done in
those methods. We'll make use of that in part 1e.

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

This patch removes the code for the visibility tracking API
(IncrementVisibleCount() and the rest) from nsImageLoadingContent. A new API has
been added - OnBecameVisible() and OnBecameNonvisible() - which allows
nsImageLoadingContent to still take care of visibility-related tasks. That new
API is intended to be called from the methods of the same name on nsIFrame.

A few tasks that nsImageLoadingContent used to take care of have been moved
elsewhere - marking all images in popups as visible is handled in nsIFrame code
now (part 1b), and triggering MaybeDecodeForPredictedSize() when appropriate is
now handled by nsImageFrame directly (part 1e).

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

This is an entirely mechanical patch - everywhere we used to refer to image
visibility, we now refer to frame visibility.

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

This final patch makes use of the new frame visibility API in nsIFrame
implementations. That involves implementing OnBecameVisible() and
OnBecameNonvisible() when there's something special that that frame type needs
to do in those situations. Right now, that mostly means calling the methods of
the same name on nsImageLoadingContent that were added in part 1c. nsImageFrame
additionally calls MaybeDecodeForPredictedSize() in OnBecameVisible().

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

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

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

Rebased against the new version of bug 1207355 part 1.

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

We now avoid recursing into subdocs if their pres shell returns true for
AssumeAllFramesVisible(). This resolves the issues with the XUL tests in the
above try job.

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

We'll now track images even if they don't have a frame, if mFrameCreateCalled is
true. This is similar behavior to the old code, and it fixes the
test_svg_filter_animation.html test failure in the above try job, which was
caused because we refused to track the image (since
SVGFEImageElement::GetPrimaryFrame() returns nullptr) and thus we didn't animate
it.

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

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

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

(Just in case it's not clear: there won't be a part 2 here, this is all the patches. Just using this numbering scheme to remind myself to fold them after review.)

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

Some general comments.

One reason I used nsImageLoadingContent and not nsIFrame's to track was because frames can be somewhat ephemeral: we can destroy and recreate them to handle style changes.

I'm hesitant to call the new notion of visibility "frame visibility" as it does not, and was not intended to track true visibility. At least when it was called "image visibility" the notion that it was only used for tracking visibility for one specific purpose was evident.

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

(In reply to Timothy Nikkel (:tn) from comment #15)
> One reason I used nsImageLoadingContent and not nsIFrame's to track was
> because frames can be somewhat ephemeral: we can destroy and recreate them
> to handle style changes.

Yep, that should be fine, because we will mark the destroyed frames as visible and then mark the new frames visible again.

> I'm hesitant to call the new notion of visibility "frame visibility" as it
> does not, and was not intended to track true visibility. At least when it
> was called "image visibility" the notion that it was only used for tracking
> visibility for one specific purpose was evident.

Right now I'm not trying to tweak the algorithm we're using, but eventually we're going to want to use this to implement Google's new visibility API, and at that point we'll need to extend this to distinguish between "actually visible now" and "may be visible soon". I'm certainly open to suggestions about name changes that would encapsulate both notions. Visibility works for me because I'm thinking of visibility as being on a continuum (completely visible, partially visible, not visible but near the viewport, far from the viewport) but I can certainly see how that might not be intuitive for someone new jumping into the code.

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

(In reply to Seth Fowler [:seth] [:s2h] from comment #16)
> Yep, that should be fine, because we will mark the destroyed frames as
> visible and then mark the new frames visible again.

We'll mark the destroyed frames nonvisible, I mean.

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

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

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

While working on bug 1218990 I realized that it's cleaner to just always force a
frame's visibility to nonvisible in nsIFrame::DestroyFrom(). Since in that bug
we'll rely on the visibility methods to clean up tracked images related to the
frame, it's vastly preferable to ensure that we always transition to nonvisible
before the frame is destroyed. This updated version of part 1b makes that change
(which essentially amounts to a couple of lines changed in
nsIFrame::DestroyFrom).

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

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

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

One more minor tweak: if the pres shell associated with a frame is assuming that
all frames are visible, we now increment that frame's visible count
unconditionally in nsFrame::Init(), to ensure that all the frames associated
with such a pres shell are marked visible.

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

Another minor tweak; we need to force the visibility count of the frame to zero
*after* frame properties are destroyed, because that will cause
DecrementVisibleCount() to be called in some cases once we visibility starts
"seeing through" -moz-element.

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

Rebased against trunk.

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

No change except that this patch now includes an nsIFrame::IsVisible() helper
method, which means the same thing as |GetVisibleCount() > 0|.

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

Updated this patch to now use IsVisible() where possible.

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

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

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

(In reply to Seth Fowler [:seth] [:s2h] from comment #16)
> (In reply to Timothy Nikkel (:tn) from comment #15)
> > One reason I used nsImageLoadingContent and not nsIFrame's to track was
> > because frames can be somewhat ephemeral: we can destroy and recreate them
> > to handle style changes.
> 
> Yep, that should be fine, because we will mark the destroyed frames as
> visible and then mark the new frames visible again.

I'm more concerned with marking the images in destroyed frames as non-visible, discarding the decoding image. Then reconstructing the frame (as part of the same operation, but not the same turn of the event loop), marking it visible, and re-decoding the image.

> > I'm hesitant to call the new notion of visibility "frame visibility" as it
> > does not, and was not intended to track true visibility. At least when it
> > was called "image visibility" the notion that it was only used for tracking
> > visibility for one specific purpose was evident.
> 
> Right now I'm not trying to tweak the algorithm we're using, but eventually
> we're going to want to use this to implement Google's new visibility API,
> and at that point we'll need to extend this to distinguish between "actually
> visible now" and "may be visible soon". I'm certainly open to suggestions
> about name changes that would encapsulate both notions. Visibility works for
> me because I'm thinking of visibility as being on a continuum (completely
> visible, partially visible, not visible but near the viewport, far from the
> viewport) but I can certainly see how that might not be intuitive for
> someone new jumping into the code.

Not sure if the same system can be used to track "almost visibility" and "actual visibility" cohesively.

My concern lies with pretty much anyone who isn't you or me, not just new people. History shows that if you give something a simple, and tempting, name which does not actually reflect what it does it will be mis-used.

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

Comment on attachment 8686384 [details] [diff] [review]
(Part 1a) - Track visibility of frames instead of images in nsPresShell code.

iid on nsIPresShell needs bump.

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

ApproxVisible/ApproxVisibility seems like a decent name. But it's hard to ask that with the stack of patches of many KBs.

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

Comment on attachment 8687048 [details] [diff] [review]
(Part 1b) - Replace visibility tracking in nsIImageLoadingContent with a similar API on nsIFrame.

The comments on the functions in nsIFrame don't seem to give any hint that this isn't real visibility, only approx visibility.

Adding a uint32_t to nsIFrame? Not sure I could r+ that by myself. It could have significant memory implications, in the past we've not added 4 byte fields to nsIFrame lightly.

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

Comment on attachment 8687051 [details] [diff] [review]
(Part 1c) - Update nsImageLoadingContent to defer visibility tracking to nsIFrame.

> nsImageLoadingContent::TrackImage(imgIRequest* aImage)
>+  // We only want to track this request if we're visible. Ordinarily we check
>+  // the visible count, but that requires a frame; in cases where
>+  // GetOurPrimaryFrame() cannot obtain a frame (e.g. <feImage>), we assume
>+  // we're visible if FrameCreated() was called.

I thought I had recorded the reason for this in a commit msg or a bug somewhere but it looks like I didn't.

FrameCreate is called from some subclass of nsIFrame in Init(). At that point the primary frame pointer of the content node hasn't been set yet, even though it will be.

It's not specific to feImage, and we most certainly do have a frame, we just can't trust the pointers at this point.

(mFrameCreateCalled is used for another purpose: to distinguish the case where an image gets an nsImageFrame (which tracks visibility) vs when it gets an nsInlineFrame (early in the loading of the image) which does not track visibility information, so we can't trust visibility information)

I should have documented this better.

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

Comment on attachment 8679672 [details] [diff] [review]
(Part 1d) - Update various layout utility classes to refer to frame, rather than image, visibility.

>+nsLayoutUtils::UpdateFrameVisibility(nsIFrame* aFrame)
>+  nsRect rect = aFrame->GetContentRectRelativeToSelf();

If you are intending to use this to track border image, then you probably want to use the border rect, not the content rect (ie GetRectRelativeToSelf()).

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

Comment on attachment 8686384 [details] [diff] [review]
(Part 1a) - Track visibility of frames instead of images in nsPresShell code.

>+PresShell::MarkFramesInSubtreeVisible(nsIFrame* aFrame, const nsRect& aRect)
>+  if (aFrame->StyleVisibility()->IsVisible()) {
>+    uint32_t count = mVisibleFrames.Count();
>+    MOZ_ASSERT(!AssumeAllFramesVisible());

You remove the check of not recursing into a presshell with AssumeAllFramesVisible below. So why add this assert? (Although this situation is likely quite unusual.)

If we are going to recurse into AssumeAllFramesVisible presshells, shouldn't we check that we are not a AssumeAllFramesVisible presshell before adding stuff to its list of visible frames?

>+    mVisibleFrames.PutEntry(aFrame);
>+    if (mVisibleFrames.Count() > count) {
>+      // The frame was added to mVisibleFrames, so increment its visible count.
>+      aFrame->IncrementVisibleCount();

So this is going to track every frame? Not just ones that might actually care about visibility? That seems like there is going to be significant overhead for doing that.


>     }
>   }
> 
>   nsSubDocumentFrame* subdocFrame = do_QueryFrame(aFrame);
>   if (subdocFrame) {
>     nsIPresShell* presShell = subdocFrame->GetSubdocumentPresShellForPainting(
>       nsSubDocumentFrame::IGNORE_PAINT_SUPPRESSION);
>-    if (presShell) {
>+    if (presShell && !presShell->AssumeAllFramesVisible()) {
>       nsRect rect = aRect;
>       nsIFrame* root = presShell->GetRootFrame();
>       if (root) {
>         rect.MoveBy(aFrame->GetOffsetToCrossDoc(root));
>       } else {
>         rect.MoveBy(-aFrame->GetContentRectRelativeToSelf().TopLeft());
>       }
>       rect = rect.ScaleToOtherAppUnitsRoundOut(
>         aFrame->PresContext()->AppUnitsPerDevPixel(),
>         presShell->GetPresContext()->AppUnitsPerDevPixel());
> 
>-      presShell->RebuildImageVisibility(&rect);
>+      presShell->RebuildFrameVisibility(&rect);
>     }
>     return;
>   }
> 
>   nsRect rect = aRect;
> 
>   nsIScrollableFrame* scrollFrame = do_QueryFrame(aFrame);
>   if (scrollFrame) {
>@@ -5709,17 +5706,17 @@ PresShell::MarkImagesInSubtreeVisible(nsIFrame* aFrame, const nsRect& aRect)
>     } else {
>       rect = rect.Intersect(scrollFrame->GetScrollPortRect());
>     }
>     rect = scrollFrame->ExpandRectToNearlyVisible(rect);
>   }
> 
>   bool preserves3DChildren = aFrame->Extend3DContext();
> 
>-  // we assume all images in popups are visible elsewhere, so we skip them here
>+  // We assume all frames in popups are visible, so we skip them here.
>   const nsIFrame::ChildListIDs skip(nsIFrame::kPopupList |
>                                     nsIFrame::kSelectPopupList);
>   for (nsIFrame::ChildListIterator childLists(aFrame);
>        !childLists.IsDone(); childLists.Next()) {
>     if (skip.Contains(childLists.CurrentID())) {
>       continue;
>     }
> 
>@@ -5735,77 +5732,75 @@ PresShell::MarkImagesInSubtreeVisible(nsIFrame* aFrame, const nsRect& aRect)
>           nsRect out;
>           if (nsDisplayTransform::UntransformRect(r, overflow, child, nsPoint(0,0), &out)) {
>             r = out;
>           } else {
>             r.SetEmpty();
>           }
>         }
>       }
>-      MarkImagesInSubtreeVisible(child, r);
>+      MarkFramesInSubtreeVisible(child, r);
>     }
>   }
> }
> 
> void
>-PresShell::RebuildImageVisibility(nsRect* aRect)
>+PresShell::RebuildFrameVisibility(nsRect* aRect)
> {
>-  MOZ_ASSERT(!mImageVisibilityVisited, "already visited?");
>-  mImageVisibilityVisited = true;
>+  MOZ_ASSERT(!mFrameVisibilityVisited, "already visited?");
>+  mFrameVisibilityVisited = true;
> 
>   nsIFrame* rootFrame = GetRootFrame();
>   if (!rootFrame) {
>     return;
>   }
> 
>-  // Remove the entries of the mVisibleImages hashtable and put them in
>-  // oldVisibleImages.
>-  nsTHashtable< nsRefPtrHashKey<nsIImageLoadingContent> > oldVisibleImages;
>-  mVisibleImages.SwapElements(oldVisibleImages);
>+  // Remove the entries of the mVisibleFrames hashtable and put them in
>+  // oldVisibleFrames.
>+  nsTHashtable<nsPtrHashKey<nsIFrame>> oldVisibleFrames;
>+  mVisibleFrames.SwapElements(oldVisibleFrames);
> 
>   nsRect vis(nsPoint(0, 0), rootFrame->GetSize());
>   if (aRect) {
>     vis = *aRect;
>   }
>-  MarkImagesInSubtreeVisible(rootFrame, vis);
>+  MarkFramesInSubtreeVisible(rootFrame, vis);
> 
>-  DecrementVisibleCount(oldVisibleImages,
>-                        nsIImageLoadingContent::ON_NONVISIBLE_NO_ACTION);
>+  DecrementVisibleCount(oldVisibleFrames, OnNonvisible::NO_ACTION);
> }
> 
> void
>-PresShell::UpdateImageVisibility()
>+PresShell::UpdateFrameVisibility()
> {
>   MOZ_ASSERT(!mPresContext || mPresContext->IsRootContentDocument(),
>-    "updating image visibility on a non-root content document?");
>+             "Updating frame visibility on a non-root content document?");
> 
>-  mUpdateImageVisibilityEvent.Revoke();
>+  mUpdateFrameVisibilityEvent.Revoke();
> 
>   if (mHaveShutDown || mIsDestroying) {
>     return;
>   }
> 
>   // call update on that frame
>   nsIFrame* rootFrame = GetRootFrame();
>   if (!rootFrame) {
>-    ClearVisibleImagesList(
>-      nsIImageLoadingContent::ON_NONVISIBLE_REQUEST_DISCARD);
>+    ClearVisibleFramesList(OnNonvisible::DISCARD_IMAGES);
>     return;
>   }
> 
>-  RebuildImageVisibility();
>-  ClearImageVisibilityVisited(rootFrame->GetView(), true);
>+  RebuildFrameVisibility();
>+  ClearFrameVisibilityVisited(rootFrame->GetView(), true);
> 
>-#ifdef DEBUG_IMAGE_VISIBILITY_DISPLAY_LIST
>-  // This can be used to debug the frame walker by comparing beforeImageList and
>-  // mVisibleImages in RebuildImageVisibilityDisplayList to see if they produce
>-  // the same results (mVisibleImages holds the images the display list thinks
>-  // are visible, beforeImageList holds the images the frame walker thinks are
>+#ifdef DEBUG_FRAME_VISIBILITY_DISPLAY_LIST
>+  // This can be used to debug the frame walker by comparing beforeFrameList and
>+  // mVisibleFrames in RebuildFrameVisibilityDisplayList to see if they produce
>+  // the same results (mVisibleFrames holds the frames the display list thinks
>+  // are visible, beforeFrameList holds the frames the frame walker thinks are
>   // visible).
>-  nsDisplayListBuilder builder(rootFrame, nsDisplayListBuilder::IMAGE_VISIBILITY, false);
>+  nsDisplayListBuilder builder(rootFrame, nsDisplayListBuilder::FRAME_VISIBILITY, false);
>   nsRect updateRect(nsPoint(0, 0), rootFrame->GetSize());
>   nsIFrame* rootScroll = GetRootScrollFrame();
>   if (rootScroll) {
>     nsIContent* content = rootScroll->GetContent();
>     if (content) {
>       nsLayoutUtils::GetDisplayPort(content, &updateRect);
>     }
> 
>@@ -5818,128 +5813,129 @@ PresShell::UpdateImageVisibility()
>     }
>   }
>   builder.IgnorePaintSuppression();
>   builder.EnterPresShell(rootFrame, updateRect);
>   nsDisplayList list;
>   rootFrame->BuildDisplayListForStackingContext(&builder, updateRect, &list);
>   builder.LeavePresShell(rootFrame, updateRect);
> 
>-  RebuildImageVisibilityDisplayList(list);
>+  RebuildFrameVisibilityDisplayList(list);
> 
>-  ClearImageVisibilityVisited(rootFrame->GetView(), true);
>+  ClearFrameVisibilityVisited(rootFrame->GetView(), true);
> 
>   list.DeleteAll();
> #endif
> }
> 
> bool
>-PresShell::AssumeAllImagesVisible()
>+PresShell::AssumeAllFramesVisible()
> {
>-  static bool sImageVisibilityEnabled = true;
>-  static bool sImageVisibilityPrefCached = false;
>+  static bool sFrameVisibilityEnabled = true;
>+  static bool sFrameVisibilityPrefCached = false;
> 
>-  if (!sImageVisibilityPrefCached) {
>-    Preferences::AddBoolVarCache(&sImageVisibilityEnabled,
>-      "layout.imagevisibility.enabled", true);
>-    sImageVisibilityPrefCached = true;
>+  if (!sFrameVisibilityPrefCached) {
>+    Preferences::AddBoolVarCache(&sFrameVisibilityEnabled,
>+      "layout.framevisibility.enabled", true);
>+    sFrameVisibilityPrefCached = true;
>   }
> 
>-  if (!sImageVisibilityEnabled || !mPresContext || !mDocument) {
>+  if (!sFrameVisibilityEnabled || !mPresContext || !mDocument) {
>     return true;
>   }
> 
>-  // We assume all images are visible in print, print preview, chrome, xul, and
>+  // We assume all frames are visible in print, print preview, chrome, xul, and
>   // resource docs and don't keep track of them.
>   if (mPresContext->Type() == nsPresContext::eContext_PrintPreview ||
>       mPresContext->Type() == nsPresContext::eContext_Print ||
>       mPresContext->IsChrome() ||
>       mDocument->IsResourceDoc() ||
>       mDocument->IsXULDocument()) {
>     return true;
>   }
> 
>   return false;
> }
> 
> void
>-PresShell::ScheduleImageVisibilityUpdate()
>+PresShell::ScheduleFrameVisibilityUpdate()
> {
>-  if (AssumeAllImagesVisible())
>+  if (AssumeAllFramesVisible())
>     return;
> 
>   if (!mPresContext->IsRootContentDocument()) {
>     nsPresContext* presContext = mPresContext->GetToplevelContentDocumentPresContext();
>     if (!presContext)
>       return;
>     MOZ_ASSERT(presContext->IsRootContentDocument(),
>       "Didn't get a root prescontext from GetToplevelContentDocumentPresContext?");
>-    presContext->PresShell()->ScheduleImageVisibilityUpdate();
>+    presContext->PresShell()->ScheduleFrameVisibilityUpdate();
>     return;
>   }
> 
>   if (mHaveShutDown || mIsDestroying)
>     return;
> 
>-  if (mUpdateImageVisibilityEvent.IsPending())
>+  if (mUpdateFrameVisibilityEvent.IsPending())
>     return;
> 
>   RefPtr<nsRunnableMethod<PresShell> > ev =
>-    NS_NewRunnableMethod(this, &PresShell::UpdateImageVisibility);
>+    NS_NewRunnableMethod(this, &PresShell::UpdateFrameVisibility);
>   if (NS_SUCCEEDED(NS_DispatchToCurrentThread(ev))) {
>-    mUpdateImageVisibilityEvent = ev;
>+    mUpdateFrameVisibilityEvent = ev;
>   }
> }
> 
> void
>-PresShell::EnsureImageInVisibleList(nsIImageLoadingContent* aImage)
>+PresShell::EnsureFrameInVisibleList(nsIFrame* aFrame)
> {
>-  if (AssumeAllImagesVisible()) {
>-    aImage->IncrementVisibleCount();
>+  if (AssumeAllFramesVisible()) {
>+    aFrame->IncrementVisibleCount();
>     return;
>   }
> 
> #ifdef DEBUG
>-  // if it has a frame make sure its in this presshell
>-  nsCOMPtr<nsIContent> content = do_QueryInterface(aImage);
>+  // Make sure it's in this presshell.
>+  nsCOMPtr<nsIContent> content = aFrame->GetContent();
>   if (content) {
>     PresShell* shell = static_cast<PresShell*>(content->OwnerDoc()->GetShell());
>     MOZ_ASSERT(!shell || shell == this, "wrong shell");
>   }
> #endif
> 
>-  if (!mVisibleImages.Contains(aImage)) {
>-    mVisibleImages.PutEntry(aImage);
>-    aImage->IncrementVisibleCount();
>+  if (!mVisibleFrames.Contains(aFrame)) {
>+    MOZ_ASSERT(!AssumeAllFramesVisible());
>+    mVisibleFrames.PutEntry(aFrame);
>+    aFrame->IncrementVisibleCount();
>   }
> }
> 
> void
>-PresShell::RemoveImageFromVisibleList(nsIImageLoadingContent* aImage)
>+PresShell::RemoveFrameFromVisibleList(nsIFrame* aFrame)
> {
> #ifdef DEBUG
>-  // if it has a frame make sure its in this presshell
>-  nsCOMPtr<nsIContent> content = do_QueryInterface(aImage);
>+  // Make sure it's in this presshell.
>+  nsCOMPtr<nsIContent> content = aFrame->GetContent();
>   if (content) {
>     PresShell* shell = static_cast<PresShell*>(content->OwnerDoc()->GetShell());
>     MOZ_ASSERT(!shell || shell == this, "wrong shell");
>   }
> #endif
> 
>-  if (AssumeAllImagesVisible()) {
>-    MOZ_ASSERT(mVisibleImages.Count() == 0, "shouldn't have any images in the table");
>+  if (AssumeAllFramesVisible()) {
>+    MOZ_ASSERT(mVisibleFrames.Count() == 0,
>+               "Shouldn't have any frames in the table");
>     return;
>   }
> 
>-  uint32_t count = mVisibleImages.Count();
>-  mVisibleImages.RemoveEntry(aImage);
>-  if (mVisibleImages.Count() < count) {
>-    // aImage was in the hashtable, so we need to decrement its visible count
>-    aImage->DecrementVisibleCount(
>-      nsIImageLoadingContent::ON_NONVISIBLE_NO_ACTION);
>+  uint32_t count = mVisibleFrames.Count();
>+  mVisibleFrames.RemoveEntry(aFrame);
>+  if (mVisibleFrames.Count() < count) {
>+    // aFrame was in the hashtable, so we need to decrement its visible count
>+    aFrame->DecrementVisibleCount(OnNonvisible::NO_ACTION);
>   }
> }
> 
> class nsAutoNotifyDidPaint
> {
> public:
>   nsAutoNotifyDidPaint(PresShell* aShell, uint32_t aFlags)
>     : mShell(aShell), mFlags(aFlags)
>@@ -6008,17 +6004,17 @@ PresShell::Paint(nsView*        aViewToPaint,
>                  uint32_t        aFlags)
> {
>   PROFILER_LABEL("PresShell", "Paint",
>     js::ProfileEntry::Category::GRAPHICS);
> 
>   NS_ASSERTION(!mIsDestroying, "painting a destroyed PresShell");
>   NS_ASSERTION(aViewToPaint, "null view");
> 
>-  MOZ_ASSERT(!mImageVisibilityVisited, "should have been cleared");
>+  MOZ_ASSERT(!mFrameVisibilityVisited, "Should have been cleared");
> 
>   if (!mIsActive || mIsZombie) {
>     return;
>   }
> 
>   nsPresContext* presContext = GetPresContext();
>   AUTO_LAYOUT_PHASE_ENTRY_POINT(presContext, Paint);
> 
>@@ -8690,17 +8686,17 @@ FreezeSubDocument(nsIDocument *aDocument, void *aData)
>     shell->Freeze();
> 
>   return true;
> }
> 
> void
> PresShell::Freeze()
> {
>-  mUpdateImageVisibilityEvent.Revoke();
>+  mUpdateFrameVisibilityEvent.Revoke();
> 
>   MaybeReleaseCapturingContent();
> 
>   mDocument->EnumerateActivityObservers(FreezeElement, nullptr);
> 
>   if (mCaret) {
>     SetCaretEnabled(false);
>   }
>@@ -10681,21 +10677,20 @@ nsresult
> PresShell::UpdateImageLockingState()
> {
>   // We're locked if we're both thawed and active.
>   bool locked = !mFrozen && mIsActive;
> 
>   nsresult rv = mDocument->SetImageLockingState(locked);
> 
>   if (locked) {
>-    // Request decodes for visible images; we want to start decoding as
>+    // Request decodes for visible image frames; we want to start decoding as
>     // quickly as possible when we get foregrounded to minimize flashing.
>-    for (auto iter = mVisibleImages.Iter(); !iter.Done(); iter.Next()) {
>-      nsCOMPtr<nsIContent> content = do_QueryInterface(iter.Get()->GetKey());
>-      nsImageFrame* imageFrame = do_QueryFrame(content->GetPrimaryFrame());
>+    for (auto iter = mVisibleFrames.Iter(); !iter.Done(); iter.Next()) {
>+      nsImageFrame* imageFrame = do_QueryFrame(iter.Get()->GetKey());
>       if (imageFrame) {
>         imageFrame->MaybeDecodeForPredictedSize();
>       }
>     }
>   }
> 
>   return rv;
> }
>@@ -10720,17 +10715,17 @@ PresShell::AddSizeOfIncludingThis(MallocSizeOf aMallocSizeOf,
>                                   size_t *aTextRunsSize,
>                                   size_t *aPresContextSize)
> {
>   mFrameArena.AddSizeOfExcludingThis(aMallocSizeOf, aArenaObjectsSize);
>   *aPresShellSize += aMallocSizeOf(this);
>   if (mCaret) {
>     *aPresShellSize += mCaret->SizeOfIncludingThis(aMallocSizeOf);
>   }
>-  *aPresShellSize += mVisibleImages.ShallowSizeOfExcludingThis(aMallocSizeOf);
>+  *aPresShellSize += mVisibleFrames.ShallowSizeOfExcludingThis(aMallocSizeOf);
>   *aPresShellSize += mFramesToDirty.ShallowSizeOfExcludingThis(aMallocSizeOf);
>   *aPresShellSize += aArenaObjectsSize->mOther;
> 
>   *aStyleSetsSize += StyleSet()->SizeOfIncludingThis(aMallocSizeOf);
> 
>   *aTextRunsSize += SizeOfTextRuns(aMallocSizeOf);
> 
>   *aPresContextSize += mPresContext->SizeOfIncludingThis(aMallocSizeOf);
>diff --git a/layout/base/nsPresShell.h b/layout/base/nsPresShell.h
>index 1cf9369..f768d3f 100644
>--- a/layout/base/nsPresShell.h
>+++ b/layout/base/nsPresShell.h
>@@ -382,26 +382,32 @@ public:
>   // This data is stored as a content property (nsGkAtoms::scrolling) on
>   // mContentToScrollTo when we have a pending ScrollIntoView.
>   struct ScrollIntoViewData {
>     ScrollAxis mContentScrollVAxis;
>     ScrollAxis mContentScrollHAxis;
>     uint32_t   mContentToScrollToFlags;
>   };
> 
>-  virtual void ScheduleImageVisibilityUpdate() override;
> 
>-  virtual void RebuildImageVisibilityDisplayList(const nsDisplayList& aList) override;
>-  virtual void RebuildImageVisibility(nsRect* aRect = nullptr) override;
>+  //////////////////////////////////////////////////////////////////////////////
>+  // Frame visibility tracking public API.
>+  //////////////////////////////////////////////////////////////////////////////
> 
>-  virtual void EnsureImageInVisibleList(nsIImageLoadingContent* aImage) override;
>+  void ScheduleFrameVisibilityUpdate() override;
> 
>-  virtual void RemoveImageFromVisibleList(nsIImageLoadingContent* aImage) override;
>+  void RebuildFrameVisibilityDisplayList(const nsDisplayList& aList) override;
>+  void RebuildFrameVisibility(nsRect* aRect = nullptr) override;
>+
>+  void EnsureFrameInVisibleList(nsIFrame* aFrame) override;
>+
>+  void RemoveFrameFromVisibleList(nsIFrame* aFrame) override;
>+
>+  bool AssumeAllFramesVisible() override;
> 
>-  virtual bool AssumeAllImagesVisible() override;
> 
>   virtual void RecordShadowStyleChange(mozilla::dom::ShadowRoot* aShadowRoot) override;
> 
>   virtual void DispatchAfterKeyboardEvent(nsINode* aTarget,
>                                           const mozilla::WidgetKeyboardEvent& aEvent,
>                                           bool aEmbeddedCancelled) override;
> 
>   void SetNextPaintCompressed() { mNextPaintCompressed = true; }
>@@ -731,27 +737,40 @@ protected:
>   bool ScheduleReflowOffTimer();
> 
>   // Widget notificiations
>   virtual void WindowSizeMoveDone() override;
>   virtual void SysColorChanged() override { mPresContext->SysColorChanged(); }
>   virtual void ThemeChanged() override { mPresContext->ThemeChanged(); }
>   virtual void BackingScaleFactorChanged() override { mPresContext->UIResolutionChanged(); }
> 
>-  void UpdateImageVisibility();
>   void UpdateActivePointerState(mozilla::WidgetGUIEvent* aEvent);
> 
>-  nsRevocableEventPtr<nsRunnableMethod<PresShell> > mUpdateImageVisibilityEvent;
> 
>-  void ClearVisibleImagesList(uint32_t aNonvisibleAction);
>-  static void ClearImageVisibilityVisited(nsView* aView, bool aClear);
>-  static void MarkImagesInListVisible(const nsDisplayList& aList);
>-  void MarkImagesInSubtreeVisible(nsIFrame* aFrame, const nsRect& aRect);
>+  //////////////////////////////////////////////////////////////////////////////
>+  // Frame visibility tracking implementation.
>+  //////////////////////////////////////////////////////////////////////////////
> 
>+  void UpdateFrameVisibility();
>+
>+  void ClearVisibleFramesList(mozilla::OnNonvisible aNonvisibleAction);
>+  static void ClearFrameVisibilityVisited(nsView* aView, bool aClear);
>+  static void MarkFramesInListVisible(const nsDisplayList& aList);
>+  void MarkFramesInSubtreeVisible(nsIFrame* aFrame, const nsRect& aRect);
>+
>+  nsRevocableEventPtr<nsRunnableMethod<PresShell> > mUpdateFrameVisibilityEvent;
>+
>+  // A set of frames that are visible or almost visible.
>+  nsTHashtable<nsPtrHashKey<nsIFrame>> mVisibleFrames;
>+
>+
>+  //////////////////////////////////////////////////////////////////////////////
>   // Methods for dispatching KeyboardEvent and BeforeAfterKeyboardEvent.
>+  //////////////////////////////////////////////////////////////////////////////
>+
>   void HandleKeyboardEvent(nsINode* aTarget,
>                            mozilla::WidgetKeyboardEvent& aEvent,
>                            bool aEmbeddedCancelled,
>                            nsEventStatus* aStatus,
>                            mozilla::EventDispatchingCallback* aEventCB);
>   void DispatchBeforeKeyboardEventInternal(
>          const nsTArray<nsCOMPtr<mozilla::dom::Element> >& aChain,
>          const mozilla::WidgetKeyboardEvent& aEvent,
>@@ -759,19 +778,16 @@ protected:
>          bool& aDefaultPrevented);
>   void DispatchAfterKeyboardEventInternal(
>          const nsTArray<nsCOMPtr<mozilla::dom::Element> >& aChain,
>          const mozilla::WidgetKeyboardEvent& aEvent,
>          bool aEmbeddedCancelled,
>          size_t aChainIndex = 0);
>   bool CanDispatchEvent(const mozilla::WidgetGUIEvent* aEvent = nullptr) const;
> 
>-  // A list of images that are visible or almost visible.
>-  nsTHashtable< nsRefPtrHashKey<nsIImageLoadingContent> > mVisibleImages;
>-
>   nsresult SetResolutionImpl(float aResolution, bool aScaleToResolution);
> 
> #ifdef DEBUG
>   // The reflow root under which we're currently reflowing.  Null when
>   // not in reflow.
>   nsIFrame*                 mCurrentReflowRoot;
>   uint32_t                  mUpdateCount;
> #endif
>@@ -871,17 +887,17 @@ protected:
> 
>   // Indicates that it is safe to unlock painting once all pending reflows
>   // have been processed.
>   bool                      mShouldUnsuppressPainting : 1;
> 
>   bool                      mAsyncResizeTimerIsActive : 1;
>   bool                      mInResize : 1;
> 
>-  bool                      mImageVisibilityVisited : 1;
>+  bool                      mFrameVisibilityVisited : 1;
> 
>   bool                      mNextPaintCompressed : 1;
> 
>   bool                      mHasCSSBackgroundColor : 1;
> 
>   // Whether content should be scaled by the resolution amount. If this is
>   // not set, a transform that scales by the inverse of the resolution is
>   // applied to rendered layers.
>

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

Comment on attachment 8686384 [details] [diff] [review]
(Part 1a) - Track visibility of frames instead of images in nsPresShell code.

The display list based image visibility code didn't get updated at all. So that means this broke it.

I kept the code in the tree because it was invaluable to compare my frame tree walker work to the display list code to make sure I got it right. It uncovered many issues that would have been down right hard to find any other way. I think it should be maintained going forward so that we can maintain correctness of this code.

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

(In reply to Timothy Nikkel (:tn) from comment #33)
> So this is going to track every frame? Not just ones that might actually
> care about visibility? That seems like there is going to be significant
> overhead for doing that.

In fact, even if a frame has an image I think we still might not want to bother tracking it individually. There are probably a lot of small background images and border images that aren't worth our time and we should just mark visible. Some threshold based on size or memory of the images would probably be a good idea.

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

(In reply to Timothy Nikkel (:tn) from comment #27)
> I'm more concerned with marking the images in destroyed frames as
> non-visible, discarding the decoding image. Then reconstructing the frame
> (as part of the same operation, but not the same turn of the event loop),
> marking it visible, and re-decoding the image.

We can in theory discard the image in such a situation under memory pressure, but we don't request a discard when a frame is destroyed. In practice I don't think this is a problem.

> Not sure if the same system can be used to track "almost visibility" and
> "actual visibility" cohesively.

I think it can. Right now the API just provides OnBecameVisible() and OnBecameNonvisible() notifications, but I intend fairly soon to include additional visibility states. The final API will probably provide an OnVisibleTransition(VisibilityState) callback, where VisibleState is an enum defining a range of possible visibilities, from "outside of viewport in hidden document" all the way to "in viewport in visible document".

At the implementation level, we'll ultimately be coalescing information from several sources. The best source of information for what's actually visible in the viewport is probably painting, so extracting this information from the display list when we paint is probably the best approach, but I haven't investigated this yet.

> My concern lies with pretty much anyone who isn't you or me, not just new
> people. History shows that if you give something a simple, and tempting,
> name which does not actually reflect what it does it will be mis-used.

The goal is to rapidly evolve this into an API which *does* satisfy these different use cases. Having a reliable visibility API will make it drastically easier to implement  optimizations related to memory and power usage that Gecko really needs.

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

(In reply to Timothy Nikkel (:tn) from comment #31)
> It's not specific to feImage, and we most certainly do have a frame, we just
> can't trust the pointers at this point.

I actually don't think that we should be calling TrackImage() in all of the situations we are now. It should be sufficient to just call it in nsImageLoadingContent::OnBecameVisible(). With that change, this will certainly only apply to feImage and the like. It's worth updating the comment to call to attention to these other scenarios, though. (Oddly enough I didn't encounter them in my testing, but it's possible that once again our test coverage is poor...)

> (mFrameCreateCalled is used for another purpose: to distinguish the case
> where an image gets an nsImageFrame (which tracks visibility) vs when it
> gets an nsInlineFrame (early in the loading of the image) which does not
> track visibility information, so we can't trust visibility information)
> 
> I should have documented this better.

At this point that distinction doesn't matter, though, right? Now the visibility of all frames is tracked.

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

(In reply to Timothy Nikkel (:tn) from comment #35)
> (In reply to Timothy Nikkel (:tn) from comment #33)
> > So this is going to track every frame? Not just ones that might actually
> > care about visibility? That seems like there is going to be significant
> > overhead for doing that.
> 
> In fact, even if a frame has an image I think we still might not want to
> bother tracking it individually. There are probably a lot of small
> background images and border images that aren't worth our time and we should
> just mark visible. Some threshold based on size or memory of the images
> would probably be a good idea.

So my original implementation had a flag that could be used to mark frame types as not caring about visibility, but unfortunately, there just aren't that many that don't care - almost everything can have a background or a border, and there are things besides images that we want to track. Implementing Google's visibility API will also force us to track this information for arbitrary frames.

What we could potentially do is dynamically mark which frames care about visibility (say, if they know that they have a background or a border, or if they're an image-related frame, or an iframe, or... etc) and avoid making the virtual OnBecameVisible() / OnBecameNonvisible() calls. We can implement this in nsIFrame without touching the frame visibility code on the pres shell.

We're already paying for the code that walks the frame tree _today_; we can't avoid that. So I think the marginal computational cost here is more or less just the cost of incrementing or decrementing a counter, and calling a virtual method when a frame becomes visible or stops being visible. I expect that cost to be small enough in practice that I think we can treat dynamically marking which frames care about visibility as an optimization that can happen in a followup bug, and I'd prefer that approach, because I am concerned that getting that right may be tricky and I'd rather debug it separately.

(Honestly, though, without some measurements I'm kinda skeptical that this is worth doing. Perhaps if the memory usage of the visible frame list itself proves problematic, it'd be justified. What I suspect we'd get a lot more benefit from in computational terms would be to continue to track all frames, but use a different update interval for frames that only care about approximate visibility than for frames that require more accurate information. We may be able to skip the vast majority of the frame tree on most updates by only walking over subtrees marked as needing accurate visibility and subtrees that are currently marked non-visible.)

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

(In reply to Timothy Nikkel (:tn) from comment #30)
> Adding a uint32_t to nsIFrame? Not sure I could r+ that by myself. It could
> have significant memory implications, in the past we've not added 4 byte
> fields to nsIFrame lightly.

We can potentially implement the visible count as a frame property which gets destroyed when it drops to zero, so if the frame property doesn't exist, that means that the frame is not visible. The downside is of course that we have to create and destroy frame properties with some regularity in that scheme.

David, what do you think? Are the memory benefits of taking this approach worth the computational overhead? (I'm not sure how expensive it is to create or destroy a frame property; are we using an arena to avoid allocations?) I suspect the answer is yes, unless creating/destroying frame properties is fairly expensive.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 40 • 9 years ago

It seems like the cases where the visibility count becomes greater than 1 are pretty rare, no?  (Is there anything other than -moz-element()?)  If so, it seems like it could be implemented pretty easily using a bit and a uintptr_t frame property, or two bits (the second to indicate the frame property exists) and a frame property.  Or am I misunderstanding something?

That said, I'm not sure we have any bits free.  We may need to split the class-specific bits into their own 32-bit member variable, separate from the 64-bit general bits.  (And probably a good time to make them an enum class at the same time, to get better typechecking; but also see https://groups.google.com/d/msg/mozilla.dev.tech.layout/eYJkMF84rsE/GuSapCWkCwAJ .)

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

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #40)
> It seems like the cases where the visibility count becomes greater than 1
> are pretty rare, no?  (Is there anything other than -moz-element()?)

For any visible frame the count will hit at least 2 on a regular basis, because whenever we recompute visibility, the algorithm we're using increments the visibiity count for all frames that are currently visible, and then decrements it for all frames that were previously visible.

> If so,
> it seems like it could be implemented pretty easily using a bit and a
> uintptr_t frame property

I suppose the bit is to avoid the expense of a hash table lookup?

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

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #40)
> That said, I'm not sure we have any bits free.

Just checked, and bit 35 is free. I'll update the patch to use it.

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

(In reply to Timothy Nikkel (:tn) from comment #34)
> Comment on attachment 8686384 [details] [diff] [review]
> (Part 1a) - Track visibility of frames instead of images in nsPresShell code.
> 
> The display list based image visibility code didn't get updated at all. So
> that means this broke it.

It did get updated... Unless I'm missing something? MarkFramesInListApproximatelyVisible(), RebuildApproximateFrameVisibilityDisplayList(), the code in #if DEBUG, it's all touched in this patch. (I'm using the new names here, from the version I'm about to upload.)

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

(In reply to Timothy Nikkel (:tn) from comment #33)
> You remove the check of not recursing into a presshell with
> AssumeAllFramesVisible below. So why add this assert? (Although this
> situation is likely quite unusual.)
> 
> If we are going to recurse into AssumeAllFramesVisible presshells, shouldn't
> we check that we are not a AssumeAllFramesVisible presshell before adding
> stuff to its list of visible frames?

Not following here; I _added_ that check. We do not recurse into AssumeAllFramesVisible pres shells with the new code.

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

OK, here are refactored versions of all of the patches in this bug. The changes are:

- Review comments about correctness above are addressed.

- Whenever there's a reference to the *current* frame visibility algorithm
  (i.e., the old image visibility algorithm, but repurposed for frames), I've
  changed the code to refer to it as the *approximate* frame visibility
  algorithm. Many method names, variable names, etc. are updated.

- The portion of the frame visibility API that is public is clarified.  Now the
  only public method is GetVisibility(), which returns an enum with values
  APPROXIMATELY_VISIBLE and APPROXIMATELY_NONVISIBLE. This clarifies the meaning
  of these values and gives us an extension point for later. The protected API
  for frame classes now only involves implementing OnVisibilityChanged(), which
  takes as an argument the new visibility state (using the same enum mentioned
  previously) and optionally a suggested action if the new state is nonvisible.

- We now only pay for the visibility state (right now just a counter, though
  eventually we'll need more) on frames that are visible, because we store the
  visibility state on a frame property. A frame state bit is used to avoid a
  hashtable lookup on nonvisible frames. This eliminates the concern about
  adding 4 bytes of overhead to every frame.

- I've tried to clarify the comments a bit.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 50 • 8 years ago

Tim, we really need to move forward on this ASAP. It's a requirement for bug 1243846, which is high priority right now. What can I do to help?

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 51 • 8 years ago

I've rebased all of these patches and I've additionally modified them by making
it so the frame state bit determines whether or not we're interested in tracking
the visibility of the frame at all, rather than whether it has a non-zero
visible count. (In our discussion about these patches, I thought I had already
implemented this, but I think this was just discussed but left for a followup
bug in the past.)

This means in practice that:

1. We should, after this patch, only track the visibility of exactly the types
   of frames that we used to track for image visibility purposes. No
   visibility-related behavior should change. I've gone out of my way to ensure
   this. In later bugs we'll probably end up changing those behaviors at least a
   little, in particular to get rid of synchronous visibility updates if we can;
   see the comments in nsImageFrame.cpp and nsSVGImageFrame.cpp. But we're not
   ready to make those changes yet.

2. For frames whose visibility we *do* want to track, we need to look up the
   frame property to determine the visibility state of the frame every time.
   That is, we no longer have a bit telling us whether the visible count is
   zero, as we used to. Since we are tracking the visibility of many fewer types
   of frames, and most visibility-related code will rely on the
   OnVisibilityChanged() event rather than querying the current visibility of a
   frame, I suspect this is the right tradeoff - it doesn't seem like it's worth
   it to spend a second frame state bit on this. However, we can certainly
   reintroduce that additional bit if we determine that it's worth it from a
   performance standpoint.

That sums up the intent of the changes. There are a number of consequences
throughout the patches, but nothing earth-shattering. Probably the most obvious
one is that frames that are interested in having their visibility tracked need
to call EnableVisibilityTracking() in their constructors.

I'm reuploading all the patches now, but I won't request review until I've
confirmed that these patches look good on try and don't cause Talos regressions.
The patches were green in the past, if I recall correctly, but it's quite
possible that either rebasing or the refactoring discussed above could've broken
something. I've already spent some time surfing with the patches and the APZ
minimap visibility visualization enabled and based on that rough testing
everything seems fine.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 56 • 8 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 57 • 8 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 58 • 8 years ago

Doh, forgot to check if we were tracking visibility of a frame before
decrementing its visible count in EnsureFrameInApproximatelyVisibleList, which
triggered an assert in debug builds in some cases.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 59 • 8 years ago

I accidentally called nsSVGImageFrame::OnVisibilityChanged() instead of
nsSVGImageFrameBase::OnVisibilityChanged() in
nsSVGImageFrame::OnVisibilityChanged(), which obviously triggered an infinite
recursion that broke a lot of SVG tests.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 60 • 8 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 61 • 8 years ago

OK, try looks green on the job in comment 60, and there are no regressions on Talos. The try job in comment 57 revealed an (apparently Windows-specific) warnings-as-error compilation failure because I used MOZ_ASSERT_UNREACHABLE without providing a reason string. I'll update part 1c to fix that issue, and then push a new try job for the remaining platforms. (Comment 60 only included OS X.)

The results in comment 60 suggest that we're ready for review at this point.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 62 • 8 years ago

Provide a reason for MOZ_ASSERT_UNREACHABLE. The patch is otherwise unchanged.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 63 • 8 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 64 • 8 years ago

The Windows tests still haven't run (though the build issue is fixed), but the Linux tests and Talos benchmarks are all looking good.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 65 • 8 years ago

If bug 1242256 lands first, a minor rebase of part 1e will be needed. Should be trivial, though.

	Markus Stange [:mstange]
	Comment 66 • 8 years ago

Comment on attachment 8733615 [details] [diff] [review]
(Part 1a) - Track visibility of frames instead of images in nsPresShell code.

Review of attachment 8733615 [details] [diff] [review]:
-----------------------------------------------------------------

It's a little confusing that this part calls nsIFrame::TrackingVisibility which is only added in part 1b.

::: layout/base/nsPresShell.cpp
@@ +5769,2 @@
>    }
>    for (nsView* v = aView->GetFirstChild(); v; v = v->GetNextSibling()) {

Why is this dealing with views? Does it want to traverse subdocuments? Oh, I suppose it wants both subdocuments and popups?

	Markus Stange [:mstange]
	Comment 67 • 8 years ago

Comment on attachment 8733183 [details] [diff] [review]
(Part 1b) - Replace visibility tracking in nsIImageLoadingContent with a similar API on nsIFrame.

Review of attachment 8733183 [details] [diff] [review]:
-----------------------------------------------------------------

I would have preferred the nsIImageLoadingContent.idl to be in part 1c.

::: layout/generic/nsFrame.cpp
@@ +1475,5 @@
> +
> +  bool isSet = false;
> +  FrameProperties props = Properties();
> +  uint32_t visibleCount =
> +    static_cast<uint32_t>(props.Get(VisibilityStateProperty(), &isSet));

Is the cast still necessary?

@@ +1512,5 @@
> +        visible = false;
> +        break;
> +      }
> +      // Move transformedRect to be contained in the scrollport as best we can
> +      // (it might not fit) to pretend that it was scrolled into view.

Please add another sentence that says why we do this.

@@ +1514,5 @@
> +      }
> +      // Move transformedRect to be contained in the scrollport as best we can
> +      // (it might not fit) to pretend that it was scrolled into view.
> +      nsRect scrollPort = sf->GetScrollPortRect();
> +      if (transformedRect.XMost() > scrollPort.XMost()) {

BaseRect has MoveInsideAndClamp. I think that's exactly what you want here.

@@ +1587,5 @@
> +
> +  bool isSet = false;
> +  FrameProperties props = Properties();
> +  uint32_t visibleCount =
> +    static_cast<uint32_t>(props.Remove(VisibilityStateProperty(), &isSet));

possibly unnecessary cast

@@ +1611,5 @@
> +
> +  bool isSet = false;
> +  FrameProperties props = Properties();
> +  uint32_t visibleCount =
> +    static_cast<uint32_t>(props.Get(VisibilityStateProperty(), &isSet));

and here

@@ +1636,5 @@
> +
> +  bool isSet = false;
> +  FrameProperties props = Properties();
> +  uint32_t visibleCount =
> +    static_cast<uint32_t>(props.Get(VisibilityStateProperty(), &isSet));

and here

	Markus Stange [:mstange]
	Comment 68 • 8 years ago

Comment on attachment 8733616 [details] [diff] [review]
(Part 1e) - Use the new frame visibility API in frame implementations.

Review of attachment 8733616 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsVideoFrame.cpp
@@ +41,5 @@
>  
>  NS_IMPL_FRAMEARENA_HELPERS(nsVideoFrame)
>  
> +nsVideoFrame::nsVideoFrame(nsStyleContext* aContext)
> +  : nsContainerFrame(aContext)

you could use nsVideoFrameBase here

::: layout/svg/SVGFEImageFrame.cpp
@@ +30,5 @@
> +
> +    // This frame isn't actually displayed, but it contains an image and we want
> +    // to use the nsImageLoadingContent machinery for managing images, which
> +    // requires visibility tracking, so we enable visibility tracking and
> +    // forcibly mark it visible below.

This is a good comment.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 69 • 8 years ago

Thanks for the quick reviews, Markus! I'll upload new versions of the patches addressing those comments ASAP.

BTW, I know it's confusing that the patches all kinda mutually depend on each other. Unfortunately none of them compile in isolation, and they'll have to all be folded into one patch to land. I was just trying to separate them into logical subpatches to make it easier to review (which I may not have done perfectly).

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 70 • 8 years ago

(In reply to Markus Stange [:mstange] from comment #66)
> Why is this dealing with views? Does it want to traverse subdocuments? Oh, I
> suppose it wants both subdocuments and popups?

Yep, I believe so. This is actually taken verbatim from the existing image visibility code. I admit that views are an aspect of Gecko that I don't understand as well as other parts of layout.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 71 • 8 years ago

All reviews comments should be addressed in this new version of the patches.
Additionally, they've been rebased against m-c.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 76 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d171d75b7464583358fcfb7524617ad5dce6a6f
Bug 1157546 - Replace the image visibility API with a more general API that tracks visibility for any kind of frame. r=mstange

	Ryan VanderMeulen [:RyanVM]
	Comment 77 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2d171d75b746

	Timothy Nikkel (:tnikkel)
	Comment 78 • 8 years ago

nsVideoFrame::OnVisibilityChange nsImageFrame::OnVisibilityChange SVGFEImageFrame::OnVisibilityChange nsSVGImageFrame::OnVisibilityChange all early return before calling OnVisibilityChange on the super class.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 79 • 8 years ago

(In reply to Timothy Nikkel (:tnikkel) from comment #78)
> nsVideoFrame::OnVisibilityChange nsImageFrame::OnVisibilityChange
> SVGFEImageFrame::OnVisibilityChange nsSVGImageFrame::OnVisibilityChange all
> early return before calling OnVisibilityChange on the super class.

That's true, in the case where there's no nsIImageLoadingContent available. I'll fix that in a followup.