Last Comment Bug 608161 - SVG SMIL: Don't use HasAttr to test for presence of animated attributes
: SVG SMIL: Don't use HasAttr to test for presence of animated attributes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Brian Birtles (:birtles)
:
:
Mentors:
Depends on: 544809 606399 617623 643419 652832 687830 689546
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-28 17:59 PDT by Brian Birtles (:birtles)
Modified: 2012-03-07 02:04 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP patch v1 (35.40 KB, patch)
2012-02-24 00:36 PST, Brian Birtles (:birtles)
no flags Details | Diff | Splinter Review
Patch v1a (35.71 KB, patch)
2012-02-28 00:09 PST, Brian Birtles (:birtles)
longsonr: review+
Details | Diff | Splinter Review
Patch v1b, r=longsonr (37.21 KB, patch)
2012-02-29 00:30 PST, Brian Birtles (:birtles)
bbirtles: review+
Details | Diff | Splinter Review

Description Brian Birtles (:birtles) 2010-10-28 17:59:12 PDT
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) 2010-11-02 14:04:30 PDT
Brian, can you take these bugs?
Comment 2 Robert Longson 2010-11-02 14:37:37 PDT
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 2011-05-02 04:58:22 PDT
pathLength is fixed by bug 643419
Comment 4 Robert Longson 2011-06-23 13:56:23 PDT
652832 removes the <use> HasAttr calls
Comment 5 Robert Longson 2011-07-02 04:41:01 PDT
bug 617623 removes filters and image so we're left with

<marker>: viewBox
gradients
<svg>: viewBox
Comment 6 Robert Longson 2011-09-21 02:26:00 PDT
bug 687830 fixes marker viewBox. Content is now done, only layout left and that's just

gradients
<svg>: viewBox
Comment 7 Robert Longson 2011-09-30 03:38:27 PDT
bug 689546 gets rid of the <svg> viewBox so only gradients are left now.
Comment 8 Brian Birtles (:birtles) 2012-02-24 00:36:43 PST
Created attachment 600328 [details] [diff] [review]
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) 2012-02-28 00:09:04 PST
Created attachment 601198 [details] [diff] [review]
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 2012-02-28 01:38:50 PST
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 2012-02-28 04:31:10 PST
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
Comment 12 Brian Birtles (:birtles) 2012-02-29 00:30:31 PST
Created attachment 601549 [details] [diff] [review]
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 2012-02-29 01:52:04 PST
(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) 2012-03-01 00:19:30 PST
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 2012-03-01 01:33:33 PST
You could mask out the problem areas using extra lines or clip regions.
Comment 16 Brian Birtles (:birtles) 2012-03-05 23:00:13 PST
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] 2012-03-07 02:04:09 PST
https://hg.mozilla.org/mozilla-central/rev/df0d1eb248dd

Note You need to log in before you can comment on or make changes to this bug.