Bug 28673 - Modifying <text rotate=""> doesn't clear the corresponding SVGAnimatedNumberList
Summary: Modifying <text rotate=""> doesn't clear the corresponding SVGAnimatedNumberList
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-23 17:12 PDT by Cameron McCormack (:heycam)
Modified: 2009-08-24 11:35 PDT (History)
1 user (show)

See Also:


Attachments
Fix and test (4.13 KB, patch)
2009-08-23 17:24 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2009-08-23 17:12:56 PDT
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>
Comment 1 Cameron McCormack (:heycam) 2009-08-23 17:24:28 PDT
Created attachment 38462 [details]
Fix and test
Comment 2 Darin Adler 2009-08-23 22:20:52 PDT
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.
Comment 3 Cameron McCormack (:heycam) 2009-08-23 22:30:10 PDT
(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!
Comment 4 Eric Seidel (no email) 2009-08-24 11:25:22 PDT
Comment on attachment 38462 [details]
Fix and test

Looks good to me too.  I wonder if other "parse" implementations are missing a clear() call.
Comment 5 Eric Seidel (no email) 2009-08-24 11:35:28 PDT
Comment on attachment 38462 [details]
Fix and test

Clearing flags on attachment: 38462

Committed r47720: <http://trac.webkit.org/changeset/47720>
Comment 6 Eric Seidel (no email) 2009-08-24 11:35:30 PDT
All reviewed patches have been landed.  Closing bug.