Bug 1512297 - Do not need to clear image broken state in BindToTree call; r=smaug
authorEdgar Chen <echen@mozilla.com>
Thu, 20 Dec 2018 13:59:34 +0100
changeset 510046 d3dbff04a1d4a04c513587daa835bfc42e12ddf8
parent 510045 e85e41f849712c4464a6a4586fa6c2bd6af273fc
child 510047 6fdbf24a1342bb7462b43b8929aefd6785c9ca8e
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1512297, 491063
milestone66.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 1512297 - Do not need to clear image broken state in BindToTree call; r=smaug ImageLoadingContent will take care of updating element to correct state, we don't need to do this. Especially for HTMLImageElement, because it may not reload the image after BindToTree ( e.g. the selected source isn't changed), clearing broken state may put element into incorrect state. The original code seems from https://bugzilla.mozilla.org/show_bug.cgi?id=491063#c32 for a performance reason, however I test the http://mozilla.pettay.fi/moztests/1x1image.html again on recent codebase, I don't see difference with/without applying this patch.
dom/base/nsImageLoadingContent.h
dom/html/HTMLImageElement.cpp
dom/html/HTMLInputElement.cpp
dom/html/reftests/bug1512297-ref.html
dom/html/reftests/bug1512297.html
dom/html/reftests/reftest.list
dom/svg/SVGFEImageElement.cpp
dom/svg/SVGImageElement.cpp
--- a/dom/base/nsImageLoadingContent.h
+++ b/dom/base/nsImageLoadingContent.h
@@ -195,18 +195,16 @@ class nsImageLoadingContent : public nsI
    * DestroyImageLoadingContent from their destructor, or earlier.  It
    * does things that cannot be done in ~nsImageLoadingContent because
    * they rely on being able to QueryInterface to other derived classes,
    * which cannot happen once the derived class destructor has started
    * calling the base class destructors.
    */
   void DestroyImageLoadingContent();
 
-  void ClearBrokenState() { mBroken = false; }
-
   /**
    * Returns the CORS mode that will be used for all future image loads. The
    * default implementation returns CORS_NONE unconditionally.
    */
   virtual mozilla::CORSMode GetCORSMode();
 
   virtual mozilla::net::ReferrerPolicy GetImageReferrerPolicy();
 
--- a/dom/html/HTMLImageElement.cpp
+++ b/dom/html/HTMLImageElement.cpp
@@ -520,21 +520,16 @@ nsresult HTMLImageElement::BindToTree(Do
     // This isn't necessary for responsive mode, since creating the
     // image load task is asynchronous we don't need to take special
     // care to avoid doing so when being filled by the parser.
 
     // Mark channel as urgent-start before load image if the image load is
     // initaiated by a user interaction.
     mUseUrgentStartForChannel = EventStateManager::IsHandlingUserInput();
 
-    // FIXME: Bug 660963 it would be nice if we could just have
-    // ClearBrokenState update our state and do it fast...
-    ClearBrokenState();
-    RemoveStatesSilently(NS_EVENT_STATE_BROKEN);
-
     // We still act synchronously for the non-responsive case (Bug
     // 1076583), but still need to delay if it is unsafe to run
     // script.
 
     // If loading is temporarily disabled, don't even launch MaybeLoadImage.
     // Otherwise MaybeLoadImage may run later when someone has reenabled
     // loading.
     if (LoadingEnabled() && OwnerDoc()->ShouldLoadImages()) {
--- a/dom/html/HTMLInputElement.cpp
+++ b/dom/html/HTMLInputElement.cpp
@@ -4334,20 +4334,16 @@ nsresult HTMLInputElement::BindToTree(Do
   if (mType == NS_FORM_INPUT_IMAGE) {
     // Our base URI may have changed; claim that our URI changed, and the
     // nsImageLoadingContent will decide whether a new image load is warranted.
     if (HasAttr(kNameSpaceID_None, nsGkAtoms::src)) {
       // Mark channel as urgent-start before load image if the image load is
       // initaiated by a user interaction.
       mUseUrgentStartForChannel = EventStateManager::IsHandlingUserInput();
 
-      // FIXME: Bug 660963 it would be nice if we could just have
-      // ClearBrokenState update our state and do it fast...
-      ClearBrokenState();
-      RemoveStatesSilently(NS_EVENT_STATE_BROKEN);
       nsContentUtils::AddScriptRunner(
           NewRunnableMethod("dom::HTMLInputElement::MaybeLoadImage", this,
                             &HTMLInputElement::MaybeLoadImage));
     }
   }
 
   // Add radio to document if we don't have a form already (if we do it's
   // already been added into that group)
new file mode 100644
--- /dev/null
+++ b/dom/html/reftests/bug1512297-ref.html
@@ -0,0 +1,7 @@
+<!DOCTYPE html>
+<html>
+<head></head>
+<body>
+<div><img src="" alt="ALT"></div>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/dom/html/reftests/bug1512297.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<head class="reftest-wait"></head>
+<body>
+<div><img src="" alt="ALT"></div>
+<script>
+var img = document.querySelector('img');
+img.remove();
+
+var div = document.querySelector('div');
+div.appendChild(img);
+</script>
+</body>
+</html>
--- a/dom/html/reftests/reftest.list
+++ b/dom/html/reftests/reftest.list
@@ -59,14 +59,16 @@ fuzzy(0-3,0-640) fuzzy-if(skiaContent,0-
 
 # Test imageset is using permissions.default.image
 pref(permissions.default.image,1) HTTP == bug1196784-with-srcset.html bug1196784-no-srcset.html
 pref(permissions.default.image,2) HTTP == bug1196784-with-srcset.html bug1196784-no-srcset.html
 
 # Test video with rotation information can be rotated.
 == bug1228601-video-rotation-90.html bug1228601-video-rotated-ref.html
 
+== bug1512297.html bug1512297-ref.html
+
 # Test that dynamically setting body margin attributes updates style appropriately
 == body-topmargin-dynamic.html body-topmargin-ref.html
 
 # Test that dynamically removing a nonmargin mapped attribute does not
 # destroy margins inherited from the frame.
 == body-frame-margin-remove-other-pres-hint.html body-frame-margin-remove-other-pres-hint-ref.html
--- a/dom/svg/SVGFEImageElement.cpp
+++ b/dom/svg/SVGFEImageElement.cpp
@@ -137,20 +137,16 @@ nsresult SVGFEImageElement::BindToTree(D
   nsresult rv =
       SVGFEImageElementBase::BindToTree(aDocument, aParent, aBindingParent);
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsImageLoadingContent::BindToTree(aDocument, aParent, aBindingParent);
 
   if (mStringAttributes[HREF].IsExplicitlySet() ||
       mStringAttributes[XLINK_HREF].IsExplicitlySet()) {
-    // FIXME: Bug 660963 it would be nice if we could just have
-    // ClearBrokenState update our state and do it fast...
-    ClearBrokenState();
-    RemoveStatesSilently(NS_EVENT_STATE_BROKEN);
     nsContentUtils::AddScriptRunner(
         NewRunnableMethod("dom::SVGFEImageElement::MaybeLoadSVGImage", this,
                           &SVGFEImageElement::MaybeLoadSVGImage));
   }
 
   return rv;
 }
 
--- a/dom/svg/SVGImageElement.cpp
+++ b/dom/svg/SVGImageElement.cpp
@@ -185,20 +185,16 @@ nsresult SVGImageElement::BindToTree(Doc
   nsresult rv =
       SVGImageElementBase::BindToTree(aDocument, aParent, aBindingParent);
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsImageLoadingContent::BindToTree(aDocument, aParent, aBindingParent);
 
   if (mStringAttributes[HREF].IsExplicitlySet() ||
       mStringAttributes[XLINK_HREF].IsExplicitlySet()) {
-    // FIXME: Bug 660963 it would be nice if we could just have
-    // ClearBrokenState update our state and do it fast...
-    ClearBrokenState();
-    RemoveStatesSilently(NS_EVENT_STATE_BROKEN);
     nsContentUtils::AddScriptRunner(
         NewRunnableMethod("dom::SVGImageElement::MaybeLoadSVGImage", this,
                           &SVGImageElement::MaybeLoadSVGImage));
   }
 
   return rv;
 }