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)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: birtles, Assigned: birtles)
References
Details
Attachments
(1 file, 2 obsolete files)
37.21 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
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?
Comment 2•13 years ago
|
||
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 | ||
Updated•13 years ago
|
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Comment 3•13 years ago
|
||
pathLength is fixed by bug 643419
Comment 5•13 years ago
|
||
bug 617623 removes filters and image so we're left with <marker>: viewBox gradients <svg>: viewBox
Comment 6•12 years ago
|
||
bug 687830 fixes marker viewBox. Content is now done, only layout left and that's just gradients <svg>: viewBox
Comment 7•12 years ago
|
||
bug 689546 gets rid of the <svg> viewBox so only gradients are left now.
Depends on: 689546
Assignee | ||
Comment 8•12 years ago
|
||
Fix use of HasAttr in gradients. I want to have another look over this before I put it up for review.
Assignee | ||
Comment 9•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #600328 -
Attachment is obsolete: true
Comment 10•12 years ago
|
||
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+
Comment 11•12 years ago
|
||
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
Assignee | ||
Comment 12•12 years ago
|
||
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+
Comment 13•12 years ago
|
||
(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.
Assignee | ||
Comment 14•12 years ago
|
||
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•12 years ago
|
||
You could mask out the problem areas using extra lines or clip regions.
Assignee | ||
Comment 16•12 years ago
|
||
I think I've finally appeased try server. Landed on m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/df0d1eb248dd
Comment 17•12 years ago
|
||
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.
Description
•