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
authorBoris Zbarsky <bzbarsky@mit.edu>
Mon, 10 Mar 2014 17:38:02 -0400
changeset 191078 bb77e0285652bcb3471f426b828279deac8c34ca
parent 191077 45003a7ef6827703fd98b06904768444b7451380
child 191079 dabf18234b5c539da24445a5054f3d30c583bb71
push id474
push userasasaki@mozilla.com
push dateMon, 02 Jun 2014 21:01:02 +0000
treeherdermozilla-release@967f4cf1b31c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssicking
bugs980243
milestone30.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 980243. Make sure that image src sets do something even if the src is being set to the value it already has. r=sicking
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
@@ -430,16 +430,17 @@ skip-if = buildapp == 'b2g' # b2g(Crash,
 skip-if = buildapp == 'b2g' || toolkit == 'android' # b2g(plugins not supported) b2g-debug(plugins not supported) b2g-desktop(plugins not supported)
 [test_iframe_sandbox_popups.html]
 skip-if = buildapp == 'b2g' # b2g(multiple concurrent window.open()s fail on B2G) b2g-debug(multiple concurrent window.open()s fail on B2G) b2g-desktop(Bug 931116, b2g desktop specific, initial triage)
 [test_iframe_sandbox_popups_inheritance.html]
 skip-if = buildapp == 'b2g' # b2g(multiple concurrent window.open()s fail on B2G) b2g-debug(multiple concurrent window.open()s fail on B2G) b2g-desktop(Bug 931116, b2g desktop specific, initial triage)
 [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]
 skip-if = (buildapp == 'b2g' && (toolkit != 'gonk' || debug)) # b2g-debug(Perma-orange on debug emulator) b2g-desktop(Bug 931116, b2g desktop specific, initial triage)
 [test_named_options.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>