Bug 1359833 - Part 3b. Split imgRequestProxy::Clone into Clone and SyncClone. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Wed, 19 Jul 2017 14:15:11 -0400
changeset 369730 e347c123dad6b1694aa967643629905293bab5c5
parent 369729 ca9a48e8a4fbfa5b236c7da73d20d626638f91ba
child 369731 30031cf13eae886774f5a75261b78ac42856c582
push id32202
push userkwierso@gmail.com
push dateThu, 20 Jul 2017 00:30:04 +0000
treeherdermozilla-central@eb1d92b2b6a4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1359833
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1359833 - Part 3b. Split imgRequestProxy::Clone into Clone and SyncClone. r=tnikkel imgRequestProxy::SyncClone preserves the original behaviour of issuing synchronous notifications once cloned. Some uses and tests depend on this behaviour but in an ideal world, it would not be required. imgRequestProxy::Clone is intended to be the replacement going forward, which issues asynchronous notifications once cloned.
image/imgRequestProxy.cpp
image/imgRequestProxy.h
--- a/image/imgRequestProxy.cpp
+++ b/image/imgRequestProxy.cpp
@@ -683,86 +683,96 @@ imgRequestProxy::GetMimeType(char** aMim
     return NS_ERROR_FAILURE;
   }
 
   *aMimeType = NS_strdup(type);
 
   return NS_OK;
 }
 
-static imgRequestProxy* NewProxy(imgRequestProxy* /*aThis*/)
+imgRequestProxy* imgRequestProxy::NewClonedProxy()
 {
   return new imgRequestProxy();
 }
 
-imgRequestProxy* NewStaticProxy(imgRequestProxy* aThis)
-{
-  nsCOMPtr<nsIPrincipal> currentPrincipal;
-  aThis->GetImagePrincipal(getter_AddRefs(currentPrincipal));
-  RefPtr<mozilla::image::Image> image = aThis->GetImage();
-  return new imgRequestProxyStatic(image, currentPrincipal);
-}
-
 NS_IMETHODIMP
 imgRequestProxy::Clone(imgINotificationObserver* aObserver,
                        imgIRequest** aClone)
 {
   nsresult result;
   imgRequestProxy* proxy;
-  result = Clone(aObserver, nullptr, &proxy);
+  result = PerformClone(aObserver, nullptr, /* aSyncNotify */ true, &proxy);
   *aClone = proxy;
   return result;
 }
 
+nsresult imgRequestProxy::SyncClone(imgINotificationObserver* aObserver,
+                                    nsIDocument* aLoadingDocument,
+                                    imgRequestProxy** aClone)
+{
+  return PerformClone(aObserver, aLoadingDocument,
+                      /* aSyncNotify */ true, aClone);
+}
+
 nsresult imgRequestProxy::Clone(imgINotificationObserver* aObserver,
                                 nsIDocument* aLoadingDocument,
                                 imgRequestProxy** aClone)
 {
-  return PerformClone(aObserver, aLoadingDocument, NewProxy, aClone);
+  return PerformClone(aObserver, aLoadingDocument,
+                      /* aSyncNotify */ false, aClone);
 }
 
 nsresult
 imgRequestProxy::PerformClone(imgINotificationObserver* aObserver,
                               nsIDocument* aLoadingDocument,
-                              imgRequestProxy* (aAllocFn)(imgRequestProxy*),
+                              bool aSyncNotify,
                               imgRequestProxy** aClone)
 {
   NS_PRECONDITION(aClone, "Null out param");
 
   LOG_SCOPE(gImgLog, "imgRequestProxy::Clone");
 
   *aClone = nullptr;
-  RefPtr<imgRequestProxy> clone = aAllocFn(this);
+  RefPtr<imgRequestProxy> clone = NewClonedProxy();
 
   // It is important to call |SetLoadFlags()| before calling |Init()| because
   // |Init()| adds the request to the loadgroup.
   // When a request is added to a loadgroup, its load flags are merged
   // with the load flags of the loadgroup.
   // XXXldb That's not true anymore.  Stuff from imgLoader adds the
   // request to the loadgroup.
   clone->SetLoadFlags(mLoadFlags);
   nsresult rv = clone->Init(mBehaviour->GetOwner(), mLoadGroup,
                             aLoadingDocument, mURI, aObserver);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  if (GetOwner() && GetOwner()->GetValidator()) {
-    clone->SetNotificationsDeferred(true);
-    GetOwner()->GetValidator()->AddProxy(clone);
-  }
-
   // Assign to *aClone before calling Notify so that if the caller expects to
   // only be notified for requests it's already holding pointers to it won't be
   // surprised.
   NS_ADDREF(*aClone = clone);
 
-  // This is wrong!!! We need to notify asynchronously, but there's code that
-  // assumes that we don't. This will be fixed in bug 580466.
-  clone->SyncNotifyListener();
+  if (GetOwner() && GetOwner()->GetValidator()) {
+    // Note that if we have a validator, we don't want to issue notifications at
+    // here because we want to defer until that completes.
+    clone->SetNotificationsDeferred(true);
+    GetOwner()->GetValidator()->AddProxy(clone);
+  } else if (aSyncNotify) {
+    // This is wrong!!! We need to notify asynchronously, but there's code that
+    // assumes that we don't. This will be fixed in bug 580466. Note that if we
+    // have a validator, we won't issue notifications anyways because they are
+    // deferred, so there is no point in requesting.
+    clone->SyncNotifyListener();
+  } else {
+    // Without a validator, we can request asynchronous notifications
+    // immediately. If there was a validator, this would override the deferral
+    // and that would be incorrect.
+    clone->NotifyListener();
+  }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 imgRequestProxy::GetImagePrincipal(nsIPrincipal** aPrincipal)
 {
   if (!GetOwner()) {
@@ -1233,15 +1243,15 @@ imgRequestProxyStatic::GetImagePrincipal
     return NS_ERROR_FAILURE;
   }
 
   NS_ADDREF(*aPrincipal = mPrincipal);
 
   return NS_OK;
 }
 
-nsresult
-imgRequestProxyStatic::Clone(imgINotificationObserver* aObserver,
-                             nsIDocument* aLoadingDocument,
-                             imgRequestProxy** aClone)
+imgRequestProxy* imgRequestProxyStatic::NewClonedProxy()
 {
-  return PerformClone(aObserver, aLoadingDocument, NewStaticProxy, aClone);
+  nsCOMPtr<nsIPrincipal> currentPrincipal;
+  GetImagePrincipal(getter_AddRefs(currentPrincipal));
+  RefPtr<mozilla::image::Image> image = GetImage();
+  return new imgRequestProxyStatic(image, currentPrincipal);
 }
--- a/image/imgRequestProxy.h
+++ b/image/imgRequestProxy.h
@@ -128,19 +128,22 @@ public:
   already_AddRefed<nsIEventTarget> GetEventTarget() const override;
 
   // Removes all animation consumers that were created with
   // IncrementAnimationConsumers. This is necessary since we need
   // to do it before the proxy itself is destroyed. See
   // imgRequest::RemoveProxy
   void ClearAnimationConsumers();
 
-  virtual nsresult Clone(imgINotificationObserver* aObserver,
-                         nsIDocument* aLoadingDocument,
-                         imgRequestProxy** aClone);
+  nsresult SyncClone(imgINotificationObserver* aObserver,
+                     nsIDocument* aLoadingDocument,
+                     imgRequestProxy** aClone);
+  nsresult Clone(imgINotificationObserver* aObserver,
+                 nsIDocument* aLoadingDocument,
+                 imgRequestProxy** aClone);
   nsresult GetStaticRequest(nsIDocument* aLoadingDocument,
                             imgRequestProxy** aReturn);
 
   nsresult GetURI(ImageURL** aURI);
 
 protected:
   friend class mozilla::image::ProgressTracker;
   friend class imgStatusNotifyRunnable;
@@ -190,28 +193,29 @@ protected:
   }
 
   already_AddRefed<Image> GetImage() const;
   bool HasImage() const;
   imgRequest* GetOwner() const;
 
   nsresult PerformClone(imgINotificationObserver* aObserver,
                         nsIDocument* aLoadingDocument,
-                        imgRequestProxy* (aAllocFn)(imgRequestProxy*),
+                        bool aSyncNotify,
                         imgRequestProxy** aClone);
 
+  virtual imgRequestProxy* NewClonedProxy();
+
 public:
   NS_FORWARD_SAFE_NSITIMEDCHANNEL(TimedChannel())
 
 protected:
   mozilla::UniquePtr<ProxyBehaviour> mBehaviour;
 
 private:
   friend class imgCacheValidator;
-  friend imgRequestProxy* NewStaticProxy(imgRequestProxy* aThis);
 
   void AddToOwner(nsIDocument* aLoadingDocument);
 
   void Dispatch(already_AddRefed<nsIRunnable> aEvent);
 
   // The URI of our request.
   RefPtr<ImageURL> mURI;
 
@@ -244,23 +248,17 @@ private:
 class imgRequestProxyStatic : public imgRequestProxy
 {
 
 public:
   imgRequestProxyStatic(Image* aImage, nsIPrincipal* aPrincipal);
 
   NS_IMETHOD GetImagePrincipal(nsIPrincipal** aPrincipal) override;
 
-  using imgRequestProxy::Clone;
-
-  virtual nsresult Clone(imgINotificationObserver* aObserver,
-                         nsIDocument* aLoadingDocument,
-                         imgRequestProxy** aClone) override;
-
 protected:
-  friend imgRequestProxy* NewStaticProxy(imgRequestProxy*);
+  imgRequestProxy* NewClonedProxy() override;
 
   // Our principal. We have to cache it, rather than accessing the underlying
   // request on-demand, because static proxies don't have an underlying request.
   nsCOMPtr<nsIPrincipal> mPrincipal;
 };
 
 #endif // mozilla_image_imgRequestProxy_h