437448 - New style nsSVGString

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch

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

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 3

[Robert Longson [:longsonr]]

Attachment: 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.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes. Thanks!!

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 324461 [details] [diff] [review]
address review comments

excellent!

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Do you want to check this in or should I?

Comment 7

[Robert Longson [:longsonr]]

Please do check it in for me thanks. I'm still trying to figure out hg.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in

Comment 9

[Robert Longson [:longsonr]]

Attachment: make mochitests run once

Comment 10

[Robert Longson [:longsonr]]

checked in b4aef95eab73 to correct mochitests so they run once rather than twice.