Status

()

Core
SVG
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

10 years ago
Created attachment 323889 [details] [diff] [review]
patch

One more step on the road to animation. bug 397159 and bug 395667 prior examples.
(Assignee)

Updated

10 years ago
Attachment #323889 - Flags: superreview?(roc)
Attachment #323889 - Flags: review?(roc)
Looks great.

+  nsString GetBaseValue() const
+    { return mBaseVal; }
+  nsString GetAnimValue() const
+    { return mAnimVal; }

These could return const nsString&.

+  nsAutoString newStr(info.mStrings[aAttrEnum].GetBaseValue());
+      nsAutoString str(sources[j]->GetAnimValue());
+      nsAutoString input(aString->GetAnimValue());
+  nsAutoString src(mStringAttributes[HREF].GetAnimValue());
+  nsAutoString href(mStringAttributes[HREF].GetAnimValue());

You don't need this temporary, Just pass GetBaseValue() directly to the following method. This is more efficient.

+  nsAutoString src(mStringAttributes[HREF].GetAnimValue());
+  nsAutoString href(mStringAttributes[HREF].GetAnimValue());
+      nsAutoString str(sources[j]->GetAnimValue());

Here you could make src a const nsString&.

This patch conflicts massively with my filters rework branch :-(. But first come, first served I suppose.
BTW would you mind writing some mochitests for the animated value datatypes? For each type, we should test that the baseVal and animVal reflect the attribute value, whether we modify the attribute before getting the animatedValue or after. We should also test that modifying baseVal is reflected in the attribute value and in the animVal.
(Assignee)

Comment 3

10 years ago
Created attachment 324461 [details] [diff] [review]
address review comments

All comment 1 issues addressed.

(In reply to comment #2)
> BTW would you mind writing some mochitests for the animated value datatypes?
> For each type, we should test that the baseVal and animVal reflect the
> attribute value, whether we modify the attribute before getting the
> animatedValue or after. We should also test that modifying baseVal is reflected
> in the attribute value and in the animVal.
> 

I've written some mochitests but "whether we modify the attribute before getting the animatedValue or after" is only true for types where the baseVal and  animVal are objects rather than POD types isn't it? I've done something for lengths and angles assuming that's what you meant.
Attachment #323889 - Attachment is obsolete: true
Attachment #324461 - Flags: superreview?(roc)
Attachment #324461 - Flags: review?(roc)
Attachment #323889 - Flags: superreview?(roc)
Attachment #323889 - Flags: review?(roc)
Comment on attachment 324461 [details] [diff] [review]
address review comments

excellent!
Attachment #324461 - Flags: superreview?(roc)
Attachment #324461 - Flags: superreview+
Attachment #324461 - Flags: review?(roc)
Attachment #324461 - Flags: review+
Do you want to check this in or should I?
(Assignee)

Comment 7

10 years ago
Please do check it in for me thanks. I'm still trying to figure out hg.
checked in
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Comment 9

10 years ago
Created attachment 342720 [details] [diff] [review]
make mochitests run once
(Assignee)

Comment 10

10 years ago
checked in b4aef95eab73 to correct mochitests so they run once rather than twice.
You need to log in before you can comment on or make changes to this bug.