Bug 980243 - Make sure that image src sets do something even if the src is being set to the value it already has. r=sicking, a=sledru
authorBoris Zbarsky <bzbarsky@mit.edu>
Mon, 10 Mar 2014 17:38:02 -0400
changeset 184296 b77120159790997187ff0072666ffdc2cb94b90e
parent 184295 634396e2442aa1d5cddad93d5a5b5c5f29d40b8e
child 184297 ffbf8e6aa0f229150384de2d6f23f4819284da12
push id462
push userraliiev@mozilla.com
push dateTue, 22 Apr 2014 00:22:30 +0000
treeherdermozilla-release@ac5db8c74ac0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssicking, sledru
bugs980243
milestone29.0a2
Bug 980243 - Make sure that image src sets do something even if the src is being set to the value it already has. r=sicking, a=sledru
content/html/content/src/HTMLImageElement.cpp
content/html/content/src/HTMLImageElement.h
content/html/content/test/mochitest.ini
content/html/content/test/test_imageSrcSet.html
--- a/content/html/content/src/HTMLImageElement.cpp
+++ b/content/html/content/src/HTMLImageElement.cpp
@@ -317,53 +317,29 @@ HTMLImageElement::AfterSetAttr(int32_t a
   }
 
   if (aNameSpaceID == kNameSpaceID_None &&
       aName == nsGkAtoms::src &&
       !aValue) {
     CancelImageRequests(aNotify);
   }
 
-  // If we plan to call LoadImage, we want to do it first so that the image load
-  // kicks off. But if aNotify is false, we are coming from the parser or some
-  // such place; we'll get bound after all the attributes have been set, so
-  // we'll do the image load from BindToTree. Skip the LoadImage call in that case.
+  // If aNotify is false, we are coming from the parser or some such place;
+  // we'll get bound after all the attributes have been set, so we'll do the
+  // image load from BindToTree. Skip the LoadImage call in that case.
   if (aNotify &&
       aNameSpaceID == kNameSpaceID_None &&
       aName == nsGkAtoms::crossorigin) {
     // We want aForce == true in this LoadImage call, because we want to force
     // a new load of the image with the new cross origin policy.
     nsAutoString uri;
     GetAttr(kNameSpaceID_None, nsGkAtoms::src, uri);
     LoadImage(uri, true, aNotify);
   }
 
-  if (aNotify &&
-      aNameSpaceID == kNameSpaceID_None &&
-      aName == nsGkAtoms::src &&
-      aValue) {
-
-    // Prevent setting image.src by exiting early
-    if (nsContentUtils::IsImageSrcSetDisabled()) {
-      return NS_OK;
-    }
-
-    // A hack to get animations to reset. See bug 594771.
-    mNewRequestsWillNeedAnimationReset = true;
-
-    // Force image loading here, so that we'll try to load the image from
-    // network if it's set to be not cacheable...  If we change things so that
-    // the state gets in Element's attr-setting happen around this
-    // LoadImage call, we could start passing false instead of aNotify
-    // here.
-    LoadImage(aValue->GetStringValue(), true, aNotify);
-
-    mNewRequestsWillNeedAnimationReset = false;
-  }
-
   return nsGenericHTMLElement::AfterSetAttr(aNameSpaceID, aName,
                                             aValue, aNotify);
 }
 
 
 nsresult
 HTMLImageElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)
 {
@@ -421,28 +397,51 @@ HTMLImageElement::IsHTMLFocusable(bool a
   return false;
 }
 
 nsresult
 HTMLImageElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                           nsIAtom* aPrefix, const nsAString& aValue,
                           bool aNotify)
 {
+  // We need to force our image to reload.  This must be done here, not in
+  // AfterSetAttr or BeforeSetAttr, because we want to do it even if the attr is
+  // being set to its existing value, which is normally optimized away as a
+  // no-op.
+  //
+  // If aNotify is false, we are coming from the parser or some such place;
+  // we'll get bound after all the attributes have been set, so we'll do the
+  // image load from BindToTree. Skip the LoadImage call in that case.
+  if (aNotify &&
+      aNameSpaceID == kNameSpaceID_None &&
+      aName == nsGkAtoms::src) {
+
+    // Prevent setting image.src by exiting early
+    if (nsContentUtils::IsImageSrcSetDisabled()) {
+      return NS_OK;
+    }
+
+    // A hack to get animations to reset. See bug 594771.
+    mNewRequestsWillNeedAnimationReset = true;
+
+    // Force image loading here, so that we'll try to load the image from
+    // network if it's set to be not cacheable...  If we change things so that
+    // the state gets in Element's attr-setting happen around this
+    // LoadImage call, we could start passing false instead of aNotify
+    // here.
+    LoadImage(aValue, true, aNotify);
+
+    mNewRequestsWillNeedAnimationReset = false;
+  }
+
   return nsGenericHTMLElement::SetAttr(aNameSpaceID, aName, aPrefix, aValue,
                                        aNotify);
 }
 
 nsresult
-HTMLImageElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                            bool aNotify)
-{
-  return nsGenericHTMLElement::UnsetAttr(aNameSpaceID, aAttribute, aNotify);
-}
-
-nsresult
 HTMLImageElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
                              nsIContent* aBindingParent,
                              bool aCompileEventHandlers)
 {
   nsresult rv = nsGenericHTMLElement::BindToTree(aDocument, aParent,
                                                  aBindingParent,
                                                  aCompileEventHandlers);
   NS_ENSURE_SUCCESS(rv, rv);
--- a/content/html/content/src/HTMLImageElement.h
+++ b/content/html/content/src/HTMLImageElement.h
@@ -60,18 +60,16 @@ public:
   nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                    const nsAString& aValue, bool aNotify)
   {
     return SetAttr(aNameSpaceID, aName, nullptr, aValue, aNotify);
   }
   virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                            nsIAtom* aPrefix, const nsAString& aValue,
                            bool aNotify) MOZ_OVERRIDE;
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify) MOZ_OVERRIDE;
 
   virtual nsresult BindToTree(nsIDocument* aDocument, nsIContent* aParent,
                               nsIContent* aBindingParent,
                               bool aCompileEventHandlers) MOZ_OVERRIDE;
   virtual void UnbindFromTree(bool aDeep, bool aNullParent) MOZ_OVERRIDE;
 
   virtual nsEventStates IntrinsicState() const MOZ_OVERRIDE;
   virtual nsresult Clone(nsINodeInfo *aNodeInfo, nsINode **aResult) const MOZ_OVERRIDE;
--- a/content/html/content/test/mochitest.ini
+++ b/content/html/content/test/mochitest.ini
@@ -398,16 +398,17 @@ support-files =
 [test_iframe_sandbox_navigation.html]
 [test_iframe_sandbox_navigation2.html]
 [test_iframe_sandbox_plugins.html]
 [test_iframe_sandbox_popups.html]
 [test_iframe_sandbox_popups_inheritance.html]
 [test_iframe_sandbox_same_origin.html]
 [test_iframe_sandbox_workers.html]
 [test_img_attributes_reflection.html]
+[test_imageSrcSet.html]
 [test_li_attributes_reflection.html]
 [test_link_attributes_reflection.html]
 [test_map_attributes_reflection.html]
 [test_meta_attributes_reflection.html]
 [test_mod_attributes_reflection.html]
 [test_mozaudiochannel.html]
 [test_named_options.html]
 [test_nested_invalid_fieldsets.html]
new file mode 100644
--- /dev/null
+++ b/content/html/content/test/test_imageSrcSet.html
@@ -0,0 +1,38 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=980243
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 980243</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+  <script type="application/javascript">
+
+  /** Test for Bug 980243 **/
+  SimpleTest.waitForExplicitFinish();
+
+  addLoadEvent(function() {
+    var img = document.querySelector("img");
+    img.onload = function() {
+      ok(true, "Reached here");
+      SimpleTest.finish();
+    }
+    // If ths spec ever changes to treat .src sets differently from
+    // setAttribute("src"), we'll need some sort of canonicalization step
+    // earlier to make the attr value an absolute URI.
+    img.setAttribute("src", img.getAttribute("src"));
+  });
+  </script>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=980243">Mozilla Bug 980243</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  <img src="file_formSubmission_img.jpg">
+</div>
+<pre id="test">
+</pre>
+</body>
+</html>