Improve the readability, maintainability and performance of SVG gradient frames

RESOLVED FIXED

Status

()

RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

13 years ago
I had a bit more trouble following nsSVGGradientFrame.cpp the first time round than I should have. While reading through it I did some work to try and make things better. Here's a summary of the changes made in the patch:

checkURITarget(nsIAtom *attr) is killed and HasAttr is called directly.

checkURITarget(void) is killed getting rid of the obfuscation of the code and the difficulty of getting things right (see bug 302243 comment 3) caused by the need to reset mLoopFlag after calling checkURITarget. The parsing of xlink:href is moved out to a new function called parseHref so we only look at mHref when necessary, and the loop checking is now contained in two new getRefedGradientFrame functions.

Killing checkURITarget meant I touched a lot of the code, so I took the opportunity to carry out some simple perf improvements while I was there (major refactoring for perf is outwith the scope of this bug).

I removed GetGradientUnits from nsISVGGradient since it doesn't need to exist, and created a helper called getGradientUnits to do it's work faster. (It takes mContent QI'ed to nsIDOMSVGGradientElement as its argument to save it from doing an unnecessary QI.)

I've changed the logic in some functions so they check for SVG_GRUNITS_USERSPACEONUSE and default to the behavour for SVG_GRUNITS_OBJECTBOUNDINGBOX since that should be the default behaviour, not vice versa. I also added assertions to check our data integrity.

I've changed C-style casts to use the NS macros.

A fair bit of commenting has been added.
(Assignee)

Comment 1

13 years ago
Created attachment 214943 [details] [diff] [review]
patch
Attachment #214943 - Flags: review?(scootermorris)
(Assignee)

Comment 2

13 years ago
Comment on attachment 214943 [details] [diff] [review]
patch

Oops, scrap the review request. This should give an idea of what I've done so far, but I'm not finished.
Attachment #214943 - Flags: review?(scootermorris)
(Assignee)

Comment 3

13 years ago
Created attachment 214966 [details] [diff] [review]
patch - references to other gradients now working

I didn't realize that initializing calling parseHref() for the first time in the constructor was to early. In fact calling it in Init() is also too early. Seems we need to call it for the first time lazily. To aid with that I've added an mInitialized member. This helps us make the check for whether we've called parseHref() faster, and it doesn't increase the size of the class since it just fills up some of the padding that is placed at the end of it.
Attachment #214943 - Attachment is obsolete: true
(Assignee)

Comment 4

13 years ago
Created attachment 215014 [details] [diff] [review]
patch ready for review

Another thing I noticed was that we are QI'ing mContent to nsIDOMSVGXxxGradientElement at the top of a lot of the Get* methods. I moved these QIs down to after we check whether we're inheriting the value so we don't needlessly QI when we aren't going to use the result. I also removed the null checking of the result, because if the QI was going to fail it would have failed in the frame constructors and the frame wouldn't even exist.
Attachment #214966 - Attachment is obsolete: true
Attachment #215014 - Flags: superreview?(roc)
Attachment #215014 - Flags: review?(scootermorris)
+  PRUint16 getGradientUnits(

Uppercase

It looks like the behaviour for transforms is: if we have a transform attribute, use it, otherwise chase hrefs to the every end and use the transform attribute of at the end of the chain, if there is one. Is that correct? Shouldn't we be using the transform from the first gradient in the chain that has one?
+nsSVGGradientFrame::getRefedGradientFrame()

This function doesn't recurse. Is that intended?
(Assignee)

Comment 7

13 years ago
(In reply to comment #5)
> +  PRUint16 getGradientUnits(
> 
> Uppercase

I used a lowercase "g" deliberately to make it a little clearer that this is a helper, and not part of the implementation of any interface.

> It looks like the behaviour for transforms is: if we have a transform
> attribute, use it, otherwise chase hrefs to the every end and use the transform
> attribute of at the end of the chain, if there is one. Is that correct?

No. We only recurse if |!mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::gradientTransform)| evaluates to true (i.e. if the attribute isn't set on our element). Note that in the |else| statement we're dereferencing grad*Element*, not grad*Frame*. I don't know if that's what made you think it would recurse for that case to?

(In reply to comment #6)
> +nsSVGGradientFrame::getRefedGradientFrame()
> 
> This function doesn't recurse. Is that intended?

Yes. Some attributes are valid on both linear and radial gradients. Others are only valid on one of the two. Whenever you want to get an attribute that falls into the former category you only want the next gradient in the reference chain - you don't care what type it is. (You could access mNextGrad directy, but that wouldn't check it had been initialized and that |this != mNextGrad|.) Whenever you want an attribute that falls into the latter category you want the first gradient of the required type, so you may have to recurse looking for it.
(Assignee)

Comment 8

13 years ago
Created attachment 215210 [details] [diff] [review]
a different approach - pass in the attribute and return the first gradient with it set

Also made GetStopElement do the loop checking rather than its consumers.

So should I use an uppercase or lowercase "G"/"g" for the helper names? Just now there's a mixture, but consistancy would be good.
Attachment #215014 - Attachment is obsolete: true
Attachment #215210 - Flags: review?(scootermorris)
Attachment #215014 - Flags: superreview?(roc)
Attachment #215014 - Flags: review?(scootermorris)
We use uppercase everywhere else.
This patch looks like the right approach to me.

When you set mNextGrad to nsnull in DidModifySVGObservable, shouldn't you set mInitialized to PR_FALSE?

Scary idea: what if someone changes the 'id' of an element in such a way that the reference now points to a different element? we have no way of knowing that we need to reference a new frame, or no frame. Maybe file a bug about that...
(Assignee)

Comment 11

13 years ago
(In reply to comment #9)
> We use uppercase everywhere else.

Okay, uppercase it is then. I'll not bother with a new patch until Scooter has done first review.

(In reply to comment #10)
> When you set mNextGrad to nsnull in DidModifySVGObservable, shouldn't you set
> mInitialized to PR_FALSE?

No. mInitialized only flags to make sure parseHref has been called at least once. If someone removes the gradient we reference from the tree then we only need to make sure we don't try and access it. We don't want to call parseHref until our xlink:href is reset, and that's handled in AttributeChanged.

> Scary idea: what if someone changes the 'id' of an element in such a way that
> the reference now points to a different element? we have no way of knowing that
> we need to reference a new frame, or no frame. Maybe file a bug about that...

I'm not sure when this can happen. If there are multiple elements in the document tree with the same id, which one are you supposed to use? The one that comes first in document order? Last? Or the one that was embedded in the document tree first? Does any specification even say what should happen in the event that elements with conflicting ids are inserted into the document using script?
(Assignee)

Comment 12

13 years ago
I gave GetGradientWithAttr an aNameSpaceID parameter while vaguely thinking I needed to be able to pass in xlink:href, but of course you don't. We only inherit SVG's own attributes if xlink:href is set on *us*. Any objections to removing that parameter before I go ahead and do it?
(Assignee)

Comment 13

13 years ago
Created attachment 215254 [details] [diff] [review]
without aNameSpace parameter

Stuff it, I'll change it back if someone strongly objects.
Attachment #215210 - Attachment is obsolete: true
Attachment #215254 - Flags: review?(scootermorris)
Attachment #215210 - Flags: review?(scootermorris)
(Assignee)

Comment 14

13 years ago
This patch fixes the "inheritance" of the fx and fy attributes, so combined with the patch for bug 330682, we will pass the testcase at:

  http://jwatt.org/svg/tests/grad-radial-fx-cx-vs-inheritance.svg
(In reply to comment #11)
> (In reply to comment #10)
> > When you set mNextGrad to nsnull in DidModifySVGObservable, shouldn't you
> > set mInitialized to PR_FALSE?
> 
> No. mInitialized only flags to make sure parseHref has been called at least
> once. If someone removes the gradient we reference from the tree then we only
> need to make sure we don't try and access it. We don't want to call parseHref
> until our xlink:href is reset, and that's handled in AttributeChanged.

The frame can be destroyed and recreated without the content changing.

Furthermore someone could just remove the content node from the document and put it back.

> > Scary idea: what if someone changes the 'id' of an element in such a way
> > that the reference now points to a different element? we have no way of
> > knowing that we need to reference a new frame, or no frame. Maybe file a bug 
> > about that...
> 
> I'm not sure when this can happen.

Anytime the author is a dolt :-)

> If there are multiple elements in the document tree with the same id

Not sure, but we have some policy. A legitimate case is when the user moves the id from one element to another.

> Does any specification even say what should happen in the
> event that elements with conflicting ids are inserted into the document using
> script?

I'm not sure.
(In reply to comment #12)
> I gave GetGradientWithAttr an aNameSpaceID parameter while vaguely thinking I
> needed to be able to pass in xlink:href, but of course you don't. We only
> inherit SVG's own attributes if xlink:href is set on *us*. Any objections to
> removing that parameter before I go ahead and do it?

That's good.
(Assignee)

Comment 17

13 years ago
> The frame can be destroyed and recreated without the content changing.
> 
> Furthermore someone could just remove the content node from the document and
> put it back.

Setting mInitialized to false isn't a foolproof way to handle this. If we were to render between frame destruction and reconstruction, then we'd try to find the frame from the reference but fail and mInitialized would just be set to true again without us ever getting the reference. I think we really need to start observing the content object instead of the frame to fix these cases, but I think I don't want to make those changes on top of the extensive changes I've already made. Maybe another bug?

> Scary idea: what if someone changes the 'id' of an element in such a way
> that the reference now points to a different element? we have no way of
> knowing that we need to reference a new frame, or no frame. Maybe file a bug 
> about that...

Yeah, let's have another bug.
Status: NEW → ASSIGNED
(Assignee)

Comment 18

13 years ago
Created attachment 215369 [details]
testcase - LIVE CRASHER - CRASHES BRANCH AND TRUNK TOO

So I discovered this reference loop case which will crash the current nsSVGGradientFrame code. With this patch we'll catch the reference loop during rendering, but then when the document is unloaded we get a crash in the grad frame dtor. There's an easy fix, but I'd prefer not to use it if possible (see below). What's happening is this:

* we have an mNextGrad loop which the patch catches during rendering
* unfortunately we also have an mObservers loop
* when ~nsSVGGradientFrame is called a willmodify notification loops round
  and back to the frame that sent it.
* that frame ends up with mModifyNestCount == 2 and stops notifying
* the dying frame is incorrectly left with mModifyNestCount == 2 and the
  other frames in the loop are incorrectly left with mModifyNestCount == 1
* now DidModify is called in the dtor, but no notification is sent because of
  mModifyNestCount == 2
* the frame then just drops its observer list and dies
* now the dtor of one of the other frames that was in the loop is called
* because incorrectly its mModifyNestCount has been left equal to 1, it never
  sends out its will/didmodify notifications
* even worse, because it never recieved its *did*modify event from the
  other frame that was destroyed, its mNextGrad is a dangling pointer
* boom! mNextGrad->RemoveObserver(this) crashes

Now the easy solution is to just do what the current code does and set mNextGrad=nsnull whenever a reference loop is detected. However this could mean we won't render correctly if the loop is broken after we detect it, so I'd rather not. I suppose if we null out mNextGrad we could call ParseHref in DidModify so we try to reset mNextGrad if something along the reference chain changes, but that seems like an unnecessary waste of cycles just to catch the contrived case of a reference loop. I need to think about this after I've had some sleep, but input would be welcome.
(Assignee)

Comment 19

13 years ago
Created attachment 215394 [details] [diff] [review]
patch - loop check notifications too

Okay, I think this is the way to do it. I've created a new nsSVGGradientFrame::Will/DidModify to mark mLoopFlag before nsSVGValue::Will/DidModify calls Will/DidModifySVGObservable on the frames observers. I then have nsSVGGradientFrame::Will/DidModifySVGObservable check the flag before it does its stuff to prevent a notification loop which does bad things such set mModifyNestCount to an incorrect value.

This fixes all the gradient testcases I have that I've crashed on.
Attachment #215254 - Attachment is obsolete: true
Attachment #215394 - Flags: review?(scootermorris)
Attachment #215254 - Flags: review?(scootermorris)
(Assignee)

Comment 20

13 years ago
Oops, ignore the |#if 0| at the top. Forgot to remove it.

Comment 21

13 years ago
Comment on attachment 215394 [details] [diff] [review]
patch - loop check notifications too

FYI, the tree seems to have shifted underneath you, so this no longer
merges.  Make sure to merge to the trunk for the next round.

>Index: mozilla/layout/svg/base/src/nsSVGGradientFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGGradientFrame.cpp,v
>retrieving revision 1.29
>diff -u -8 -p -r1.29 nsSVGGradientFrame.cpp
>--- mozilla/layout/svg/base/src/nsSVGGradientFrame.cpp	9 Mar 2006 18:55:20 -0000	1.29
>+++ mozilla/layout/svg/base/src/nsSVGGradientFrame.cpp	17 Mar 2006 11:59:45 -0000
>@@ -67,43 +67,54 @@
> #include "nsIContent.h"
> #include "nsSVGNumber.h"
> #include "nsIDOMSVGStopElement.h"
> #include "nsSVGUtils.h"
> #include "nsWeakReference.h"
> #include "nsISVGValueObserver.h"
> #include "nsContentUtils.h"
> #include "nsSVGDefsFrame.h"
>-  

-- as you pointed out, this should be removed

>+
>+#if 0
>+#define WillModify(aModType)                                                  \
>+  mLoopFlag = PR_TRUE;                                                        \
>+  WillModify(aModType);
>+
>+#define DidModify(aModType)                                                   \
>+  DidModify(aModType);                                                        \
>+  mLoopFlag = PR_FALSE;
>+#endif
>+
>+class nsSVGLinearGradientFrame;
>+class nsSVGRadialGradientFrame;
>+
> typedef nsSVGDefsFrame  nsSVGGradientFrameBase;
> 
> class nsSVGGradientFrame : public nsSVGGradientFrameBase,
>                            public nsSVGValue,
>                            public nsISVGValueObserver,
>                            public nsSupportsWeakReference,
>                            public nsISVGGradient
> {
>-protected:
>+public:
>   friend nsIFrame* NS_NewSVGLinearGradientFrame(nsIPresShell* aPresShell, 
>                                                 nsIContent*   aContent);
> 
>   friend nsIFrame* NS_NewSVGRadialGradientFrame(nsIPresShell* aPresShell, 
>                                                 nsIContent*   aContent);
> 
>   friend nsIFrame* NS_NewSVGStopFrame(nsIPresShell* aPresShell, 
>                                       nsIContent*   aContent, 
>                                       nsIFrame*     aParentFrame);
> 
>   friend nsresult NS_GetSVGGradientFrame(nsIFrame**      result, 
>                                          nsIURI*         aURI, 
>                                          nsIContent*     aContent,
>                                          nsIPresShell*   aPresShell);
> 
>-
>-public:
>   // nsISupports interface:
>   NS_IMETHOD QueryInterface(const nsIID& aIID, void** aInstancePtr);
>   NS_IMETHOD_(nsrefcnt) AddRef() { return NS_OK; }
>   NS_IMETHOD_(nsrefcnt) Release() { return NS_OK; }
> 
>   // nsISVGGradient interface:
>   NS_DECL_NSISVGGRADIENT
> 
>@@ -117,58 +128,117 @@ public:
>   NS_IMETHOD DidModifySVGObservable(nsISVGValue* observable, 
>                                     nsISVGValue::modificationType aModType);
> 
>   // nsIFrame interface:
>   NS_IMETHOD DidSetStyleContext();
>   NS_IMETHOD RemoveFrame(nsIAtom*        aListName,
>                          nsIFrame*       aOldFrame);
> 
>-  /**
>-   * Get the "type" of the frame
>-   *
>-   * @see nsLayoutAtoms::svgGradientFrame
>-   */
>-  virtual nsIAtom* GetType() const;
>+  virtual nsIAtom* GetType() const;  // frame type: nsGkAtoms::svgGradientFrame
> 
>   NS_IMETHOD AttributeChanged(PRInt32         aNameSpaceID,
>                               nsIAtom*        aAttribute,
>                               PRInt32         aModType);
> 
>-  // nsISVGChildFrame interface:
>-  // Override PaintSVG (our frames don't directly render)
>-  NS_IMETHOD PaintSVG(nsISVGRendererCanvas* canvas,
>-                      const nsRect& dirtyRectTwips,
>-                      PRBool ignoreFilter) {return NS_OK;}
>-  
> #ifdef DEBUG
>+  // nsIFrameDebug interface:
>   NS_IMETHOD GetFrameName(nsAString& aResult) const
>   {
>     return MakeFrameName(NS_LITERAL_STRING("SVGGradient"), aResult);
>   }
> #endif // DEBUG
> 

The signature for this has changed -- it no longer takes the nsRect or ignoreFile, I think

>+  // nsISVGChildFrame interface:
>+  NS_IMETHOD PaintSVG(nsISVGRendererCanvas* canvas,
>+                      const nsRect& dirtyRectTwips,
>+                      PRBool ignoreFilter)
>+  {
>+    return NS_OK;  // override - our frames don't directly render
>+  }
>+  
>+private:
>+
>+  // Helper methods to aid gradient implementation
>+  // ---------------------------------------------
>+  // The SVG specification allows gradient elements to reference another
>+  // gradient element to "inherit" its attributes or gradient stops. Reference
>+  // chains of arbitrary length are allowed, and loop checking is essential!
>+  // Use the following helpers to safely get attributes and stops.
>+
>+  // Parse our xlink:href and set mNextGrad if we reference another gradient.
>+  void ParseHref();
>+
>+  // Helpers to look at our gradient and then along its reference chain (if any)
>+  // to find the first gradient with the specified attribute.
>+  nsIContent* GetGradientWithAttr(nsIAtom *aAttrName);
>+
>+  // Some attributes are only valid on one type of gradient, and we *must* get
>+  // the right type or we won't have the data structures we require.
>+  nsIContent* GetGradientWithAttr(nsIAtom *aAttrName, nsIAtom *aGradType);
>+
>+  // Get the stop element and (optionally) its frame (returns stop index/count)
>+  PRInt32 GetStopElement(PRInt32 aIndex, 
>+                         nsIDOMSVGStopElement * *aStopElement,
>+                         nsIFrame * *aStopFrame);
>+
> protected:
>+
>+  // Use these inline methods instead of GetGradientWithAttr(..., aGradType)
>+  nsIContent* GetLinearGradientWithAttr(nsIAtom *aAttrName)
>+  {
>+    return GetGradientWithAttr(aAttrName, nsGkAtoms::svgLinearGradientFrame);
>+  }
>+  nsIContent* GetRadialGradientWithAttr(nsIAtom *aAttrName)
>+  {
>+    return GetGradientWithAttr(aAttrName, nsGkAtoms::svgRadialGradientFrame);
>+  }
>+
>+  // We must loop check notifications too: see bug 330387 comment 18 + testcase
>+  // and comment 19. The mLoopFlag check is in Will/DidModifySVGObservable.
>+  void WillModify(modificationType aModType = mod_other)
>+  {
>+    mLoopFlag = PR_TRUE;
>+    nsSVGValue::WillModify(aModType);
>+    mLoopFlag = PR_FALSE;
>+  }
>+  void DidModify(modificationType aModType = mod_other)
>+  {
>+    mLoopFlag = PR_TRUE;
>+    nsSVGValue::DidModify(aModType);
>+    mLoopFlag = PR_FALSE;
>+  }
>+
>+  // Get the value of our gradientUnits attribute
>+  PRUint16 GetGradientUnits();
>+
>   virtual ~nsSVGGradientFrame();
> 
>-  // Internal methods for handling referenced gradients
>-  PRBool checkURITarget(nsIAtom *);
>-  PRBool checkURITarget();
>+  // The graphic element our gradient is (currently) being applied to
>+  nsCOMPtr<nsIContent>                   mSourceContent;
>+
>+private:
> 
>-  nsCOMPtr<nsIDOMSVGAnimatedString> 	 mHref;
>+  // href of the other gradient we reference (if any)
>+  nsCOMPtr<nsIDOMSVGAnimatedString>      mHref;
> 

Um, I think you mean 'reference'

>+  // Frame of the gradient we references (if any). Do NOT use this directly.
>+  // Use Get[Xxx]GradientWithAttr instead to ensure proper loop checking.
>   nsSVGGradientFrame                    *mNextGrad;
>-  nsCOMPtr<nsIContent>                   mSourceContent;
>+
>+  // Flag to mark this frame as "in use" during recursive calls along our
>+  // gradient's reference chain so we can detect reference loops. See:
>+  // http://www.w3.org/TR/SVG11/pservers.html#LinearGradientElementHrefAttribute
>   PRPackedBool                           mLoopFlag;
>-  
>-private:
>-  PRInt32 GetStopElement(PRInt32 aIndex, 
>-                         nsIDOMSVGStopElement * *aStopElement,
>-                         nsIFrame * *aStopFrame);
> 
>+  // Ideally we'd set mNextGrad by implementing Init(), but the frame of the
>+  // gradient we reference isn't available at that stage. Our only option is to
>+  // set mNextGrad lazily in GetGradientWithAttr, and to make that efficient
>+  // we need this flag. Our class size is the same since it just fills padding.
>+  PRPackedBool                           mInitialized;
> };
> 
> // -------------------------------------------------------------------------
> // Linear Gradients
> // -------------------------------------------------------------------------
> 
> typedef nsSVGGradientFrame nsSVGLinearGradientFrameBase;
> 
>@@ -181,28 +251,25 @@ public:
>   NS_IMETHOD_(nsrefcnt) AddRef() { return NS_OK; }
>   NS_IMETHOD_(nsrefcnt) Release() { return NS_OK; }
> 
>   // nsISVGLinearGradient interface:
>   NS_DECL_NSISVGLINEARGRADIENT
> 
>   // nsISVGGradient interface gets inherited from nsSVGGradientFrame
> 
>-  /**
>-   * Get the "type" of the frame
>-   *
>-   * @see nsLayoutAtoms::svgLinearGradientFrame
>-   */
>-  virtual nsIAtom* GetType() const;
>+  // nsIFrame interface:
>+  virtual nsIAtom* GetType() const;  // frame type: nsGkAtoms::svgLinearGradientFrame
> 
>   NS_IMETHOD AttributeChanged(PRInt32         aNameSpaceID,
>                               nsIAtom*        aAttribute,
>                               PRInt32         aModType);
> 
> #ifdef DEBUG
>+  // nsIFrameDebug interface:
>   NS_IMETHOD GetFrameName(nsAString& aResult) const
>   {
>     return MakeFrameName(NS_LITERAL_STRING("SVGLinearGradient"), aResult);
>   }
> #endif // DEBUG
> 
> };
> 
>@@ -221,41 +288,38 @@ public:
>   NS_IMETHOD_(nsrefcnt) AddRef() { return NS_OK; }
>   NS_IMETHOD_(nsrefcnt) Release() { return NS_OK; }
> 
>   // nsISVGRadialGradient interface:
>   NS_DECL_NSISVGRADIALGRADIENT
> 
>   // nsISVGGradient interface gets inherited from nsSVGGradientFrame
> 
>-  /**
>-   * Get the "type" of the frame
>-   *
>-   * @see nsLayoutAtoms::svgLinearGradientFrame
>-   */
>-  virtual nsIAtom* GetType() const;
>+  // nsIFrame interface:
>+  virtual nsIAtom* GetType() const;  // frame type: nsGkAtoms::svgRadialGradientFrame
> 
>   NS_IMETHOD AttributeChanged(PRInt32         aNameSpaceID,
>                               nsIAtom*        aAttribute,
>                               PRInt32         aModType);
> 
> #ifdef DEBUG
>+  // nsIFrameDebug interface:
>   NS_IMETHOD GetFrameName(nsAString& aResult) const
>   {
>     return MakeFrameName(NS_LITERAL_STRING("SVGRadialGradient"), aResult);
>   }
> #endif // DEBUG
> };
> 
> //----------------------------------------------------------------------
> // Implementation
> 
> nsSVGGradientFrame::~nsSVGGradientFrame()
> {
>-  WillModify();
>+  WillModify(mod_die);
>   // Notify the world that we're dying
>   DidModify(mod_die);
> 
>   if (mNextGrad) 
>     mNextGrad->RemoveObserver(this);
> }
> 
> //----------------------------------------------------------------------
>@@ -270,35 +334,47 @@ NS_INTERFACE_MAP_BEGIN(nsSVGGradientFram
> NS_INTERFACE_MAP_END_INHERITING(nsSVGGradientFrameBase)
> 
> //----------------------------------------------------------------------
> // nsISVGValueObserver methods:
> NS_IMETHODIMP
> nsSVGGradientFrame::WillModifySVGObservable(nsISVGValue* observable,
>                                             modificationType aModType)
> {
>+  // return if we have an mObservers loop
>+  if (mLoopFlag)
>+    return NS_OK;
>+
>+  // XXXjwatt: I don't think we want to pass on mod_die if it's not us that's
>+  // dying since our observers will then incorrectly stop observing us, no?
>   WillModify(aModType);
>   return NS_OK;
> }
>                                                                                 
> NS_IMETHODIMP
> nsSVGGradientFrame::DidModifySVGObservable(nsISVGValue* observable, 
>                                            nsISVGValue::modificationType aModType)
> {
>-  nsCOMPtr<nsISVGGradient> gradient = do_QueryInterface(observable);
>-  // Is this a gradient we are observing that is going away?
>-  if (mNextGrad && aModType == nsISVGValue::mod_die && gradient) {
>-    // Yes, we need to handle this differently
>+  // return if we have an mObservers loop
>+  if (mLoopFlag)
>+    return NS_OK;
>+
>+  // If we reference another gradient and it's going away, null out mNextGrad
>+  nsCOMPtr<nsISVGGradient> gradient;
>+  if (mNextGrad && aModType == nsISVGValue::mod_die &&
>+      (gradient = do_QueryInterface(observable))) {
>     nsISVGGradient *grad;
>     CallQueryInterface(mNextGrad, &grad);
>     if (grad == gradient) {
>       mNextGrad = nsnull;
>     }
>   }
>   // Something we depend on was modified -- pass it on!
>+  // XXXjwatt: I don't think we want to pass on mod_die if it's not us that's
>+  // dying since our observers will then incorrectly stop observing us, no?

Agreed, but we definitely need to notify our observers that we've changed!  For now,
we may just want to do a DidModify(mod_other) above?

>   DidModify(aModType);
>   return NS_OK;
> }
> 
> //----------------------------------------------------------------------
> // nsIFrame methods:
> 
> NS_IMETHODIMP
>@@ -338,201 +414,154 @@ nsSVGGradientFrame::AttributeChanged(PRI
>     DidModify();
>     return NS_OK;
>   } 
> 
>   if (aNameSpaceID == kNameSpaceID_XLink &&
>       aAttribute == nsGkAtoms::href) {
>     if (mNextGrad)
>       mNextGrad->RemoveObserver(this);
>-    mNextGrad = nsnull;
>     WillModify();
>+    ParseHref();
>     DidModify();
>     return NS_OK;
>   }
> 
>   return nsSVGGradientFrameBase::AttributeChanged(aNameSpaceID,
>                                                   aAttribute, aModType);
> }
> 
> //----------------------------------------------------------------------
> //  nsISVGGradient implementation
> //----------------------------------------------------------------------
> 
> NS_IMETHODIMP
> nsSVGGradientFrame::GetStopCount(PRUint32 *aStopCount)
> {
>-  nsresult rv = NS_OK;
>   nsIDOMSVGStopElement *stopElement = nsnull;
>   *aStopCount = GetStopElement(-1, &stopElement, nsnull);
>-  if (*aStopCount == 0) {
>-    // No stops?  check for URI target
>-    if (checkURITarget())
>-      rv = mNextGrad->GetStopCount(aStopCount);
>-    else
>-      rv = NS_ERROR_FAILURE;
>-  }
>-  mLoopFlag = PR_FALSE;
>-  return rv;
>+  return NS_OK;  // no stops is valid - paint as for 'none'
> }
> 
> NS_IMETHODIMP
> nsSVGGradientFrame::GetStopOffset(PRInt32 aIndex, float *aOffset)
> {
>   nsIDOMSVGStopElement *stopElement = nsnull;
>-  nsresult rv = NS_OK;
>   PRInt32 stopCount = GetStopElement(aIndex, &stopElement, nsnull);
> 
>   if (stopElement) {
>     nsCOMPtr<nsIDOMSVGAnimatedNumber> aNum;
>     stopElement->GetOffset(getter_AddRefs(aNum));
>     aNum->GetAnimVal(aOffset);
>     if (*aOffset < 0.0f)
>       *aOffset = 0.0f;
>-    if (*aOffset > 1.0f)
>+    else if (*aOffset > 1.0f)
>       *aOffset = 1.0f;
> 
>     return NS_OK;
>   }
> 
>-  // if our gradient doesn't have its own stops we must check if it references
>-  // another gradient in which case we must attempt to "inherit" its stops
>-  if (stopCount == 0 && checkURITarget()) {
>-    rv = mNextGrad->GetStopOffset(aIndex, aOffset);
>-  }
>-  else {
>-    *aOffset = 0.0f;
>-    rv = NS_ERROR_FAILURE;
>-  }
>-  mLoopFlag = PR_FALSE;
>-  return rv;
>+  NS_ASSERTION(stopCount == 0, "Don't call me with an invalid stop index!");
>+  *aOffset = 0.0f;
>+  return NS_ERROR_FAILURE;
> }
> 
> NS_IMETHODIMP
> nsSVGGradientFrame::GetStopColor(PRInt32 aIndex, nscolor *aStopColor) 
> {
>   nsIDOMSVGStopElement *stopElement = nsnull;
>   nsIFrame *stopFrame = nsnull;
>-  nsresult rv = NS_OK;

The compiler whines about "unused variable" here

>   PRInt32 stopCount = GetStopElement(aIndex, &stopElement, &stopFrame);
>-
>-  if (stopElement) {
>-    if (!stopFrame) {
>-      NS_ERROR("No stop frame found!");
>-      *aStopColor = 0;
>-      return NS_ERROR_FAILURE;
>-    }
>+  if (stopFrame) {
>     *aStopColor = stopFrame->GetStyleSVGReset()->mStopColor.mPaint.mColor;
>-
>     return NS_OK;
>   }
> 
>-  // if our gradient doesn't have its own stops we must check if it references
>-  // another gradient in which case we must attempt to "inherit" its stops
>-  if (stopCount == 0 && checkURITarget()) {
>-    rv = mNextGrad->GetStopColor(aIndex, aStopColor);
>+  // One way or another we have an implementation problem if we get here
>+#ifdef DEBUG
>+  if (stopElement) {
>+    NS_WARNING("We *do* have a stop but can't use it because it doesn't have "
>+               "a frame - we need frame free gradients and stops!");
>   }
>   else {
>-    *aStopColor = 0;
>-    rv = NS_ERROR_FAILURE;
>+    NS_ERROR("Don't call me with an invalid stop index!");
>   }
>-  mLoopFlag = PR_FALSE;
>-  return rv;
>+#endif
>+
>+  *aStopColor = 0;
>+  return NS_ERROR_FAILURE;
> }
> 
> NS_IMETHODIMP
> nsSVGGradientFrame::GetStopOpacity(PRInt32 aIndex, float *aStopOpacity) 
> {
>   nsIDOMSVGStopElement *stopElement = nsnull;
>   nsIFrame *stopFrame = nsnull;
>-  nsresult rv = NS_OK;

Unused variable

>   PRInt32 stopCount = GetStopElement(aIndex, &stopElement, &stopFrame);
>-
>-  if (stopElement) {
>-    if (!stopFrame) {
>-      NS_ERROR("No stop frame found!");
>-      *aStopOpacity = 1.0f;
>-      return NS_ERROR_FAILURE;
>-    }
>+  if (stopFrame) {
>     *aStopOpacity = stopFrame->GetStyleSVGReset()->mStopOpacity;
>-
>     return NS_OK;
>   }
> 
>-  // if our gradient doesn't have its own stops we must check if it references
>-  // another gradient in which case we must attempt to "inherit" its stops
>-  if (stopCount == 0 && checkURITarget()) {
>-    rv = mNextGrad->GetStopOpacity(aIndex, aStopOpacity);
>+  // One way or another we have an implementation problem if we get here
>+#ifdef DEBUG
>+  if (stopElement) {
>+    NS_WARNING("We *do* have a stop but can't use it because it doesn't have "
>+               "a frame - we need frame free gradients and stops!");
>   }
>   else {
>-    *aStopOpacity = 0;
>-    rv = NS_ERROR_FAILURE;
>+    NS_ERROR("Don't call me with an invalid stop index!");
>   }
>-  mLoopFlag = PR_FALSE;
>-  return rv;
>+#endif
>+
>+  *aStopOpacity = 1;
>+  return NS_ERROR_FAILURE;
> }
> 
> NS_IMETHODIMP
> nsSVGGradientFrame::GetGradientType(PRUint32 *aType)
> {
>-  nsCOMPtr<nsIDOMSVGLinearGradientElement> lGradElement = do_QueryInterface(mContent);
>-  if (lGradElement) {
>+  if (GetType() == nsGkAtoms::svgLinearGradientFrame) {
>     *aType = nsISVGGradient::SVG_LINEAR_GRADIENT;
>     return NS_OK;
>   }
>-
>-  nsCOMPtr<nsIDOMSVGRadialGradientElement> rGradElement = do_QueryInterface(mContent);
>-  if (rGradElement) {
>+  if (GetType() == nsGkAtoms::svgRadialGradientFrame) {
>     *aType = nsISVGGradient::SVG_RADIAL_GRADIENT;
>     return NS_OK;
>   }
>+  NS_NOTREACHED("Unknown gradient type!");
>   *aType = nsISVGGradient::SVG_UNKNOWN_GRADIENT;
>-  return NS_OK;
>-}
>-
>-NS_IMETHODIMP
>-nsSVGGradientFrame::GetGradientUnits(PRUint16 *aUnits)
>-{
>-  nsresult rv = NS_OK;
>-  nsCOMPtr<nsIDOMSVGGradientElement> gradElement = do_QueryInterface(mContent);
>-  NS_ASSERTION(gradElement, "Wrong content element (not gradient)");
>-  if (!gradElement) {
>-    return NS_ERROR_FAILURE;
>-  }
>-
>-  // See if we need to get the value from another gradient
>-  if (!checkURITarget(nsGkAtoms::gradientUnits)) {
>-    // No, return the values
>-    nsCOMPtr<nsIDOMSVGAnimatedEnumeration> units;
>-    gradElement->GetGradientUnits(getter_AddRefs(units));
>-    units->GetAnimVal(aUnits);
>-  } else {
>-    // Yes, get it from the target
>-    rv = mNextGrad->GetGradientUnits(aUnits);
>-  }
>-  mLoopFlag = PR_FALSE;
>-  return rv;
>+  return NS_ERROR_UNEXPECTED;
> }
> 
> NS_IMETHODIMP
> nsSVGGradientFrame::GetGradientTransform(nsIDOMSVGMatrix **aGradientTransform,
>                                          nsISVGGeometrySource *aSource)
> {
>-  *aGradientTransform = nsnull;
>-  nsCOMPtr<nsIDOMSVGAnimatedTransformList> animTrans;
>-  nsCOMPtr<nsIDOMSVGGradientElement> gradElement = do_QueryInterface(mContent);
>-  NS_ASSERTION(gradElement, "Wrong content element (not gradient)");
>-  if (!gradElement) {
>-    return NS_ERROR_FAILURE;
>+  nsCOMPtr<nsIDOMSVGMatrix> bboxTransform;
>+  PRUint16 gradientUnits = GetGradientUnits();
>+  if (gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_USERSPACEONUSE) {
>+    nsIFrame *frame = nsnull;
>+    CallQueryInterface(aSource, &frame);
>+
>+    // If this gradient is applied to text, our caller
>+    // will be the glyph, which is not a container, so we
>+    // need to get the parent
>+    nsIAtom *callerType = frame->GetType();
>+    if (callerType ==  nsGkAtoms::svgGlyphFrame)
>+      mSourceContent = frame->GetContent()->GetParent();
>+    else
>+      mSourceContent = frame->GetContent();
>+    NS_ASSERTION(mSourceContent, "Can't get content for gradient");
>   }
>+  else {
>+    NS_ASSERTION(gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX,
>+                 "Unknown gradientUnits type");
>+    // objectBoundingBox is the default anyway
> 
>-  nsCOMPtr<nsIDOMSVGMatrix> bboxTransform;
>-  PRUint16 bbox;
>-  GetGradientUnits(&bbox);
>-  if (bbox == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX) {
>     nsISVGChildFrame *frame = nsnull;
>     if (aSource)
>       CallQueryInterface(aSource, &frame);
>     nsCOMPtr<nsIDOMSVGRect> rect;
>     if (frame) {
>       frame->SetMatrixPropagation(PR_FALSE);
>       frame->NotifyCanvasTMChanged(PR_TRUE);
>       frame->GetBBox(getter_AddRefs(rect));
>@@ -543,176 +572,201 @@ nsSVGGradientFrame::GetGradientTransform
>       float x, y, width, height;
>       rect->GetX(&x);
>       rect->GetY(&y);
>       rect->GetWidth(&width);
>       rect->GetHeight(&height);
>       NS_NewSVGMatrix(getter_AddRefs(bboxTransform),
>                       width, 0, 0, height, x, y);
>     }
>-  } else {
>-    nsIFrame *frame = nsnull;
>-    CallQueryInterface(aSource, &frame);
>-
>-    // If this gradient is applied to text, our caller
>-    // will be the glyph, which is not a container, so we
>-    // need to get the parent
>-    nsIAtom *callerType = frame->GetType();
>-    if (callerType ==  nsLayoutAtoms::svgGlyphFrame)
>-      mSourceContent = frame->GetContent()->GetParent();
>-    else
>-      mSourceContent = frame->GetContent();
>-    NS_ASSERTION(mSourceContent, "Can't get content for gradient");
>   }
> 
>   if (!bboxTransform)
>     NS_NewSVGMatrix(getter_AddRefs(bboxTransform));
> 
>+  nsIContent *gradient = GetGradientWithAttr(nsGkAtoms::gradientTransform);
>+  if (!gradient)
>+    gradient = mContent;  // use our gradient to get the correct default value
>+
>+  nsCOMPtr<nsIDOMSVGGradientElement> gradElement = do_QueryInterface(gradient);
>+  nsCOMPtr<nsIDOMSVGAnimatedTransformList> animTrans;
>+  gradElement->GetGradientTransform(getter_AddRefs(animTrans));
>+  nsCOMPtr<nsIDOMSVGTransformList> trans;
>+  animTrans->GetAnimVal(getter_AddRefs(trans));
>   nsCOMPtr<nsIDOMSVGMatrix> gradientTransform;
>-  // See if we need to get the value from another gradient
>-  if (!checkURITarget(nsGkAtoms::gradientTransform)) {
>-    // No, return the values
>-    gradElement->GetGradientTransform(getter_AddRefs(animTrans));
>-    nsCOMPtr<nsIDOMSVGTransformList> lTrans;
>-    animTrans->GetAnimVal(getter_AddRefs(lTrans));
>-    lTrans->GetConsolidationMatrix(getter_AddRefs(gradientTransform));
>-  } else {
>-    // Yes, get it from the target
>-    mNextGrad->GetGradientTransform(getter_AddRefs(gradientTransform), nsnull);
>-  }
>+  trans->GetConsolidationMatrix(getter_AddRefs(gradientTransform));
> 
>-  bboxTransform->Multiply(gradientTransform, aGradientTransform);
>-  mLoopFlag = PR_FALSE;
>-  return NS_OK;
>+  return bboxTransform->Multiply(gradientTransform, aGradientTransform);
> }
> 
> NS_IMETHODIMP
> nsSVGGradientFrame::GetSpreadMethod(PRUint16 *aSpreadMethod)
> {
>-  nsresult rv = NS_OK;
>-  nsCOMPtr<nsIDOMSVGGradientElement> gradElement = do_QueryInterface(mContent);
>-  NS_ASSERTION(gradElement, "Wrong content element (not gradient)");
>-  if (!gradElement) {
>-    return NS_ERROR_FAILURE;
>-  }
>-  // See if we need to get the value from another gradient
>-  if (!checkURITarget(nsGkAtoms::spreadMethod)) {
>-    // No, return the values
>-    nsCOMPtr<nsIDOMSVGAnimatedEnumeration> method;
>-    gradElement->GetSpreadMethod(getter_AddRefs(method));
>-    method->GetAnimVal(aSpreadMethod);
>-  } else {
>-    // Yes, get it from the target
>-    rv = mNextGrad->GetSpreadMethod(aSpreadMethod);
>-  }
>-  mLoopFlag = PR_FALSE;
>-  return rv;
>+  nsIContent *gradient = GetGradientWithAttr(nsGkAtoms::spreadMethod);
>+  if (!gradient)
>+    gradient = mContent;  // use our gradient to get the correct default value
>+
>+  nsCOMPtr<nsIDOMSVGGradientElement> gradElement = do_QueryInterface(gradient);
>+  nsCOMPtr<nsIDOMSVGAnimatedEnumeration> method;
>+  gradElement->GetSpreadMethod(getter_AddRefs(method));
>+  return method->GetAnimVal(aSpreadMethod);
> }
> 
>-NS_IMETHODIMP
>-nsSVGGradientFrame::GetNextGradient(nsISVGGradient * *aNextGrad, PRUint32 aType) {
>-  PRUint32 nextType;
>-  if (!mNextGrad) {
>-    *aNextGrad = nsnull;
>-    return NS_ERROR_FAILURE;
>-  }
>-  mNextGrad->GetGradientType(&nextType);
>-  if (nextType == aType) {
>-    *aNextGrad = mNextGrad;
>-    return NS_OK;
>-  } else {
>-    return mNextGrad->GetNextGradient(aNextGrad, aType);
>-  }
>-}
> 
> // Private (helper) methods
>-PRBool 
>-nsSVGGradientFrame::checkURITarget(nsIAtom *attr) {
>-  // Was the attribute explicitly set?
>-  if (mContent->HasAttr(kNameSpaceID_None, attr)) {
>-    // Yes, just return
>-    return PR_FALSE;
>-  }
>-  return checkURITarget();
>-}
>-
>-PRBool
>-nsSVGGradientFrame::checkURITarget(void) {
>-  mLoopFlag = PR_TRUE; // Set our loop detection flag
>-  // Have we already figured out the next Gradient?
>-  if (mNextGrad != nsnull) {
>-    return PR_TRUE;
>-  }
> 
>-  // check if we reference another gradient to "inherit" 
>-  // its stops or attributes

This routine does a lot more the just parseing an href.  I think a more
appropriate name is in order: GetNextRef, or GetNextGrad, GetNextGradFromHref,
or something....

>+void
>+nsSVGGradientFrame::ParseHref()
>+{
>+  mNextGrad = nsnull;
>+  mInitialized = PR_TRUE;
>+
>+  // Fetch our gradient element's xlink:href attribute
>   nsAutoString href;
>   mHref->GetAnimVal(href);
>-  // Do we have URI?
>   if (href.IsEmpty()) {
>-    return PR_FALSE; // No, return the default
>+    return; // no URL
>   }
> 
>+  // Convert href to an nsIURI
>   nsCOMPtr<nsIURI> targetURI;
>   nsCOMPtr<nsIURI> base = mContent->GetBaseURI();
>-  nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(targetURI),
>-    href, mContent->GetCurrentDoc(), base);
>+  nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(targetURI), href,
>+                                            mContent->GetCurrentDoc(), base);
> 
>-  // Note that we are using *our* frame tree for this call, otherwise we're going to have
>-  // to get the PresShell in each call
>+  // Fetch and store a pointer to the referenced gradient element's frame.
>+  // Note that we are using *our* frame tree for this call, otherwise we're
>+  // going to have to get the PresShell in each call
>   nsIFrame *nextGrad;
>-  if (NS_SUCCEEDED(nsSVGUtils::GetReferencedFrame(&nextGrad, targetURI, mContent, GetPresContext()->PresShell()))) {
>+  if (NS_SUCCEEDED(nsSVGUtils::GetReferencedFrame(&nextGrad, targetURI, mContent,
>+                                                  GetPresContext()->PresShell()))) {
>     nsIAtom* frameType = nextGrad->GetType();
>-    if ((frameType != nsLayoutAtoms::svgLinearGradientFrame) && 
>-        (frameType != nsLayoutAtoms::svgRadialGradientFrame))
>-      return PR_FALSE;
>-
>-    mNextGrad = (nsSVGGradientFrame *)nextGrad;
>-    if (mNextGrad->mLoopFlag) {
>-      // Yes, remove the reference and return an error
>-      NS_WARNING("Gradient loop detected!");
>-      mNextGrad = nsnull;
>-      return PR_FALSE;
>-    }
>+    if (frameType != nsGkAtoms::svgLinearGradientFrame && 
>+        frameType != nsGkAtoms::svgRadialGradientFrame)
>+      return;
>+
>+    mNextGrad = NS_REINTERPRET_CAST(nsSVGGradientFrame*, nextGrad);
>+
>     // Add ourselves to the observer list
>     if (mNextGrad) {
>       // Can't use the NS_ADD macro here because of nsISupports ambiguity
>       mNextGrad->AddObserver(this);
>     }
>-    return PR_TRUE;
>   }
>-  return PR_FALSE;
> }
> 
>-// -------------------------------------------------------------------------
>-// Private helper method to simplify getting stop elements
>-// returns non-addrefed stop element
>-// -------------------------------------------------------------------------

Er, implemented? :-)

>+// This is impelemented to return nsnull if the attribute is not set so that
>+// GetFx and GetFy can use the values of cx and cy instead of the defaults.
>+nsIContent*
>+nsSVGGradientFrame::GetGradientWithAttr(nsIAtom *aAttrName)
>+{
>+  if (mContent->HasAttr(kNameSpaceID_None, aAttrName))
>+    return mContent;
>+
>+  if (!mInitialized)  // make sure mNextGrad has been initialized
>+    ParseHref();
>+
>+  if (!mNextGrad)
>+    return nsnull;
>+
>+  nsIContent *grad = nsnull;
>+

With all this surgery, maybe now is the time (and Gradients are the venue) for figuring
out how to write to the JS console.  I'm uncomfortable with just returning after we've
detected a loop without putting up some kind of warning to the user.

>+  // Set mLoopFlag before checking mNextGrad->mLoopFlag in case we are mNextGrad
>+  mLoopFlag = PR_TRUE;
>+  if (!mNextGrad->mLoopFlag)
>+    grad = mNextGrad->GetGradientWithAttr(aAttrName);
>+  mLoopFlag = PR_FALSE;
>+
>+  return grad;
>+}
>+
>+nsIContent*
>+nsSVGGradientFrame::GetGradientWithAttr(nsIAtom *aAttrName, nsIAtom *aGradType)
>+{
>+  if (GetType() == aGradType && mContent->HasAttr(kNameSpaceID_None, aAttrName))
>+    return mContent;
>+
>+  if (!mInitialized)
>+    ParseHref();  // make sure mNextGrad has been initialized
>+
>+  if (!mNextGrad)
>+    return nsnull;
>+
>+  nsIContent *grad = nsnull;
>+
>+  // Set mLoopFlag before checking mNextGrad->mLoopFlag in case we are mNextGrad
>+  mLoopFlag = PR_TRUE;
>+  if (!mNextGrad->mLoopFlag)
>+    grad = mNextGrad->GetGradientWithAttr(aAttrName, aGradType);
>+  mLoopFlag = PR_FALSE;
>+
>+  return grad;
>+}
>+
> PRInt32 
> nsSVGGradientFrame::GetStopElement(PRInt32 aIndex, nsIDOMSVGStopElement * *aStopElement,
>                                    nsIFrame * *aStopFrame)
> {
>   PRInt32 stopCount = 0;
>-  nsIFrame *stopFrame;
>+  nsIFrame *stopFrame = nsnull;
>   for (stopFrame = mFrames.FirstChild(); stopFrame;
>        stopFrame = stopFrame->GetNextSibling()) {
>     nsCOMPtr<nsIDOMSVGStopElement>stopElement = do_QueryInterface(stopFrame->GetContent());
>     if (stopElement) {
>       // Is this the one we're looking for?
>       if (stopCount++ == aIndex) {
>         *aStopElement = stopElement;
>         break; // Yes, break out of the loop
>       }
>     }
>   }
>-  if (aStopFrame)
>-    *aStopFrame = stopFrame;
>+  if (stopCount > 0) {
>+    if (aStopFrame)
>+      *aStopFrame = stopFrame;
>+    return stopCount;
>+  }
>+

I know why you have "inherit" in quotes, and what you mean, but when you first
introduce it, it might be useful to provide a more detailed explanation -- this
is *not* inheritance in the way that CSS or other attributes work.  Maybe just
use "get from referenced gradient"?

>+  // Our gradient element doesn't have stops - try to "inherit" them
>+
>+  if (!mInitialized)
>+    ParseHref();  // make sure mNextGrad has been initialized
>+
>+  if (!mNextGrad) {
>+    *aStopElement = nsnull;
>+    if (aStopFrame)
>+      *aStopFrame = nsnull;
>+    return 0;
>+  }
>+
>+  // Set mLoopFlag before checking mNextGrad->mLoopFlag in case we are mNextGrad
>+  mLoopFlag = PR_TRUE;
>+  if (!mNextGrad->mLoopFlag)
>+    stopCount = mNextGrad->GetStopElement(aIndex, aStopElement, aStopFrame);
>+  mLoopFlag = PR_FALSE;
>+
>   return stopCount;
> }
> 
>+PRUint16
>+nsSVGGradientFrame::GetGradientUnits()
>+{
>+  // This getter is called every time the others are called - maybe cache it?

You could certainly cache the animValue, but wouldn't you have to observe it, then?
And if you cache the result, how would you know if it was modified via the DOM?
 
>+
>+  nsIContent *gradient = GetGradientWithAttr(nsGkAtoms::gradientUnits);
>+  if (!gradient)
>+    gradient = mContent;  // use our gradient to get the correct default value
>+
>+  nsCOMPtr<nsIDOMSVGGradientElement> gradElement = do_QueryInterface(gradient);
>+  nsCOMPtr<nsIDOMSVGAnimatedEnumeration> animUnits;
>+  gradElement->GetGradientUnits(getter_AddRefs(animUnits));
>+  PRUint16 units;
>+  animUnits->GetAnimVal(&units);
>+  return units;
>+}
>+
> // -------------------------------------------------------------------------
> // Linear Gradients
> // -------------------------------------------------------------------------
> 
> nsIAtom*
> nsSVGLinearGradientFrame::GetType() const
> {
>   return nsLayoutAtoms::svgLinearGradientFrame;
>@@ -743,183 +797,127 @@ nsSVGLinearGradientFrame::AttributeChang
> NS_INTERFACE_MAP_BEGIN(nsSVGLinearGradientFrame)
>   NS_INTERFACE_MAP_ENTRY(nsISVGLinearGradient)
> NS_INTERFACE_MAP_END_INHERITING(nsSVGLinearGradientFrameBase)
> 
> // nsISVGLinearGradient
> NS_IMETHODIMP
> nsSVGLinearGradientFrame::GetX1(float *aX1)
> {
>-  nsCOMPtr<nsIDOMSVGLinearGradientElement> lGrad = do_QueryInterface(mContent);
>-  NS_ASSERTION(lGrad, "Wrong content element (not linear gradient)");
>-  if (!lGrad) {
>-    return NS_ERROR_FAILURE;
>-  }
>-  // See if we need to get the value from another gradient
>-  if (checkURITarget(nsGkAtoms::x1)) {
>-    // Yes, get it from the target
>-    nsISVGGradient *nextGrad;
>-    if (GetNextGradient(&nextGrad,
>-                        nsISVGGradient::SVG_LINEAR_GRADIENT) == NS_OK) {
>-      nsSVGLinearGradientFrame *lNgrad =
>-        (nsSVGLinearGradientFrame *)nextGrad;
>-      lNgrad->GetX1(aX1);
>-      mLoopFlag = PR_FALSE;
>-      return NS_OK;
>-    }
>-    // There are no gradients in the list with our type -- fall through
>-    // and return our default value
>-  }
>+  nsIContent *gradient = GetLinearGradientWithAttr(nsGkAtoms::x1);
>+  if (!gradient)
>+    gradient = mContent;  // use our gradient to get the correct default value
> 
>-  // No, return the values
>+  nsCOMPtr<nsIDOMSVGLinearGradientElement> lGrad = do_QueryInterface(gradient);
>   nsCOMPtr<nsIDOMSVGAnimatedLength> animLength;
>   lGrad->GetX1(getter_AddRefs(animLength));
>   nsCOMPtr<nsIDOMSVGLength> length;
>   animLength->GetAnimVal(getter_AddRefs(length));
> 
>   // Object bounding box units are handled by setting the appropriate
>   // transform in GetGradientTransfrom, but we need to handle user
>   // space units as part of the individual Get* routines.  Fixes 323669.
> 
>-  PRUint16 bbox;
>-  GetGradientUnits(&bbox);
>-  if (bbox == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX) {
>-    length->GetValue(aX1);
>-  } else {
>+  PRUint16 gradientUnits = GetGradientUnits();
>+  if (gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_USERSPACEONUSE) {
>     *aX1 = nsSVGUtils::UserSpace(mSourceContent, length, nsSVGUtils::X);
>+    return NS_OK;
>   }
>-  mLoopFlag = PR_FALSE;
>-  return NS_OK;
>+
>+  NS_ASSERTION(gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX,
>+               "Unknown gradientUnits type");
>+
>+  return length->GetValue(aX1);  // objectBoundingBox is the default anyway
> }
>-  
>+
> nsresult
> nsSVGLinearGradientFrame::GetY1(float *aY1)
> {
>-  nsCOMPtr<nsIDOMSVGLinearGradientElement> lGrad = do_QueryInterface(mContent);
>-  NS_ASSERTION(lGrad, "Wrong content element (not linear gradient)");
>-  if (!lGrad) {
>-    return NS_ERROR_FAILURE;
>-  }
>-  // See if we need to get the value from another gradient
>-  if (checkURITarget(nsGkAtoms::y1)) {
>-    // Yes, get it from the target
>-    nsISVGGradient *nextGrad;
>-    if (GetNextGradient(&nextGrad,
>-                        nsISVGGradient::SVG_LINEAR_GRADIENT) == NS_OK) {
>-      nsSVGLinearGradientFrame *lNgrad =
>-        (nsSVGLinearGradientFrame *)nextGrad;
>-      lNgrad->GetY1(aY1);
>-      mLoopFlag = PR_FALSE;
>-      return NS_OK;
>-    }
>-    // There are no gradients in the list with our type -- fall through
>-    // and return our default value
>-  }
>+  nsIContent *gradient = GetLinearGradientWithAttr(nsGkAtoms::y1);
>+  if (!gradient)
>+    gradient = mContent;  // use our gradient to get the correct default value
> 
>-  // No, return the values
>+  nsCOMPtr<nsIDOMSVGLinearGradientElement> lGrad = do_QueryInterface(gradient);
>   nsCOMPtr<nsIDOMSVGAnimatedLength> animLength;
>   lGrad->GetY1(getter_AddRefs(animLength));
>   nsCOMPtr<nsIDOMSVGLength> length;
>   animLength->GetAnimVal(getter_AddRefs(length));
> 
>   // Object bounding box units are handled by setting the appropriate
>   // transform in GetGradientTransfrom, but we need to handle user
>   // space units as part of the individual Get* routines.  Fixes 323669.
> 
>-  PRUint16 bbox;
>-  GetGradientUnits(&bbox);
>-  if (bbox == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX) {
>-    length->GetValue(aY1);
>-  } else {
>+  PRUint16 gradientUnits = GetGradientUnits();
>+  if (gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_USERSPACEONUSE) {
>     *aY1 = nsSVGUtils::UserSpace(mSourceContent, length, nsSVGUtils::Y);
>+    return NS_OK;
>   }
>-  mLoopFlag = PR_FALSE;
>-  return NS_OK;
>+
>+  NS_ASSERTION(gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX,
>+               "Unknown gradientUnits type");
>+
>+  return length->GetValue(aY1);  // objectBoundingBox is the default anyway
> }
> 
> NS_IMETHODIMP
> nsSVGLinearGradientFrame::GetX2(float *aX2)
> {
>-  nsCOMPtr<nsIDOMSVGLinearGradientElement> lGrad = do_QueryInterface(mContent);
>-  NS_ASSERTION(lGrad, "Wrong content element (not linear gradient)");
>-  if (!lGrad) {
>-    return NS_ERROR_FAILURE;
>-  }
>-  // See if we need to get the value from another gradient
>-  if (checkURITarget(nsGkAtoms::x2)) {
>-    // Yes, get it from the target
>-    nsISVGGradient *nextGrad;
>-    if (GetNextGradient(&nextGrad,
>-                        nsISVGGradient::SVG_LINEAR_GRADIENT) == NS_OK) {
>-      nsSVGLinearGradientFrame *lNgrad =
>-        (nsSVGLinearGradientFrame *)nextGrad;
>-      lNgrad->GetX2(aX2);
>-      mLoopFlag = PR_FALSE;
>-      return NS_OK;
>-    }
>-    // There are no gradients in the list with our type -- fall through
>-    // and return our default value
>-  }
>+  nsIContent *gradient = GetLinearGradientWithAttr(nsGkAtoms::x2);
>+  if (!gradient)
>+    gradient = mContent;  // use our gradient to get the correct default value
> 
>-  // No, return the values
>+  nsCOMPtr<nsIDOMSVGLinearGradientElement> lGrad = do_QueryInterface(gradient);
>   nsCOMPtr<nsIDOMSVGAnimatedLength> animLength;
>   lGrad->GetX2(getter_AddRefs(animLength));
>   nsCOMPtr<nsIDOMSVGLength> length;
>   animLength->GetAnimVal(getter_AddRefs(length));
> 
>-  PRUint16 bbox;
>-  GetGradientUnits(&bbox);
>-  if (bbox == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX) {
>-    length->GetValue(aX2);
>-  } else {
>+  // Object bounding box units are handled by setting the appropriate
>+  // transform in GetGradientTransfrom, but we need to handle user
>+  // space units as part of the individual Get* routines.  Fixes 323669.
>+
>+  PRUint16 gradientUnits = GetGradientUnits();
>+  if (gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_USERSPACEONUSE) {
>     *aX2 = nsSVGUtils::UserSpace(mSourceContent, length, nsSVGUtils::X);
>+    return NS_OK;
>   }
>-  mLoopFlag = PR_FALSE;
>-  return NS_OK;
>+
>+  NS_ASSERTION(gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX,
>+               "Unknown gradientUnits type");
>+
>+  return length->GetValue(aX2);  // objectBoundingBox is the default anyway
> }
> 
> NS_IMETHODIMP
> nsSVGLinearGradientFrame::GetY2(float *aY2)
> {
>-  nsCOMPtr<nsIDOMSVGLinearGradientElement> lGrad = do_QueryInterface(mContent);
>-  NS_ASSERTION(lGrad, "Wrong content element (not linear gradient)");
>-  if (!lGrad) {
>-    return NS_ERROR_FAILURE;
>-  }
>-  // See if we need to get the value from another gradient
>-  if (checkURITarget(nsGkAtoms::y2)) {
>-    // Yes, get it from the target
>-    nsISVGGradient *nextGrad;
>-    if (GetNextGradient(&nextGrad,
>-                        nsISVGGradient::SVG_LINEAR_GRADIENT) == NS_OK) {
>-      nsSVGLinearGradientFrame *lNgrad =
>-        (nsSVGLinearGradientFrame *)nextGrad;
>-      lNgrad->GetY2(aY2);
>-      mLoopFlag = PR_FALSE;
>-      return NS_OK;
>-    }
>-    // There are no gradients in the list with our type -- fall through
>-    // and return our default value
>-  }
>+  nsIContent *gradient = GetLinearGradientWithAttr(nsGkAtoms::y2);
>+  if (!gradient)
>+    gradient = mContent;  // use our gradient to get the correct default value
> 
>-  // No, return the values
>+  nsCOMPtr<nsIDOMSVGLinearGradientElement> lGrad = do_QueryInterface(gradient);
>   nsCOMPtr<nsIDOMSVGAnimatedLength> animLength;
>   lGrad->GetY2(getter_AddRefs(animLength));
>   nsCOMPtr<nsIDOMSVGLength> length;
>   animLength->GetAnimVal(getter_AddRefs(length));
> 
>-  PRUint16 bbox;
>-  GetGradientUnits(&bbox);
>-  if (bbox == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX) {
>-    length->GetValue(aY2);
>-  } else {
>+  // Object bounding box units are handled by setting the appropriate
>+  // transform in GetGradientTransfrom, but we need to handle user
>+  // space units as part of the individual Get* routines.  Fixes 323669.
>+
>+  PRUint16 gradientUnits = GetGradientUnits();
>+  if (gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_USERSPACEONUSE) {
>     *aY2 = nsSVGUtils::UserSpace(mSourceContent, length, nsSVGUtils::Y);
>+    return NS_OK;
>   }
>-  mLoopFlag = PR_FALSE;
>-  return NS_OK;
>+
>+  NS_ASSERTION(gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX,
>+               "Unknown gradientUnits type");
>+
>+  return length->GetValue(aY2);  // objectBoundingBox is the default anyway
> }
> 
> // -------------------------------------------------------------------------
> // Radial Gradients
> // -------------------------------------------------------------------------
> 
> nsIAtom*
> nsSVGRadialGradientFrame::GetType() const
>@@ -953,224 +951,156 @@ nsSVGRadialGradientFrame::AttributeChang
> NS_INTERFACE_MAP_BEGIN(nsSVGRadialGradientFrame)
>   NS_INTERFACE_MAP_ENTRY(nsISVGRadialGradient)
> NS_INTERFACE_MAP_END_INHERITING(nsSVGRadialGradientFrameBase)
> 
> // nsISVGRadialGradient
> NS_IMETHODIMP
> nsSVGRadialGradientFrame::GetFx(float *aFx)
> {
>-  nsCOMPtr<nsIDOMSVGRadialGradientElement> rGradElement = do_QueryInterface(mContent);
>-  NS_ASSERTION(rGradElement, "Wrong content element (not linear gradient)");
>-  if (!rGradElement) {
>-    return NS_ERROR_FAILURE;
>-  }
>-  // See if we need to get the value from another gradient
>-  if (checkURITarget(nsGkAtoms::fx)) {
>-    // Yes, get it from the target
>-    nsISVGGradient *nextGrad;
>-    if (GetNextGradient(&nextGrad,
>-                        nsISVGGradient::SVG_RADIAL_GRADIENT) == NS_OK) {
>-      nsSVGRadialGradientFrame *rNgrad =
>-        (nsSVGRadialGradientFrame *)nextGrad;
>-      rNgrad->GetFx(aFx);
>-      mLoopFlag = PR_FALSE;
>-      return NS_OK;
>-    }
>-    // There are no gradients in the list with our type -- fall through
>-    // and return our default value
>-  }
>-  // See if the value was explicitly set --  the spec
>-  // states that if there is no explicit fx value, we return the cx value
>-  // see http://www.w3.org/TR/SVG11/pservers.html#RadialGradients
>-  if (!mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::fx))
>-    return GetCx(aFx);
>+  nsIContent *gradient = GetRadialGradientWithAttr(nsGkAtoms::fx);
>+  if (!gradient)
>+    return GetCx(aFx);  // if fx isn't set, we must use cx
> 
>-  // No, return the values
>+  nsCOMPtr<nsIDOMSVGRadialGradientElement> rGradElement = do_QueryInterface(gradient);
>   nsCOMPtr<nsIDOMSVGAnimatedLength> animLength;
>   rGradElement->GetFx(getter_AddRefs(animLength));
>   nsCOMPtr<nsIDOMSVGLength> length;
>   animLength->GetAnimVal(getter_AddRefs(length));
> 
>-  PRUint16 bbox;
>-  GetGradientUnits(&bbox);
>-  if (bbox == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX) {
>-    length->GetValue(aFx);
>-  } else {
>+  // Object bounding box units are handled by setting the appropriate
>+  // transform in GetGradientTransfrom, but we need to handle user
>+  // space units as part of the individual Get* routines.  Fixes 323669.
>+
>+  PRUint16 gradientUnits = GetGradientUnits();
>+  if (gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_USERSPACEONUSE) {
>     *aFx = nsSVGUtils::UserSpace(mSourceContent, length, nsSVGUtils::X);
>+    return NS_OK;
>   }
>-  mLoopFlag = PR_FALSE;
>-  return NS_OK;
>+
>+  NS_ASSERTION(gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX,
>+               "Unknown gradientUnits type");
>+
>+  return length->GetValue(aFx);  // objectBoundingBox is the default anyway
> }
> 
> NS_IMETHODIMP
> nsSVGRadialGradientFrame::GetFy(float *aFy)
> {
>-  nsCOMPtr<nsIDOMSVGRadialGradientElement> rGradElement = do_QueryInterface(mContent);
>-  NS_ASSERTION(rGradElement, "Wrong content element (not linear gradient)");
>-  if (!rGradElement) {
>-    return NS_ERROR_FAILURE;
>-  }
>-  // See if we need to get the value from another gradient
>-  if (checkURITarget(nsGkAtoms::fy)) {
>-    // Yes, get it from the target
>-    nsISVGGradient *nextGrad;
>-    if (GetNextGradient(&nextGrad,
>-                        nsISVGGradient::SVG_RADIAL_GRADIENT) == NS_OK) {
>-      nsSVGRadialGradientFrame *rNgrad =
>-        (nsSVGRadialGradientFrame *)nextGrad;
>-      rNgrad->GetFy(aFy);
>-      mLoopFlag = PR_FALSE;
>-      return NS_OK;
>-    }
>-    // There are no gradients in the list with our type -- fall through
>-    // and return our default value
>-  }
>-  // See if the value was explicitly set --  the spec
>-  // states that if there is no explicit fx value, we return the cx value
>-  // see http://www.w3.org/TR/SVG11/pservers.html#RadialGradients
>-  if (!mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::fy))
>-    return GetCy(aFy);
>+  nsIContent *gradient = GetRadialGradientWithAttr(nsGkAtoms::fy);
>+  if (!gradient)
>+    return GetCy(aFy);  // if fy isn't set, we must use cy
> 
>-  // No, return the values
>+  nsCOMPtr<nsIDOMSVGRadialGradientElement> rGradElement = do_QueryInterface(gradient);
>   nsCOMPtr<nsIDOMSVGAnimatedLength> animLength;
>   rGradElement->GetFy(getter_AddRefs(animLength));
>   nsCOMPtr<nsIDOMSVGLength> length;
>   animLength->GetAnimVal(getter_AddRefs(length));
> 
>-  PRUint16 bbox;
>-  GetGradientUnits(&bbox);
>-  if (bbox == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX) {
>-    length->GetValue(aFy);
>-  } else {
>+  // Object bounding box units are handled by setting the appropriate
>+  // transform in GetGradientTransfrom, but we need to handle user
>+  // space units as part of the individual Get* routines.  Fixes 323669.
>+
>+  PRUint16 gradientUnits = GetGradientUnits();
>+  if (gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_USERSPACEONUSE) {
>     *aFy = nsSVGUtils::UserSpace(mSourceContent, length, nsSVGUtils::Y);
>+    return NS_OK;
>   }
>-  mLoopFlag = PR_FALSE;
>-  return NS_OK;
>+
>+  NS_ASSERTION(gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX,
>+               "Unknown gradientUnits type");
>+
>+  return length->GetValue(aFy);  // objectBoundingBox is the default anyway
> }
> 
> NS_IMETHODIMP
> nsSVGRadialGradientFrame::GetCx(float *aCx)
> {
>-  nsCOMPtr<nsIDOMSVGRadialGradientElement> rGradElement = do_QueryInterface(mContent);
>-  NS_ASSERTION(rGradElement, "Wrong content element (not linear gradient)");
>-  if (!rGradElement) {
>-    return NS_ERROR_FAILURE;
>-  }
>-  // See if we need to get the value from another gradient
>-  if (checkURITarget(nsGkAtoms::cx)) {
>-    // Yes, get it from the target
>-    nsISVGGradient *nextGrad;
>-    if (GetNextGradient(&nextGrad,
>-                        nsISVGGradient::SVG_RADIAL_GRADIENT) == NS_OK) {
>-      nsSVGRadialGradientFrame *rNgrad =
>-        (nsSVGRadialGradientFrame *)nextGrad;
>-      rNgrad->GetCx(aCx);
>-      mLoopFlag = PR_FALSE;
>-      return NS_OK;
>-    }
>-    // There are no gradients in the list with our type -- fall through
>-    // and return our default value
>-  }
>+  nsIContent *gradient = GetRadialGradientWithAttr(nsGkAtoms::cx);
>+  if (!gradient)
>+    gradient = mContent;  // use our gradient to get the correct default value
> 
>-  // No, return the values
>+  nsCOMPtr<nsIDOMSVGRadialGradientElement> rGradElement = do_QueryInterface(gradient);
>   nsCOMPtr<nsIDOMSVGAnimatedLength> animLength;
>   rGradElement->GetCx(getter_AddRefs(animLength));
>   nsCOMPtr<nsIDOMSVGLength> length;
>   animLength->GetAnimVal(getter_AddRefs(length));
> 
>-  PRUint16 bbox;
>-  GetGradientUnits(&bbox);
>-  if (bbox == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX) {
>-    length->GetValue(aCx);
>-  } else {
>+  // Object bounding box units are handled by setting the appropriate
>+  // transform in GetGradientTransfrom, but we need to handle user
>+  // space units as part of the individual Get* routines.  Fixes 323669.
>+
>+  PRUint16 gradientUnits = GetGradientUnits();
>+  if (gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_USERSPACEONUSE) {
>     *aCx = nsSVGUtils::UserSpace(mSourceContent, length, nsSVGUtils::X);
>+    return NS_OK;
>   }
>-  mLoopFlag = PR_FALSE;
>-  return NS_OK;
>+
>+  NS_ASSERTION(gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX,
>+               "Unknown gradientUnits type");
>+
>+  return length->GetValue(aCx);  // objectBoundingBox is the default anyway
> }
> 
> NS_IMETHODIMP
> nsSVGRadialGradientFrame::GetCy(float *aCy)
> {
>-  nsCOMPtr<nsIDOMSVGRadialGradientElement> rGradElement = do_QueryInterface(mContent);
>-  NS_ASSERTION(rGradElement, "Wrong content element (not linear gradient)");
>-  if (!rGradElement) {
>-    return NS_ERROR_FAILURE;
>-  }
>-  // See if we need to get the value from another gradient
>-  if (checkURITarget(nsGkAtoms::cy)) {
>-    // Yes, get it from the target
>-    nsISVGGradient *nextGrad;
>-    if (GetNextGradient(&nextGrad,
>-                        nsISVGGradient::SVG_RADIAL_GRADIENT) == NS_OK) {
>-      nsSVGRadialGradientFrame *rNgrad =
>-        (nsSVGRadialGradientFrame *)nextGrad;
>-      rNgrad->GetCy(aCy);
>-      mLoopFlag = PR_FALSE;
>-      return NS_OK;
>-    }
>-    // There are no gradients in the list with our type -- fall through
>-    // and return our default value
>-  }
>-  // No, return the values
>+  nsIContent *gradient = GetRadialGradientWithAttr(nsGkAtoms::cy);
>+  if (!gradient)
>+    gradient = mContent;  // use our gradient to get the correct default value
>+
>+  nsCOMPtr<nsIDOMSVGRadialGradientElement> rGradElement = do_QueryInterface(gradient);
>   nsCOMPtr<nsIDOMSVGAnimatedLength> animLength;
>   rGradElement->GetCy(getter_AddRefs(animLength));
>   nsCOMPtr<nsIDOMSVGLength> length;
>   animLength->GetAnimVal(getter_AddRefs(length));
> 
>-  PRUint16 bbox;
>-  GetGradientUnits(&bbox);
>-  if (bbox == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX) {
>-    length->GetValue(aCy);
>-  } else {
>+  // Object bounding box units are handled by setting the appropriate
>+  // transform in GetGradientTransfrom, but we need to handle user
>+  // space units as part of the individual Get* routines.  Fixes 323669.
>+
>+  PRUint16 gradientUnits = GetGradientUnits();
>+  if (gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_USERSPACEONUSE) {
>     *aCy = nsSVGUtils::UserSpace(mSourceContent, length, nsSVGUtils::Y);
>+    return NS_OK;
>   }
>-  mLoopFlag = PR_FALSE;
>-  return NS_OK;
>+
>+  NS_ASSERTION(gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX,
>+               "Unknown gradientUnits type");
>+
>+  return length->GetValue(aCy);  // objectBoundingBox is the default anyway
> }
> 
> NS_IMETHODIMP
> nsSVGRadialGradientFrame::GetR(float *aR)
> {
>-  nsCOMPtr<nsIDOMSVGRadialGradientElement> rGradElement = do_QueryInterface(mContent);
>-  NS_ASSERTION(rGradElement, "Wrong content element (not linear gradient)");
>-  if (!rGradElement) {
>-    return NS_ERROR_FAILURE;
>-  }
>-  // See if we need to get the value from another gradient
>-  if (checkURITarget(nsGkAtoms::r)) {
>-    // Yes, get it from the target
>-    nsISVGGradient *nextGrad;
>-    if (GetNextGradient(&nextGrad,
>-                        nsISVGGradient::SVG_RADIAL_GRADIENT) == NS_OK) {
>-      nsSVGRadialGradientFrame *rNgrad =
>-        (nsSVGRadialGradientFrame *)nextGrad;
>-      rNgrad->GetR(aR);
>-      mLoopFlag = PR_FALSE;
>-      return NS_OK;
>-    }
>-    // There are no gradients in the list with our type -- fall through
>-    // and return our default value
>-  }
>-  // No, return the values
>+  nsIContent *gradient = GetRadialGradientWithAttr(nsGkAtoms::r);
>+  if (!gradient)
>+    gradient = mContent;  // use our gradient to get the correct default value
>+
>+  nsCOMPtr<nsIDOMSVGRadialGradientElement> rGradElement = do_QueryInterface(gradient);
>   nsCOMPtr<nsIDOMSVGAnimatedLength> animLength;
>   rGradElement->GetR(getter_AddRefs(animLength));
>   nsCOMPtr<nsIDOMSVGLength> length;
>   animLength->GetAnimVal(getter_AddRefs(length));
> 
>-  PRUint16 bbox;
>-  GetGradientUnits(&bbox);
>-  if (bbox == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX) {
>-    length->GetValue(aR);
>-  } else {
>+  // Object bounding box units are handled by setting the appropriate
>+  // transform in GetGradientTransfrom, but we need to handle user
>+  // space units as part of the individual Get* routines.  Fixes 323669.
>+
>+  PRUint16 gradientUnits = GetGradientUnits();
>+  if (gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_USERSPACEONUSE) {
>     *aR = nsSVGUtils::UserSpace(mSourceContent, length, nsSVGUtils::XY);
>+    return NS_OK;
>   }
>-  mLoopFlag = PR_FALSE;
>-  return NS_OK;
>+
>+  NS_ASSERTION(gradientUnits == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX,
>+               "Unknown gradientUnits type");
>+
>+  return length->GetValue(aR);  // objectBoundingBox is the default anyway
> }
> 
> // -------------------------------------------------------------------------
> // Public functions
> // -------------------------------------------------------------------------
> 
> nsIFrame* 
> NS_NewSVGLinearGradientFrame(nsIPresShell* aPresShell, 
>@@ -1183,21 +1113,22 @@ NS_NewSVGLinearGradientFrame(nsIPresShel
>   
>   nsSVGLinearGradientFrame* it = new (aPresShell) nsSVGLinearGradientFrame;
>   if (nsnull == it)
>     return nsnull;
> 
>   nsCOMPtr<nsIDOMSVGURIReference> aRef = do_QueryInterface(aContent);
>   NS_ASSERTION(aRef, "NS_NewSVGLinearGradientFrame -- Content doesn't support nsIDOMSVGURIReference");
>   if (aRef) {
>-    // Get the hRef
>+    // Get the href
>     aRef->GetHref(getter_AddRefs(it->mHref));
>   }
>   it->mNextGrad = nsnull;
>   it->mLoopFlag = PR_FALSE;
>+  it->mInitialized = PR_FALSE;
>   return it;
> }
> 
> nsIFrame*
> NS_NewSVGRadialGradientFrame(nsIPresShell* aPresShell, 
>                              nsIContent*   aContent)
> {
>   nsCOMPtr<nsIDOMSVGRadialGradientElement> grad = do_QueryInterface(aContent);
>@@ -1207,21 +1138,22 @@ NS_NewSVGRadialGradientFrame(nsIPresShel
>   
>   nsSVGRadialGradientFrame* it = new (aPresShell) nsSVGRadialGradientFrame;
>   if (nsnull == it)
>     return nsnull;
> 
>   nsCOMPtr<nsIDOMSVGURIReference> aRef = do_QueryInterface(aContent);
>   NS_ASSERTION(aRef, "NS_NewSVGRadialGradientFrame -- Content doesn't support nsIDOMSVGURIReference");
>   if (aRef) {
>-    // Get the hRef
>+    // Get the href
>     aRef->GetHref(getter_AddRefs(it->mHref));
>   }
>   it->mNextGrad = nsnull;
>   it->mLoopFlag = PR_FALSE;
>+  it->mInitialized = PR_FALSE;
>   return it;
> }
> 
> // Public function to locate the SVGGradientFrame structure pointed to by a URI
> // and return a nsISVGGradient
> nsresult NS_GetSVGGradient(nsISVGGradient **aGrad, nsIURI *aURI, nsIContent *aContent, 
>                            nsIPresShell *aPresShell)
> {
>Index: mozilla/layout/svg/renderer/public/nsISVGGradient.idl
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/renderer/public/nsISVGGradient.idl,v
>retrieving revision 1.4
>diff -u -8 -p -r1.4 nsISVGGradient.idl
>--- mozilla/layout/svg/renderer/public/nsISVGGradient.idl	16 Sep 2005 22:07:24 -0000	1.4
>+++ mozilla/layout/svg/renderer/public/nsISVGGradient.idl	17 Mar 2006 11:59:45 -0000
>@@ -57,32 +57,30 @@ interface nsISVGGeometrySource;
>  */
> 
> /**
>  * Describes the 'gradient' objects (either linear or a radial) to the
>  * rendering backends.
>  *
>  * @nosubgrouping
>  */
>-[uuid(62e79ab2-5bf9-4372-b397-7a942bc4c649)]
>+[uuid(0f43d022-03b9-4d0f-81b2-14881ac11831)]
> interface nsISVGGradient : nsISupports
> {
>   const unsigned long SVG_UNKNOWN_GRADIENT = 0;
>   const unsigned long SVG_LINEAR_GRADIENT = 1;
>   const unsigned long SVG_RADIAL_GRADIENT = 2;
> 
>   readonly attribute PRUint32 gradientType; 
>-  readonly attribute PRUint16 gradientUnits;
>   readonly attribute PRUint16 spreadMethod;
> 
>   void GetStopCount(out PRUint32 aStopCount);
>   void GetStopOffset(in PRInt32 aIndex, out float aOffset);
>   void GetStopColor(in PRInt32 aIndex, out nscolor aStopColor);
>   void GetStopOpacity(in PRInt32 aIndex, out float aStopOpacity);
>-  void GetNextGradient(out nsISVGGradient aNextGrad, in PRUint32 aType);
>   void GetGradientTransform(out nsIDOMSVGMatrix retval,
>                             in nsISVGGeometrySource aSource);
>   /** @} */
> };
> 
> [uuid(995ad9e6-6bb1-47c5-b402-fc93ce12f5e7)]
> interface nsISVGLinearGradient : nsISupports
> {

This should be deleted

>Index: mozilla/layout/svg/renderer/src/gdiplus/nsSVGGDIPlusGradient.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/renderer/src/gdiplus/nsSVGGDIPlusGradient.cpp,v
>retrieving revision 1.6
>diff -u -8 -p -r1.6 nsSVGGDIPlusGradient.cpp
>--- mozilla/layout/svg/renderer/src/gdiplus/nsSVGGDIPlusGradient.cpp	30 Sep 2005 19:19:46 -0000	1.6
>+++ mozilla/layout/svg/renderer/src/gdiplus/nsSVGGDIPlusGradient.cpp	17 Mar 2006 11:59:45 -0000
>@@ -315,20 +315,16 @@ GDIPlusGradient(nsISVGGDIPlusRegion *aRe
>                 Graphics *aGFX,
>                 nsISVGGeometrySource *aSource,
>                 void(*aCallback)(Graphics *, Brush*, void *), void *aData)
> {
>   NS_ASSERTION(aGrad, "Called GDIPlusGradient without a gradient!");
>   if (!aGrad)
>     return;
> 
>-  // Get the gradientUnits
>-  PRUint16 bbox;
>-  aGrad->GetGradientUnits(&bbox);
>-
>   // Get the transform list (if there is one)
>   nsCOMPtr<nsIDOMSVGMatrix> svgMatrix;
>   aGrad->GetGradientTransform(getter_AddRefs(svgMatrix), aSource);
>   NS_ASSERTION(svgMatrix, "GDIPlusGradient: GetGradientTransform returns null");
> 
>   // GDI+ gradients don't like having a zero on the diagonal (zero
>   // width or height in the bounding box)
>   float val;

Comment 22

13 years ago
Oops sorry about leaving all the diffs in there -- I meant to go back and strip things down, but I hit submit too soon!  :-(
(Assignee)

Comment 23

13 years ago
Created attachment 215941 [details] [diff] [review]
address scooter's review comments

Ouch! ;-)

Comments addressed with some exceptions below.

(In reply to comment #21)
> For now, we may just want to do a DidModify(mod_other) above?

Agreed. Done.

> With all this surgery, maybe now is the time (and Gradients are the venue)
> for figuring out how to write to the JS console.  I'm uncomfortable with
> just returning after we've detected a loop without putting up some kind
> of warning to the user.

I've got higher priority stuff I'm working on, and that's not really in the scope of this bug. I'm sending a warning to the console, so we'll be no worse off than we are just now. Perhaps we can have a big "console warnings to js errors" bug for that?

> I know why you have "inherit" in quotes, and what you mean, but when you
>  first introduce it, it might be useful to provide a more detailed
> explanation -- this is *not* inheritance in the way that CSS or other
> attributes work.  Maybe just use "get from referenced gradient"?

I thought I had already done that up in the comment in the class declaration starting with:
  // The SVG specification allows gradient elements to reference...

> You could certainly cache the animValue, but wouldn't you have to observe
> it, then? And if you cache the result, how would you know if it was
> modified via the DOM?

I'm not sure. I put in that comment to remind us that it's something we could look at in the future.
Attachment #215394 - Attachment is obsolete: true
Attachment #215941 - Flags: review?(scootermorris)
Attachment #215394 - Flags: review?(scootermorris)
(In reply to comment #23)
> Perhaps we can have a big "console warnings to js errors" bug for that?

See bug 329026.

Comment 25

13 years ago
(In reply to comment #23)
> 
> (In reply to comment #21)
> > For now, we may just want to do a DidModify(mod_other) above?
> 
> Agreed. Done.
> 
> > With all this surgery, maybe now is the time (and Gradients are the venue)
> > for figuring out how to write to the JS console.  I'm uncomfortable with
> > just returning after we've detected a loop without putting up some kind
> > of warning to the user.
> 
> I've got higher priority stuff I'm working on, and that's not really in the
> scope of this bug. I'm sending a warning to the console, so we'll be no worse
> off than we are just now. Perhaps we can have a big "console warnings to js
> errors" bug for that?

Actually, I can't find where you're sending a warning to the console, so maybe I'm missing something in the code.  I'm fine with putting this off into a separate bug, but I think that we should at least have an NS_WARNING when we detect a loop.

> 
> > I know why you have "inherit" in quotes, and what you mean, but when you
> >  first introduce it, it might be useful to provide a more detailed
> > explanation -- this is *not* inheritance in the way that CSS or other
> > attributes work.  Maybe just use "get from referenced gradient"?
> 
> I thought I had already done that up in the comment in the class declaration
> starting with:
>   // The SVG specification allows gradient elements to reference...

OK

> 
> > You could certainly cache the animValue, but wouldn't you have to observe
> > it, then? And if you cache the result, how would you know if it was
> > modified via the DOM?
> 
> I'm not sure. I put in that comment to remind us that it's something we could
> look at in the future.
> 

That's fine.  Just add the appropriate NS_WARNING and I think this will be good.

(Assignee)

Comment 26

13 years ago
Created attachment 216272 [details] [diff] [review]
add warnings about loops

Hmm, could have sworn I had warnings in there. Maybe I removed them at some point. Sorry about that.
Attachment #215941 - Attachment is obsolete: true
Attachment #216272 - Flags: superreview?(tor)
Attachment #216272 - Flags: review?(scootermorris)
Attachment #215941 - Flags: review?(scootermorris)

Comment 27

13 years ago
Comment on attachment 216272 [details] [diff] [review]
add warnings about loops

Looks fine, now.  BTW, roc's checkin to bug 330934 has moved the tree a little out from underneath you.
Attachment #216272 - Flags: review?(scootermorris) → review+
(Assignee)

Comment 28

13 years ago
Thanks. I forgot to add my name to the contributors. Mind if I do that?

Comment 29

13 years ago
(In reply to comment #28)
> Thanks. I forgot to add my name to the contributors. Mind if I do that?
> 

Nope, feel free.

Updated

13 years ago
Attachment #216272 - Flags: superreview?(tor) → superreview+
(Assignee)

Comment 30

13 years ago
thanks, checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 31

13 years ago
Perf wasn't the main focus of this bug, but in case anyone's interested, the mean of the six times for the SVG test in Tr (gearflowers) prior to this checkin was 782.408 ms and the mean of the six after was  776.051 ms. This would suggest we got about 6.357 ms faster, or slightly less than a 1% improvement. Not too bad considering most of the gradient grunt work happens in cairo land.
You need to log in before you can comment on or make changes to this bug.