Status

()

RESOLVED FIXED
15 years ago
5 years ago

People

(Reporter: scootermorris, Assigned: scootermorris)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 42 obsolete attachments)

5.62 KB, application/x-compressed-tar
Details
17.87 KB, patch
scootermorris
: review+
Details | Diff | Splinter Review
26.92 KB, patch
alex
: review+
Details | Diff | Splinter Review
15.74 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
26.32 KB, patch
alex
: review+
Details | Diff | Splinter Review
10.13 KB, patch
tor
: review+
Details | Diff | Splinter Review
74.57 KB, patch
tor
: review+
Details | Diff | Splinter Review
51.27 KB, patch
tor
: review+
Details | Diff | Splinter Review
35.30 KB, patch
dbaron
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a2) Gecko/20040526
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a2) Gecko/20040526

The W3C defines support for gradients on both fills and strokes.  The
implementation in Mozilla currently doesn't support gradients.


Reproducible: Always
Steps to Reproduce:
1.
2.
3.
(Assignee)

Comment 1

15 years ago
Created attachment 149561 [details] [diff] [review]
Changes to DOM interfaces to support Gradients

First of 6 patches to support SVG gradients in the Libart renderer
(Assignee)

Comment 2

15 years ago
Created attachment 149562 [details] [diff] [review]
New dom files for SVG Gradient support

Second patch
(Assignee)

Comment 3

15 years ago
Created attachment 149563 [details] [diff] [review]
Changes to content files to support SVG Gradients

Third file
(Assignee)

Comment 4

15 years ago
Created attachment 149564 [details] [diff] [review]
New content files for SVG Gradient support

Fourth patch file
(Assignee)

Comment 5

15 years ago
Created attachment 149565 [details] [diff] [review]
Changes to layout files for SVG Gradient support

Fifth patch for SVG Gradients
(Assignee)

Comment 6

15 years ago
Created attachment 149566 [details] [diff] [review]
New files in layout for SVG Gradient support

Final patch for SVG Gradient support with libart.
(Assignee)

Comment 7

15 years ago
The six patches above should be all that you need to support SVG Gradients using
the Libart renderer.  It does not provide perfect support, but enough to get
people testing it.

Updated

15 years ago
Assignee: alex → scootermorris
(Assignee)

Updated

15 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 8

15 years ago
Could you attach the testcases you used developing this patch?  Thanks.
(Assignee)

Comment 9

15 years ago
Created attachment 149760 [details]
Test Cases (from W3C SVG 1.1 Test) for paint servers

Here are the test cases I used for gradients.  This is the complete W3C SVG 1.1
series of tests for paint servers.  Most of these excersize gradient support. 
One or two use patterns, which are not supported.

Comment 10

15 years ago
Gradients are looking good.  One thing I noticed while testing this and
doing the cairo backend work is that GetStopOpacity is returning garbage
if the stop-opacity wasn't explicity set on a stop.

Comment 11

15 years ago
Created attachment 150017 [details] [diff] [review]
cairo renderer support for gradients

Comment 12

15 years ago
Comment on attachment 149561 [details] [diff] [review]
Changes to DOM interfaces to support Gradients

The dom patches and new dom files look good, but you should remove me & Will
from the contributor lists and insert yourself. Also, in
nsIDOMSVGGradientElement:
// Extended to provide enumerations for gradientUnits

Could you please add that these constants are taken from SVGUnitTypes.

You can ask jst or sicking for r/sr on these 2 patches.

Comment 13

15 years ago
Comments on the content changes:

  Index: content/base/src/nsRuleNode.cpp
  Index: content/html/style/src/nsCSSParser.cpp
  Index: content/html/style/src/nsCSSStruct.cpp
  Index: content/html/style/src/nsCSSStruct.h
  Index: content/shared/public/nsCSSKeywordList.h
  Index: content/shared/public/nsCSSPropList.h
  Index: content/shared/public/nsStyleStruct.h

   struct nsStyleSVGPaint
   {
     nsStyleSVGPaintType mType;
     nscolor mColor;
  +  nsIURI *mPaintServer;
   };

How about converting this to a union:

  struct nsStyleSVGPaint {
    nsStyleSVGPaintType mType;
    union {
      nscolor mColor;
      nsIURI *mPaintServer;
    } mPaint;
  }

Also you need to add a destructor for releasing mPaintServer. It's
probably a good idea to implement operator= (for possible AddRefing)
and operator== as well and use them in nsStyleStruct.cpp. See also
nsStyleContentData.

  Index: content/shared/src/nsStyleStruct.cpp
  +  else if (mFill.mType == eStyleSVGPaintType_Server)
  +    aSource.mFill.mPaintServer->Clone(&mFill.mPaintServer);
  +

Instead of cloning you can just copy & addref. Or better let mFill's
operator=() handle this (see above).

  +  else if (mStroke.mType == eStyleSVGPaintType_Server)
  +    aSource.mStroke.mPaintServer->Clone(&mStroke.mPaintServer);
  +

Ditto

  -       (mFill.mType   == eStyleSVGPaintType_Color && mFill.mColor   !=
aOther.mFill.mColor) )
  +       (mFill.mType   == eStyleSVGPaintType_Color && mFill.mColor   !=
aOther.mFill.mColor)  ||
  +       (mStroke.mType == eStyleSVGPaintType_Server &&
!EqualURIs(mStroke.mPaintServer,aOther.mStroke.mPaintServer)) ||
  +       (mFill.mType == eStyleSVGPaintType_Server &&
!EqualURIs(mFill.mPaintServer,aOther.mFill.mPaintServer)) )

Use nsStyleSVGPaint::operator==()


All of the above files need r= from some css-savvy person. You could
try dbaron or bzbarzky. 

I'm reading the other files in that patch just now. I'm a bit puzzled at the
moment as to why you need those nsISVGNumber changes.

Comment 14

15 years ago
Comments on the SVG part of the content/ patches:

>  Index: content/svg/content/src/nsSVGElement.cpp
>  
>  @@ -341,29 +342,38 @@ nsSVGElement::sTextContentElementsMap[] 
>     { &nsSVGAtoms::letter_spacing },
>     { &nsSVGAtoms::text_anchor },
>     { &nsSVGAtoms::text_decoration },
>     { &nsSVGAtoms::unicode_bidi },
>     { &nsSVGAtoms::word_spacing },
>     { nsnull }
>   };
>   
>  +
>   // PresentationAttributes-FontSpecification

Stray whitespace.

>  Index: content/svg/content/src/nsSVGElement.cpp
>
>  +  { &nsSVGAtoms::color }, // This is required for color support in the <g>
element

What do you mean by the comment? color is a presentation attribute on
all elements that take stroke/fill, right?

>  @@ -341,29 +342,38 @@ nsSVGElement::sTextContentElementsMap[] 
>     { &nsSVGAtoms::letter_spacing },
>     { &nsSVGAtoms::text_anchor },
>     { &nsSVGAtoms::text_decoration },
>     { &nsSVGAtoms::unicode_bidi },
>     { &nsSVGAtoms::word_spacing },
>     { nsnull }
>   };
>   
>  +

Stray whitespace.

>  Index: content/svg/content/src/nsSVGNumber.cpp
>  Index: content/svg/content/src/nsSVGNumber.h
>  Index: content/svg/content/src/nsSVGSVGElement.cpp
>  Index: content/svg/content/src/nsSVGViewportRect.cpp

I'm not sure that we should have nsISVGNumber and that SVGNumbers
should ever observe the viewport axis, since their values don't change
when the viewport changes. Emitting a SVGNumber change-notification in
this situation seems wrong.
(The situation is different with nsSVGLengths. If they are percentage
lengths then their actual value will change when the viewport changes)

>  +++ content/svg/content/src/nsISVGEnum.h	2004-04-03 18:20:42.000000000 -0800

Needs a license header.

>  +  static const nsIID& GetIID() { static nsIID iid = NS_ISVGENUM_IID; return
iid; }

You can use NS_DEFINE_STATIC_IID_ACCESSOR(NS_ISVGENUM_IID)


>  +++ content/svg/content/src/nsISVGNumber.h	2004-04-09 15:14:55.000000000 -0700

Not sure we want this, see above.

>  +++ content/svg/content/src/nsISVGNumberList.h	2004-04-13 20:42:14.000000000
-0700

Again, I don't think we should have this.

>  +++ content/svg/content/src/nsSVGAnimatedEnumeration.cpp	2004-05-26
19:27:23.000000000 -0700
>  
>  +NS_IMETHODIMP
>  +nsSVGAnimatedEnumeration::SetBaseVal(PRUint16 aBaseVal)
>  +{
>  +  mBaseVal->SetIntegerValue(aBaseVal);
>  +  return NS_OK;
>  +}

How about 

     return mBaseVal->SetIntegerValue(aBaseVal); 

just in case mBaseVal has something to complain about.

>  +++ content/svg/content/src/nsSVGAnimatedNumber.cpp	2004-04-09
10:11:01.000000000 -0700
>  
>  + *    Alex Fritze <alex.fritze@crocodile-clips.com> (original author)

Should be you.

>  +++ content/svg/content/src/nsSVGAnimatedNumber.h	2004-04-09
10:10:23.000000000 -0700

Here also.

>  +++ content/svg/content/src/nsSVGAnimatedNumberList.cpp	2004-04-09
10:07:04.000000000 -0700
>  
>  + *    Alex Fritze <alex.fritze@crocodile-clips.com> (original author)

And here.

>  +++ content/svg/content/src/nsSVGAnimatedNumberList.h	2004-04-09
09:51:20.000000000 -0700

And here.

>  +++ content/svg/content/src/nsSVGEnum.cpp	2004-05-25 12:34:31.000000000 -0700

Needs license header.

I guess we could use a template<> for getting the atom->int mapping,
but I'm happy keeping the dynamic nsSVGEnumMapping way.

>  +////////////////////////////////////////////////////////////////////////
>  +// nsSVGLength class
>  +
>  +class nsSVGEnum : public nsISVGEnum,

Comment should say "nsSVGEnum".

>  +                  public nsISVGValueObserver,
>  +                  public nsSupportsWeakReference

No need for this class to implement nsISVGValueObserver or
nsISupportsWeakRef AFAICS.

>  +
>  +  void Print_mapping();

Should be taken out or #ifdef DEBUG'ed

>  +NS_IMETHODIMP
>  +nsSVGEnum::SetValueString(const nsAString& aValue)
>  +{
>  +  nsCOMPtr<nsIAtom> valAtom = do_GetAtom(aValue);
>  +
>  +  nsSVGEnumMapping *tmp = mMapping;
>  +
>  +  while (tmp->key) {
>  +    if (valAtom == tmp->key) {
>  +      mValue = tmp->val;
>  +      break;
>  +    }
>  +    tmp++;
>  +  }
>  +
>  +  return NS_OK;
>  +}

Should return an error if the value isn't found.
Also you should probably call WillModify()/DidModify().

>  +//----------------------------------------------------------------------
>  +// nsISVGValueObserver methods
>  + ...

Not needed, see above.

>  +NS_IMETHODIMP
>  +nsSVGEnum::SetIntegerValue(PRUint16 aValue)
>  +{
>  +  mValue = aValue;
>  +  return NS_OK;
>  +}

Should call WillModify()/DidModify()

>  +++ content/svg/content/src/nsSVGEnum.h	2004-03-25 13:27:23.000000000 -0800

Needs license header.

>  +struct nsSVGEnumMapping {
>  +    nsIAtom *key;
>  +    PRUint16 val;
>  +};

This needs to be
  nsIAtom **key;
because the atoms you reference might not be initialized at the time the static
vars get initialized.

>  +++ content/svg/content/src/nsSVGGradientElement.cpp	2004-05-26
19:28:12.000000000 -0700
>  
>  + * Contributor(s):
>  + *    William Cook <william.cook@crocodile-clips.com> (original author)
>  + *    Alex Fritze <alex.fritze@crocodile-clips.com>

Should be you.

>  +  static struct nsSVGEnumMapping gUnitMap[] = {
>  +	{nsSVGAtoms::objectBoundingBox, SVG_GRUNITS_OBJECTBOUNDINGBOX},
>  +	{nsSVGAtoms::userSpaceOnUse, SVG_GRUNITS_USERSPACEONUSE},

  {&nsSVGAtoms::objectBoundingBox, ...} 
  etc.

>  +void nsSVGGradientElement::ParentChainChanged()
>  +{
>  +  // We don't change our length properties when our parent
>  +  // changes because our length properties are relative to
>  +  // the target element (the one calling the gradient)
>  +
>  +
>  +  // set new context information on our length-properties:
>  +  
>  +  nsCOMPtr<nsIDOMSVGSVGElement> dom_elem;
>  +  GetOwnerSVGElement(getter_AddRefs(dom_elem));
>  +  if (!dom_elem) return;
>  +
>  +  nsCOMPtr<nsISVGSVGElement> svg_elem = do_QueryInterface(dom_elem);
>  +  NS_ASSERTION(svg_elem, "<svg> element missing interface");
>  +
>  +  // XXX call baseclass version to recurse into children?
>  +}  

Gradients don't have any SVGLengths and stops don't either, so there
doesn't seem to be any parent-chain sensitive information in either
this class or any children. It's thus a good to keep this function so
that we don't call the baseclass default which recurses into
children. You can remove the unused code in it and the XXX comment,
though.

>  +void nsSVGLinearGradientElement::ParentChainChanged()
>  +{
>  +  // We don't change our length properties when our parent
>  +  // changes because our length properties are relative to
>  +  // the target element (the one calling the gradient)
>  +
>  +
>  +  // set new context information on our length-properties:
>  +  
>  +  nsCOMPtr<nsIDOMSVGSVGElement> dom_elem;
>  +  GetOwnerSVGElement(getter_AddRefs(dom_elem));
>  +  if (!dom_elem) return;
>  +
>  +  nsCOMPtr<nsISVGSVGElement> svg_elem = do_QueryInterface(dom_elem);
>  +  NS_ASSERTION(svg_elem, "<svg> element missing interface");
>  +
>  +  // XXX call baseclass version to recurse into children?
>  +}  

This can be removed.

>  +void nsSVGRadialGradientElement::ParentChainChanged()
>  +{
>  +  // We don't change our length properties when our parent
>  +  // changes because our length properties are relative to
>  +  // the target element (the one calling the gradient)
>  +
>  +  // set new context information on our length-properties:
>  +  
>  +  nsCOMPtr<nsIDOMSVGSVGElement> dom_elem;
>  +  GetOwnerSVGElement(getter_AddRefs(dom_elem));
>  +  if (!dom_elem) return;
>  +
>  +  nsCOMPtr<nsISVGSVGElement> svg_elem = do_QueryInterface(dom_elem);
>  +  NS_ASSERTION(svg_elem, "<svg> element missing interface");
>  +
>  +  // XXX call baseclass version to recurse into children?
>  +}  

Ditto here.

>  +++ content/svg/content/src/nsSVGGradientElement.h	2004-05-22
18:01:30.000000000 -0700
>  
>  +#define SVG_GRUNITS_OBJECTBOUNDINGBOX 1U
>  +#define SVG_GRUNITS_USERSPACEONUSE 2U
>  +
>  +#define SVG_SPREADMETHOD_PAD 1U
>  +#define SVG_SPREADMETHOD_REFLECT 2U
>  +#define SVG_SPREADMETHOD_REPEAT 3U

I would remove these and use
nsIDOMSVGGradientElement::SVG_GRUNITS_USERSPACEONUSE etc directly.

>  +++ content/svg/content/src/nsSVGNumberList.cpp	2004-05-28 22:36:56.000000000
-0700
>  
>  + * Contributor(s):
>  + *    Alex Fritze <alex.fritze@crocodile-clips.com> (original author)

Should be you.

>  +  nsCOMPtr<nsISVGViewportAxis> mContext;

As nsSVGNumber, I don't think nsSVGNumberList should depend on
nsISVGViewportAxis.

>  +++ content/svg/content/src/nsSVGNumberList.h	2004-05-28 22:34:24.000000000 -0700
>
>  + * Contributor(s):
>  + *    Alex Fritze <alex.fritze@crocodile-clips.com> (original author)

Should be you again.

+++ content/svg/content/src/nsSVGStopElement.cpp	2004-05-23 22:15:14.000000000 -0700

+//----------------------------------------------------------------------
+// nsISVGContent methods
+
+void nsSVGStopElement::ParentChainChanged()
+{
+  // set new context information on our length-properties:
+  
+  nsCOMPtr<nsIDOMSVGSVGElement> dom_elem;
+  GetOwnerSVGElement(getter_AddRefs(dom_elem));
+  if (!dom_elem) return;
+
+  nsCOMPtr<nsISVGSVGElement> svg_elem = do_QueryInterface(dom_elem);
+  NS_ASSERTION(svg_elem, "<svg> element missing interface");
+
+  // offset:
+  {
+    nsCOMPtr<nsIDOMSVGNumber> dom_number;
+    mOffset->GetBaseVal(getter_AddRefs(dom_number));
+    nsCOMPtr<nsISVGNumber> number = do_QueryInterface(dom_number);
+    NS_ASSERTION(number, "svg number missing interface");
+
+    nsCOMPtr<nsIDOMSVGRect> vp_dom;
+    svg_elem->GetViewport(getter_AddRefs(vp_dom));
+    nsCOMPtr<nsISVGViewportRect> vp = do_QueryInterface(vp_dom);
+    nsCOMPtr<nsISVGViewportAxis> ctx;
+    vp->GetXAxis(getter_AddRefs(ctx));
+    
+    number->SetContext(ctx);
+  }
+
+  // recurse into child content:
+  nsSVGStopElementBase::ParentChainChanged();
+}  

There's no need for this AFAICS.


OK, layout patches next :-)

Comment 15

15 years ago
> Gradients don't have any SVGLengths and stops don't either, so there
> doesn't seem to be any parent-chain sensitive information in either
> this class or any children. It's thus a good to keep this function so
> that we don't call the baseclass default which recurses into
> children. You can remove the unused code in it and the XXX comment,
> though.

What am I saying? Of course they do. For some reason I thought x1, y1, etc.
where SVGNumbers, not SVGLengths. So we need
nsSVG{Linear,Radial}GradientElement::ParentChainChanged() to readjust
viewport-percentage based lengths for viewport changes after all. Something like
this:

void nsSVGLinearGradientElement::ParentChainChanged() {
  nsCOMPtr<nsIDOMSVGSVGElement> dom_elem;
  GetOwnerSVGElement(getter_AddRefs(dom_elem));
  if (!dom_elem) return;

  nsCOMPtr<nsISVGSVGElement> svg_elem = do_QueryInterface(dom_elem);

  nsCOMPtr<nsIDOMSVGRect> vp_dom;
  svg_elem->GetViewport(getter_AddRefs(vp_dom));
  nsCOMPtr<nsISVGViewportRect> vp = do_QueryInterface(vp_dom);

  nsCOMPtr<nsISVGViewportAxis> xaxis;
  vp->GetXAxis(getter_AddRefs(xaxis));

  nsCOMPtr<nsISVGViewportAxis> yaxis;
  vp->GetYAxis(getter_AddRefs(yaxis));

  // x1:
  {
    nsCOMPtr<nsIDOMSVGLength> dom_length;
    mX1->GetBaseVal(getter_AddRefs(dom_length));
    nsCOMPtr<nsISVGLength> length = do_QueryInterface(dom_length);
    NS_ASSERTION(length, "svg length missing interface");
    length->SetContext(xaxis);
  }

  // y1:
  ...
  // x2:
  ...
  // y2:
  ...
}

And similar for the radial gradient.
No need to recurse into children because stops don't have percentage-based
lengths. The offset for stops can be specified as a percentage, but it is always
a "percentage of 100" as opposed to a percentage of some axis length, so the
conversion between the value & the percentage is always fixed:
value=percentage/100. 
While it seems a special case for stops, I think for now we could just accept
percentages for number-attributes everywhere by checking for "%" in
nsSVGNumber::SetValueString():

  if (*number) {
    char *rest;
    double value = PR_strtod(number, &rest);
+   if (*rest=='%' && rest!=number) {
+     rv = SetValue(value/100.0);
+   } 
    else if (rest!=number) {
	rv = SetValue(value);
    } else {
      rv = NS_ERROR_FAILURE;
      // no number
    }

Updated

15 years ago
Blocks: 246480

Comment 16

15 years ago
(In reply to comment #15)
> What am I saying? Of course they do. For some reason I thought x1, y1, etc.
> where SVGNumbers, not SVGLengths. So we need
> nsSVG{Linear,Radial}GradientElement::ParentChainChanged() to readjust
> viewport-percentage based lengths for viewport changes after all. Something like
> this:
> 
> void nsSVGLinearGradientElement::ParentChainChanged() {
>   nsCOMPtr<nsIDOMSVGSVGElement> dom_elem;
>   GetOwnerSVGElement(getter_AddRefs(dom_elem));
>   if (!dom_elem) return;
>   ... 

Ok, I'm beginning to understand that this is all more complicated than I
thought. The above only works for gradientUnits="userSpaceOnUse". To get this to
work for "objectBoundingBox", we need the gradientelement to become the
viewportrect. *sigh*. maybe we should leave that for another bug?
No longer blocks: 246480

Updated

15 years ago
Blocks: 246480

Updated

15 years ago
No longer blocks: 246480

Comment 17

15 years ago
*** Bug 246480 has been marked as a duplicate of this bug. ***

Comment 18

15 years ago
Created attachment 151897 [details] [diff] [review]
gdi+ renderer support for gradients
(Assignee)

Comment 19

15 years ago
Created attachment 152382 [details] [diff] [review]
New DOM files for SVG Gradients (updated based on Alex's comments)
Attachment #149562 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #152382 - Attachment description: Updated based on alex's comments → New DOM files for SVG Gradients (updated based on Alex's comments)
Attachment #152382 - Flags: review?(dbaron)
(Assignee)

Updated

15 years ago
Attachment #152382 - Flags: review?(dbaron) → review?(bugmail)
(Assignee)

Comment 20

15 years ago
Created attachment 152384 [details] [diff] [review]
Changes to DOM interfaces to support Gradients (updated based on Alex's comments)
Attachment #149561 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #152384 - Flags: review?(bugmail)
Comment on attachment 152384 [details] [diff] [review]
Changes to DOM interfaces to support Gradients (updated based on Alex's comments)

>Index: dom/public/idl/svg/Makefile.in
> 		nsIDOMSVGURIReference.idl \
>+		nsIDOMSVGGradientElement.idl \
>+		nsIDOMSVGStopElement.idl \
> 		$(NULL)

Please insert these so that the files are still alphabetically sorted.

r=me
Attachment #152384 - Flags: review?(bugmail) → review+
Comment on attachment 152382 [details] [diff] [review]
New DOM files for SVG Gradients (updated based on Alex's comments)

r=me

(for future reference, i think it's ok for alex to review these types of
patches. Assuming he has time and wants to of course)
Attachment #152382 - Flags: review?(bugmail) → review+
(Assignee)

Comment 23

15 years ago
Created attachment 153044 [details] [diff] [review]
Changes to content files to support SVG Gradients (reflects Alex's comments)
Attachment #149563 - Attachment is obsolete: true
(Assignee)

Comment 24

15 years ago
Created attachment 153045 [details] [diff] [review]
New content files for SVG Gradient support (reflects Alex's comments)
Attachment #149564 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #153044 - Flags: review?(alex)
(Assignee)

Updated

15 years ago
Attachment #153045 - Flags: review?(alex)

Comment 25

15 years ago
Comment on attachment 153045 [details] [diff] [review]
New content files for SVG Gradient support (reflects Alex's comments)

r=afri

> +++ content/svg/content/src/nsSVGAnimatedNumberList.cpp	2004-07-09 23:12:03.000000000 -0700

> +void
> +nsSVGAnimatedNumberList::Init(nsIDOMSVGNumberList* baseVal)
> +{
> +  mBaseVal = baseVal;
> +  if (!mBaseVal) return;
> +  nsCOMPtr<nsISVGValue> val = do_QueryInterface(mBaseVal);
> +  if (!val) return;
> +  val->AddObserver(this);
> +}

We should either (a) make sure that nsSVGAnimatedNumberList::mBaseVal
is always valid, or (b) check for !mBaseVal everywhere it is getting
used. I gravitate towards (a), i.e. having Init() return a failure
when baseVal is null or doesn't implement nsISVGValue:
nsresult nsSVGAnimatedNumberList::Init(.) {...}
Any checks for (!mBaseVal), e.g. in the dtor, can be removed then.

> +nsresult
> +NS_NewSVGAnimatedNumberList(nsIDOMSVGAnimatedNumberList** result,
> +                            nsIDOMSVGNumberList* baseVal)
> +{
> +  *result = nsnull;
> +  
> +  nsSVGAnimatedNumberList* animatedNumberList = new nsSVGAnimatedNumberList();
> +  if(!animatedNumberList) return NS_ERROR_OUT_OF_MEMORY;
> +  NS_ADDREF(animatedNumberList);
> +
> +  animatedNumberList->Init(baseVal);

if (NS_FAILED(animatedNumberList->Init(baseVal))) {
  NS_RELEASE(animatedNumberList);
  return NS_ERROR_FAILURE;
}


> +++ content/svg/content/src/nsSVGEnum.cpp	2004-07-10 23:44:27.000000000 -0700

> +nsresult
> +NS_NewSVGEnum(nsISVGEnum** result,
> +              PRUint16 value,
> +              nsSVGEnumMapping *mapping)
> +{

This looks like a good place for:
NS_ASSERTION(mapping, "no mapping");

> +  nsSVGEnum *pe = new nsSVGEnum(value, mapping);
> +  NS_ENSURE_TRUE(pe, NS_ERROR_OUT_OF_MEMORY);
> +  NS_ADDREF(pe);
> +  *result = pe;
> +  return NS_OK;
> +}
> +
> +nsresult
> +NS_NewSVGEnum(nsISVGEnum** result,
> +              const nsAString &value,
> +              nsSVGEnumMapping *mapping)
> +{

And here as well:
NS_ASSERTION(mapping, "no mapping");


> +NS_IMETHODIMP
> +nsSVGEnum::GetValueString(nsAString& aValue)
> +{
> +  nsSVGEnumMapping *tmp = mMapping;
> +
> +  while (tmp->key) {
> +    if (mValue == tmp->val) {
> +      (*tmp->key)->ToString(aValue);
> +      break;
> +    }
> +    tmp++;
> +  }
> +
> +  return NS_OK;
> +}

There's the potential for mValue not being found (because it's not
checked in the ctor or in SetIntegerValue), so we should have
something like:

  while (tmp->key) {
    if (mValue == tmp->val) {
      (*tmp->key)->ToString(aValue);
      return NS_OK;
    }
    tmp++;
  }
  NS_ERROR("unknown enumeration value");
  return NS_ERROR_FAILURE;
}


> +++ content/svg/content/src/nsSVGGradientElement.cpp	2004-07-09 23:37:10.000000000 -0700

> +  NS_FORWARD_NSIDOMELEMENT(nsSVGGradientElementBase::)
> +  NS_FORWARD_NSIDOMSVGELEMENT(nsSVGGradientElementBase::)

Not needed, I think.

> +
> +  // nsIStyledContent interface
> +  // NS_IMETHOD_(PRBool) IsAttributeMapped(const nsIAtom* aAttribute) const;

Not needed.

> +void nsSVGGradientElement::ParentChainChanged()
> +{
> +  // We don't change our length properties when our parent
> +  // changes because our length properties are relative to
> +  // the target element (the one calling the gradient)
> +
> +
> +  // set new context information on our length-properties:
> +  
> +  nsCOMPtr<nsIDOMSVGSVGElement> dom_elem;
> +  GetOwnerSVGElement(getter_AddRefs(dom_elem));
> +  if (!dom_elem) return;
> +
> +  nsCOMPtr<nsISVGSVGElement> svg_elem = do_QueryInterface(dom_elem);
> +  NS_ASSERTION(svg_elem, "<svg> element missing interface");
> +
> +  // XXX call baseclass version to recurse into children?
> +}  

This should be something like:

void nsSVGGradientElement::ParentChainChanged() 
{
  // Gradient length properties are relative to the target element
  // (the one calling the gradient), so we don't set their
  // contexts here.
  // Also, gradient child elements (stops) don't have any length
  // properties, so no need to recurse into children here.
}

> +class nsSVGLinearGradientElement : public nsSVGLinearGradientElementBase,
> +                                   public nsIDOMSVGLinearGradientElement
> +                               //  public nsIDOMSVGURIReference,
> +                               //  public nsIDOMSVGExternalResourcesRequired,

Are these commented-out interfaces here for a reason?

+  NS_FORWARD_NSIDOMSVGELEMENT(nsSVGLinearGradientElementBase::)

Not needed, I think.

> +
> +  // nsIStyledContent interface
> +  // NS_IMETHOD_(PRBool) IsAttributeMapped(const nsIAtom* aAttribute) const;

Can be removed. Implemented by one of the baseclasses.


> +
> +  // nsIDOMSVGURIReference properties
> +  nsCOMPtr<nsIDOMSVGAnimatedString> mHref;

Can be removed; have it already in base class

> +//----------------------------------------------------------------------
> +// nsISVGContent methods
> +
> +void nsSVGLinearGradientElement::ParentChainChanged()
> +{
>  [...]
> +}  

Can be removed. Baseclass version does us.


> +//-------------------------- Radial Gradients ----------------------------
> 
> +  // nsIDOMSVGURIReference
> +  NS_FORWARD_NSIDOMSVGURIREFERENCE(nsSVGRadialGradientElementBase::)

Not needed.

> +  NS_FORWARD_NSIDOMSVGELEMENT(nsSVGRadialGradientElementBase::)

I think this isn't needed.

> +
> +  // nsIStyledContent interface
> +  // NS_IMETHOD_(PRBool) IsAttributeMapped(const nsIAtom* aAttribute) const;

Can be removed. Implemented by one of the baseclasses.

> +
> +  // nsISVGContent specializations:
> +  virtual void ParentChainChanged();

Not needed.

> +//----------------------------------------------------------------------
> +// nsISVGContent methods
> +
> +void nsSVGRadialGradientElement::ParentChainChanged()
> +{
>  [...]
> +}  


Can be removed.


> +++ content/svg/content/src/nsSVGGradientElement.h	2004-07-09 23:22:19.000000000 -0700

Do we need this file?


> +++ content/svg/content/src/nsSVGNumberList.cpp	2004-07-09 23:25:10.000000000 -0700

> +void
> +nsSVGNumberList::AppendElement(nsIDOMSVGNumber* aElement)
> +{
> +  WillModify();
> +  NS_ADDREF(aElement);
> +  
> +  // The SVG specs state that 'if newItem is already in a list, it
> +  // is removed from its previous list before it is inserted into this
> +  // list':
> +  //  aElement->SetListOwner(this);
> +  
> +  mNumbers.AppendElement((void*)aElement);

nsSVGNumberList should register as an observer here:

NS_ADD_SVGVALUE_OBSERVER(aElement)


> +  DidModify();
> +}
> +
> +void
> +nsSVGNumberList::RemoveElementAt(PRInt32 index)
> +{
> +  WillModify();
> +  nsIDOMSVGNumber* number = ElementAt(index);
> +  NS_ASSERTION(number, "null number");

... and remove itself here:

NS_REMOVE_SVGVALUE_OBSERVER(number)

> +  mNumbers.RemoveElementAt(index);
> +  NS_RELEASE(number);
> +  DidModify();
> +}
> +
> +void
> +nsSVGNumberList::InsertElementAt(nsIDOMSVGNumber* aElement, PRInt32 index)
> +{
> +  WillModify();
> +  NS_ADDREF(aElement);
> +
> +  // The SVG specs state that 'if newItem is already in a list, it
> +  // is removed from its previous list before it is inserted into this
> +  // list':
> +  //  aElement->SetListOwner(this);
> +  
> +  mNumbers.InsertElementAt((void*)aElement, index);

again, should register as observer here


> +++ content/svg/content/src/nsSVGStopElement.cpp	2004-07-09 22:58:59.000000000 -0700
> 
> + * Contributor(s):
> + *    Alex Fritze <alex.fritze@crocodile-clips.com> (original author)

Should be you.

> +//----------------------------------------------------------------------
> +// nsISVGContent methods
> +
> +void nsSVGStopElement::ParentChainChanged()
> +{
> +  // set new context information on our length-properties:
> +  // which we don't have any of...
> +}  

I would remove this function. It is never getting called. And even if
it would get called the baseclass implementation is fine.
Attachment #153045 - Flags: review?(alex) → review+

Comment 26

15 years ago
Comment on attachment 153044 [details] [diff] [review]
Changes to content files to support SVG Gradients (reflects Alex's comments)

r=afri


>Index: content/base/src/nsStyleContext.cpp
>===================================================================
>@@ -829,23 +829,23 @@ void nsStyleContext::DumpRegressionData(
>   fprintf(out, "\" />\n");
> 
> #ifdef MOZ_SVG
>   // SVG
>   IndentBy(out,aIndent);
>   const nsStyleSVG* svg = GetStyleSVG();
>   fprintf(out, "<svg data=\"%d %ld %f %d %d %d %d %ld %s %f %d %d %f %f %f %d %d\" />\n",
>           (int)svg->mFill.mType,
>-          (long)svg->mFill.mColor,
>+          (long)svg->mFill.mPaint.mColor,
>           svg->mFillOpacity,
>           (int)svg->mFillRule,
>           (int)svg->mPointerEvents,
>           (int)svg->mShapeRendering,
>           (int)svg->mStroke.mType,
>-          (long)svg->mStroke.mColor,
>+          (long)svg->mStroke.mPaint.mColor,

mColor will only be valid for painttype != eStyleSVGPaintType_Server.
You need to print mColor or mPaintServer depending on painttype.  

>Index: content/shared/src/nsStyleStruct.cpp
>===================================================================

>@@ -814,18 +810,17 @@ nsChangeHint nsStyleSVG::CalcDifference(
>        mStrokeLinejoin   != aOther.mStrokeLinejoin   ||
>        mStrokeMiterlimit != aOther.mStrokeMiterlimit ||
>        mStrokeOpacity    != aOther.mStrokeOpacity    ||
>        mStrokeWidth      != aOther.mStrokeWidth      ||
>        mTextAnchor       != aOther.mTextAnchor       ||
>        mTextRendering    != aOther.mTextRendering)
>     return NS_STYLE_HINT_VISUAL;
> 
>-  if ( (mStroke.mType == eStyleSVGPaintType_Color && mStroke.mColor != aOther.mStroke.mColor) ||
>-       (mFill.mType   == eStyleSVGPaintType_Color && mFill.mColor   != aOther.mFill.mColor) )
>+  if ( (mStroke != aOther.mStroke) || (mFill != aOther.mFill) )
>     return NS_STYLE_HINT_VISUAL;

This if(.) can now be folded into the if(.) above.

>Index: content/svg/content/src/nsSVGNumber.cpp
>===================================================================

>+#include "nsWeakReference.h"

Not needed

> class nsSVGNumber : public nsIDOMSVGNumber,
>-                    public nsSVGValue
>+                    public nsSVGValue,
>+                    public nsISVGValueObserver,
>+                    public nsSupportsWeakReference

You don't need to implement nsISVGValueObserver or nsSupportsWeakReference.
They would only be required if nsSVGNumber sets itself as an observer to
something.

>+  // nsISVGValueObserver interface:
>+  NS_IMETHOD WillModifySVGObservable(nsISVGValue* observable);
>+  NS_IMETHOD DidModifySVGObservable (nsISVGValue* observable);
>+
>+  // nsISupportsWeakReference
>+  // implementation inherited from nsSupportsWeakReference

Not needed

> nsresult
> NS_NewSVGNumber(nsIDOMSVGNumber** result, float val)
> {
>-  *result = new nsSVGNumber(val);
>-  if(!*result) return NS_ERROR_OUT_OF_MEMORY;
>-  
>-  NS_ADDREF(*result);
>+  nsSVGNumber *pl = new nsSVGNumber(val);
>+  NS_ENSURE_TRUE(pl, NS_ERROR_OUT_OF_MEMORY);
>+  NS_ADDREF(pl);
>+  *result = pl;
>   return NS_OK;
> }

Assigning to a temporary is unneccessary here. 

> NS_INTERFACE_MAP_BEGIN(nsSVGNumber)
>   NS_INTERFACE_MAP_ENTRY(nsISVGValue)
>+  NS_INTERFACE_MAP_ENTRY(nsISVGValueObserver)

Not needed

>   NS_INTERFACE_MAP_ENTRY(nsIDOMSVGNumber)
>+  NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)

Ditto

>   NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO(SVGNumber)
>   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsISVGValue)
> NS_INTERFACE_MAP_END
> 

>+NS_IMETHODIMP
> nsSVGNumber::SetValueString(const nsAString& aValue)
> {
>-  NS_NOTYETIMPLEMENTED("write me!");
>-  return NS_ERROR_UNEXPECTED;
>+  nsresult rv = NS_OK;
>+  WillModify();
>+  
>+  char *str = ToNewCString(aValue);
>+
>+  char* number = str;
>+  while (*number && isspace(*number))
>+    ++number;
>+
>+  if (*number) {
>+    char *rest;
>+    double value = PR_strtod(number, &rest);
>+    if (rest!=number) {
>+	rv = SetValue(value);
>+    } else {
>+      rv = NS_ERROR_FAILURE;
>+      // no number
>+    }
>+  }
>+  nsMemory::Free(str);
>+  DidModify();
>+  return rv;
> }
> 

To support percentages for stops, we'll need something like:

  if (*number) {
    char *rest;
    double value = PR_strtod(number, &rest);
+   if (*rest=='%' && rest!=number) {
+     rv = SetValue(value/100.0);
+   } 
    else if (rest!=number) {
	rv = SetValue(value);
    } else {
      rv = NS_ERROR_FAILURE;
      // no number
    }

>+//----------------------------------------------------------------------
>+// nsISVGValueObserver methods
>+
> [...]

Not needed
Attachment #153044 - Flags: review?(alex) → review+
(Assignee)

Comment 27

15 years ago
(In reply to comment #25)
> (From update of attachment 153045 [details] [diff] [review])
> r=afri
> 
> > +++ content/svg/content/src/nsSVGAnimatedNumberList.cpp	2004-07-09
23:12:03.000000000 -0700
> 
> 
> We should either (a) make sure that nsSVGAnimatedNumberList::mBaseVal
> is always valid, or (b) check for !mBaseVal everywhere it is getting
> used. I gravitate towards (a), i.e. having Init() return a failure
> when baseVal is null or doesn't implement nsISVGValue:
> nsresult nsSVGAnimatedNumberList::Init(.) {...}
> Any checks for (!mBaseVal), e.g. in the dtor, can be removed then.
> 
OK, I will change the signature of nsSVGAnimatedNumberList::Init to return an
nsresult, but that's inconsistent with nsSVGAnimatedLengthList and friends.
Should we (in a separate bug) update all of those similarly?
> 
> > +++ content/svg/content/src/nsSVGEnum.cpp	2004-07-10 23:44:27.000000000 -0700
> 
> > +nsresult
> > +NS_NewSVGEnum(nsISVGEnum** result,
> > +              PRUint16 value,
> > +              nsSVGEnumMapping *mapping)
> > +{
> 
> This looks like a good place for:
> NS_ASSERTION(mapping, "no mapping");
Done.
> 
> > +  nsSVGEnum *pe = new nsSVGEnum(value, mapping);
> > +  NS_ENSURE_TRUE(pe, NS_ERROR_OUT_OF_MEMORY);
> > +  NS_ADDREF(pe);
> > +  *result = pe;
> > +  return NS_OK;
> > +}
> > +
> > +nsresult
> > +NS_NewSVGEnum(nsISVGEnum** result,
> > +              const nsAString &value,
> > +              nsSVGEnumMapping *mapping)
> > +{
> 
> And here as well:
> NS_ASSERTION(mapping, "no mapping");
Done.
> 
> 
> > +NS_IMETHODIMP
> > +nsSVGEnum::GetValueString(nsAString& aValue)
> > +{
> > +  nsSVGEnumMapping *tmp = mMapping;
> > +
> > +  while (tmp->key) {
> > +    if (mValue == tmp->val) {
> > +      (*tmp->key)->ToString(aValue);
> > +      break;
> > +    }
> > +    tmp++;
> > +  }
> > +
> > +  return NS_OK;
> > +}
> 
> There's the potential for mValue not being found (because it's not
> checked in the ctor or in SetIntegerValue), so we should have
> something like:
> 
>   while (tmp->key) {
>     if (mValue == tmp->val) {
>       (*tmp->key)->ToString(aValue);
>       return NS_OK;
>     }
>     tmp++;
>   }
>   NS_ERROR("unknown enumeration value");
>   return NS_ERROR_FAILURE;
> }
Done.
> 
> 
> > +++ content/svg/content/src/nsSVGGradientElement.cpp	2004-07-09
23:37:10.000000000 -0700
> 
> > +  NS_FORWARD_NSIDOMELEMENT(nsSVGGradientElementBase::)
> > +  NS_FORWARD_NSIDOMSVGELEMENT(nsSVGGradientElementBase::)
> 
> Not needed, I think.
> 
> > +
> > +  // nsIStyledContent interface
> > +  // NS_IMETHOD_(PRBool) IsAttributeMapped(const nsIAtom* aAttribute) const;
> 
> Not needed.
> 
> > +void nsSVGGradientElement::ParentChainChanged()
> > +{
> > +  // We don't change our length properties when our parent
> > +  // changes because our length properties are relative to
> > +  // the target element (the one calling the gradient)
> > +
> > +
> > +  // set new context information on our length-properties:
> > +  
> > +  nsCOMPtr<nsIDOMSVGSVGElement> dom_elem;
> > +  GetOwnerSVGElement(getter_AddRefs(dom_elem));
> > +  if (!dom_elem) return;
> > +
> > +  nsCOMPtr<nsISVGSVGElement> svg_elem = do_QueryInterface(dom_elem);
> > +  NS_ASSERTION(svg_elem, "<svg> element missing interface");
> > +
> > +  // XXX call baseclass version to recurse into children?
> > +}  
> 
> This should be something like:
> 
> void nsSVGGradientElement::ParentChainChanged() 
> {
>   // Gradient length properties are relative to the target element
>   // (the one calling the gradient), so we don't set their
>   // contexts here.
>   // Also, gradient child elements (stops) don't have any length
>   // properties, so no need to recurse into children here.
> }
> 
Cool -- Done.
> > +class nsSVGLinearGradientElement : public nsSVGLinearGradientElementBase,
> > +                                   public nsIDOMSVGLinearGradientElement
> > +                               //  public nsIDOMSVGURIReference,
> > +                               //  public nsIDOMSVGExternalResourcesRequired,
> 
> Are these commented-out interfaces here for a reason?
No, just left over trial-end-error -- I've deleted them
> 
> +  NS_FORWARD_NSIDOMSVGELEMENT(nsSVGLinearGradientElementBase::)
> 
> Not needed, I think.
Actually this is required or I get a bunch of unimplemented virtual interfaces
because the gradient element base class actually implements them.
> 
> > +
> > +  // nsIStyledContent interface
> > +  // NS_IMETHOD_(PRBool) IsAttributeMapped(const nsIAtom* aAttribute) const;
> 
> Can be removed. Implemented by one of the baseclasses.
Done.
> 
> 
> > +
> > +  // nsIDOMSVGURIReference properties
> > +  nsCOMPtr<nsIDOMSVGAnimatedString> mHref;
> 
> Can be removed; have it already in base class
Oops -- right you are!
> 
> > +//----------------------------------------------------------------------
> > +// nsISVGContent methods
> > +
> > +void nsSVGLinearGradientElement::ParentChainChanged()
> > +{
> >  [...]
> > +}  
> 
> Can be removed. Baseclass version does us.
> 
> 
> > +//-------------------------- Radial Gradients ----------------------------
> > 
> > +  // nsIDOMSVGURIReference
> > +  NS_FORWARD_NSIDOMSVGURIREFERENCE(nsSVGRadialGradientElementBase::)
> 
> Not needed.
OK
> 
> > +  NS_FORWARD_NSIDOMSVGELEMENT(nsSVGRadialGradientElementBase::)
> 
> I think this isn't needed.
Actually, it is (see above)
> 
> > +
> > +  // nsIStyledContent interface
> > +  // NS_IMETHOD_(PRBool) IsAttributeMapped(const nsIAtom* aAttribute) const;
> 
> Can be removed. Implemented by one of the baseclasses.
> 
Done.
> > +
> > +  // nsISVGContent specializations:
> > +  virtual void ParentChainChanged();
> 
> Not needed.
Gone.
> 
> > +//----------------------------------------------------------------------
> > +// nsISVGContent methods
> > +
> > +void nsSVGRadialGradientElement::ParentChainChanged()
> > +{
> >  [...]
> > +}  
> 
> 
> Can be removed.
OK.
> 
> 
> > +++ content/svg/content/src/nsSVGGradientElement.h	2004-07-09
23:22:19.000000000 -0700
> 
> Do we need this file?
Nope, its a vestige of an earlier attempt!
> 
> 
> > +++ content/svg/content/src/nsSVGNumberList.cpp	2004-07-09
23:25:10.000000000 -0700
> 
> > +void
> > +nsSVGNumberList::AppendElement(nsIDOMSVGNumber* aElement)
> > +{
> > +  WillModify();
> > +  NS_ADDREF(aElement);
> > +  
> > +  // The SVG specs state that 'if newItem is already in a list, it
> > +  // is removed from its previous list before it is inserted into this
> > +  // list':
> > +  //  aElement->SetListOwner(this);
> > +  
> > +  mNumbers.AppendElement((void*)aElement);
> 
> nsSVGNumberList should register as an observer here:
> 
> NS_ADD_SVGVALUE_OBSERVER(aElement)
Done.
> 
> 
> > +  DidModify();
> > +}
> > +
> > +void
> > +nsSVGNumberList::RemoveElementAt(PRInt32 index)
> > +{
> > +  WillModify();
> > +  nsIDOMSVGNumber* number = ElementAt(index);
> > +  NS_ASSERTION(number, "null number");
> 
> ... and remove itself here:
> 
> NS_REMOVE_SVGVALUE_OBSERVER(number)
Done.
> 
> > +  mNumbers.RemoveElementAt(index);
> > +  NS_RELEASE(number);
> > +  DidModify();
> > +}
> > +
> > +void
> > +nsSVGNumberList::InsertElementAt(nsIDOMSVGNumber* aElement, PRInt32 index)
> > +{
> > +  WillModify();
> > +  NS_ADDREF(aElement);
> > +
> > +  // The SVG specs state that 'if newItem is already in a list, it
> > +  // is removed from its previous list before it is inserted into this
> > +  // list':
> > +  //  aElement->SetListOwner(this);
> > +  
> > +  mNumbers.InsertElementAt((void*)aElement, index);
> 
> again, should register as observer here
> 
> 
> > +++ content/svg/content/src/nsSVGStopElement.cpp	2004-07-09
22:58:59.000000000 -0700
> > 
> > + * Contributor(s):
> > + *    Alex Fritze <alex.fritze@crocodile-clips.com> (original author)
> 
> Should be you.
OK.
> 
> > +//----------------------------------------------------------------------
> > +// nsISVGContent methods
> > +
> > +void nsSVGStopElement::ParentChainChanged()
> > +{
> > +  // set new context information on our length-properties:
> > +  // which we don't have any of...
> > +}  
> 
> I would remove this function. It is never getting called. And even if
> it would get called the baseclass implementation is fine.
> 

New patch on the way!
(Assignee)

Comment 28

15 years ago
Created attachment 153519 [details] [diff] [review]
New content files for SVG Gradient support (3rd version)
Attachment #153045 - Attachment is obsolete: true
(Assignee)

Comment 29

15 years ago
Created attachment 153562 [details] [diff] [review]
New content files for SVG Gradient support (4th version)

Oops -- got an undefined when I tested.  This should be better!
(Assignee)

Updated

15 years ago
Attachment #153519 - Attachment is obsolete: true
(Assignee)

Comment 30

15 years ago
Created attachment 153563 [details] [diff] [review]
Changes to content files to support SVG Gradients (3rd version)
Attachment #153044 - Attachment is obsolete: true
(Assignee)

Comment 31

15 years ago
Created attachment 154884 [details] [diff] [review]
New DOM files for SVG Gradients (refreshed to current tree)
Attachment #152382 - Attachment is obsolete: true
(Assignee)

Comment 32

15 years ago
Created attachment 154885 [details] [diff] [review]
Changes to DOM files for SVG Gradients (refreshed to current tree)
Attachment #152384 - Attachment is obsolete: true
(Assignee)

Comment 33

15 years ago
Created attachment 154886 [details] [diff] [review]
New content files for SVG Gradients (refreshed to current tree)
Attachment #153562 - Attachment is obsolete: true
(Assignee)

Comment 34

15 years ago
Created attachment 154887 [details] [diff] [review]
Changes to content files to support SVG Gradients (refreshed to current tree)
Attachment #153563 - Attachment is obsolete: true
(Assignee)

Comment 35

15 years ago
Created attachment 154888 [details] [diff] [review]
New layout files for SVG Gradients (refreshed to current tree)
Attachment #149566 - Attachment is obsolete: true
(Assignee)

Comment 36

15 years ago
Created attachment 154889 [details] [diff] [review]
Changes to layout files for SVG Gradient support (refreshed to current tree)
Attachment #149565 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #154888 - Flags: review?(alex)
(Assignee)

Updated

15 years ago
Attachment #154889 - Flags: review?(alex)
(Assignee)

Updated

15 years ago
Attachment #154884 - Flags: superreview?(bugmail)
(Assignee)

Updated

15 years ago
Attachment #154885 - Flags: superreview?(bugmail)
Comment on attachment 154884 [details] [diff] [review]
New DOM files for SVG Gradients (refreshed to current tree)

sorry, i don't have superreview powers
Attachment #154884 - Flags: superreview?(bugmail)
Scooter, you're other option for DOM sr is Peter: peterv@propagandism.org
(posting here since the emails I tried to send you don't seem to be getting
through). 
(Assignee)

Updated

15 years ago
Attachment #154884 - Flags: superreview?(peterv)
(Assignee)

Updated

15 years ago
Attachment #154885 - Flags: superreview?(peterv)
(Assignee)

Comment 39

14 years ago
Created attachment 156745 [details] [diff] [review]
New layout files for SVG Gradients (updated based on Alex's comments)

Updated to incorporate E-Mail comments from Alex.  Removed internal gecko types
from the renderer interface.  Also fixed bug in the userSpaceOnUse code.
(Assignee)

Updated

14 years ago
Attachment #154888 - Attachment is obsolete: true
(Assignee)

Comment 40

14 years ago
Created attachment 156746 [details] [diff] [review]
Chages to layout files for SVG Gradient support (updated based on Alex's comments)

Updated to incorporate E-Mail comments from Alex.  Removed internal gecko types
from the renderer interface.  Also fixed bug in the userSpaceOnUse code.
Attachment #154889 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #156745 - Flags: review?(alex)
(Assignee)

Updated

14 years ago
Attachment #154889 - Flags: review?(alex)
(Assignee)

Updated

14 years ago
Attachment #154888 - Flags: review?(alex)
(Assignee)

Updated

14 years ago
Attachment #156746 - Flags: review?(alex)

Comment 41

14 years ago
Created attachment 156893 [details] [diff] [review]
update cairo gradient code to latest set of gradient patches

Updated

14 years ago
Attachment #150017 - Attachment is obsolete: true

Comment 42

14 years ago
Created attachment 156900 [details] [diff] [review]
update gdi+ gradient code to latest set of gradient patches
Attachment #151897 - Attachment is obsolete: true
(Assignee)

Comment 43

14 years ago
Created attachment 157006 [details] [diff] [review]
New DOM files for SVG Gradients (updated copyright stuff)
Attachment #154884 - Attachment is obsolete: true
(Assignee)

Comment 44

14 years ago
Created attachment 157007 [details] [diff] [review]
New layout files for SVG Gradients -- fixes erisapple2.svg crasher

This updated fixes the issue where a radial gradient references a linear
gradient which may reference a radial gradient, etc.  Its important that if we
are attempted to get attributes specific to linear gradients (e.g. X1) then
skip down the chain until we actually get a linear gradient, or return the
default.
(Assignee)

Updated

14 years ago
Attachment #156745 - Attachment is obsolete: true

Updated

14 years ago
Attachment #154885 - Flags: superreview?(peterv) → superreview+

Updated

14 years ago
Attachment #157006 - Flags: superreview+
Comment on attachment 157006 [details] [diff] [review]
New DOM files for SVG Gradients (updated copyright stuff)

We usualy put a reference in the DOM IDL files to the DOM spec that they
correspond to. Why don't these interfaces correspond to the ones in the SVG 1.1
specification? Either they're nsIDOM* interfaces and they should, or they're
nsIDOMNS* interfaces.
Attachment #154884 - Flags: superreview?(peterv)
(Assignee)

Comment 46

14 years ago
(In reply to comment #45)
> (From update of attachment 157006 [details] [diff] [review])
> We usualy put a reference in the DOM IDL files to the DOM spec that they
> correspond to. Why don't these interfaces correspond to the ones in the SVG 1.1
> specification? Either they're nsIDOM* interfaces and they should, or they're
> nsIDOMNS* interfaces.
> 

Peter, I'm happy to add the references, and nsIDOMGradientElement.idl *does*
correspond to the SVG1.1 specification (minus the usual multiple inheritance
stuff..).  I'll provide an update, but since this is documentation only, are you
OK with not going through another r/sr pass?  I'm very anxious to get this stuff
checked in.
There's differences with the interfaces in
http://www.w3.org/TR/2003/REC-SVG11-20030114/idl.html: SVGAnimatedNumber has
float attributes and baseVal is not readonly in the spec and there's the
SVG_GRUNITS stuff in nsIDOMSVGGradientElement. I suppose you could land it as
is, but why does SVGAnimatedNumber differ so much?
(Assignee)

Comment 48

14 years ago
(In reply to comment #47)
Yes, after my reply, I rechecked the spec and realized that I had really messed
up on AnimatedNumber.  I'm in the process of fixing it to reflect the spec and
should have a new patch up this weekend.  I had copied AnimatedLengths assuming
some similarity, but it turns out that they are quite different.  I'm learning,
but its a slow process....
(Assignee)

Comment 49

14 years ago
Created attachment 157292 [details] [diff] [review]
New DOM files for SVG Gradients (changed to reflect spec)
Attachment #157006 - Attachment is obsolete: true
(Assignee)

Comment 50

14 years ago
Created attachment 157293 [details] [diff] [review]
Changes to DOM files for SVG Gradients (another tree refresh)
Attachment #154885 - Attachment is obsolete: true
(Assignee)

Comment 51

14 years ago
Created attachment 157294 [details] [diff] [review]
New content files for SVG Gradients (changes to AnimatedNumber)
Attachment #154887 - Attachment is obsolete: true
(Assignee)

Comment 52

14 years ago
Created attachment 157295 [details] [diff] [review]
Changes to content files for SVG Gradients (refreshed to current tree)
Attachment #154886 - Attachment is obsolete: true
(Assignee)

Comment 53

14 years ago
Created attachment 157296 [details] [diff] [review]
New layout files for SVG Gradients (SVGAnimatedNumber fix)
Attachment #157007 - Attachment is obsolete: true
(Assignee)

Comment 54

14 years ago
Created attachment 157297 [details] [diff] [review]
Changes to layout files for SVG Gradient support (refreshed to current tree)

I've updated all patches (except the renderer patches, which should not have
changed) to:
  1) Correctly reflect the spec for DOMSVGAnimatedNumber
  2) Account for drift in the tree
(Assignee)

Updated

14 years ago
Attachment #156746 - Attachment is obsolete: true
Comment on attachment 157292 [details] [diff] [review]
New DOM files for SVG Gradients (changed to reflect spec)

Looks good. One last thing (sorry!): I wouldn't freeze them just yet, so remove
"@status FROZEN".
Comment on attachment 157294 [details] [diff] [review]
New content files for SVG Gradients (changes to AnimatedNumber)

>+/* attribute nsIDOMSVGNumber baseVal; */
>+NS_IMETHODIMP
>+nsSVGAnimatedNumber::SetBaseVal(float aBaseVal)
>+{
>+  mBaseVal = aBaseVal;
>+  return NS_OK;

The spec says
DOMException		
NO_MODIFICATION_ALLOWED_ERR: Raised on an attempt to change the value of a
readonly attribute.

I looked through the interfaces, there don't seem to be any writable attributes
of type SVGAnimatedNumber, so this should probably just return
NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR. At least, that's how I understood it.
(Assignee)

Comment 57

14 years ago
(In reply to comment #56)
> I looked through the interfaces, there don't seem to be any writable attributes

Actually, baseVal is a writable attribute for AnimatedNumber.  That was one of
the errors I made in my previous idl file.
(In reply to comment #57)
> Actually, baseVal is a writable attribute for AnimatedNumber.

Sure, but the spec still says to raise an exception on setting, you even copied
it in the comment in the IDL file: "// raises DOMException on setting" :-).
The way I read it is that

readonly SVGAnimatedNumber foo;

should raise the exception when someone tries to set a value in foo.baseVal but

SVGAnimatedNumber bar;

should not for bar.baseVal. And the spec doesn't contain any interface that has
the latter so imho we should just always raise it.
(Assignee)

Comment 59

14 years ago
(In reply to comment #58)
> 
> Sure, but the spec still says to raise an exception on setting, you even copied
> it in the comment in the IDL file: "// raises DOMException on setting" :-).
> The way I read it is that
> 
> readonly SVGAnimatedNumber foo;
> 
> should raise the exception when someone tries to set a value in foo.baseVal but
> 
> SVGAnimatedNumber bar;
> 
> should not for bar.baseVal. And the spec doesn't contain any interface that has
> the latter so imho we should just always raise it.
OK, I see your point.  I'm a little uncomfortable just returning
NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR because its so inconsistent with the
rest of the svg content tree as well as the html content tree.  None of the
implementations in either tree return NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR.
 Its an easy change, but if you are correct, there is a lot of incorrect
implementation out there.  But perhaps there is a rationale for the other cases
that I'm not aware of.  In any case, another content patch forthcoming...
(Assignee)

Comment 60

14 years ago
Created attachment 157468 [details] [diff] [review]
New content files for SVG Gradients (more changes to AnimatedNumber)

The only change is a small change to nsSVGAnimatedNumber that always returns
NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR on a call to SetBaseVal.
Attachment #157294 - Attachment is obsolete: true
(Assignee)

Comment 61

14 years ago
Created attachment 157469 [details] [diff] [review]
New DOM files for SVG Gradients (removed @frozen)
(Assignee)

Updated

14 years ago
Attachment #157292 - Attachment is obsolete: true
AFAIK there are no interfaces in DOM Level 2 HTML that return
NO_MODIFICATION_ALLOWED_ERR. As for SVG DOM interfaces, there seem to be others
in the spec so if our code doesn't raise them then it's probably incorrect.

Comment 63

14 years ago
Comment on attachment 157468 [details] [diff] [review]
New content files for SVG Gradients (more changes to AnimatedNumber)


>--- nnn/svg/content/src/nsSVGAnimatedNumber.cpp	1969-12-31 16:00:00.000000000 -0800
>+++ content/svg/content/src/nsSVGAnimatedNumber.cpp	2004-08-30 22:29:50.000000000 -0700
>@@ -0,0 +1,234 @@

>+  // nsISVGValueObserver
>+  // NS_IMETHOD WillModifySVGObservable(nsISVGValue* observable);
>+  // NS_IMETHOD DidModifySVGObservable (nsISVGValue* observable);
>+  
>+  // nsISupportsWeakReference
>+  // implementation inherited from nsSupportsWeakReference

Can be removed.

>+  
>+protected:
>+  float mBaseVal;
>+};
>+
>+
>+
>+//----------------------------------------------------------------------
>+// Implementation
>+
>+nsSVGAnimatedNumber::nsSVGAnimatedNumber()
>+{
>+}
>+
>+nsSVGAnimatedNumber::~nsSVGAnimatedNumber()
>+{
>+}
>+
>+void
>+nsSVGAnimatedNumber::Init(float baseVal)
>+{
>+  mBaseVal = baseVal;
>+}
>+
>+//----------------------------------------------------------------------
>+// nsISupports methods:
>+
>+NS_IMPL_ADDREF(nsSVGAnimatedNumber)
>+NS_IMPL_RELEASE(nsSVGAnimatedNumber)
>+
>+
>+NS_INTERFACE_MAP_BEGIN(nsSVGAnimatedNumber)
>+  NS_INTERFACE_MAP_ENTRY(nsISVGValue)
>+  NS_INTERFACE_MAP_ENTRY(nsIDOMSVGAnimatedNumber)

>+//  NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
>+//  NS_INTERFACE_MAP_ENTRY(nsISVGValueObserver)

You can remove these

>+  NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO(SVGAnimatedNumber)
>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsISVGValue)
>+NS_INTERFACE_MAP_END
>+  
>+


>+/* attribute nsIDOMSVGNumber baseVal; */
>+NS_IMETHODIMP
>+nsSVGAnimatedNumber::SetBaseVal(float aBaseVal)
>+{
>+  // According to the spec, if this is a readonly AnimatedNumber
>+  // then we should return NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR.
>+  // There is also no example within the spec that uses anything
>+  // *but* a readonly AnimatedNumber (so why did they define this
>+  // to be writable?)
>+  // mBaseVal = aBaseVal;
>+  // return NS_OK;
>+  return NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR;
>+}

In the generic case baseVal should be writable. An attribute of the form 
"readonly attribute DOMAnimatedNumber foo" does *not* imply that foo.baseVal
should be readonly. It just says that setting a.foo = bar is disallowed.
Also, you should call WillModify/DidModify, otherwise modifications will not
feed through to whoever is listening in on the object.

>+//----------------------------------------------------------------------
>+// nsISVGValueObserver methods
>+
>+/*
>+NS_IMETHODIMP
>+nsSVGAnimatedNumber::WillModifySVGObservable(nsISVGValue* observable)
>+{
>+  WillModify();
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsSVGAnimatedNumber::DidModifySVGObservable (nsISVGValue* observable)
>+{
>+  DidModify();
>+  return NS_OK;
>+}
>+*/

You should remove this.

Comment 64

14 years ago
Comment on attachment 157296 [details] [diff] [review]
New layout files for SVG Gradients (SVGAnimatedNumber fix)

>--- nnn/svg/base/src/nsSVGGradientFrame.cpp	1969-12-31 16:00:00.000000000 -0800
>+++ layout/svg/base/src/nsSVGGradientFrame.cpp	2004-08-28 15:58:16.000000000 -0700
>+class nsSVGLinearGradient;
>+class nsSVGRadialGradient;

Why do you need these declarations?

>+
>+nsresult NS_NewSVGGenericContainerFrame(nsIPresShell *aPresShell, nsIContent *aContent, nsIFrame **aNewFrame);
>+static nsresult GetSVGGradient(nsISVGGradient **result, nsCAutoString& aSpec, nsIContent *aContent);
>+
>+typedef nsContainerFrame nsSVGGradientFrameBase;
>+
>+class nsSVGGradientFrame : public nsSVGGradientFrameBase,
>+                           public nsISVGValueObserver,
>+                           public nsSupportsWeakReference,
>+                           public nsISVGGradient

Since you don't (yet?) observe any properties, you should remove
nsISVGValueObserver and nsSupportsWeakReference.

>+//  nsresult Init();

please remove

>+
>+  // nsISupports interface:
>+  // NS_DECL_ISUPPORTS

please remove

>+  // nsISVGValueObserver
>+  NS_IMETHOD WillModifySVGObservable(nsISVGValue* observable);
>+  NS_IMETHOD DidModifySVGObservable (nsISVGValue* observable);
>+
>+  // nsISupportsWeakReference
>+  // implementation inherited from nsSupportsWeakReference

can go

>+  nsAutoVoidArray                      mStops;
>+  nsISVGGradient                      *mNextGrad;

Frames are not reference-counted, so you can't just hold onto them
without implementing some more management functions, or otherwise
things will fall over when content is dynamically manipulated.

The stop frames should probably not register themselves with their
parent, but should be managed by the parent with nsIFrame methods:
AppendFrames/InsertFrames/RemoveFrame etc.

As for the nsISVGGradients, it is probably easiest to resolve them each time,
or alternatively reference the corresponding content element (but you need the
style context, right?).

>+  NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
>+  NS_INTERFACE_MAP_ENTRY(nsISVGValueObserver)

Don't need these

>+//----------------------------------------------------------------------
>+// nsISVGValueObserver methods:
>+
>+NS_IMETHODIMP
>+nsSVGGradientFrame::WillModifySVGObservable(nsISVGValue* observable)
>+{
>+  return NS_OK;
>+}
>+
>+
>+NS_IMETHODIMP
>+nsSVGGradientFrame::DidModifySVGObservable(nsISVGValue* observable)
>+{
>+  return NS_OK;
>+}

Don't need this

>+#if 0
>+NS_IMETHODIMP
>+nsSVGGradientFrame::Init(nsIPresContext*  aPresContext,
>+                         nsIContent*      aContent,
>+                         nsStyleContext*  aContext,
>+                         nsIFrame*        aPrevInFlow)
>+{
>+
>+  mContent = aContent;
>+
>+  //nsresult rv = nsSVGGradientFrameBase::Init(aPresContext, aContent, (nsIFrame *)nsnull, 
>+  //                                          aContext, aPrevInFlow);
>+  // if (NS_FAILED(rv)) return rv;
>+
>+  // Init();
>+
>+  SetStyleContext(aPresContext, aContext);
>+
>+  return NS_OK;
>+}
>+#endif


Don't need this.

>+NS_IMETHODIMP
>+nsSVGGradientFrame::GetLinearGradient(nsISVGLinearGradient **aLgradient)
>+{
>+  nsCOMPtr<nsISVGLinearGradient> aLe = do_QueryInterface(mContent);
>+  *aLgradient = aLe;
>+  if (!aLe)
>+    return NS_ERROR_FAILURE;
>+  return NS_OK;
>+}

How about simplifying this to:

{
  return mContent->QueryInterface(NS_GET_IID(nsISVGLinearGradient),
(void**)aLgradient);
}

>+
>+NS_IMETHODIMP
>+nsSVGGradientFrame::GetRadialGradient(nsISVGRadialGradient **aRgradient)
>+{
>+  nsCOMPtr<nsISVGRadialGradient> aRe = do_QueryInterface(mContent);
>+  *aRgradient = aRe;
>+  if (!aRe)
>+    return NS_ERROR_FAILURE;
>+  return NS_OK;
>+}

ditto

>+
>+NS_IMETHODIMP
>+nsSVGGradientFrame::AddStop(nsIFrame *aStopFrame)
>+{
>+//  NS_ADDREF(aStopFrame);
>+  mStops.AppendElement((void*)aStopFrame);
>+  return NS_OK;
>+}

Not sure you want this; see comment above.

>+
>+NS_IMETHODIMP
>+nsSVGGradientFrame::GetGradientUnits(PRUint16 *aUnits)
>+{
>+  nsCOMPtr<nsIDOMSVGAnimatedEnumeration> aEnum;
>+  nsCOMPtr<nsIDOMSVGGradientElement> aGrad = do_QueryInterface(mContent);
>+  NS_ASSERTION(aGrad, "Tried to get Linear value from a non-linear gradient");

Misleading assertion comment.

>+NS_IMETHODIMP
>+nsSVGGradientFrame::GetGradientTransform(nsIDOMSVGMatrix **aGradientTransform)
>+{
>+  nsCOMPtr<nsIDOMSVGAnimatedTransformList> aTrans;
>+  nsCOMPtr<nsIDOMSVGGradientElement> aGrad = do_QueryInterface(mContent);
>+  NS_ASSERTION(aGrad, "Tried to get Linear value from a non-linear gradient");

Misleading assertion comment.

>+
>+NS_IMETHODIMP
>+nsSVGGradientFrame::GetSpreadMethod(PRUint16 *aSpreadMethod)
>+{
>+  nsCOMPtr<nsIDOMSVGAnimatedEnumeration> aEnum;
>+  nsCOMPtr<nsIDOMSVGGradientElement> aGrad = do_QueryInterface(mContent);
>+  NS_ASSERTION(aGrad, "Tried to get Linear value from a non-linear gradient");

Misleading assertion comment.

>+NS_INTERFACE_MAP_BEGIN(nsSVGLinearGradientFrame)
>+  NS_INTERFACE_MAP_ENTRY(nsISVGLinearGradient)

>+  NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
>+  NS_INTERFACE_MAP_ENTRY(nsISVGValueObserver)

These 2 can be removed

>+NS_INTERFACE_MAP_END_INHERITING(nsSVGLinearGradientFrameBase)


>+
>+// Implementation
>+nsresult
>+nsSVGLinearGradientFrame::GetX1(float *aX1)
>+{
>+  nsCOMPtr<nsIDOMSVGLinearGradientElement> aLgrad = do_QueryInterface(mContent);
>+  NS_ASSERTION(aLgrad, "Tried to get Linear value from a non-linear gradient");

Misleading assertion comment (mContent could be something other than a
non-linear gradient).
Elsewhere as well.


>+NS_INTERFACE_MAP_BEGIN(nsSVGRadialGradientFrame)
>+  NS_INTERFACE_MAP_ENTRY(nsISVGRadialGradient)

>+  NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
>+  NS_INTERFACE_MAP_ENTRY(nsISVGValueObserver)

These 2 can go.

>+NS_INTERFACE_MAP_END_INHERITING(nsSVGRadialGradientFrameBase)


>+nsresult NS_NewSVGStopFrame(nsIPresShell* aPresShell, 
>+                            nsIContent*   aContent, 
>+                            nsIFrame*     aParentFrame, 
>+                            nsIFrame**    aNewFrame)
>+{

>+  // Add ourselves in
>+  aGFrame->AddStop(*aNewFrame);

See comments above.


>+// Static (helper) function to get a gradient from URI spec
>+static nsresult GetSVGGradient(nsISVGGradient **result, nsCAutoString& aSpec, nsIContent *aContent)
>+{

>+  nsCOMPtr<nsIPresShell> ps = do_QueryInterface(aContent->GetDocument()->GetShellAt(0));

Instead of taking the first shell, GetSVGGradient should take a
presshell argument, so that we get the correct frame tree.


>+  // Note: the following queries are not really required, but we want to make sure
>+  // we're pointing to gradient elements.

Do you mean they are really not required (in which case this should
only be compiled into debug builds), or are they required, but only
for the sideeffect of testing whether the given interfaces are
implemented?

>--- nnn/svg/renderer/public/nsISVGGradient.idl	1969-12-31 16:00:00.000000000 -0800
>+++ layout/svg/renderer/public/nsISVGGradient.idl	2004-08-24 18:23:57.000000000 -0700
>@@ -0,0 +1,114 @@
>+ * Contributor(s):
>+ *    Alex Fritze <alex.fritze@crocodile-clips.com> (original author)

Should be you.

>+/**
>+ * Describes the 'gradient' objects (either linear or a radial) in the SVG
>+ * rendering backend. The rendering backend maintains an object
>+ * implementing this interface for each rendering engine-native
>+ * gradient object.

No, gradient objects native to the rendering engine are not exposed
(i.e. we don't have a "nsISVGRendererGradient"). The comment applies
only to situations where we have an object in the rendering engine and
a corresponding 'callback' object on the backend side
(e.g. "nsISVGRendererPathGeometry" -- "nsISVGPathGeometrySource") and
both are managed by the rendering backend.


>+[scriptable, uuid(e56bc6e1-26ea-4e30-afec-531a53179022)]

If you remove "scriptable" here, then you don't need all those [noscript]s
below.

>+interface nsISVGGradient : nsISupports
>+{

>+  [noscript] void GetLinearGradient(out nsISVGLinearGradient aLinearGradient);
>+  [noscript] void GetRadialGradient(out nsISVGRadialGradient aRadialGradient);

Why are these needed? (Why not just QI?)


>+  [noscript] void AddStop(in nsIFrame aStopFrame);
>+  [noscript] void GetNextGradient(out nsISVGGradient aGradient, in PRUint32 aGradType);

These 2 don't really belong into an interface exposed to rendering
engines. Rendering engines shouldn't have to know about frames, and
also they shouldn't have to know about implementations details, such
as the fact that in SVG gradients can be chained together. (I could
just about live with GetNextGradient in here though :-).

>--- nnn/svg/renderer/src/libart/nsSVGLibartGradient.cpp	1969-12-31 16:00:00.000000000 -0800
>+++ layout/svg/renderer/src/libart/nsSVGLibartGradient.cpp	2004-08-21 00:48:50.000000000 -0700
>@@ -0,0 +1,309 @@

>+ * Contributor(s):
>+ *   Alex Fritze <alex.fritze@crocodile-clips.com> (original author)
>+ *   Bradley Baetz <bbaetz@cs.mcgill.ca>

Should be you.

>+// Calculate the bounding box for this gradient
>+static void GetBounds(nsISVGLibartRegion *aRegion, ArtIRect *rect)
>+{

The region doesn't give you the exact BB. Microtiles sometimes
overshoot. I can live with that though.

>+
>+static ArtGradientStop *
>+GetStops(nsISVGGradient *aGrad, nsISVGLibartCanvas *aCanvas) {

>+static void
>+LibartLinearGradient(ArtRender *render, nsISVGGradient *aGrad, double *affine,
>+                     nsISVGLibartCanvas *aCanvas)

>+static void
>+LibartRadialGradient(ArtRender *render, nsISVGGradient *aGrad, double *affine,
>+                     nsISVGLibartCanvas *aCanvas)

>+void
>+LibartGradient(ArtRender *render, nsIDOMSVGMatrix *aMatrix, 
>+               nsISVGGradient *aGrad, nsISVGLibartRegion *aRegion,
>+               nsISVGLibartCanvas *aCanvas)

Why are you passing the canvas into all of these functions?


>+  if (bbox == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX) 
>+  {
>+    // BoundingBox
>+    // We need to calculate this from the Region (Uta?) in
>+    // the object we're filling
>+    ArtIRect aBoundsRect;
>+    GetBounds(aRegion, &aBoundsRect);

Hmm, not sure if this is right. What happens if you fill a <g> with
several children? Shouldn't we fill on the <g>'s boundingbox?
(I.e. shouldn't we get the bb from nsISVGGradient?)
Comment on attachment 157293 [details] [diff] [review]
Changes to DOM files for SVG Gradients (another tree refresh)

Here's some review comments from me too. Some of my comments are nits, but I
think they might as well be tidied. 

First attachment: Changes to DOM files for SVG Gradients (another tree refresh)

You seem to be listing things in alphabetical order, except that you have just
appended nsIDOMSVGGradientElement.idl and nsIDOMSVGStopElement.idl to the end
of XPIDLSRCS in dom/public/idl/svg/Makefile.in
Comment on attachment 157469 [details] [diff] [review]
New DOM files for SVG Gradients (removed @frozen)

File: New DOM files for SVG Gradients (removed @frozen)

Many of the IDL files in the "New DOM files for SVG Gradients (removed
@frozen)" patch still contain "@status FROZEN". Is this intentional? 

The indentation in dom/public/idl/svg/nsIDOMSVGAnimatedNumber.idl is wrong at
the following line:
		// raises DOMException on setting
you've used tabs instead of the equivalent number of spaces.

Personally I'd prefer not to have the comments such as "The
nsIDOMSVGAnimatedNumber interface is the interface to an SVG Animated Number"
at the top of the IDL files. The existing IDL files don't have such
descriptions, and I think superficial descriptions add clutter without adding
any real value. The links to the parts of the spec that describe the interface
could be useful, but maybe you could use the "Corresponds to" convetion at
http://lxr.mozilla.org/seamonkey/source/dom/public/idl/svg/nsIDOMSVGException.i
dl#40 just for consistancy (nit). 

nsIDOMSVGGradientElement.idl:
This file doesn't seem to end with a blank line. Having the EOF at the end of a
non-blank line can cause problems on some operating systems or something I
believe. 

dom/public/idl/svg/nsIDOMSVGStopElement.idl also appears not to end in a blank
line.
Comment on attachment 157295 [details] [diff] [review]
Changes to content files for SVG Gradients (refreshed to current tree)

File: Changes to content files for SVG Gradients (refreshed to current tree)

mozilla/content/shared/public/nsSVGAtomList.h:
Can use:
SVG_ATOM(linearGradient, "linearGradient")
instead of:
SVG_ATOM(linear_gradient, "linearGradient")
and:
SVG_ATOM(radialGradient, "radialGradient")
instead of:
SVG_ATOM(radial_gradient, "radialGradient")
and save the underscore for being a replacement for the dash in attribute
names? As you can see foreignObject etc. doesn't use an underscore, so it is
consistant with the existing code and looks better. 


mozilla/content/svg/content/src/Makefile.in:
In CPPSRCS, nsSVGStopElement.cpp would come before nsSVGSVGElement.cpp


mozilla/content/svg/content/src/nsSVGNumber.cpp:
You shouldn't change:
  if(!*result) return NS_ERROR_OUT_OF_MEMORY;
to:
  NS_ENSURE_TRUE(*result, NS_ERROR_OUT_OF_MEMORY);
The NS macros issue warnings which you don't want for an out of memory
condition.
(Assignee)

Comment 68

14 years ago
(In reply to comment #64)
> 
> >+  nsAutoVoidArray                      mStops;
> >+  nsISVGGradient                      *mNextGrad;
> 
> Frames are not reference-counted, so you can't just hold onto them
> without implementing some more management functions, or otherwise
> things will fall over when content is dynamically manipulated.
> 
> The stop frames should probably not register themselves with their
> parent, but should be managed by the parent with nsIFrame methods:
> AppendFrames/InsertFrames/RemoveFrame etc.
I'm a little confused by the Frames interface.  I need to be able to provide
information about StopElements to the renderers, most probably by index.  If I
use AppendFrames, etc., is there an interface I can use to get access to the
child frames, or do I just use mFrames.FirstChild() and GetNextSibling()?  Then
the AddStop() call would be replaced in the StopFrame code by a call to
AppendFrames()?
> 
> As for the nsISVGGradients, it is probably easiest to resolve them each time,
> or alternatively reference the corresponding content element (but you need the
> style context, right?).
> 
I definitely need the style context, so I can't just use the content element (in
fact, that's where I started, and why I felt I had to go this way).


> 
> >+// Static (helper) function to get a gradient from URI spec
> >+static nsresult GetSVGGradient(nsISVGGradient **result, nsCAutoString&
aSpec, nsIContent *aContent)
> >+{
> 
> >+  nsCOMPtr<nsIPresShell> ps =
do_QueryInterface(aContent->GetDocument()->GetShellAt(0));
> 
> Instead of taking the first shell, GetSVGGradient should take a
> presshell argument, so that we get the correct frame tree.
> 
OK, I think I went this way because it wasn't clear that I could get the correct
frame tree from SVGPathGeometryFrame and SVGGlyphFrame.   Is there a simple way
to access the nsIPresShell for these?
> 
> >+  // Note: the following queries are not really required, but we want to
make sure
> >+  // we're pointing to gradient elements.
> 
> Do you mean they are really not required (in which case this should
> only be compiled into debug builds), or are they required, but only
> for the sideeffect of testing whether the given interfaces are
> implemented?

The latter.

> >+  if (bbox == nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX) 
> >+  {
> >+    // BoundingBox
> >+    // We need to calculate this from the Region (Uta?) in
> >+    // the object we're filling
> >+    ArtIRect aBoundsRect;
> >+    GetBounds(aRegion, &aBoundsRect);
> 
> Hmm, not sure if this is right. What happens if you fill a <g> with
> several children? Shouldn't we fill on the <g>'s boundingbox?
> (I.e. shouldn't we get the bb from nsISVGGradient?)
> 
Probably, but I couldn't find an easy way to get the bb for the Gradient.  The
reason that I had initially had the Canvas passed was to try to get the bb.  Is
there an interface I could use to get the bounding box?

Comment 69

14 years ago
(In reply to comment #68)
>  If I use AppendFrames, etc., is there an interface I can use to get access to the
> child frames, or do I just use mFrames.FirstChild() and GetNextSibling()?  

Yes, you'd use FirstChild/GetNextSibling to traverse the children.

> Then the AddStop() call would be replaced in the StopFrame code by a call to
> AppendFrames()?

No, you don't need to call anything from the StopFrame. Appending (and removing)
stop frames to the parent frame is handled in nsCSSFrameConstructor.cpp.

> SVGPathGeometryFrame and SVGGlyphFrame.   Is there a simple way
> to access the nsIPresShell for these?

GetPresContext()->PresShell()
 
> Probably, but I couldn't find an easy way to get the bb for the Gradient.  The
>  Is
> there an interface I could use to get the bounding box?

You can ask tor; he's just done something like this.
Comment on attachment 157468 [details] [diff] [review]
New content files for SVG Gradients (more changes to AnimatedNumber)

File: New content files for SVG Gradients (more changes to AnimatedNumber)

nsSVGAnimatedEnumeration.cpp

remove one of these:
+#include "nsSVGValue.h"
+#include "nsSVGValue.h"

can you replace the tabs in the second of the following two lines with spaces
please:
+  friend nsresult NS_NewSVGAnimatedEnumeration(nsIDOMSVGAnimatedEnumeration**
result,
+					       nsISVGEnum* baseVal);
and line up nsIDOMSVGAnimatedEnumeration and nsISVGEnum?

do_QueryInterface is null-safe so, according to brendan, you should change:

+nsSVGAnimatedEnumeration::~nsSVGAnimatedEnumeration()
+{
+  if (!mBaseVal) return;
+  nsCOMPtr<nsISVGValue> val = do_QueryInterface(mBaseVal);
+  if (!val) return;
+  val->RemoveObserver(this);
+}

to:

nsSVGAnimatedEnumeration::~nsSVGAnimatedEnumeration()
{
  nsCOMPtr<nsISVGValue> val = do_QueryInterface(mBaseVal);
  if (val) val->RemoveObserver(this);
}

also, maybe nsSVGAnimatedEnumeration::Init should be returning a success code?
In:

+NS_IMETHODIMP
+nsSVGAnimatedEnumeration::SetValueString(const nsAString& aValue)
+{
+  nsCOMPtr<nsISVGValue> value = do_QueryInterface(mBaseVal);
+  return value->SetValueString(aValue);
+}

since do_QueryInterface could fail (bad given the ->SetValueString) you should
use something like:

  nsresult rv;
  nsCOMPtr<nsISVGValue> value = do_QueryInterface(mBaseVal, &rv);
  return NS_FAILED(rv)? rv: value->SetValueString(aValue);

The same for GetValueString, use something like:

  nsresult rv;
  nsCOMPtr<nsISVGValue> value = do_QueryInterface(mBaseVal);
  return NS_FAILED(rv)? rv: value->GetValueString(aValue);

Nits: In NS_NewSVGAnimatedEnumeration can you put in some more spaces to get
the following lined up?

+NS_NewSVGAnimatedEnumeration(nsIDOMSVGAnimatedEnumeration** aResult,
+			 nsISVGEnum* baseVal)

and change baseVal to aBaseVal, and put a space after the "if" at:

+  if(!animatedEnum) return NS_ERROR_OUT_OF_MEMORY;

and maybe you should return the rv from animatedEnum->Init, even though you
should probably ignore whether it fails or not in NS_NewSVGAnimatedEnumeration.


content/svg/content/src/nsSVGAnimatedNumber.cpp

Can you change "baseVal" to "aBaseVal" in nsSVGAnimatedNumber::Init?


content/svg/content/src/nsSVGAnimatedNumber.cpp

In NS_NewSVGAnimatedNumber can you put a space after the "if" please at:

+  if(!animatedNumber) return NS_ERROR_OUT_OF_MEMORY;


content/svg/content/src/nsSVGAnimatedNumberList.cpp

In nsSVGAnimatedNumberList::~nsSVGAnimatedNumberList, can you change:

+  if (!mBaseVal) return;
+    nsCOMPtr<nsISVGValue> val = do_QueryInterface(mBaseVal);
+  if (!val) return;
+  val->RemoveObserver(this);

to:

  if (!mBaseVal) return;
  nsCOMPtr<nsISVGValue> val = do_QueryInterface(mBaseVal);
  if (val)
    val->RemoveObserver(this);

since the do_QueryInterface line isn't controlled by the if statement above,
and there isn't any point in putting in a return statement below that. 

In nsSVGAnimatedNumberList::Init can you change "baseVal" to "aBaseVal".

Can you change SetValueString and GetValueString to safeguard against nsnull
being returned from do_QueryInterface as shown above. 

In NS_NewSVGAnimatedNumberList can you change "baseVal" to "aBaseVal", and I'm
not sure you should 


content/svg/content/src/nsSVGEnum.cpp

In the two NS_NewSVGEnum functions we don't want warnings for out of memory
conditions, please change:

+  NS_ENSURE_TRUE(pe, NS_ERROR_OUT_OF_MEMORY);

to:

  if (!pe) return NS_ERROR_OUT_OF_MEMORY;


content/svg/content/src/nsSVGEnum.h:

The first whitespace characters for the indentation of the following two lines:
+	      PRUint16 value,
+	      struct nsSVGEnumMapping *mapping);
are tabs, not spaces.

Same for:
+	      const nsAString &value,
+	      struct nsSVGEnumMapping *mapping);


content/svg/content/src/nsSVGGradientElement.cpp:

You've used tabs again here:
+	{&nsSVGAtoms::objectBoundingBox,
nsIDOMSVGGradientElement::SVG_GRUNITS_OBJECTBOUNDINGBOX},
+	{&nsSVGAtoms::userSpaceOnUse,
nsIDOMSVGGradientElement::SVG_GRUNITS_USERSPACEONUSE},
+	{nsnull, 0}

and here:
+	{&nsSVGAtoms::pad, nsIDOMSVGGradientElement::SVG_SPREADMETHOD_PAD},
+	{&nsSVGAtoms::reflect,
nsIDOMSVGGradientElement::SVG_SPREADMETHOD_REFLECT},
+	{&nsSVGAtoms::repeat,
nsIDOMSVGGradientElement::SVG_SPREADMETHOD_REPEAT},
+	{nsnull, 0}
(Assignee)

Comment 71

14 years ago
(In reply to comment #70)
> (From update of attachment 157468 [details] [diff] [review])
Jonathan, most of these are in, but this comment is certainly inconsistent with
the rest of the SVG code base, where we usually just use NS_ASSERTION after a
call to do_QueryInterface (which I don't even do).  I'll add an assertion test
to avoid the crash.
> In:
> 
> +NS_IMETHODIMP
> +nsSVGAnimatedEnumeration::SetValueString(const nsAString& aValue)
> +{
> +  nsCOMPtr<nsISVGValue> value = do_QueryInterface(mBaseVal);
> +  return value->SetValueString(aValue);
> +}
> 
> since do_QueryInterface could fail (bad given the ->SetValueString) you should
> use something like:
> 
>   nsresult rv;
>   nsCOMPtr<nsISVGValue> value = do_QueryInterface(mBaseVal, &rv);
>   return NS_FAILED(rv)? rv: value->SetValueString(aValue);
> 
> The same for GetValueString, use something like:
> 
>   nsresult rv;
>   nsCOMPtr<nsISVGValue> value = do_QueryInterface(mBaseVal);
>   return NS_FAILED(rv)? rv: value->GetValueString(aValue);
> 
Scooter, I just discussed this in #developers. Certainly the SVG code that does
not protect against do_QueryInterface returning NULL should be corrected.
However an NS_ASSERTION line by itself does not protect against crashes in
release builds because it is simply stripped out leaving a blank line. On the
other hand apparently my suggestion is also defficient since it does not
guarantee that the failure code is propogated and brought to our attention. So
the options are these:

  nsresult rv;
  nsCOMPtr<nsISVGValue> value = do_QueryInterface(mBaseVal, &rv);
  NS_ASSERTION(value, "blah");  // or NS_ASSERTION(NS_SUCCEEDED(rv), "blah");
  return NS_FAILED(rv)? rv: value->GetValueString(aValue);

Using this we will get an assertion in debug builds bringing our attention to
the problem, but in non-debug builds we still protect against a crash and the
failure code is propogated as best we can. 

  nsresult rv;
  nsCOMPtr<nsISVGValue> value = do_QueryInterface(mBaseVal, &rv);
  NS_ENSURE_SUCCESS(rv,rv);
  return value->GetValueString(aValue);

This is roughly the same, but in debug builds we only get a warning in the
console, and the compiler may not optimise it to be as fast and small as if we
use the :? operator. 

Personally I prefer the first way of doing it because I think we should assert
for something as serious as a QI failure at this bit of the code. It is also
guaranteed to be as fast as you can get while still propogating the failure code
in release builds that strip out the assert. I know this isn't how we do it in
the code just now, but then I think we should start to asap. 
(Assignee)

Comment 73

14 years ago
(In reply to comment #72)
> Scooter, I just discussed this in #developers. Certainly the SVG code that does
> not protect against do_QueryInterface returning NULL should be corrected.
> However an NS_ASSERTION line by itself does not protect against crashes in
> release builds because it is simply stripped out leaving a blank line. On the
> other hand apparently my suggestion is also defficient since it does not
> guarantee that the failure code is propogated and brought to our attention. So
> the options are these:
> 
>   nsresult rv;
>   nsCOMPtr<nsISVGValue> value = do_QueryInterface(mBaseVal, &rv);
>   NS_ASSERTION(value, "blah");  // or NS_ASSERTION(NS_SUCCEEDED(rv), "blah");
>   return NS_FAILED(rv)? rv: value->GetValueString(aValue);
> 
> Using this we will get an assertion in debug builds bringing our attention to
> the problem, but in non-debug builds we still protect against a crash and the
> failure code is propogated as best we can. 
> 
>   nsresult rv;
>   nsCOMPtr<nsISVGValue> value = do_QueryInterface(mBaseVal, &rv);
>   NS_ENSURE_SUCCESS(rv,rv);
>   return value->GetValueString(aValue);
> 
> This is roughly the same, but in debug builds we only get a warning in the
> console, and the compiler may not optimise it to be as fast and small as if we
> use the :? operator. 
> 
> Personally I prefer the first way of doing it because I think we should assert
> for something as serious as a QI failure at this bit of the code. It is also
> guaranteed to be as fast as you can get while still propogating the failure code
> in release builds that strip out the assert. I know this isn't how we do it in
> the code just now, but then I think we should start to asap. 

OK, I looked at the code a little more closely, and assert that the check is
probably unnecessary, but I'm happy to add it in, if that's the consensus.  In
this particular case (nsSVGAnimatedEnum) the only way to set mBaseVal is through
Init, which checks for the appropriate interface at that point.  I suppose its
possible to have somebody free it out from underneath us, but that would be
pretty ugly for a variety of reasons....

Comment 74

14 years ago
(In reply to comment #73)

> OK, I looked at the code a little more closely, and assert that the check is
> probably unnecessary, but I'm happy to add it in, if that's the consensus.  In
> this particular case (nsSVGAnimatedEnum) the only way to set mBaseVal is through
> Init, which checks for the appropriate interface at that point. 

No, it's not the consensus. The QI can't fail unless the nsSVGAnimatedEnum code
is changed significantly. You want an assertion at most, nothing more. 
(Assignee)

Comment 75

14 years ago
Created attachment 159686 [details] [diff] [review]
New DOM files for SVG Gradients (incorporates last round of reviews)
Attachment #157469 - Attachment is obsolete: true
(Assignee)

Comment 76

14 years ago
Created attachment 159687 [details] [diff] [review]
Changes to DOM files for SVG Gradients (incorporates last round of reviews)
Attachment #157293 - Attachment is obsolete: true
(Assignee)

Comment 77

14 years ago
Created attachment 159688 [details] [diff] [review]
New content files for SVG Gradients (incorporates last round of reviews)
Attachment #157468 - Attachment is obsolete: true
(Assignee)

Comment 78

14 years ago
Created attachment 159689 [details] [diff] [review]
Changes to content files for SVG Gradients (incorporates last round of reviews)
(Assignee)

Comment 79

14 years ago
Created attachment 159690 [details] [diff] [review]
New layout files for SVG Gradients (incorporates *most* of last round of reviews)
Attachment #157296 - Attachment is obsolete: true
(Assignee)

Comment 80

14 years ago
Created attachment 159691 [details] [diff] [review]
Changes to layout files for SVG Gradient support (incorporates last round of reviews)
Attachment #157297 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #157295 - Attachment is obsolete: true
(Assignee)

Comment 81

14 years ago
OK, here are the updated patches.  After looking at Tim's BBox stuff, it seemed
like the best short term approach was to punt until we see how that shakes out.
 So, the bounding box computations might be wrong.  Its not clear to me how to
get the bounding box of the gradient anyways since the gradient is probably a
child of a <defs> element frame and not the actual graphic element that is using
it.  I think that this is going to be a big issue when we deal with <use>
(Assignee)

Updated

14 years ago
Attachment #159686 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #159687 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #159688 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #159689 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #159690 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #159691 - Flags: review?

Comment 82

14 years ago
Comment on attachment 159690 [details] [diff] [review]
New layout files for SVG Gradients (incorporates *most* of last round of reviews)

r=afri
There is probably a better way to manage the stop frames (by putting them into
their own frame list or something like that). 
Also, as I said before 'GetNextGradient' shouldn't really be in nsISVGGradient
- but we can live with that.
Finally, the implementation will probably have problems with dynamic content
changes. But I guess that's also something we can sort out later, along with
the bbox stuff
Attachment #159690 - Flags: review? → review+

Comment 83

14 years ago
Comment on attachment 159691 [details] [diff] [review]
Changes to layout files for SVG Gradient support (incorporates last round of reviews)

r=afri
Attachment #159691 - Flags: review? → review+

Comment 84

14 years ago
Comment on attachment 159687 [details] [diff] [review]
Changes to DOM files for SVG Gradients (incorporates last round of reviews)

nsRuleNode.cpp:

  Is the new include of nsCSSKeywords.h needed?

nsStyleStruct.h:

  Elements of nsStyleSVG are sorted alphabetically.

nsStyleStruct.cpp:

  mStopColor/mStopOpacity missing from constructors and CalcDifference

nsSVGNumber.cpp:

  Instead of doing whitespace skipping, perhaps use the string library's
  Trim to clean the string (leading and trailing).

  Check to make sure nothing follows the number, so we don't accept
  "50% foobar" as a number?

Comment 85

14 years ago
Comment on attachment 159689 [details] [diff] [review]
Changes to content files for SVG Gradients (incorporates last round of reviews)

Two more -

nsSVGStyleStruct.cpp:

  Use EqualURIs in nsStyleSVGPaint::operator==

nsSVGNumber.cpp:

  Should check if rest is NULL.
(Assignee)

Comment 86

14 years ago
Created attachment 160298 [details] [diff] [review]
Changes to content files for SVG Gradients (incorporates tor's comments)

Tim,
  Thanks for the input.  I incorporated most of your comments except for using
Trim, which (I think) would have required some string conversion which I wasn't
sure was justified.  I can certainly do it if you think I really should.
Attachment #159689 - Attachment is obsolete: true

Comment 87

14 years ago
Comment on attachment 160298 [details] [diff] [review]
Changes to content files for SVG Gradients (incorporates tor's comments)

nsRuleNode.cpp:

  SetSVGOpacity - now that the stop opacity is set in the style
  structure constructor, do we need this default value set anymore?

nsStyleStruct.cpp:

  CalcDifference - with the comparison operator, you don't need the
  check of m{Stroke,Fill}.mType.

nsSVGNumber.cpp:

  SetValueString - strtod skips leading whitespace, so you don't need
  to skip it yourself.	The non-percent case should probably also have
  the trailing garbage check.  Pulling the rest!=number test out might
  simplify the logic.

Comment 88

14 years ago
Comment on attachment 160298 [details] [diff] [review]
Changes to content files for SVG Gradients (incorporates tor's comments)

nsSVGNumber.cpp:

  SetValueString - should we allow trailing whitespace, ex: " 42% "?

Updated

14 years ago
Attachment #159687 - Flags: review? → superreview+

Updated

14 years ago
Attachment #156900 - Flags: review?(alex)

Updated

14 years ago
Attachment #156893 - Flags: review?(scootermorris)

Updated

14 years ago
Attachment #159686 - Flags: review? → review+

Updated

14 years ago
Attachment #159689 - Flags: review?

Comment 89

14 years ago
Comment on attachment 159688 [details] [diff] [review]
New content files for SVG Gradients (incorporates last round of reviews)

All:

  Copyright dates are all 2001,2002.  Files created by me should
  have an initial developer of IBM Corporation, files created by
  you should probably have someone other than Crocodile Clips as
  the originator.

nsSVGAnimatedNumber:

  Parser has the same issues as the nsSVGNumber one.

nsSVGGradientElement:

  Remove commented include of nsSVGAtoms.h.

  Use NS_IMPL_NS_NEW_SVG_ELEMENT/NS_IMPL_SVG_DOM_CLONENODE for
  Linear/Radial.

nsSVGNumberList:

  SetValueString - for those of us who don't have the ascii table
  memorized, could the delimiter list be written ", \t\r\n"?

  Any reason you're putting off writing InsertItemBefore and
  ReplaceItem?

nsSVGStopElement:

  Use NS_IMPL_NS_NEW_SVG_ELEMENT/NS_IMPL_SVG_DOM_CLONENODE.
In reply to this (and similar) comments:
>   SetValueString - should we allow trailing whitespace, ex: " 42% "?

I looked at the SVG spec, and couldn't see anywhere that allowed either leading
or trailing whitespace in attributes, except around commas and between values in
attributes that contain lists. So it looks like we can simplify this even
further and just not do any whitespace skipping, unless you saw something in the
spec that said otherwise.
Hixie:

Or maybe the fact that it doesn't say white space isn't allowed means we
shouldn't add unnecessary restrictions? Should we even check for "50% foobar"?
If we obtain something valid should we just ignore whatever follows to save
footprint? Do we have to validate? 

Scooter:

Don't worry too much about SVGNumberList, I intend to fix up all the SVGXxxList
classes as part of a bug to use a common base class to reduce footprint. Here
are some more comments on your current patches:

dom/public/idl/svg/nsIDOMSVGGradientElement.idl:

nsIDOMSVGGradientElement is inheriting the interface
nsIDOMSVGExternalResourcesRequired, but I don't see the file for this interface
anywhere.


mozilla/content/svg/content/src/nsSVGNumber.cpp:

You shouldn't change:
  if(!*result) return NS_ERROR_OUT_OF_MEMORY;
to:
  NS_ENSURE_TRUE(*result, NS_ERROR_OUT_OF_MEMORY);
The NS macros issue warnings which you don't want for an out of memory
condition. 


can you replace the tabs in the second of the following two lines with spaces
please:
+  friend nsresult NS_NewSVGAnimatedEnumeration(nsIDOMSVGAnimatedEnumeration**
result,
+					       nsISVGEnum* baseVal);
and line up nsIDOMSVGAnimatedEnumeration with nsISVGEnum, and change "baseVal"
to "aBaseVal"?


do_QueryInterface is null-safe so you don't need the null check in:

+nsSVGAnimatedEnumeration::~nsSVGAnimatedEnumeration()
+{
+  if (!mBaseVal) return;
+  nsCOMPtr<nsISVGValue> val = do_QueryInterface(mBaseVal);
+  if (val) val->RemoveObserver(this);
+}


content/svg/content/src/nsSVGAnimatedNumber.cpp

+  friend nsresult NS_NewSVGAnimatedNumber(nsIDOMSVGAnimatedNumber** result,
+                                          float baseVal);

nit: baseVal -> aBaseVal ?


content/svg/content/src/nsSVGAnimatedNumberList.cpp

In nsSVGAnimatedNumberList::~nsSVGAnimatedNumberList the first line simply
increases footprint and slows things by making an unnecessary null check.
do_QueryInterface is null safe. Also the second line is indented two spaces to many:

+  if (!mBaseVal) return;
+    nsCOMPtr<nsISVGValue> val = do_QueryInterface(mBaseVal);

In NS_NewSVGAnimatedNumberList can you put a space after the "if" statement and
the opening parenthesis. 

In nsSVGAnimatedNumberList::Set/GetValueString can you put in an assertion to
check if do_QueryInterface returns null. 
(Assignee)

Comment 92

14 years ago
Created attachment 160778 [details] [diff] [review]
New dom files for SVG Gradient support
Attachment #159686 - Attachment is obsolete: true
(Assignee)

Comment 93

14 years ago
Created attachment 160779 [details] [diff] [review]
Changes to DOM files for SVG Gradient support (no changes)
Attachment #159687 - Attachment is obsolete: true
(Assignee)

Comment 94

14 years ago
Created attachment 160780 [details] [diff] [review]
New content files for SVG Gradient Support
Attachment #159688 - Attachment is obsolete: true
(Assignee)

Comment 95

14 years ago
Created attachment 160781 [details] [diff] [review]
Changes to content files for SVG Gradient support
Attachment #160298 - Attachment is obsolete: true
(Assignee)

Comment 96

14 years ago
Created attachment 160782 [details] [diff] [review]
New layout files for SVG Gradient support (updated copyrights)
Attachment #159690 - Attachment is obsolete: true
(Assignee)

Comment 97

14 years ago
Created attachment 160783 [details] [diff] [review]
Changes to layout files for SVG Gradient support (no changes from previous patch)
Attachment #159691 - Attachment is obsolete: true
(Assignee)

Comment 98

14 years ago
Comment on attachment 160779 [details] [diff] [review]
Changes to DOM files for SVG Gradient support (no changes)

No changes from previous, reviewed patch
(Assignee)

Updated

14 years ago
Attachment #159687 - Attachment is obsolete: false
(Assignee)

Updated

14 years ago
Attachment #160779 - Attachment description: Changes to DOM files for SVG Gradient support → Changes to DOM files for SVG Gradient support (no changes)
(Assignee)

Updated

14 years ago
Attachment #160782 - Attachment description: New layout files for SVG Gradient support → New layout files for SVG Gradient support (updated copyrights)
(Assignee)

Updated

14 years ago
Attachment #160783 - Attachment description: Changes to layout files for SVG Gradient support → Changes to layout files for SVG Gradient support (no changes)

Updated

14 years ago
Attachment #160779 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #160783 - Attachment description: Changes to layout files for SVG Gradient support (no changes) → Changes to layout files for SVG Gradient support (no changes from previous patch)

Updated

14 years ago
Attachment #160783 - Attachment is obsolete: true

Comment 99

14 years ago
Comment on attachment 160782 [details] [diff] [review]
New layout files for SVG Gradient support (updated copyrights)

Only copyright comment changes - transferring r=afri.
Attachment #160782 - Flags: review+

Updated

14 years ago
Attachment #159691 - Attachment is obsolete: false
(Assignee)

Comment 100

14 years ago
Created attachment 160790 [details] [diff] [review]
Changes to content files for SVG Gradient Support (alphabetized entries in nsCSSParser and nsCSSStruct)
Attachment #160781 - Attachment is obsolete: true
> Or maybe the fact that it doesn't say white space isn't allowed means we
> shouldn't add unnecessary restrictions? Should we even check for "50% foobar"?
> If we obtain something valid should we just ignore whatever follows to save
> footprint? Do we have to validate? 

SVG clearly says we do have to validate (see section F.2 in SVG 1.1) but the 
spec is rather unclear about whitespace in non-list attributes. I've contacted 
the working group (see www-svg). When that's resolved I'll update my test cases 
on hixie.ch to match what the group decides.

Comment 102

14 years ago
Comment on attachment 159691 [details] [diff] [review]
Changes to layout files for SVG Gradient support (incorporates last round of reviews)

sr=tor for nsCSSFrameConstructor.cpp changes.
Attachment #159691 - Flags: superreview+

Comment 103

14 years ago
Comment on attachment 160790 [details] [diff] [review]
Changes to content files for SVG Gradient Support (alphabetized entries in nsCSSParser and nsCSSStruct)

We might have to revisit the number parsing based on WG feedback, 
but this will work for now.
Attachment #160790 - Flags: review+

Comment 104

14 years ago
Comment on attachment 160790 [details] [diff] [review]
Changes to content files for SVG Gradient Support (alphabetized entries in nsCSSParser and nsCSSStruct)

dbaron, could you sr the non-svg-only files in this patch?
It's the changes to the style system for svg gradients.
Attachment #160790 - Flags: superreview?(dbaron)

Updated

14 years ago
Attachment #159687 - Flags: review?(jonathan.watt)
(Assignee)

Comment 105

14 years ago
Comment on attachment 156893 [details] [diff] [review]
update cairo gradient code to latest set of gradient patches

Just a couple of minor changes -- some include files that aren't necessary, but
other than that, it looks good to me!


>--- nsSVGCairoGlyphGeometry.cpp	5 Aug 2004 09:01:12 -0000	1.5
>+++ nsSVGCairoGlyphGeometry.cpp	24 Aug 2004 15:26:15 -0000
>@@ -49,16 +49,20 @@

...

> 
>+#include "nsISVGGradient.h"
>+#include "nsIDOMSVGGradientElement.h"
Do you need to include nsIDOMSVGGradientElement here?



>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ nsSVGCairoGradient.cpp	24 Aug 2004 15:26:15 -0000

>+#include "nsCOMPtr.h"
>+#include "nsIDOMSVGMatrix.h"
>+#include "nsIDOMSVGNumber.h"
>+#include "nsIDOMSVGLength.h"
>+#include "nsIDOMSVGGradientElement.h"
>+#include "nsIDOMSVGTransformList.h"

Doesn't look like SVGNumber, SVGLength, or SVGTransformList are ever used


>+++ nsSVGCairoPathGeometry.cpp	24 Aug 2004 15:26:15 -0000
>@@ -49,16 +49,21 @@
> #include "nsISVGPathGeometrySource.h"
> #include "nsISVGRendererPathBuilder.h"
> #include "nsSVGCairoPathBuilder.h"
> #include "nsMemory.h"
> #include <float.h>
> #include <cairo.h>
> #include "nsSVGCairoRegion.h"
> 
>+#include "nsISVGGradient.h"
>+#include "nsIDOMSVGGradientElement.h"

Doesn't look like this is needed here...
Attachment #156893 - Flags: review?(scootermorris) → review+

Comment 106

14 years ago
Comment on attachment 156900 [details] [diff] [review]
update gdi+ gradient code to latest set of gradient patches

r=afri

>+ * The Original Code is the Mozilla SVG GDI+ Renderer project.
>+ *
>+ * The Initial Developer of the Original Code is IBM Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2004
>+ * the Initial Developer. All Rights Reserved.

IANAL and I don't want to be pedantic here, but now that we have several
organizations and individuals contributing new files to the SVG project, we
should probably change the license headers across the whole SVG code. The
problem is that the above license header can be interpreted as saying that IBM
is the initial developer of the whole svg gdi+ renderer project, not just of
that particular file. I don't mind, Crocodile Clips probably don't mind, but
IBM or other parties might. 
We might want to replace "The Original Code is the Mozilla SVG XXX project"
with "The Original Code is Mozilla SVG XXX project code" throughout the SVG
code. But as I say, I don't really mind :-)


>Index: nsSVGGDIPlusGradient.cpp
>===================================================================
[...]
>+void
>+GDIPlusGradient(nsISVGGDIPlusRegion *aRegion, nsISVGGradient *aGrad,
>+                nsIDOMSVGMatrix *aCTM,
>+                Graphics *aGFX,
>+                void(*aCallback)(Graphics *, Brush*, void *), void *aData)
>+{
>+  NS_ASSERTION(aGrad, "Called CairoGradient without a gradient!");

s/CairoGradient/GDIPlusGradient/
Attachment #156900 - Flags: review?(alex) → review+

Updated

14 years ago
Attachment #159687 - Flags: review?(jonathan.watt) → review+
Comment on attachment 160790 [details] [diff] [review]
Changes to content files for SVG Gradient Support (alphabetized entries in nsCSSParser and nsCSSStruct)

>+  } else {
>+    // Check for "currentColor"
>+    // Set it to the default color

This should be quite easy to do.  But if you're not doing it now, at least add
"XXX" before the comment.

>+  // stop-color: 
>+  SetSVGPaint(SVGData.mStopColor, parentSVG->mStopColor, mPresContext, svg->mStopColor, inherited);

Why are you using SetSVGPaint for this?  It's just a color.  Should there be
something simpler like SVGColor?  That would be useful for 'flood-color' and
'lighting-color' as well, and SVGPaint could be derived from it (and the
function could call it).

>-    return ParseVariant(aErrorCode, aValue, VARIANT_HC | VARIANT_NONE,
>+    return ParseVariant(aErrorCode, aValue, VARIANT_HC | VARIANT_NONE | VARIANT_URL,

You really shouldn't implement the URL without implementing the fallback.  That
discourages authors from using a fallback, which is important, e.g., for
implementations that don't implement the URL value.  This means that both the
parsing code and the computation code are wrong.

sr- because of the last issue
Attachment #160790 - Flags: superreview?(dbaron) → superreview-
Note that you can use nsCSSValuePair for the nsCSSSVG part of the URI bit -- one
nsCSSValue for the URI, one for the rest.
(Assignee)

Comment 109

14 years ago
David,
    Thanks for the review!  See my comments below...

(In reply to comment #107)
> (From update of attachment 160790 [details] [diff] [review])
> >+  } else {
> >+    // Check for "currentColor"
> >+    // Set it to the default color
> 
> This should be quite easy to do.  But if you're not doing it now, at least add
> "XXX" before the comment.
OK, I'll add the XXX.  There is a separate bug covering "currentColor" and I
want to get some information about the defintion from the SVG WG.  In
particular, is it only the forground color that is a candidate for
"currentColor" (which makes things easy) or do other colors participate.
> 
> >+  // stop-color: 
> >+  SetSVGPaint(SVGData.mStopColor, parentSVG->mStopColor, mPresContext,
svg->mStopColor, inherited);
> 
> Why are you using SetSVGPaint for this?  It's just a color.  Should there be
> something simpler like SVGColor?  That would be useful for 'flood-color' and
> 'lighting-color' as well, and SVGPaint could be derived from it (and the
> function could call it).
At some point, the whole area of color in SVG needs to be rationalized and
things like ICC-Color, currentColor, and other areas need to get dealt with.  I
would like to stick with this for now and open a separate bug since it will
probably touch a lot more code than just gradients to do it right.
> 
> >-    return ParseVariant(aErrorCode, aValue, VARIANT_HC | VARIANT_NONE,
> >+    return ParseVariant(aErrorCode, aValue, VARIANT_HC | VARIANT_NONE |
VARIANT_URL,
> 
> You really shouldn't implement the URL without implementing the fallback.  That
> discourages authors from using a fallback, which is important, e.g., for
> implementations that don't implement the URL value.  This means that both the
> parsing code and the computation code are wrong.
> 
> sr- because of the last issue
> 
You are right, I really should have implemented the fallback parsing and carry
that through to the layout code in Glyph and Path geometries.  Would it be
acceptable to open a separate bug and assign it to myself?  There are other
patches awaiting the gradient stuff, and as you can see, there have been a *lot*
of versions of this.  
(In reply to comment #109)
> OK, I'll add the XXX.  There is a separate bug covering "currentColor" and I
> want to get some information about the defintion from the SVG WG.  In
> particular, is it only the forground color that is a candidate for
> "currentColor" (which makes things easy) or do other colors participate.

currentColor comes from the 'color' property, so it's exactly the same as the
NS_STYLE_COLOR_MOZ_USE_TEXT_COLOR / BORDER_COLOR_FOREGROUND that's currently
implemented for 'border'.

> You are right, I really should have implemented the fallback parsing and carry
> that through to the layout code in Glyph and Path geometries.  Would it be
> acceptable to open a separate bug and assign it to myself?  There are other
> patches awaiting the gradient stuff, and as you can see, there have been a *lot*
> of versions of this.  

OK, but please mark the bug as a dependency of the bug to turn on SVG.
(Assignee)

Comment 111

14 years ago
(In reply to comment #110)
> 
> OK, but please mark the bug as a dependency of the bug to turn on SVG.
Done.  Bug #264132 has been filed.


(Assignee)

Comment 112

14 years ago
Created attachment 161927 [details] [diff] [review]
Minor change (added XXX to currentColor comment.
Attachment #160790 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #161927 - Flags: superreview?(dbaron)
(Assignee)

Updated

14 years ago
Attachment #160778 - Flags: review?(tor)

Comment 113

14 years ago
Comment on attachment 160778 [details] [diff] [review]
New dom files for SVG Gradient support

Some funky indentation in nsIDOMSVGAnimatedNumber.idl, but don't 
bother attaching another patch for that.
Attachment #160778 - Flags: review?(tor) → review+

Updated

14 years ago
Attachment #160780 - Flags: review+
(Assignee)

Updated

14 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Attachment #159688 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #156745 - Flags: review?(alex)
(Assignee)

Updated

14 years ago
Attachment #156746 - Flags: review?(alex)

Updated

11 years ago
Duplicate of this bug: 246480
You need to log in before you can comment on or make changes to this bug.