276316 - Gradients should be "live"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Scooter Morris]

The current gradient implementation does not respond properly to changes in
attributes.

Comment 1

[Scooter Morris]

Attachment: Work in progress -- not ready for review

This is just a work-in-progress patch.	It should be getting close, but I don't
think that this will correctly handle inserted gradients in a reference chain,
nor inserted attributes in the chain.  It also turns out that nsSVGGlyphFrame
is not an SVGObserver, which will have to change for all of this to work
properly.

Comment 2

[Scooter Morris]

Attachment: Ready for review

This is getting pretty close, so I'd appreciate a review.  The missing piece is
that nsSVGGlyphFrame does not currently support nsISVGValueObserver, which is
required for the notification mechanism between Gradients and the users of
those gradients.

Comment 3

[Scooter Morris]

Attachment: Slight modification

I found a race condition in testing where a PathGeometryFrame would sometimes
try to get a gradient before it had its content.  I catch that now and the
crash is gone.

Comment 4

[Jonathan Watt [:jwatt]]

Okay, after a first skim of this patch my first comment is that it also contains
your patch for bug 273740. Maybe it's better if the patch to bug 274886 gets
reviewed and checked in first? Then the patch for bug 273740 can get checked in
and you can rediff to get the changes for this bug only.

Comment 5

[Scooter Morris]

(In reply to comment #4)
> Okay, after a first skim of this patch my first comment is that it also contains
> your patch for bug 273740. Maybe it's better if the patch to bug 274886 gets
> reviewed and checked in first? Then the patch for bug 273740 can get checked in
> and you can rediff to get the changes for this bug only.

OK by me.

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 170793 [details] [diff] [review]
Slight modification

In nsSVGGradientFrame.cpp there is one occurance of:

  nsCOMPtr<nsIDOMSVGTransformList> lTrans;

and eight occurances of:

  nsCOMPtr<nsIDOMSVGLength> sLen;

that you forgot to delete.

Comment 7

[Scooter Morris]

Attachment: Diffs for making gradients "live"

Here are the diffs for making gradients live.  This includes modifications to
nsSVGGlyphFrame.cpp and nsSVGPathGeometryFrame.cpp to call the new gradient
interface and set themselves as observers.

Comment 8

[Scooter Morris]

Attachment: Code for the new nsSVGStopFrame.cpp

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 171793 [details] [diff] [review]
Diffs for making gradients "live"

From another very quick skim:

1) change NULL to nsnull
2) change !NS_FAILED(rv) to NS_SUCCEEDED(rv)

more later

Comment 10

[tor]

Comment on attachment 171793 [details] [diff] [review]
Diffs for making gradients "live"

Now that you're making gradients live, shouldn't the href be hooked up
with the normal observer mechanism so that changes to it will cause
updates?

You're not using the value of mChangedFlag, so why the Get?

Use NS_{ADD,REMOVE}_SVGVALUE_OBSERVER.

Why the split between the GetFoo and PrivGetFoo methods?  Is it to
avoid adding observers down the chain?	If so, wouldn't these
intermediate gradients need the observers if they are being used?

I'd prefer that the PrivFoo methods be named PrivateFoo.

It seems the change to nsSVGPathGeometryFrame.h isn't used.

Why did you move the gradient frame interfaces to the header?  They
don't appear to be used by any other files.

The memory leak in GetStopElement need to be fixed, as discussed on
IRC.

Comment 11

[Scooter Morris]

(In reply to comment #10)
> (From update of attachment 171793 [details] [diff] [review] [edit])
> Now that you're making gradients live, shouldn't the href be hooked up
> with the normal observer mechanism so that changes to it will cause
> updates?
There doesn't seem to be a reason to.  If I change the href, the gradient
already "notices it" and repaints.
> 
> You're not using the value of mChangedFlag, so why the Get?
Good point.
> 
> Use NS_{ADD,REMOVE}_SVGVALUE_OBSERVER.
OK
> 
> Why the split between the GetFoo and PrivGetFoo methods?  Is it to
> avoid adding observers down the chain?	If so, wouldn't these
> intermediate gradients need the observers if they are being used?
Maybe, but they may not be used (they might only be intermediates)
> 
> I'd prefer that the PrivFoo methods be named PrivateFoo.
OK
> 
> It seems the change to nsSVGPathGeometryFrame.h isn't used.
Hmmm...OK, could be vestigal
> 
> Why did you move the gradient frame interfaces to the header?  They
> don't appear to be used by any other files.
Mostly because I wasn't sure where things were headed initially, and it seemed a
little cleaner.  I can merge them back if someone really wants me to.
> 
> The memory leak in GetStopElement need to be fixed, as discussed on
> IRC.
> 
Absolutely.

Comment 12

[Scooter Morris]

Attachment: Updated patch

Here is an updated patch that incorporates the fix for the leaking of
nsGenericElements and tor's and jwatt's comments.

Comment 13

[Scooter Morris]

Attachment: Sample file (change href)

Comment 14

[Scooter Morris]

Attachment: Sample file (change attributes)

Comment 15

[Scooter Morris]

Attachment: Sample file (change color)

Comment 16

[tor]

At a quick glance, you're not using NS_REMOVE_SVGVALUE_OBSERVER and
when using NS_ADD_SVGVALUE_OBSERVER you're duplicating the
do_QueryInterface.

Comment 17

[Scooter Morris]

Attachment: Changes to existing files to make gradients "live"

Sorry, I wasn't very careful with the last patch.  I think this is better.

Comment 18

[tor]

Comment on attachment 172394 [details] [diff] [review]
Changes to existing files to make gradients "live"

NS_GetSVGGradient - adds observer each time called (ex: each paint).
Should do something to limit that - maybe make nsSVGValue::AddObserver
check for duplicates?  Might be something more intelligent we can do.

DidModifySVGObservable, DidSetStyleContext - just pass 0.0f to
SetValue, no need for a local.

AddObserver - why not use NS_ADD_SVGVALUE_OBSERVER for the end?
Related to that, the return value is never checked (ok?), so you can
make the method NS_IMETHOD_(void)

DidObserver - never called.  If not an error, remove.

NS_GetSVGGradient - 4-arg version not used anymore.  remove.

Missing whitespace:
  * NS_GetSVGGradient calls - space before "this"
  * GetStopElement - space between ">" and "stopElement"

Odd trailing semicolons after methods:
  * nsSVGGradientFrame::AddObserver
		      ::RemoveObserver
		      ::GetStopCount
		      ::GetStopOpacity
		      ::GetGradientUnits
		      ::GetGradientTransform
		      ::GetSpreadMethod
		      ::AddObserver
  * nsSVGLinearGradientFrame::GetX1
			    ::GetY1
			    ::GetX2
			    ::GetY2
  * nsSVGRadialGradientFrame::GetFx
			    ::GetFy
			    ::GetCx
			    ::GetCy
			    ::GetR

Comment 19

[Scooter Morris]

Attachment: Reworked to make gradients inherit from SVGValue

This is the reworked patch that has gradients inheriting from SVGValue.

Comment 20

[Alex Fritze]

Attachment: clean up nsISVGGeometrySource.idl

nsISVGGeometrySource.idl (+ the other backend interfaces) should really be
cleaned up a bit more.
* 'void GetFillGradient(out nsISVGGradient)' should really be  
'nsISVGGradient getFillGradient()', or better 
'readonly attribute nsISVGGradient fillGradient'.
* all interfaces should be non-scriptable by default.
* some of the new methods should be documented (e.g. what is IsClipChild()??)

I guess that's a different bug though...

Comment 21

[Scooter Morris]

Attachment: Slightly updated to refine update masks

Comment 22

[Alex Fritze]

Comment on attachment 173291 [details] [diff] [review]
Slightly updated to refine update masks

Some initial comments. More to come later.

>Index: layout/svg/base/src/nsSVGGlyphFrame.cpp

>   NS_IMETHOD  CharacterDataChanged(nsPresContext* aPresContext,
>                                    nsIContent*     aChild,
>-                                   PRBool          aAppend);
>+                                   PRBool          aAppend,
>+                                   PRUint32        aFlags);

This is an overridden nsIFrame method; you need to leave the signature as it
is, or dynamic text updates will break.


>     : mGeometryUpdateFlags(0), mMetricsUpdateFlags(0),
>-      mCharOffset(0), mFragmentTreeDirty(PR_FALSE)
>+      mCharOffset(0), mFragmentTreeDirty(PR_FALSE),
>+      mFillGradient(nsnull), mStrokeGradient(nsnull)

For nsCOMPtrs you don't need this. They are automatically initialized to
nsnull.



> NS_IMETHODIMP
> nsSVGGlyphFrame::CharacterDataChanged(nsPresContext* aPresContext,
>                                       nsIContent*     aChild,
>-                                      PRBool          aAppend)
>+                                      PRBool          aAppend,
>+                                      PRUint32        aFlags)

See above.

>+{
>+  // Is this a gradient?
>+  nsCOMPtr<nsISVGGradient>val = do_QueryInterface(observable);
>+  if (val) {
>+    // Yes, we need to handle this differently
>+    nsCOMPtr<nsISVGGradient>fill = do_QueryInterface(mFillGradient);
>+    if (fill == val) {
>+      if (aModType == nsISVGValue::mod_die) {
>+        mFillGradient = nsnull;
>+      }
>+      CharacterDataChanged(nsnull,nsnull,PR_FALSE,nsISVGGeometrySource::UPDATEMASK_FILL_PAINT);
>+    } else {
>+      // No real harm in assuming a stroke gradient at this point
>+      if (aModType == nsISVGValue::mod_die) {
>+        mStrokeGradient = nsnull;
>+      }
>+      CharacterDataChanged(nsnull,nsnull,PR_FALSE,nsISVGGeometrySource::UPDATEMASK_STROKE_PAINT);
>+    }

Either you need to bite the bullet and call the original CharacterDataChanged()
for all cases, or write a new method that does the same as CharacterDataChanged
but takes flags as an arg. (This method could then also be called from
CharacterDataChanged & InitialUpdate).

Comment 23

[Alex Fritze]

Comment on attachment 171794 [details] [diff] [review]
Code for the new nsSVGStopFrame.cpp

Scooter: Is there any reason why nsSVGStopFrame/nsSVGGradientFrame should
inherit from nsSVGDefsFrame?

Comment 24

[Scooter Morris]

(In reply to comment #23)
> (From update of attachment 171794 [details] [diff] [review] [edit])
> Scooter: Is there any reason why nsSVGStopFrame/nsSVGGradientFrame should
> inherit from nsSVGDefsFrame?
> 

Not particularly, I think I used DefsFrame because they must be children of the
DefsContent, which is not a really good reason.  Got a better candidate?

Comment 25

[Scooter Morris]

(In reply to comment #22)
> (From update of attachment 173291 [details] [diff] [review] [edit])
> Some initial comments. More to come later.
....
> 
> Either you need to bite the bullet and call the original CharacterDataChanged()
> for all cases, or write a new method that does the same as CharacterDataChanged
> but takes flags as an arg. (This method could then also be called from
> CharacterDataChanged & InitialUpdate).
> 
> 

I think we should opt for the latter to gain the efficiency.  It could be just
me (I don't have any objective data on this), but when I switched things over in
nsSVGPathGeometryFrame to only update the fill, I noticed a significant
performance improvement when I changed the attributes using the test cases
posted in this bug.  I would like to make sure to carry that over to the Glyph
fills.  After I get your other comments, I'll submit a patch with a new method
that reimplements CharacterDataChanged with the additional argument.

Comment 26

[Alex Fritze]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 171794 [details] [diff] [review] [edit] [edit])
> > Scooter: Is there any reason why nsSVGStopFrame/nsSVGGradientFrame should
> > inherit from nsSVGDefsFrame?
> > 
> 
> Not particularly, I think I used DefsFrame because they must be children of the
> DefsContent, which is not a really good reason.  Got a better candidate?

Yeah, nsContainerFrame. nsSVGDefsFrame contains a lot of stuff that isn't really
applicable to stop frames or gradient frames, such as e.g. a member 'mCanvasTM'.
(Actually I'm not sure the latter is ever needed for <defs> either, but that's a
different issue).

Comment 27

[Alex Fritze]

Comment on attachment 173291 [details] [diff] [review]
Slightly updated to refine update masks

>Index: layout/svg/base/src/nsSVGGradientFrame.cpp
>===================================================================

>+
>+NS_IMETHODIMP
>+nsSVGGradientFrame::DidSetStyleContext(nsPresContext* aPresContext)
>+{
>+  WillModify(mod_context);
>+  DidModify(mod_context);
>+  return NS_OK;
>+}

'mod_context' is wrong here. It should be 'mod_other' (i.e. no argument). 
'mod_context' is used to inform observers on svglengths that the context in 
which the length is to be measured has changed and not the actual length.

>+PRInt32 
>+nsSVGGradientFrame::GetStopElement(PRInt32 aIndex, nsIDOMSVGStopElement * *aStopElement,
>+                                   nsIFrame * *aStopFrame)

Conventionally functions that return an (XP-)COM pointer via an out-parameter
are expected to add-ref the pointer. You don't do that here for
nsIDOMSVGStopElement (it doesn't matter for nsIFrame, because frames are not
ref-counted), so I think you should at least have a comment about the
behaviour. Something like: "// returns non-addrefed stop element."

>Index: layout/svg/base/src/nsSVGGradientFrame.h
>===================================================================

What's the rationale behind making the class definitions of nsSVG*GradientFrame
public?

Comment 28

[Scooter Morris]

Attachment: Update of nsSVGStopFrame.cpp to inherit from nsContainerFrame

Comment 29

[Scooter Morris]

Attachment: Updated patch with Alex's comments incorporated

Comment 30

[Scooter Morris]

Attachment: Refreshed after patch for gradientTransforms landed

Comment 31

[Alex Fritze]

Comment on attachment 173770 [details] [diff] [review]
Update of nsSVGStopFrame.cpp to inherit from nsContainerFrame

>--- nnn/layout/svg/base/src/nsSVGStopFrame.cpp	1969-12-31 16:00:00.000000000 -0800
[...]
>+class nsSVGStopFrame : public nsSVGStopFrameBase,
>+                       public nsISVGValueObserver

No need for implementing nsISVGValueObserver, since you're not actually
observing any values...

>+public:
>+   // nsISupports interface:
>+  NS_IMETHOD QueryInterface(const nsIID& aIID, void** aInstancePtr);
>+private:
>+  NS_IMETHOD_(nsrefcnt) AddRef() { return NS_OK; }
>+  NS_IMETHOD_(nsrefcnt) Release() { return NS_OK; }  

... and since you're not implementing any new interfaces the nsISupports stuff
can go as well.

Comment 32

[Alex Fritze]

Comment on attachment 175620 [details] [diff] [review]
Refreshed after patch for gradientTransforms landed

r=afri

>Index: layout/svg/base/src/nsSVGGlyphFrame.cpp
>===================================================================
>+  NS_IMETHOD  UpdateCharacterData(PRUint32 aFlags);

I think UpdateCharacterData is a misleading name for this method since it
neither updates the character data (it updates the text layout, metrics &
glyphs), nor is it only called when the character data changes. How about just
'Update'?

Also, nsISVGValue.h is missing from your most recent patches (don't bother
attaching a new patch - just warning you that the just checking in the layout
changes without nsISVGValue.h will break the build).

Comment 33

[Scooter Morris]

Attachment: Final live gradients diffs -- already r+ by Alex

This is the reviewed patch with some simple nameing changes as recommended by
Alex.

Comment 34

[Scooter Morris]

Attachment: nsSVGStopFrame.cpp -- with Alex's changes

Alex,
   Nice catch!

Comment 35

[Jesse Ruderman]

See also bug 309924, Removing <stop> elements from gradients is not live.