Bug 447567. Instead of brutally wiping out effects properties whenever the style context changes for an SVG frame (which wouldn't work for non-SVG frames), create a new style change hint to handle it. r=longsonr,sr=dbaron
authorRobert O'Callahan <robert@ocallahan.org>
Wed, 06 Aug 2008 12:55:07 +1200
changeset 16411 2e7da4485029468d7b1bf00dd6bb917a3414fcc0
parent 16410 d9167d3c4207f461b551ea7aa509b19bc54be418
child 16412 0445e533b7fc20bd26ff2b28604907bcf539bb0d
push id1011
push userrocallahan@mozilla.com
push dateWed, 06 Aug 2008 00:55:18 +0000
treeherdermozilla-central@2e7da4485029 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslongsonr, dbaron
bugs447567
milestone1.9.1a2pre
Bug 447567. Instead of brutally wiping out effects properties whenever the style context changes for an SVG frame (which wouldn't work for non-SVG frames), create a new style change hint to handle it. r=longsonr,sr=dbaron
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsChangeHint.h
layout/style/nsStyleContext.cpp
layout/style/nsStyleStruct.cpp
layout/svg/base/src/nsSVGAFrame.cpp
layout/svg/base/src/nsSVGContainerFrame.cpp
layout/svg/base/src/nsSVGContainerFrame.h
layout/svg/base/src/nsSVGForeignObjectFrame.cpp
layout/svg/base/src/nsSVGForeignObjectFrame.h
layout/svg/base/src/nsSVGGFrame.cpp
layout/svg/base/src/nsSVGGFrame.h
layout/svg/base/src/nsSVGInnerSVGFrame.cpp
layout/svg/base/src/nsSVGPathGeometryFrame.cpp
layout/svg/base/src/nsSVGTextFrame.cpp
layout/svg/base/src/nsSVGTextFrame.h
layout/svg/base/src/nsSVGUtils.cpp
layout/svg/base/src/nsSVGUtils.h
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -142,16 +142,19 @@
 
 #include "nsIXBLService.h"
 
 #undef NOISY_FIRST_LETTER
 
 #ifdef MOZ_MATHML
 #include "nsMathMLParts.h"
 #endif
+#ifdef MOZ_SVG
+#include "nsSVGUtils.h"
+#endif
 
 nsIFrame*
 NS_NewHTMLCanvasFrame (nsIPresShell* aPresShell, nsStyleContext* aContext);
 
 #if defined(MOZ_MEDIA)
 nsIFrame*
 NS_NewHTMLVideoFrame (nsIPresShell* aPresShell, nsStyleContext* aContext);
 #endif
@@ -9816,16 +9819,21 @@ nsCSSFrameConstructor::ProcessRestyledFr
       if (NS_PROPTABLE_PROP_NOT_THERE == res)
         continue;
     }
 
     if (hint & nsChangeHint_ReconstructFrame) {
       RecreateFramesForContent(content);
     } else {
       NS_ASSERTION(frame, "This shouldn't happen");
+#ifdef MOZ_SVG
+      if (hint & nsChangeHint_UpdateEffects) {
+        nsSVGUtils::UpdateEffects(frame);
+      }
+#endif
       if (hint & nsChangeHint_ReflowFrame) {
         StyleChangeReflow(frame);
       }
       if (hint & (nsChangeHint_RepaintFrame | nsChangeHint_SyncFrameView)) {
         ApplyRenderingChangeToTree(mPresShell->GetPresContext(), frame, hint);
       }
       if (hint & nsChangeHint_UpdateCursor) {
         nsIViewManager* viewMgr = mPresShell->GetViewManager();
--- a/layout/base/nsChangeHint.h
+++ b/layout/base/nsChangeHint.h
@@ -44,17 +44,27 @@
 
 // Defines for various style related constants
 
 enum nsChangeHint {
   nsChangeHint_RepaintFrame = 0x01,  // change was visual only (e.g., COLOR=)
   nsChangeHint_ReflowFrame = 0x02,   // change requires reflow (e.g., WIDTH=)
   nsChangeHint_SyncFrameView = 0x04, // change requires view to be updated, if there is one (e.g., clip:)
   nsChangeHint_UpdateCursor = 0x08,  // The currently shown mouse cursor needs to be updated
-  nsChangeHint_ReconstructFrame = 0x10   // change requires frame change (e.g., display:)
+  /**
+   * SVG filter/mask/clip effects need to be recomputed because the URI
+   * in the filter/mask/clip-path property has changed. This wipes
+   * out cached nsSVGPropertyBase and subclasses which hold a reference to
+   * the element referenced by the URI, and a mutation observer for
+   * the DOM subtree rooted at that element. Also, for filters they store a
+   * bounding-box for the filter result so that if the filter changes we can
+   * invalidate the old covered area.
+   */
+  nsChangeHint_UpdateEffects = 0x10,
+  nsChangeHint_ReconstructFrame = 0x20   // change requires frame change (e.g., display:)
                                          // This subsumes all the above
   // TBD: add nsChangeHint_ForceFrameView to force frame reconstruction if the frame doesn't yet
   // have a view
 };
 
 #ifdef DEBUG_roc
 // Redefine these operators to return nothing. This will catch any use
 // of these operators on hints. We should not be using these operators
--- a/layout/style/nsStyleContext.cpp
+++ b/layout/style/nsStyleContext.cpp
@@ -442,16 +442,21 @@ nsStyleContext::CalcStyleDifference(nsSt
   DO_STRUCT_DIFFERENCE(TableBorder);
   DO_STRUCT_DIFFERENCE(Table);
   DO_STRUCT_DIFFERENCE(UIReset);
   DO_STRUCT_DIFFERENCE(List);
   // If the quotes implementation is ever going to change we might not need
   // a framechange here and a reflow should be sufficient.  See bug 35768.
   DO_STRUCT_DIFFERENCE(Quotes);
 
+#ifdef MOZ_SVG
+  maxHint = nsChangeHint(NS_STYLE_HINT_REFLOW | nsChangeHint_UpdateEffects);
+  DO_STRUCT_DIFFERENCE(SVGReset);
+#endif
+
   // At this point, we know that the worst kind of damage we could do is
   // a reflow.
   maxHint = NS_STYLE_HINT_REFLOW;
       
   // The following structs cause (as their maximal difference) a reflow
   // to occur.  REFLOW Structs: Font, Margin, Padding, Border, List,
   // Position, Text, TextReset
   DO_STRUCT_DIFFERENCE(Font);
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -880,35 +880,44 @@ nsStyleSVGReset::nsStyleSVGReset(const n
   mMask = aSource.mMask;
   mStopOpacity = aSource.mStopOpacity;
   mFloodOpacity = aSource.mFloodOpacity;
   mDominantBaseline = aSource.mDominantBaseline;
 }
 
 nsChangeHint nsStyleSVGReset::CalcDifference(const nsStyleSVGReset& aOther) const
 {
+  nsChangeHint hint = nsChangeHint(0);
+
+  if (!EqualURIs(mClipPath, aOther.mClipPath) ||
+      !EqualURIs(mFilter, aOther.mFilter)     ||
+      !EqualURIs(mMask, aOther.mMask)) {
+    NS_UpdateHint(hint, nsChangeHint_UpdateEffects);
+    NS_UpdateHint(hint, nsChangeHint_ReflowFrame);
+    NS_UpdateHint(hint, nsChangeHint_RepaintFrame);
+  }
+
   if (mStopColor             != aOther.mStopColor     ||
       mFloodColor            != aOther.mFloodColor    ||
       mLightingColor         != aOther.mLightingColor ||
-      !EqualURIs(mClipPath, aOther.mClipPath)         ||
-      !EqualURIs(mFilter, aOther.mFilter)             ||
-      !EqualURIs(mMask, aOther.mMask)                 ||
       mStopOpacity           != aOther.mStopOpacity   ||
       mFloodOpacity          != aOther.mFloodOpacity  ||
       mDominantBaseline != aOther.mDominantBaseline)
-    return NS_STYLE_HINT_VISUAL;
-  
-  return NS_STYLE_HINT_NONE;
+    NS_UpdateHint(hint, nsChangeHint_RepaintFrame);
+
+  return hint;
 }
 
 #ifdef DEBUG
 /* static */
 nsChangeHint nsStyleSVGReset::MaxDifference()
 {
-  return NS_STYLE_HINT_VISUAL;
+  return NS_CombineHint(NS_CombineHint(nsChangeHint_UpdateEffects,
+                                       nsChangeHint_ReflowFrame),
+                                       nsChangeHint_RepaintFrame);
 }
 #endif
 
 // nsStyleSVGPaint implementation
 nsStyleSVGPaint::~nsStyleSVGPaint()
 {
   if (mType == eStyleSVGPaintType_Server) {
     NS_IF_RELEASE(mPaint.mPaintServer);
--- a/layout/svg/base/src/nsSVGAFrame.cpp
+++ b/layout/svg/base/src/nsSVGAFrame.cpp
@@ -60,18 +60,16 @@ protected:
     nsSVGAFrameBase(aContext) {}
 
 public:
   // nsIFrame:
   NS_IMETHOD  AttributeChanged(PRInt32         aNameSpaceID,
                                nsIAtom*        aAttribute,
                                PRInt32         aModType);
 
-  NS_IMETHOD DidSetStyleContext();
-
   /**
    * Get the "type" of the frame
    *
    * @see nsGkAtoms::svgAFrame
    */
   virtual nsIAtom* GetType() const;
 
 #ifdef DEBUG
@@ -124,24 +122,16 @@ nsSVGAFrame::AttributeChanged(PRInt32   
     mCanvasTM = nsnull;
     
     nsSVGUtils::NotifyChildrenOfSVGChange(this, TRANSFORM_CHANGED);
   }
 
  return NS_OK;
 }
 
-NS_IMETHODIMP
-nsSVGAFrame::DidSetStyleContext()
-{
-  nsSVGUtils::StyleEffects(this);
-
-  return NS_OK;
-}
-
 nsIAtom *
 nsSVGAFrame::GetType() const
 {
   return nsGkAtoms::svgAFrame;
 }
 
 //----------------------------------------------------------------------
 // nsISVGChildFrame methods
--- a/layout/svg/base/src/nsSVGContainerFrame.cpp
+++ b/layout/svg/base/src/nsSVGContainerFrame.cpp
@@ -100,23 +100,16 @@ nsSVGDisplayContainerFrame::Init(nsICont
 {
   if (!(GetStateBits() & NS_STATE_IS_OUTER_SVG)) {
     AddStateBits(aParent->GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD);
   }
   nsresult rv = nsSVGContainerFrameBase::Init(aContent, aParent, aPrevInFlow);
   return rv;
 }
 
-void
-nsSVGDisplayContainerFrame::Destroy()
-{
-  nsSVGUtils::StyleEffects(this);
-  nsSVGContainerFrame::Destroy();
-}
-
 NS_IMETHODIMP
 nsSVGDisplayContainerFrame::InsertFrames(nsIAtom* aListName,
                                          nsIFrame* aPrevFrame,
                                          nsIFrame* aFrameList)
 {
   // memorize last new frame
   nsIFrame* lastNewFrame = nsnull;
   {
--- a/layout/svg/base/src/nsSVGContainerFrame.h
+++ b/layout/svg/base/src/nsSVGContainerFrame.h
@@ -86,17 +86,16 @@ public:
   // nsISupports interface:
   NS_IMETHOD QueryInterface(const nsIID& aIID, void** aInstancePtr);
 private:
   NS_IMETHOD_(nsrefcnt) AddRef() { return 1; }
   NS_IMETHOD_(nsrefcnt) Release() { return 1; }
 
 public:
   // nsIFrame:
-  virtual void Destroy();
   NS_IMETHOD InsertFrames(nsIAtom*        aListName,
                           nsIFrame*       aPrevFrame,
                           nsIFrame*       aFrameList);
   NS_IMETHOD RemoveFrame(nsIAtom*        aListName,
                          nsIFrame*       aOldFrame);
   NS_IMETHOD Init(nsIContent*      aContent,
                   nsIFrame*        aParent,
                   nsIFrame*        aPrevInFlow);
--- a/layout/svg/base/src/nsSVGForeignObjectFrame.cpp
+++ b/layout/svg/base/src/nsSVGForeignObjectFrame.cpp
@@ -101,20 +101,16 @@ nsSVGForeignObjectFrame::Init(nsIContent
     nsSVGUtils::GetOuterSVGFrame(this)->RegisterForeignObject(this);
   }
   return rv;
 }
 
 void nsSVGForeignObjectFrame::Destroy()
 {
   nsSVGUtils::GetOuterSVGFrame(this)->UnregisterForeignObject(this);
-  // Delete any clipPath/filter/mask properties _before_ we die. The properties
-  // and property hash table have weak pointers to us that are dereferenced
-  // when the properties are destroyed.
-  nsSVGUtils::StyleEffects(this);
   nsSVGForeignObjectFrameBase::Destroy();
 }
 
 nsIAtom *
 nsSVGForeignObjectFrame::GetType() const
 {
   return nsGkAtoms::svgForeignObjectFrame;
 }
@@ -139,23 +135,16 @@ nsSVGForeignObjectFrame::AttributeChange
       UpdateGraphic();
     }
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsSVGForeignObjectFrame::DidSetStyleContext()
-{
-  nsSVGUtils::StyleEffects(this);
-  return NS_OK;
-}
-
-NS_IMETHODIMP
 nsSVGForeignObjectFrame::Reflow(nsPresContext*           aPresContext,
                                 nsHTMLReflowMetrics&     aDesiredSize,
                                 const nsHTMLReflowState& aReflowState,
                                 nsReflowStatus&          aStatus)
 {
   // InitialUpdate and AttributeChanged make sure mRect is up to date before
   // we're called (UpdateCoveredRegion sets mRect).
 
--- a/layout/svg/base/src/nsSVGForeignObjectFrame.h
+++ b/layout/svg/base/src/nsSVGForeignObjectFrame.h
@@ -70,18 +70,16 @@ public:
   NS_IMETHOD  AttributeChanged(PRInt32         aNameSpaceID,
                                nsIAtom*        aAttribute,
                                PRInt32         aModType);
 
   virtual nsIFrame* GetContentInsertionFrame() {
     return GetFirstChild(nsnull)->GetContentInsertionFrame();
   }
 
-  NS_IMETHOD DidSetStyleContext();
-
   NS_IMETHOD Reflow(nsPresContext*           aPresContext,
                     nsHTMLReflowMetrics&     aDesiredSize,
                     const nsHTMLReflowState& aReflowState,
                     nsReflowStatus&          aStatus);
 
   /**
    * Get the "type" of the frame
    *
--- a/layout/svg/base/src/nsSVGGFrame.cpp
+++ b/layout/svg/base/src/nsSVGGFrame.cpp
@@ -136,23 +136,16 @@ nsSVGGFrame::GetCanvasTM()
   }
 
   nsIDOMSVGMatrix* retval = mCanvasTM.get();
   NS_IF_ADDREF(retval);
   return retval;
 }
 
 NS_IMETHODIMP
-nsSVGGFrame::DidSetStyleContext()
-{
-  nsSVGUtils::StyleEffects(this);
-  return NS_OK;
-}
-
-NS_IMETHODIMP
 nsSVGGFrame::AttributeChanged(PRInt32         aNameSpaceID,
                               nsIAtom*        aAttribute,
                               PRInt32         aModType)
 {
   if (aNameSpaceID == kNameSpaceID_None &&
       aAttribute == nsGkAtoms::transform) {
     // make sure our cached transform matrix gets (lazily) updated
     mCanvasTM = nsnull;
--- a/layout/svg/base/src/nsSVGGFrame.h
+++ b/layout/svg/base/src/nsSVGGFrame.h
@@ -62,17 +62,16 @@ public:
 #ifdef DEBUG
   NS_IMETHOD GetFrameName(nsAString& aResult) const
   {
     return MakeFrameName(NS_LITERAL_STRING("SVGG"), aResult);
   }
 #endif
 
   // nsIFrame interface:
-  NS_IMETHOD DidSetStyleContext();
   NS_IMETHOD AttributeChanged(PRInt32         aNameSpaceID,
                               nsIAtom*        aAttribute,
                               PRInt32         aModType);
 
   // nsISVGChildFrame interface:
   virtual void NotifySVGChanged(PRUint32 aFlags);
   NS_IMETHOD SetMatrixPropagation(PRBool aPropagate);
   NS_IMETHOD SetOverrideCTM(nsIDOMSVGMatrix *aCTM);
--- a/layout/svg/base/src/nsSVGInnerSVGFrame.cpp
+++ b/layout/svg/base/src/nsSVGInnerSVGFrame.cpp
@@ -59,19 +59,16 @@ protected:
   
    // nsISupports interface:
   NS_IMETHOD QueryInterface(const nsIID& aIID, void** aInstancePtr);
 private:
   NS_IMETHOD_(nsrefcnt) AddRef() { return 1; }
   NS_IMETHOD_(nsrefcnt) Release() { return 1; }
 
 public:
-  // nsIFrame:
-  NS_IMETHOD DidSetStyleContext();
-
   // We don't define an AttributeChanged method since changes to the
   // 'x', 'y', 'width' and 'height' attributes of our content object
   // are handled in nsSVGSVGElement::DidModifySVGObservable
 
   /**
    * Get the "type" of the frame
    *
    * @see nsGkAtoms::svgInnerSVGFrame
@@ -399,15 +396,8 @@ nsSVGInnerSVGFrame::WillModifySVGObserva
 	
 NS_IMETHODIMP
 nsSVGInnerSVGFrame::DidModifySVGObservable (nsISVGValue* observable,
                                             nsISVGValue::modificationType aModType)
 {
   NotifyViewportChange();
   return NS_OK;
 }
-
-NS_IMETHODIMP
-nsSVGInnerSVGFrame::DidSetStyleContext()
-{
-  nsSVGUtils::StyleEffects(this);
-  return NS_OK;
-}
--- a/layout/svg/base/src/nsSVGPathGeometryFrame.cpp
+++ b/layout/svg/base/src/nsSVGPathGeometryFrame.cpp
@@ -594,18 +594,16 @@ nsSVGPathGeometryFrame::UpdateMarkerProp
     NS_ERROR("Could not create marker property");
     return;
   }
 }
 
 void
 nsSVGPathGeometryFrame::RemovePathProperties()
 {
-  nsSVGUtils::StyleEffects(this);
-
   if (GetStateBits() & NS_STATE_SVG_HAS_MARKERS)
     DeleteProperty(nsGkAtoms::marker);
 }
 
 void
 nsSVGPathGeometryFrame::Render(nsSVGRenderState *aContext)
 {
   gfxContext *gfx = aContext->GetGfxContext();
--- a/layout/svg/base/src/nsSVGTextFrame.cpp
+++ b/layout/svg/base/src/nsSVGTextFrame.cpp
@@ -94,24 +94,16 @@ nsSVGTextFrame::AttributeChanged(PRInt32
              aAttribute == nsGkAtoms::dx ||
              aAttribute == nsGkAtoms::dy) {
     NotifyGlyphMetricsChange();
   }
 
  return NS_OK;
 }
 
-NS_IMETHODIMP
-nsSVGTextFrame::DidSetStyleContext()
-{
-  nsSVGUtils::StyleEffects(this);
-
-  return NS_OK;
-}
-
 nsIAtom *
 nsSVGTextFrame::GetType() const
 {
   return nsGkAtoms::svgTextFrame;
 }
 
 //----------------------------------------------------------------------
 // nsISVGTextContentMetrics
--- a/layout/svg/base/src/nsSVGTextFrame.h
+++ b/layout/svg/base/src/nsSVGTextFrame.h
@@ -54,17 +54,16 @@ protected:
       mPropagateTransform(PR_TRUE),
       mPositioningDirty(PR_TRUE) {}
 
 public:
   // nsIFrame:
   NS_IMETHOD  AttributeChanged(PRInt32         aNameSpaceID,
                                nsIAtom*        aAttribute,
                                PRInt32         aModType);
-  NS_IMETHOD DidSetStyleContext();
 
   /**
    * Get the "type" of the frame
    *
    * @see nsGkAtoms::svgTextFrame
    */
   virtual nsIAtom* GetType() const;
 
--- a/layout/svg/base/src/nsSVGUtils.cpp
+++ b/layout/svg/base/src/nsSVGUtils.cpp
@@ -1386,17 +1386,17 @@ nsSVGUtils::PaintChildWithEffects(nsSVGR
   } else if (opacity != 1.0f) {
     gfx->Paint(opacity);
   }
 
   gfx->Restore();
 }
 
 void
-nsSVGUtils::StyleEffects(nsIFrame *aFrame)
+nsSVGUtils::UpdateEffects(nsIFrame *aFrame)
 {
   aFrame->DeleteProperty(nsGkAtoms::filter);
   aFrame->DeleteProperty(nsGkAtoms::mask);
   aFrame->DeleteProperty(nsGkAtoms::clipPath);
 }
 
 PRBool
 nsSVGUtils::HitTestClip(nsIFrame *aFrame, float x, float y)
--- a/layout/svg/base/src/nsSVGUtils.h
+++ b/layout/svg/base/src/nsSVGUtils.h
@@ -331,21 +331,22 @@ public:
 
   /* Paint frame with SVG effects - aDirtyRect is the area being
    * redrawn, in frame offset pixel coordinates */
   static void
   PaintChildWithEffects(nsSVGRenderState *aContext,
                         nsRect *aDirtyRect,
                         nsIFrame *aFrame);
 
-  /* Style change for effects (filter/clip/mask/opacity) - call when
-   * the frame's style has changed to make sure the effects properties
-   * stay in sync. */
+  /**
+   * Called by nsCSSFrameConstructor when style changes require the
+   * effect properties on aFrame to be updated
+   */
   static void
-  StyleEffects(nsIFrame *aFrame);
+  UpdateEffects(nsIFrame *aFrame);
 
   /* Hit testing - check if point hits the clipPath of indicated
    * frame.  (x,y) are specified in device pixels relative to the
    * origin of the outer svg frame.  Returns true if no clipPath
    * set. */
   static PRBool
   HitTestClip(nsIFrame *aFrame, float x, float y);