Closed Bug 608161 Opened 13 years ago Closed 12 years ago

SVG SMIL: Don't use HasAttr to test for presence of animated attributes

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(1 file, 2 obsolete files)

As noted by Robert Longson in bug 606399 comment 1, we shouldn't be relying on HasAttr to test for the presence of animated attributes inside content or layout.

The problem is that whilst an attribute may not be specified in markup or via a DOM call, it can still nevertheless have a value if it is animated. If we rely on HasAttr we'll end up ignoring animated values.

I've done a quick grep of svg/content and it seems we use HasAttr on animated attributes in the following cases:

* <image>: xlink:href (not sure if this is actually a problem--perhaps only if we initially have no attr, then <set> one, and then bind to the tree?)
* <marker>: viewBox
* <path>: pathLength
* <rect>: rx,ry (bug 606399)
* <use>: width, height, and viewBox on target content
* filters: kernelUnitLength, values, targetX, targetY, divisor, bias, limitingConeAngle, xlink:href

In layout it seems we have the following:

* filters: filter effects region (x, y, width, height), filterRes
* gradients: gradientUnits, gradientTransform, spreadMethod
* <svg>: viewBox
* patterns: patternUnits, patternContentUnits, patternTransform, viewBox, preserveAspectRatio, x, y, width, height (bug 544809)

All of these are potential bugs although some are not currently manifest since, for example, we don't animate number lists yet (bug 589439).
Brian, can you take these bugs?
viewBox has an IsValid member so that should be easy to convert. Other attribute types may need changing to add an IsValid member as they don't yet have one.
Assignee: nobody → birtles
Status: NEW → ASSIGNED
pathLength is fixed by bug 643419
Depends on: 643419
652832 removes the <use> HasAttr calls
Depends on: 652832
bug 617623 removes filters and image so we're left with

<marker>: viewBox
gradients
<svg>: viewBox
Depends on: 617623
bug 687830 fixes marker viewBox. Content is now done, only layout left and that's just

gradients
<svg>: viewBox
Depends on: 687830
bug 689546 gets rid of the <svg> viewBox so only gradients are left now.
Depends on: 689546
Attached patch WIP patch v1 (obsolete) — Splinter Review
Fix use of HasAttr in gradients. I want to have another look over this before I put it up for review.
Attached patch Patch v1a (obsolete) — Splinter Review
This patch removes HasAttr using the approach we applied to patterns.

I may post another patch to fix some const-correctness with the underlying code so we can make all these getters const but I've yet to check if it's going to be worthwhile.
Attachment #601198 - Flags: review?(longsonr)
Attachment #600328 - Attachment is obsolete: true
Comment on attachment 601198 [details] [diff] [review]
Patch v1a

>diff --git a/layout/reftests/svg/smil/anim-gradient-attr-presence-01-ref.svg b/layout/reftests/svg/smil/anim-gradient-attr-presence-01-ref.svg
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/svg/smil/anim-gradient-attr-presence-01-ref.svg
>@@ -0,0 +1,126 @@

<!--
     Any copyright is dedicated to the Public Domain.
     http://creativecommons.org/publicdomain/zero/1.0/
-->

would be nice on the testcases assuming you're happy to do that.

>+<svg xmlns="http://www.w3.org/2000/svg"
>+  xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 200 900">
>+  <!-- 1. gradientUnits -->
>   *aStopColor   = stopFrame->GetStyleSVGReset()->mStopColor;
>   *aStopOpacity = stopFrame->GetStyleSVGReset()->mStopOpacity;
> }
> 
>+PRUint16
>+nsSVGGradientFrame::GetEnumValue(PRUint32 aIndex, nsIContent *aDefault)
>+{
>+  nsSVGEnum& thisEnum =
>+    static_cast<nsSVGGradientElement *>(mContent)->mEnumAttributes[aIndex];

I think you could use a const reference couldn't you?

>+SVGAnimatedTransformList*
>+nsSVGGradientFrame::GetGradientTransformList(nsIContent* aDefault)

Return a const SVGAnimatedTransformList*

>+nsSVGGradientFrame::GetLinearGradientWithLength(PRUint32 aIndex,
>+  nsSVGLinearGradientElement* aDefault)
> {

>+nsSVGGradientFrame::GetRadialGradientWithLength(PRUint32 aIndex,
>+  nsSVGRadialGradientElement* aDefault)
>+{

Same implementation isn't it? Can't we have GetGradientWithLength in the base class?

>-
>-  if (!(gradient = GetRadialGradientWithAttr(nsGkAtoms::fx, nsnull)))
>+  if (!GetRadialGradientWithLength(nsSVGRadialGradientElement::FX, nsnull))
>     fx = cx;  // if fx isn't set, we must use cx
>   else
>-    fx = GradientLookupAttribute(nsGkAtoms::fx, nsSVGRadialGradientElement::FX, gradient);
>+    fx = GetLengthValue(nsSVGRadialGradientElement::FX);

Better if GetLengthValue took a default so we had something like this...

fx = GetLengthValue(nsSVGRadialGradientElement::FX, cx);

This could be a follow up.

r=longsonr with comments addressed or follow ups raised.
Attachment #601198 - Flags: review?(longsonr) → review+
Try run for 12c124460adc is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=12c124460adc
Results (out of 219 total builds):
    exception: 2
    success: 176
    warnings: 27
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-12c124460adc
Thanks for the review Robert!

(In reply to Robert Longson from comment #10)
> <!--
>      Any copyright is dedicated to the Public Domain.
>      http://creativecommons.org/publicdomain/zero/1.0/
> -->
> 
> would be nice on the testcases assuming you're happy to do that.

Fixed.

> >+PRUint16
> >+nsSVGGradientFrame::GetEnumValue(PRUint32 aIndex, nsIContent *aDefault)
> >+{
> >+  nsSVGEnum& thisEnum =
> >+    static_cast<nsSVGGradientElement *>(mContent)->mEnumAttributes[aIndex];
> 
> I think you could use a const reference couldn't you?

Fixed.

> >+SVGAnimatedTransformList*
> >+nsSVGGradientFrame::GetGradientTransformList(nsIContent* aDefault)
> 
> Return a const SVGAnimatedTransformList*

Fixed.

> >+nsSVGGradientFrame::GetLinearGradientWithLength(PRUint32 aIndex,
> >+  nsSVGLinearGradientElement* aDefault)
> > {
> 
> >+nsSVGGradientFrame::GetRadialGradientWithLength(PRUint32 aIndex,
> >+  nsSVGRadialGradientElement* aDefault)
> >+{
> 
> Same implementation isn't it? Can't we have GetGradientWithLength in the
> base class?

It's not quite the same, the last line differs, but I had a look at what we could do to remove redundancy.

The current arrangement allows us to traverse linear and radial gradient frames and return the typed element without any casting of the frames, or checking of type parameters. The only casting of the mContent member happens within the subclasses and so should be safe.

Most of the options for factoring out common code would mean we'd end up adding back some casting or switching based on GetType() both of which I'd rather avoid. I looked at templatising GetGradientWithLength but, of course, that doesn't play well with virtual overrides.

As it stands, it's three lines, two of which are the same, repeated twice. I think it's ok?

> >-  if (!(gradient = GetRadialGradientWithAttr(nsGkAtoms::fx, nsnull)))
> >+  if (!GetRadialGradientWithLength(nsSVGRadialGradientElement::FX, nsnull))
> >     fx = cx;  // if fx isn't set, we must use cx
> >   else
> >-    fx = GradientLookupAttribute(nsGkAtoms::fx, nsSVGRadialGradientElement::FX, gradient);
> >+    fx = GetLengthValue(nsSVGRadialGradientElement::FX);
> 
> Better if GetLengthValue took a default so we had something like this...
> 
> fx = GetLengthValue(nsSVGRadialGradientElement::FX, cx);

Good idea, fixed.

Thanks again Robert!

Just waiting for another pass through the Try server (https://tbpl.mozilla.org/?tree=Try&rev=741fc0125151). The previous run had failures on OSX for the added test case.
Attachment #601198 - Attachment is obsolete: true
Attachment #601549 - Flags: review+
(In reply to Brian Birtles (:birtles) from comment #12)
> 
> As it stands, it's three lines, two of which are the same, repeated twice. I
> think it's ok?

Sure.
Still can't get the reftest to pass on OSX. I keep getting anti-aliasing differences on the border of the gradientTransform test case. All other platforms seem fine though.
You could mask out the problem areas using extra lines or clip regions.
I think I've finally appeased try server.

Landed on m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df0d1eb248dd
https://hg.mozilla.org/mozilla-central/rev/df0d1eb248dd
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.