541882 - Add support for SMIL animation of the preserveAspectRatio attribute in SVG

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

We should add support for SMIL animation of the preserveAspectRatio attribute in SVG.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

I'm just reusing SMILEnumType here (stuffing the multiple values into it), since the logic is the same.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 423281 [details] [diff] [review]
patch

>+nsresult
>+nsSVGPreserveAspectRatio::SetBaseValueString(const nsAString &aValueAsString,

>+#ifdef MOZ_SMIL
>+  if (mIsAnimated) {
>+    aSVGElement->AnimationNeedsResample();
>+  }
>+#endif  return NS_OK;

Put "return NS_OK;" on its own line there.

>+nsresult
>+nsSVGPreserveAspectRatio::SMILPreserveAspectRatio
>+                        ::ValueFromString(const nsAString& aStr,
>+                                          const nsISMILAnimationElement* /*aSrcElement*/,
>+                                          nsSMILValue& aValue) const
[SNIP]
>+  PRUint64 compound = 0;
>+  compound |= PRUint64(par.GetDefer() ? 1 : 0) << 16;
>+  compound |= PRUint64(par.GetAlign()) << 8;
>+  compound |= PRUint64(par.GetMeetOrSlice());

The SMILPreserveAspectRatio implementation has three copies of the above four lines, in various methods.  Can we refactor that out into a helper function that these methods can call? Maybe something like:

  static PRUint64 GetCompoundRepresentation(nsSVGPreserveAspectRatio* par) {
     [the four lines quoted above]
     return compound;
  }

>+++ b/content/svg/content/src/nsSVGPreserveAspectRatio.h
>+  const PreserveAspectRatio &GetAnimValue(nsSVGElement *aSVGElement) const
>+  {
>+  #ifdef MOZ_SMIL
>+    aSVGElement->FlushAnimations();
>+  #endif
>+    return mAnimVal;
>+  }

Remove the indentation on the #ifdef & #endif there.

>+++ b/layout/svg/base/src/nsSVGPatternFrame.cpp
> const nsSVGPreserveAspectRatio &
> nsSVGPatternFrame::GetPreserveAspectRatio()
> {
>+  // XXX this should also check for animated values
>   nsSVGPatternElement *patternElement =
>     GetPatternWithAttr(nsGkAtoms::preserveAspectRatio, mContent);
> 
>   return patternElement->mPreserveAspectRatio;
> }


RE that XXX comment -- do animated values actually present a problem there?  If we're animated, wouldn't they have updated |patternElement| via SetAnimValue already?

Maybe we'd need to call mContent->FlushAnimations() to make sure we're returning a value with up-to-date animation effects... but aside from that, is there anything else problematic there?

r=dholbert with the above addressed

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to comment #2)
> RE that XXX comment -- do animated values actually present a problem there?  If
> we're animated, wouldn't they have updated |patternElement| via SetAnimValue
> already?

My comment was clearly lacking. I've moved it and expanded it to clarify, and I've filed bug 544809 for the issue.

Comment 4

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/8dc04dca2a59