Last Comment Bug 696210 - Make DidAnimate methods non-virtual now that all overrides have been eliminated
: Make DidAnimate methods non-virtual now that all overrides have been eliminated
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Robert Longson
: Jet Villegas (:jet)
Depends on: 696078
  Show dependency treegraph
Reported: 2011-10-20 14:03 PDT by Robert Longson
Modified: 2012-02-20 10:23 PST (History)
1 user (show)
longsonr: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (48.99 KB, patch)
2011-10-20 14:06 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review
updated patch (2.50 KB, patch)
2012-02-18 13:24 PST, Robert Longson
jwatt: review+
Details | Diff | Splinter Review

Description Robert Longson 2011-10-20 14:03:31 PDT

Comment 1 Robert Longson 2011-10-20 14:06:58 PDT
Created attachment 568510 [details] [diff] [review]
Comment 2 Daniel Holbert [:dholbert] 2011-11-08 14:47:26 PST
Comment on attachment 568510 [details] [diff] [review]

>@@ -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] 2011-11-08 14:49:07 PST
(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 2012-02-18 13:24:35 PST
Created attachment 598572 [details] [diff] [review]
updated patch
Comment 5 Robert Longson 2012-02-18 13:26:17 PST
Brian's bug 629200 did nearly all of this including implementing the changes in comment 2 and it had tests.
Comment 7 Ed Morley [:emorley] 2012-02-20 10:23:37 PST

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