696210 - Make DidAnimate methods non-virtual now that all overrides have been eliminated

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Daniel Holbert [:dholbert]]

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

>@@ -356,19 +356,19 @@ nsSVGAngle::SetBaseValueString(const nsA
>     mAnimValUnit = mBaseValUnit;
>   }
> #ifdef MOZ_SMIL
>   else {
>     aSVGElement->AnimationNeedsResample();
>   }
> #endif
> 
>-  // We don't need to call DidChange* here - we're only called by
>-  // nsSVGElement::ParseAttribute under nsGenericElement::SetAttr,
>-  // which takes care of notifying.
>+  if (aDoSetAttr) {
>+    aSVGElement->DidChangeAngle(mAttrEnum);
>+  }
>   return NS_OK;
> }

Why this change? Is the deleted comment incorrect?
(Hmm, looks like it might be incorrect -- nsSVGAngle::DOMBaseVal::SetValueAsString does appear to call this method.)

This change doesn't seem related to this bug. Unless it's needed for this patch to work for some reason, I'd prefer that we split this off into its own bug. (ideally with a regression test, as I'd imagine it would affect behavior (whereas the rest of this bug's patch should have no functional effect.))

Other than that, r=me

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> This change doesn't seem related to this bug

(well, it's related to this bug in that it's got a DidChange call, of course :) but I meant that this is the only chunk in this patch (I think) that adds a *new* DidChange call, rather than just tweaking an existing one)

Comment 4

[Robert Longson [:longsonr]]

Attachment: updated patch

Comment 5

[Robert Longson [:longsonr]]

Brian's bug 629200 did nearly all of this including implementing the changes in comment 2 and it had tests.

Comment 6

[Robert Longson [:longsonr]]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/e6f6d622d286

Comment 7

[Ed Morley [:emorley]]

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