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

RESOLVED FIXED in mozilla13

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Robert Longson, Assigned: Robert Longson)

Tracking

Trunk
mozilla13
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.50 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

6 years ago
Depends on: 696078
(Assignee)

Comment 1

6 years ago
Created attachment 568510 [details] [diff] [review]
patch
Assignee: nobody → longsonr
Attachment #568510 - Flags: review?(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
Attachment #568510 - Flags: review?(dholbert) → review+
(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)
(Assignee)

Updated

6 years ago
Whiteboard: not ready to land
(Assignee)

Updated

5 years ago
Summary: Make DidChange and DidAnimate methods non-virtual now that all overrides have been eliminated → Make DidAnimate methods non-virtual now that all overrides have been eliminated
(Assignee)

Comment 4

5 years ago
Created attachment 598572 [details] [diff] [review]
updated patch
Attachment #598572 - Flags: review?(dholbert)
(Assignee)

Updated

5 years ago
Attachment #568510 - Attachment is obsolete: true
(Assignee)

Comment 5

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

Updated

5 years ago
Attachment #598572 - Flags: review?(dholbert) → review+
(Assignee)

Comment 6

5 years ago
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/e6f6d622d286
Flags: in-testsuite-

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/e6f6d622d286
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: not ready to land
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.