1240275 - Support radialGradient fr attribute

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch with test

Comment 2

[Cameron McCormack (:heycam)]

Comment on attachment 8708672 [details] [diff] [review]
patch with test

Review of attachment 8708672 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Would you be able to add two more tests: (1) a reftest where the result isn't just a solid color everywhere (e.g. use fr="20" in the test, and then in the reference have fr="0" and add stops so that the colors between 20 and 0 are the same), and (2) a reftest that shows that inheritance of the attribute from an xlink:href=""-referenced <radialGradient> works.  You'll also need a DOM peer's review on the .webidl change.

Comment 3

[Robert Longson [:longsonr]]

Attachment: with more tests

DOM peer review for the webidl change.

I've added the additional tests (there are two tests in gradient-fr-01.svg now)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8709153 [details] [diff] [review]
with more tests

Punting to Peter.

Comment 5

[Robert Longson [:longsonr]]

The patch has already been reviewed by cam, all I you need look at is the one line webidl change.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yes, I'm explicitly punting all basic "webidl hook requires this" reviews to Peter.

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 8709153 [details] [diff] [review]
with more tests

Review of attachment 8709153 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/SVGRadialGradientElement.webidl
@@ +20,5 @@
>    [Constant]
>    readonly attribute SVGAnimatedLength fx;
>    [Constant]
>    readonly attribute SVGAnimatedLength fy;
> +  [Constant]

Why not SameObject, like in the spec? But I don't really understand how that's possible: 1) afaict the value of this attribute changes when the content attribute is changed, and 2) even if mLengthAttributes[ATTR_FR] doesn't change I don't see anything that guarantees that ToDOMAnimatedLength(...) always returns the same value.

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Peter Van der Beken [:peterv] from comment #7)
> Why not SameObject, like in the spec? But I don't really understand how
> that's possible: 1) afaict the value of this attribute changes when the
> content attribute is changed

The same object is always returned, but its contents change when the attribute changes.  (I don't know why we use [Constant] rather than [SameObject] or what the difference really is.)

> 2) even if mLengthAttributes[ATTR_FR]
> doesn't change I don't see anything that guarantees that
> ToDOMAnimatedLength(...) always returns the same value.

ToDOMAnimatedLength's use of the nsSVGAttrTearoffTable should ensure we return the same object.

Comment 9

[Boris Zbarsky [:bzbarsky]]

[Constant] predates the existence of [SameObject].  It can also be applied to arbitrary return types, whereas [SameObject] has restrictions in the IDL spec as to which return types it can be applied to.  Apart from that difference, they do exactly the same thing.

Comment 10

[Robert Longson [:longsonr]]

I'm happy to replace [Constant] with [SameObject] throughout the SVG DOM as a follow up but this bug is not the place to do that.

Comment 11

[Robert Longson [:longsonr]]

where do we go from here?

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 8709153 [details] [diff] [review]
with more tests

Review of attachment 8709153 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Robert Longson from comment #11)
> where do we go from here?

I needed to understand why it's ok to mark this Constant/SameObject.

(In reply to Cameron McCormack (:heycam) (away January 23–26) from comment #8)
> The same object is always returned, but its contents change when the
> attribute changes.

Ok, so this works because we use nsSVGLength2* as the key.

> ToDOMAnimatedLength's use of the nsSVGAttrTearoffTable should ensure we
> return the same object.

It doesn't, since nsSVGAttrTearoffTable holds a weak pointer to the value and if the value's reflector is GC'ed then we could end up deleting the value (and removing it from the table). I discussed this with Boris, and we think it's ok because we took care of making this undetectable from JS. If the value is GC'ed then JS can't be referencing it, except through a WeakMap. If SVGAnimatedLength ended up being used as a key in a WeakMap then the GC'ing of the value would be detectable, but we refuse to use it as a key in a WeakMap (TryPreserveWrapper will return false because it implements cycle collection but doesn't inherit from nsISupports).

(In reply to Robert Longson from comment #10)
> I'm happy to replace [Constant] with [SameObject] throughout the SVG DOM as
> a follow up but this bug is not the place to do that.

Fine, please file a bug on that. But I don't see any reason to add another use of Constant if the spec uses SameObject, so use SameObject for fr.

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 8709153 [details] [diff] [review]
with more tests

Review of attachment 8709153 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, I take that back. The GC'ing is still detectable with expandos, because we'll preserve the wrapper but the tearoff is not held alive by the hashtable. If you don't want to fix that in this bug then please file another bug and remove the Constant/SameObject.

Comment 14

[Robert Longson [:longsonr]]

I've filed another bug, but I need your help implementing it. Can I land this as-is in the meantime?

Comment 15

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=935c829ce276

Comment 16

[Peter Van der Beken [:peterv]]

I'm ok with landing this without the Constant annotation.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/173a14d9e1d2
Support SVG 2 radialGradient fr attribute. r=cam r=peterv (see comment 16)

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing layout/reftests/svg/radialGradient-fr-02.svg on Windows 8 x64:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2b24e7245aa50295023d8f765c502389eda195

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=173a14d9e1d221a3af2b592524269cb35245078c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=100837312&repo=mozilla-inbound

> TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/radialGradient-fr-02.svg == file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/radialGradient-fr-02-ref.svg | image comparison, max difference: 1, number of differing pixels: 6855

Comment 19

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d20bf7dee89e05ca4fe205ffcc197aea275e077

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eabcbe4f0a1
Support SVG 2 radialGradient fr attribute. r=cam r=peterv (see comment 16)

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for reftest failures on Windows 64:

https://treeherder.mozilla.org/logviewer.html#?job_id=101038010&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/054f0b1fca63

Comment 22

[Robert Longson [:longsonr]]

added fuzzy-if for windows

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6be9ccac5f1e04fa29e7edb3d574549021f5a800

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c18a711575f5
Support SVG 2 radialGradient fr attribute. r=cam r=peterv (see comment 16)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c18a711575f5

Comment 25

[Chris Mills [:cmills]]

I've documented this, creating a new page for fr:

https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/fr

linking to it on the SVG attribute reference and <radialGradient> pages:

https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute
https://developer.mozilla.org/en-US/docs/Web/SVG/Element/radialGradient

and adding a note to the Fx55 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/55#SVG

Let me know if there's anything else you think I need to do. Thanks!