Bug 1346501. Don't mark every image as visible when a frame is created for it. r=mats

This is a bug from https://hg.mozilla.org/mozilla-central/rev/2d171d75b746 (bug 1157546). It took a shortcut in trying to get around one of the downsides of tracking visibility on frames instead of content nodes.

We cannot get our primary frame during FrameCreate calls because FrameCreate is called during the frame's Init() function, which happens before the primary frame pointer is set.

So when TrackImage is called from FrameCreate |frame| will be null but mFrameCreateCalled will be true. So we won't hit the early return that tries to detect nonvisible images.

The comment being removed is just wrong. We can obtain a frame for <feImage> just as well as any other image type.

The thing that is different about <feImage> is that it calls IncApproximateVisibleCount() followed by FrameCreated() in the frame's Init() function. This means that the frame is marked visible at the time of the FrameCreated, and there will be no further calls to TrackImage (because there are no further changes). So the FrameCreated call is the last chance to mark this image visible. The regressing changeset tries to get around this by just considering the image visible whenever we know a frame exists (because of mFrameCreateCalled) but can't access it. This ends up affecting all types of images, not just <feImage>.

The above paragraph is also true for SVG <image> that are non-display.

dom/base/nsImageLoadingContent.cpp

@@ -484,8 +484,8 @@ nsImageLoadingContent::FrameCreated(nsIFrame* aFrame)
 
   mFrameCreateCalled = true;
 
-  TrackImage(mCurrentRequest);
-  TrackImage(mPendingRequest);
+  TrackImage(mCurrentRequest, aFrame);
+  TrackImage(mPendingRequest, aFrame);
 
   // We need to make sure that our image request is registered, if it should
   // be registered.
@@ -1464,7 +1464,8 @@ nsImageLoadingContent::OnVisibilityChange(Visibility aNewVisibility,
 }
 
 void
-nsImageLoadingContent::TrackImage(imgIRequest* aImage)
+nsImageLoadingContent::TrackImage(imgIRequest* aImage,
+                                  nsIFrame* aFrame /*= nullptr */)
 {
   if (!aImage)
     return;
@@ -1477,13 +1478,21 @@ nsImageLoadingContent::TrackImage(imgIRequest* aImage)
     return;
   }
 
-  // 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.
-  nsIFrame* frame = GetOurPrimaryFrame();
-  if ((frame && frame->GetVisibility() == Visibility::APPROXIMATELY_NONVISIBLE) ||
-      (!frame && !mFrameCreateCalled)) {
+  if (!aFrame) {
+    aFrame = GetOurPrimaryFrame();
+  }
+
+  /* This line is deceptively simple. It hides a lot of subtlety. Before we
+   * create an nsImageFrame we call nsImageFrame::ShouldCreateImageFrameFor
+   * to determine if we should create an nsImageFrame or create a frame based
+   * on the display of the element (ie inline, block, etc). Inline, block, etc
+   * frames don't register for visibility tracking so they will return UNTRACKED
+   * from GetVisibility(). So this line is choosing to mark such images as
+   * visible. Once the image loads we will get an nsImageFrame and the proper
+   * visibility. This is a pitfall of tracking the visibility on the frames
+   * instead of the content node.
+   */
+  if (!aFrame || aFrame->GetVisibility() == Visibility::APPROXIMATELY_NONVISIBLE) {
     return;
   }
 

dom/base/nsImageLoadingContent.h

@@ -368,6 +368,11 @@ protected:
    *
    * No-op if aImage is null.
    *
+   * @param aFrame If called from FrameCreated the frame passed to FrameCreated.
+   *               This is our frame, but at the time of the FrameCreated call
+   *               our primary frame pointer hasn't been set yet, so this is
+   *               only way to get our frame.
+   *
    * @param aNonvisibleAction A requested action if the frame has become
    *                          nonvisible. If Nothing(), no action is
    *                          requested. If DISCARD_IMAGES is specified, the
@@ -375,7 +380,7 @@ protected:
    *                          associated with to discard their surfaces if
    *                          possible.
    */
-  void TrackImage(imgIRequest* aImage);
+  void TrackImage(imgIRequest* aImage, nsIFrame* aFrame = nullptr);
   void UntrackImage(imgIRequest* aImage,
                     const Maybe<OnNonvisible>& aNonvisibleAction = Nothing());
 

layout/svg/SVGFEImageFrame.cpp

@@ -107,6 +107,12 @@ SVGFEImageFrame::Init(nsIContent*       aContent,
   nsFrame::Init(aContent, aParent, aPrevInFlow);
 
   // We assume that feImage's are always visible.
+  // This call must happen before the FrameCreated. This is because the
+  // primary frame pointer on our content node isn't set until after this
+  // function ends, so there is no way for the resulting OnVisibilityChange
+  // notification to get a frame. FrameCreated has a workaround for this in
+  // that it passes our frame around so it can be accessed. OnVisibilityChange
+  // doesn't have that workaround.
   IncApproximateVisibleCount();
 
   nsCOMPtr<nsIImageLoadingContent> imageLoader =

layout/svg/nsSVGImageFrame.cpp

@@ -159,6 +159,12 @@ nsSVGImageFrame::Init(nsIContent*       aContent,
   if (GetStateBits() & NS_FRAME_IS_NONDISPLAY) {
     // Non-display frames are likely to be patterns, masks or the like.
     // Treat them as always visible.
+    // This call must happen before the FrameCreated. This is because the
+    // primary frame pointer on our content node isn't set until after this
+    // function ends, so there is no way for the resulting OnVisibilityChange
+    // notification to get a frame. FrameCreated has a workaround for this in
+    // that it passes our frame around so it can be accessed. OnVisibilityChange
+    // doesn't have that workaround.
     IncApproximateVisibleCount();
   }