508496 - SVG: SVGAnimatedLength's animVal and baseVal attributes always return different objects

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

If you request the baseVal attribute of an animated length twice and compare the two objects (with ===) they will be different objects. This seems wrong. You should get back the same object so that width.baseVal === width.baseVal.

The same problem occurs for animVal as well. We return a new object every time.

This was originally raised in bug 506856 but is now being split off into a separate bug because it's quite unrelated to readonly behaviour (which is what bug 506856 deals with) and it might take quite a bit of work.

See discussion on www-svg: http://lists.w3.org/Archives/Public/www-svg/2008Sep/0023.html

Comment 1

[Brian Birtles (:birtles)]

Attachment: proposed patch v1a

First attempt at a patch to address this.

Comment 2

[Brian Birtles (:birtles)]

Attachment: patch v1b

Fixed a permissions bug in test case that caused it to fail on TryServer.

Comment 3

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

Comment on attachment 395180 [details] [diff] [review]
patch v1b

Is there no better way to detect that SMIL is disabled? That's a bit disturbing if true.

+  TearoffType*
+  GetTearoff(SimpleType* aSimple);
+
+  void
+  AddTearoff(SimpleType* aSimple, TearoffType* aTearoff);
+
+  void
+  RemoveTearoff(SimpleType* aSimple);

One line each

I suggest adding a destructor to nsSVGAttrTearoffTable which asserts that the hashtable is empty.

r+ with those changes

Comment 4

[Brian Birtles (:birtles)]

Attachment: patch v1c (address roc's review comments)

Will give it another run on TryServer before marking checkin-needed.

Comment 5

[Brian Birtles (:birtles)]

Attachment: patch v1d

Removed querying of preferences from test case.

Comment 6

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

Is this checkin-needed now?

Comment 7

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

Although I don't understand how this test works. There will always be an element with id 'animate'. The question is whether it exports the <animate> element's DOM interface.

Comment 8

[Brian Birtles (:birtles)]

Attachment: patch v1e

Updated check for whether SMIL is enabled.

It turns out, and I haven't tracked down exactly why, but getElementById doesn't pick up <animate> elements with ids when SMIL is not enabled. Anyway, in case that behaviour changes, I've also made it test for an SVGAnimationElement interface property: targetElement.

Comment 9

[Brian Birtles (:birtles)]

I've got this on the try server now. Previous tryserver run produced warnings that I think are ok but I want to run it again to be sure. Will add checkin-needed when done.

Comment 10

[Brian Birtles (:birtles)]

All good on TryServer.

Comment 11

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/ce760960b7ee