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
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Robert Longson
:
Mentors:
Depends on: 696078
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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]
patch
Comment 2 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2011-11-08 14:47:26 PST
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] (mostly OOTO until Aug 9th) 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
https://hg.mozilla.org/mozilla-central/rev/e6f6d622d286

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