Bug 1502658 - Remove SVG use attribute change handling code from nsSVGUseFrame to SVGUseElement. r=longsonr
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sun, 28 Oct 2018 23:20:43 +0000
changeset 443270 627720590ce22dcf92d9ffc0ac3a7ec3d1a45f84
parent 443269 eb4b57c7a3e2c6cca1a124aca117da4eddd2351b
child 443271 619b0df072f35e2a06d998e368e99bc093f00357
push id34950
push usercsabou@mozilla.com
push dateMon, 29 Oct 2018 04:16:25 +0000
treeherdermozilla-central@d2e24bdf0648 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslongsonr
bugs1502658
milestone65.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 1502658 - Remove SVG use attribute change handling code from nsSVGUseFrame to SVGUseElement. r=longsonr Most of those shouldn't rely on a frame to run. Differential Revision: https://phabricator.services.mozilla.com/D9996
dom/svg/SVGUseElement.cpp
dom/svg/SVGUseElement.h
layout/svg/nsSVGUseFrame.cpp
layout/svg/nsSVGUseFrame.h
testing/web-platform/tests/svg/linking/reftests/use-hidden-attr-change.html
--- a/dom/svg/SVGUseElement.cpp
+++ b/dom/svg/SVGUseElement.cpp
@@ -87,16 +87,68 @@ SVGUseElement::~SVGUseElement()
     !OwnerDoc()->SVGUseElementNeedsShadowTreeUpdate(*this),
     "Dying without unbinding?"
   );
 }
 
 //----------------------------------------------------------------------
 // nsINode methods
 
+void
+SVGUseElement::ProcessAttributeChange(int32_t aNamespaceID, nsAtom* aAttribute)
+{
+  if (aNamespaceID == kNameSpaceID_None) {
+    if (aAttribute == nsGkAtoms::x || aAttribute == nsGkAtoms::y) {
+      if (auto* frame = GetFrame()) {
+        frame->PositionAttributeChanged();
+      }
+    } else if (aAttribute == nsGkAtoms::width ||
+               aAttribute == nsGkAtoms::height) {
+      const bool hadValidDimensions = HasValidDimensions();
+      const bool isUsed = OurWidthAndHeightAreUsed();
+      if (isUsed) {
+        SyncWidthOrHeight(aAttribute);
+      }
+
+      if (auto* frame = GetFrame()) {
+        frame->DimensionAttributeChanged(hadValidDimensions, isUsed);
+      }
+    }
+  }
+
+  if ((aNamespaceID == kNameSpaceID_XLink ||
+       aNamespaceID == kNameSpaceID_None) &&
+      aAttribute == nsGkAtoms::href) {
+    // We're changing our nature, clear out the clone information.
+    if (auto* frame = GetFrame()) {
+      frame->HrefChanged();
+    }
+    mOriginal = nullptr;
+    UnlinkSource();
+    TriggerReclone();
+  }
+}
+
+nsresult
+SVGUseElement::AfterSetAttr(int32_t aNamespaceID,
+                            nsAtom* aAttribute,
+                            const nsAttrValue* aValue,
+                            const nsAttrValue* aOldValue,
+                            nsIPrincipal* aSubjectPrincipal,
+                            bool aNotify)
+{
+  ProcessAttributeChange(aNamespaceID, aAttribute);
+  return SVGUseElementBase::AfterSetAttr(aNamespaceID,
+                                         aAttribute,
+                                         aValue,
+                                         aOldValue,
+                                         aSubjectPrincipal,
+                                         aNotify);
+}
+
 nsresult
 SVGUseElement::Clone(dom::NodeInfo* aNodeInfo, nsINode** aResult) const
 {
   *aResult = nullptr;
   SVGUseElement *it = new SVGUseElement(do_AddRef(aNodeInfo));
 
   nsCOMPtr<nsINode> kungFuDeathGrip(it);
   nsresult rv1 = it->Init();
@@ -174,17 +226,17 @@ SVGUseElement::CharacterDataChanged(nsIC
 {
   if (nsContentUtils::IsInSameAnonymousTree(this, aContent)) {
     TriggerReclone();
   }
 }
 
 void
 SVGUseElement::AttributeChanged(Element* aElement,
-                                int32_t aNameSpaceID,
+                                int32_t aNamespaceID,
                                 nsAtom* aAttribute,
                                 int32_t aModType,
                                 const nsAttrValue* aOldValue)
 {
   if (nsContentUtils::IsInSameAnonymousTree(this, aElement)) {
     TriggerReclone();
   }
 }
--- a/dom/svg/SVGUseElement.h
+++ b/dom/svg/SVGUseElement.h
@@ -82,16 +82,28 @@ public:
 
   nsIURI* GetSourceDocURI();
   URLExtraData* GetContentURLData() const { return mContentURLData; }
 
   // Updates the internal shadow tree to be an up-to-date clone of the
   // referenced element.
   void UpdateShadowTree();
 
+  // Shared code between AfterSetAttr and nsSVGUseFrame::AttributeChanged.
+  //
+  // This is needed because SMIL doesn't go through AfterSetAttr unfortunately.
+  void ProcessAttributeChange(int32_t aNamespaceID, nsAtom* aAttribute);
+
+  nsresult AfterSetAttr(int32_t aNamespaceID,
+                        nsAtom* aAttribute,
+                        const nsAttrValue* aValue,
+                        const nsAttrValue* aOldValue,
+                        nsIPrincipal* aSubjectPrincipal,
+                        bool aNotify) final;
+
 protected:
   /**
    * Helper that provides a reference to the element with the ID that is
    * referenced by the 'use' element's 'href' attribute, and that will update
    * the 'use' element if the element that that ID identifies changes to a
    * different element (or none).
    */
   class ElementTracker final : public IDTracker {
--- a/layout/svg/nsSVGUseFrame.cpp
+++ b/layout/svg/nsSVGUseFrame.cpp
@@ -1,16 +1,17 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsSVGUseFrame.h"
 
+#include "mozilla/dom/MutationEvent.h"
 #include "mozilla/dom/SVGUseElement.h"
 #include "SVGObserverUtils.h"
 
 using namespace mozilla;
 using namespace mozilla::dom;
 
 //----------------------------------------------------------------------
 // Implementation
@@ -36,69 +37,68 @@ nsSVGUseFrame::Init(nsIContent*       aC
 
   mHasValidDimensions =
     static_cast<SVGUseElement*>(aContent)->HasValidDimensions();
 
   nsSVGGFrame::Init(aContent, aParent, aPrevInFlow);
 }
 
 nsresult
-nsSVGUseFrame::AttributeChanged(int32_t aNameSpaceID,
+nsSVGUseFrame::AttributeChanged(int32_t aNamespaceID,
                                 nsAtom* aAttribute,
                                 int32_t aModType)
 {
-  auto* useElement = static_cast<SVGUseElement*>(GetContent());
-
-  if (aNameSpaceID == kNameSpaceID_None) {
-    if (aAttribute == nsGkAtoms::x || aAttribute == nsGkAtoms::y) {
-      // make sure our cached transform matrix gets (lazily) updated
-      mCanvasTM = nullptr;
-      nsLayoutUtils::PostRestyleEvent(
-        useElement, nsRestyleHint(0),
-        nsChangeHint_InvalidateRenderingObservers);
-      nsSVGUtils::ScheduleReflowSVG(this);
-      nsSVGUtils::NotifyChildrenOfSVGChange(this, TRANSFORM_CHANGED);
-    } else if (aAttribute == nsGkAtoms::width ||
-               aAttribute == nsGkAtoms::height) {
-      bool invalidate = false;
-      if (mHasValidDimensions != useElement->HasValidDimensions()) {
-        mHasValidDimensions = !mHasValidDimensions;
-        invalidate = true;
-      }
-
-      // FIXME(emilio): This shouldn't really belong to nsSVGUseFrame, but
-      // SVGUseElement.
-      if (useElement->OurWidthAndHeightAreUsed()) {
-        invalidate = true;
-        useElement->SyncWidthOrHeight(aAttribute);
-      }
-
-      if (invalidate) {
-        nsLayoutUtils::PostRestyleEvent(
-          useElement, nsRestyleHint(0),
-          nsChangeHint_InvalidateRenderingObservers);
-        nsSVGUtils::ScheduleReflowSVG(this);
-      }
-    }
+  // Currently our SMIL implementation does not modify the DOM attributes. Once
+  // we implement the SVG 2 SMIL behaviour this can be removed
+  // SVGUseElement::AfterSetAttr's implementation will be sufficient.
+  if (aModType == MutationEvent_Binding::SMIL) {
+    auto* content = SVGUseElement::FromNode(GetContent());
+    content->ProcessAttributeChange(aNamespaceID, aAttribute);
   }
 
-  if ((aNameSpaceID == kNameSpaceID_XLink ||
-       aNameSpaceID == kNameSpaceID_None) &&
-      aAttribute == nsGkAtoms::href) {
-    // we're changing our nature, clear out the clone information
+  return nsSVGGFrame::AttributeChanged(aNamespaceID, aAttribute, aModType);
+}
+
+void
+nsSVGUseFrame::PositionAttributeChanged()
+{
+  // make sure our cached transform matrix gets (lazily) updated
+  mCanvasTM = nullptr;
+  nsLayoutUtils::PostRestyleEvent(
+    GetContent()->AsElement(), nsRestyleHint(0),
+    nsChangeHint_InvalidateRenderingObservers);
+  nsSVGUtils::ScheduleReflowSVG(this);
+  nsSVGUtils::NotifyChildrenOfSVGChange(this, TRANSFORM_CHANGED);
+}
+
+void
+nsSVGUseFrame::DimensionAttributeChanged(bool aHadValidDimensions,
+                                         bool aAttributeIsUsed)
+{
+  bool invalidate = aAttributeIsUsed;
+  if (mHasValidDimensions != aHadValidDimensions) {
+    mHasValidDimensions = !mHasValidDimensions;
+    invalidate = true;
+  }
+
+  if (invalidate) {
     nsLayoutUtils::PostRestyleEvent(
-      useElement, nsRestyleHint(0),
+      GetContent()->AsElement(), nsRestyleHint(0),
       nsChangeHint_InvalidateRenderingObservers);
     nsSVGUtils::ScheduleReflowSVG(this);
-    useElement->mOriginal = nullptr;
-    useElement->UnlinkSource();
-    useElement->TriggerReclone();
   }
+}
 
-  return nsSVGGFrame::AttributeChanged(aNameSpaceID, aAttribute, aModType);
+void
+nsSVGUseFrame::HrefChanged()
+{
+  nsLayoutUtils::PostRestyleEvent(
+    GetContent()->AsElement(), nsRestyleHint(0),
+    nsChangeHint_InvalidateRenderingObservers);
+  nsSVGUtils::ScheduleReflowSVG(this);
 }
 
 //----------------------------------------------------------------------
 // nsSVGDisplayableFrame methods
 
 void
 nsSVGUseFrame::ReflowSVG()
 {
--- a/layout/svg/nsSVGUseFrame.h
+++ b/layout/svg/nsSVGUseFrame.h
@@ -26,19 +26,29 @@ protected:
 public:
   NS_DECL_FRAMEARENA_HELPERS(nsSVGUseFrame)
 
   // nsIFrame interface:
   void Init(nsIContent* aContent,
             nsContainerFrame* aParent,
             nsIFrame* aPrevInFlow) override;
 
-  nsresult AttributeChanged(int32_t aNameSpaceID,
+  // Called when the x or y attributes changed.
+  void PositionAttributeChanged();
+
+  // Called when the href attributes changed.
+  void HrefChanged();
+
+  // Called when the width or height attributes changed.
+  void DimensionAttributeChanged(bool aHadValidDimensions,
+                                 bool aAttributeIsUsed);
+
+  nsresult AttributeChanged(int32_t aNamespaceID,
                             nsAtom* aAttribute,
-                            int32_t aModType) override;
+                            int32_t aModType) final;
 
 #ifdef DEBUG_FRAME_DUMP
   nsresult GetFrameName(nsAString& aResult) const override
   {
     return MakeFrameName(NS_LITERAL_STRING("SVGUse"), aResult);
   }
 #endif
 
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/svg/linking/reftests/use-hidden-attr-change.html
@@ -0,0 +1,32 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>use element reacts to attribute changes when it's not rendered</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="help" href="https://svgwg.org/svg2-draft/struct.html#UseElement">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1502658">
+<link rel="match" href="/svg/linking/reftests/use-descendant-combinator-ref.html">
+<style>
+</style>
+<p>
+  You should see a green square, and no red.
+</p>
+<svg
+  version="1.1"
+  xmlns="http://www.w3.org/2000/svg"
+  xmlns:xlink="http://www.w3.org/1999/xlink"
+  style="display: none">
+  <defs>
+    <g id="square">
+      <rect width="100" height="100" fill="green" />
+    </g>
+  </defs>
+  <g id="test">
+    <use />
+  </g>
+</svg>
+<script>
+  onload = () => {
+    document.querySelector("use").setAttributeNS("http://www.w3.org/1999/xlink", "href", "#square");
+    document.querySelector("svg").style.display = "";
+  }
+</script>