Bug 1365092 - Moves side effects of HTMLImageElement's SetAttr function to the corresponding BeforeSetAttr and AfterSetAttr functions draft
authorKirk Steuber <ksteuber@mozilla.com>
Wed, 24 May 2017 10:44:49 -0700
changeset 584104 a5eafba1a1cb895c7e192c9f9c5de76bac89b2e0
parent 583822 e0dcaef2c04e28c532e2204851b1f0ba1b7a3583
child 585245 599ecf8e6a1d47839f5c3bead3ff00b46fbd5f58
child 585504 9359845cb096f7e7558f488cea9b0876bf19e976
child 586705 729eb9ed0a63bc9f4bea912ca4e0f8cc3acddc5e
child 587153 3c286ceebb23350001c0c60ab22a6039a80e7749
push id60631
push userksteuber@mozilla.com
push dateWed, 24 May 2017 22:12:49 +0000
bugs1365092
milestone55.0a1
Bug 1365092 - Moves side effects of HTMLImageElement's SetAttr function to the corresponding BeforeSetAttr and AfterSetAttr functions This is necessary to facilitate the transition to cloning attributes instead of reparsing them. HTMLImageElement's side effects proved to be a bit trickier than those of many other classes because HTMLImageElement::SetAttr intentionally forces an image reload, even if the attribute value has not been changed. Element::SetAttr, on the other hand, usually ignores attribute changes that do not change the attribute value, exiting before BeforeSetAttr is even called. In order to preserve this behavior, another virtual function |OnAttrSetButNotChanged| was added to the Element class. This function will be called in the case that Element::SetAttr exits early, allowing a forced reload to take place at that time. MozReview-Commit-ID: 4CrH30bo5GT
dom/base/Element.cpp
dom/base/Element.h
dom/html/HTMLImageElement.cpp
dom/html/HTMLImageElement.h
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -2387,17 +2387,17 @@ Element::SetAttr(int32_t aNamespaceID, n
   // changing, so just init our nsAttrValueOrString with aValue for the
   // OnlyNotifySameValueSet call.
   nsAttrValueOrString value(aValue);
   nsAttrValue oldValue;
   bool oldValueSet;
 
   if (OnlyNotifySameValueSet(aNamespaceID, aName, aPrefix, value, aNotify,
                              oldValue, &modType, &hasListeners, &oldValueSet)) {
-    return NS_OK;
+    return OnAttrSetButNotChanged(aNamespaceID, aName, value, aNotify);
   }
 
   nsAttrValue attrValue;
   nsAttrValue* preparsedAttrValue;
   if (aNamespaceID == kNameSpaceID_None && aName == nsGkAtoms::_class) {
     attrValue.ParseAtomArray(aValue);
     value.ResetToAttrValue(attrValue);
     preparsedAttrValue = &attrValue;
@@ -2451,17 +2451,17 @@ Element::SetParsedAttr(int32_t aNamespac
   uint8_t modType;
   bool hasListeners;
   nsAttrValueOrString value(aParsedValue);
   nsAttrValue oldValue;
   bool oldValueSet;
 
   if (OnlyNotifySameValueSet(aNamespaceID, aName, aPrefix, value, aNotify,
                              oldValue, &modType, &hasListeners, &oldValueSet)) {
-    return NS_OK;
+    return OnAttrSetButNotChanged(aNamespaceID, aName, value, aNotify);
   }
 
   if (aNotify) {
     nsNodeUtils::AttributeWillChange(this, aNamespaceID, aName, modType,
                                      &aParsedValue);
   }
 
   nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
--- a/dom/base/Element.h
+++ b/dom/base/Element.h
@@ -1512,16 +1512,36 @@ protected:
    * @param aNamespaceID the namespace of the attr being set
    * @param aName the localname of the attribute being set
    * @param aValue the new id value. Will be null if the id is being unset.
    */
   nsresult PostIdMaybeChange(int32_t aNamespaceID, nsIAtom* aName,
                              const nsAttrValue* aValue);
 
   /**
+   * Usually, setting an attribute to the value that it already has results in
+   * no action. However, in some cases, setting an attribute to its current
+   * value should have the effect of, for example, forcing a reload of
+   * network data. To address that, this function will be called in this
+   * situation to allow the handling of such a case.
+   *
+   * @param aNamespaceID the namespace of the attr being set
+   * @param aName the localname of the attribute being set
+   * @param aValue the value it's being set to represented as either a string or
+   *        a parsed nsAttrValue.
+   * @param aNotify Whether we plan to notify document observers.
+   */
+  virtual nsresult OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                          const nsAttrValueOrString& aValue,
+                                          bool aNotify)
+  {
+    return NS_OK;
+  }
+
+  /**
    * Hook to allow subclasses to produce a different EventListenerManager if
    * needed for attachment of attribute-defined handlers
    */
   virtual EventListenerManager*
     GetEventListenerManagerForAttr(nsIAtom* aAttrName, bool* aDefer);
 
   /**
    * Internal hook for converting an attribute name-string to nsAttrName in
--- a/dom/html/HTMLImageElement.cpp
+++ b/dom/html/HTMLImageElement.cpp
@@ -113,16 +113,17 @@ private:
   // True if we want to set nsIClassOfService::UrgentStart to the channel to
   // get the response ASAP for better user responsiveness.
   bool mUseUrgentStartForChannel;
 };
 
 HTMLImageElement::HTMLImageElement(already_AddRefed<mozilla::dom::NodeInfo>& aNodeInfo)
   : nsGenericHTMLElement(aNodeInfo)
   , mForm(nullptr)
+  , mForceReload(false)
   , mInDocResponsiveContent(false)
   , mCurrentDensity(1.0)
 {
   // We start out broken
   AddStatesSilently(NS_EVENT_STATE_BROKEN);
 }
 
 HTMLImageElement::~HTMLImageElement()
@@ -367,22 +368,25 @@ HTMLImageElement::IsAttributeMapped(cons
 
 
 nsMapRuleToAttributesFunc
 HTMLImageElement::GetAttributeMappingFunction() const
 {
   return &MapAttributesIntoRule;
 }
 
-
 nsresult
 HTMLImageElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                 const nsAttrValueOrString* aValue,
                                 bool aNotify)
 {
+  if (aValue) {
+    nsresult rv = BeforeMaybeChangeAttr(aNameSpaceID, aName, *aValue, aNotify);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
 
   if (aNameSpaceID == kNameSpaceID_None && mForm &&
       (aName == nsGkAtoms::name || aName == nsGkAtoms::id)) {
     // remove the image from the hashtable as needed
     nsAutoString tmp;
     GetAttr(kNameSpaceID_None, aName, tmp);
 
     if (!tmp.IsEmpty()) {
@@ -394,16 +398,21 @@ HTMLImageElement::BeforeSetAttr(int32_t 
                                              aValue, aNotify);
 }
 
 nsresult
 HTMLImageElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                const nsAttrValue* aValue,
                                const nsAttrValue* aOldValue, bool aNotify)
 {
+  if (aValue) {
+    nsresult rv = AfterMaybeChangeAttr(aNameSpaceID, aName, aNotify);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
   if (aNameSpaceID == kNameSpaceID_None && mForm &&
       (aName == nsGkAtoms::name || aName == nsGkAtoms::id) &&
       aValue && !aValue->IsEmptyString()) {
     // add the image to the hashtable as needed
     MOZ_ASSERT(aValue->Type() == nsAttrValue::eAtom,
                "Expected atom value for name/id");
     mForm->AddImageElementToTable(this,
       nsDependentAtomString(aValue->GetAtomValue()));
@@ -450,16 +459,134 @@ HTMLImageElement::AfterSetAttr(int32_t a
     PictureSourceSizesChanged(this, attrVal.String(), aNotify);
   }
 
   return nsGenericHTMLElement::AfterSetAttr(aNameSpaceID, aName,
                                             aValue, aOldValue, aNotify);
 }
 
 nsresult
+HTMLImageElement::OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                         const nsAttrValueOrString& aValue,
+                                         bool aNotify)
+{
+  nsresult rv = BeforeMaybeChangeAttr(aNamespaceID, aName, aValue, aNotify);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  rv = AfterMaybeChangeAttr(aNamespaceID, aName, aNotify);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  return nsGenericHTMLElement::OnAttrSetButNotChanged(aNamespaceID, aName,
+                                                      aValue, aNotify);
+}
+
+nsresult
+HTMLImageElement::BeforeMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                        const nsAttrValueOrString& aValue,
+                                        bool aNotify)
+{
+  mForceReload = false;
+  // 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 we are in responsive mode, we drop the forced reload behavior,
+  // but still trigger a image load task for img.src = img.src per
+  // spec.
+  //
+  // Both cases handle unsetting src in AfterSetAttr
+  if (aNamespaceID == kNameSpaceID_None &&
+      aName == nsGkAtoms::src) {
+
+    // Mark channel as urgent-start before load image if the image load is
+    // initaiated by a user interaction.
+    mUseUrgentStartForChannel = EventStateManager::IsHandlingUserInput();
+
+    if (InResponsiveMode()) {
+      if (mResponsiveSelector &&
+          mResponsiveSelector->Content() == this) {
+        mResponsiveSelector->SetDefaultSource(aValue.String());
+      }
+      QueueImageLoadTask(true);
+    } else if (aNotify && OwnerDoc()->IsCurrentActiveDocument()) {
+      // 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
+      // sync image load from BindToTree. Skip the LoadImage call in that case.
+
+      // Note that this sync behavior is partially removed from the spec, bug 1076583
+
+      // 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.String(), true, aNotify, eImageLoadType_Normal);
+
+      mNewRequestsWillNeedAnimationReset = false;
+    }
+  } else if (aNamespaceID == kNameSpaceID_None &&
+             aName == nsGkAtoms::crossorigin &&
+             aNotify) {
+    nsAttrValue attrValue;
+    ParseCORSValue(aValue.String(), attrValue);
+    if (GetCORSMode() != AttrValueToCORSMode(&attrValue)) {
+      // Force a new load of the image with the new cross origin policy.
+      mForceReload = true;
+    }
+  } else if (aName == nsGkAtoms::referrerpolicy &&
+      aNamespaceID == kNameSpaceID_None &&
+      aNotify) {
+    ReferrerPolicy referrerPolicy = AttributeReferrerPolicyFromString(aValue.String());
+    if (!InResponsiveMode() &&
+        referrerPolicy != RP_Unset &&
+        referrerPolicy != GetImageReferrerPolicy()) {
+      // XXX: Bug 1076583 - We still use the older synchronous algorithm
+      // Because referrerPolicy is not treated as relevant mutations, setting
+      // the attribute will neither trigger a reload nor update the referrer
+      // policy of the loading channel (whether it has previously completed or
+      // not). Force a new load of the image with the new referrerpolicy.
+      mForceReload = true;
+    }
+  }
+
+  return NS_OK;
+}
+
+nsresult
+HTMLImageElement::AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                       bool aNotify)
+{
+  // Because we load image synchronously in non-responsive-mode, we need to do
+  // reload after the attribute has been set if the reload is triggerred by
+  // cross origin changing.
+  if (mForceReload) {
+    // Mark channel as urgent-start before load image if the image load is
+    // initaiated by a user interaction.
+    mUseUrgentStartForChannel = EventStateManager::IsHandlingUserInput();
+
+    if (InResponsiveMode()) {
+      // per spec, full selection runs when this changes, even though
+      // it doesn't directly affect the source selection
+      QueueImageLoadTask(true);
+    } else if (OwnerDoc()->IsCurrentActiveDocument()) {
+      // Bug 1076583 - We still use the older synchronous algorithm in
+      // non-responsive mode. Force a new load of the image with the
+      // new cross origin policy
+      ForceReload(aNotify);
+    }
+  }
+
+  return NS_OK;
+}
+
+nsresult
 HTMLImageElement::GetEventTargetParent(EventChainPreVisitor& aVisitor)
 {
   // We handle image element with attribute ismap in its corresponding frame
   // element. Set mMultipleActionsPrevented here to prevent the click event
   // trigger the behaviors in Element::PostHandleEventForLinks
   WidgetMouseEvent* mouseEvent = aVisitor.mEvent->AsMouseEvent();
   if (mouseEvent && mouseEvent->IsLeftClickEvent() && IsMap()) {
     mouseEvent->mFlags.mMultipleActionsPrevented = true;
@@ -502,115 +629,16 @@ HTMLImageElement::IsHTMLFocusable(bool a
     (!aWithMouse || nsFocusManager::sMouseFocusesFormControl) &&
 #endif
     (tabIndex >= 0 || HasAttr(kNameSpaceID_None, nsGkAtoms::tabindex));
 
   return false;
 }
 
 nsresult
-HTMLImageElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
-                          nsIAtom* aPrefix, const nsAString& aValue,
-                          bool aNotify)
-{
-  bool forceReload = false;
-  // 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 we are in responsive mode, we drop the forced reload behavior,
-  // but still trigger a image load task for img.src = img.src per
-  // spec.
-  //
-  // Both cases handle unsetting src in AfterSetAttr
-  if (aNameSpaceID == kNameSpaceID_None &&
-      aName == nsGkAtoms::src) {
-
-    // Mark channel as urgent-start before load image if the image load is
-    // initaiated by a user interaction.
-    mUseUrgentStartForChannel = EventStateManager::IsHandlingUserInput();
-
-    if (InResponsiveMode()) {
-      if (mResponsiveSelector &&
-          mResponsiveSelector->Content() == this) {
-        mResponsiveSelector->SetDefaultSource(aValue);
-      }
-      QueueImageLoadTask(true);
-    } else if (aNotify && OwnerDoc()->IsCurrentActiveDocument()) {
-      // 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
-      // sync image load from BindToTree. Skip the LoadImage call in that case.
-
-      // Note that this sync behavior is partially removed from the spec, bug 1076583
-
-      // 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, eImageLoadType_Normal);
-
-      mNewRequestsWillNeedAnimationReset = false;
-    }
-  } else if (aNameSpaceID == kNameSpaceID_None &&
-             aName == nsGkAtoms::crossorigin &&
-             aNotify) {
-    nsAttrValue attrValue;
-    ParseCORSValue(aValue, attrValue);
-    if (GetCORSMode() != AttrValueToCORSMode(&attrValue)) {
-      // Force a new load of the image with the new cross origin policy.
-      forceReload = true;
-    }
-  } else if (aName == nsGkAtoms::referrerpolicy &&
-      aNameSpaceID == kNameSpaceID_None &&
-      aNotify) {
-    ReferrerPolicy referrerPolicy = AttributeReferrerPolicyFromString(aValue);
-    if (!InResponsiveMode() &&
-        referrerPolicy != RP_Unset &&
-        referrerPolicy != GetImageReferrerPolicy()) {
-      // XXX: Bug 1076583 - We still use the older synchronous algorithm
-      // Because referrerPolicy is not treated as relevant mutations, setting
-      // the attribute will neither trigger a reload nor update the referrer
-      // policy of the loading channel (whether it has previously completed or
-      // not). Force a new load of the image with the new referrerpolicy.
-      forceReload = true;
-    }
-  }
-
-  nsresult rv = nsGenericHTMLElement::SetAttr(aNameSpaceID, aName, aPrefix,
-                                              aValue, aNotify);
-
-  // Because we load image synchronously in non-responsive-mode, we need to do
-  // reload after the attribute has been set if the reload is triggerred by
-  // cross origin changing.
-  if (forceReload) {
-    // Mark channel as urgent-start before load image if the image load is
-    // initaiated by a user interaction.
-    mUseUrgentStartForChannel = EventStateManager::IsHandlingUserInput();
-
-    if (InResponsiveMode()) {
-      // per spec, full selection runs when this changes, even though
-      // it doesn't directly affect the source selection
-      QueueImageLoadTask(true);
-    } else if (OwnerDoc()->IsCurrentActiveDocument()) {
-      // Bug 1076583 - We still use the older synchronous algorithm in
-      // non-responsive mode. Force a new load of the image with the
-      // new cross origin policy
-      ForceReload(aNotify);
-    }
-  }
-
-  return rv;
-}
-
-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/dom/html/HTMLImageElement.h
+++ b/dom/html/HTMLImageElement.h
@@ -70,27 +70,16 @@ public:
   NS_IMETHOD_(bool) IsAttributeMapped(const nsIAtom* aAttribute) const override;
   virtual nsMapRuleToAttributesFunc GetAttributeMappingFunction() const override;
 
   virtual nsresult GetEventTargetParent(
                      EventChainPreVisitor& aVisitor) override;
 
   bool IsHTMLFocusable(bool aWithMouse, bool *aIsFocusable, int32_t *aTabIndex) override;
 
-  // SetAttr override.  C++ is stupid, so have to override both
-  // overloaded methods.
-  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) override;
-
   virtual nsresult BindToTree(nsIDocument* aDocument, nsIContent* aParent,
                               nsIContent* aBindingParent,
                               bool aCompileEventHandlers) override;
   virtual void UnbindFromTree(bool aDeep, bool aNullParent) override;
 
   virtual EventStates IntrinsicState() const override;
   virtual nsresult Clone(mozilla::dom::NodeInfo *aNodeInfo, nsINode **aResult,
                          bool aPreallocateChildren) const override;
@@ -347,32 +336,64 @@ protected:
   virtual nsresult BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                  const nsAttrValueOrString* aValue,
                                  bool aNotify) override;
 
   virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                 const nsAttrValue* aValue,
                                 const nsAttrValue* aOldValue,
                                 bool aNotify) override;
+  virtual nsresult OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
+                                          const nsAttrValueOrString& aValue,
+                                          bool aNotify) override;
 
   // Override for nsImageLoadingContent.
   nsIContent* AsContent() override { return this; }
 
   // This is a weak reference that this element and the HTMLFormElement
   // cooperate in maintaining.
   HTMLFormElement* mForm;
 
   // Created when we're tracking responsive image state
   RefPtr<ResponsiveImageSelector> mResponsiveSelector;
 
 private:
   bool SourceElementMatches(nsIContent* aSourceNode);
 
   static void MapAttributesIntoRule(const nsMappedAttributes* aAttributes,
                                     GenericSpecifiedValues* aGenericData);
+  /**
+   * This function is called by BeforeSetAttr and OnAttrSetButNotChanged.
+   * It will not be called if the value is being unset.
+   *
+   * @param aNamespaceID the namespace of the attr being set
+   * @param aName the localname of the attribute being set
+   * @param aValue the value it's being set to represented as either a string or
+   *        a parsed nsAttrValue.
+   * @param aNotify Whether we plan to notify document observers.
+   */
+  nsresult BeforeMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                 const nsAttrValueOrString& aValue,
+                                 bool aNotify);
+  /**
+   * This function is called by AfterSetAttr and OnAttrSetButNotChanged.
+   * It will not be called if the value is being unset.
+   *
+   * @param aNamespaceID the namespace of the attr being set
+   * @param aName the localname of the attribute being set
+   * @param aNotify Whether we plan to notify document observers.
+   */
+  nsresult AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                                bool aNotify);
+  /**
+   * Used by BeforeMaybeChangeAttr and AfterMaybeChangeAttr to keep track of
+   * whether a reload needs to be forced after an attribute change that is
+   * currently in progress.
+   */
+  bool mForceReload;
 
   bool mInDocResponsiveContent;
   RefPtr<ImageLoadTask> mPendingImageLoadTask;
 
   // Last URL that was attempted to load by this element.
   nsCOMPtr<nsIURI> mLastSelectedSource;
   // Last pixel density that was selected.
   double mCurrentDensity;