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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

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).

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Brian, can you take these bugs?

Comment 2

[Robert Longson [:longsonr]]

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.

Comment 3

[Robert Longson [:longsonr]]

pathLength is fixed by bug 643419

Comment 4

[Robert Longson [:longsonr]]

652832 removes the <use> HasAttr calls

Comment 5

[Robert Longson [:longsonr]]

bug 617623 removes filters and image so we're left with

<marker>: viewBox
gradients
<svg>: viewBox

Comment 6

[Robert Longson [:longsonr]]

bug 687830 fixes marker viewBox. Content is now done, only layout left and that's just

gradients
<svg>: viewBox

Comment 7

[Robert Longson [:longsonr]]

bug 689546 gets rid of the <svg> viewBox so only gradients are left now.

Comment 8

[Brian Birtles (:birtles)]

Attachment: WIP patch v1

Fix use of HasAttr in gradients. I want to have another look over this before I put it up for review.

Comment 9

[Brian Birtles (:birtles)]

Attachment: Patch v1a

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.

Comment 10

[Robert Longson [:longsonr]]

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.

Comment 11

[Mozilla RelEng Bot]

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/[e-mail]-12c124460adc

Comment 12

[Brian Birtles (:birtles)]

Attachment: Patch v1b, r=longsonr

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.

Comment 13

[Robert Longson [:longsonr]]

(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.

Comment 14

[Brian Birtles (:birtles)]

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.

Comment 15

[Robert Longson [:longsonr]]

You could mask out the problem areas using extra lines or clip regions.

Comment 16

[Brian Birtles (:birtles)]

I think I've finally appeased try server.

Landed on m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df0d1eb248dd

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/df0d1eb248dd