Bug 28673 – Modifying <text rotate=""> doesn't clear the corresponding SVGAnimatedNumberList

• Component: SVG
• Status: RESOLVED FIXED

[Cameron McCormack (:heycam)]

When script modifies the rotate="" attribute on a <text> (or <tspan>, etc.) element, the baseVal of the SVGAnimatedNumberList that corresponds to the attribute is not cleared before the attribute is re-parsed.  E.g.:

<text id="t" rotate="10 20 30"/>
<script>
t = document.getElementById('t');
l = t.rotate.baseVal;
t.setAttribute('rotate', '40 50');
assert(l.numberOfItems == 2);  // should be 2 but it's 5
</script>

[Cameron McCormack (:heycam)]

Created attachment 38462 [details]
Fix and test

[Darin Adler]

Comment on attachment 38462 [details]
Fix and test

> +        * svg/dom/resources/text-rotate-live.js: Added.
> +        (getRotate):
> +        (getAndSetRotate):

If you're not commenting on these functions individually and the file is new, then there's no need to leave this list of functions in the change log. The idea of prepare-ChangeLog is that it helps you write the ChangeLog entry, not that you should use that it generates as-is.

The test code doesn't follow our usual brace style. Open braces for functions go on their own lines. Single line for loop bodies don't get braces around them. I would have put the "var" for the "i" variable inside the for loop. I believe the function results could be arrays rather than strings -- the shouldBe macro knows how to check arrays for equality. We don't use "get" in the names of functions that simply return results.

None of these are significant issues, r=me on this patch as it is.

[Cameron McCormack (:heycam)]

(In reply to comment #2)
> If you're not commenting on these functions individually and the file is new,
> then there's no need to leave this list of functions in the change log. The
> idea of prepare-ChangeLog is that it helps you write the ChangeLog entry, not
> that you should use that it generates as-is.

I will keep that in mind for future patches.

> The test code doesn't follow our usual brace style.

OK.  I see there's no mention of JS style in http://webkit.org/coding/coding-style.html but I'll admit I didn't check it before writing the code.

> Open braces for functions
> go on their own lines. Single line for loop bodies don't get braces around
> them. I would have put the "var" for the "i" variable inside the for loop.

I used to too, but now tend to put vars at the top of functions to emphasise that their scope is indeed the whole function and not just the loop.

> I believe the function results could be arrays rather than strings -- the
> shouldBe macro knows how to check arrays for equality.

Acknowledged.  I didn't look up shouldBe to see exactly what it was checking; I just copied another test that was doing string comparisons and thought that it was just safer to do that.

> We don't use "get" in the names of functions that simply return results.

I guess that's Java style rubbing off on me. :)

> None of these are significant issues, r=me on this patch as it is.

Thanks!

[Eric Seidel (no email)]

Comment on attachment 38462 [details]
Fix and test

Looks good to me too.  I wonder if other "parse" implementations are missing a clear() call.

[Eric Seidel (no email)]

Comment on attachment 38462 [details]
Fix and test

Clearing flags on attachment: 38462

Committed r47720: <http://trac.webkit.org/changeset/47720>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.