Support radialGradient fr attribute

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

3 years ago
No description provided.
Assignee

Comment 1

3 years ago
Posted patch patch with test (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #8708672 - Flags: review?(cam)
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.
Attachment #8708672 - Flags: review?(cam) → review+
Assignee

Comment 3

3 years ago
DOM peer review for the webidl change.

I've added the additional tests (there are two tests in gradient-fr-01.svg now)
Attachment #8708672 - Attachment is obsolete: true
Attachment #8709153 - Flags: review?(bzbarsky)
Comment on attachment 8709153 [details] [diff] [review]
with more tests

Punting to Peter.
Attachment #8709153 - Flags: review?(bzbarsky) → review?(peterv)
Assignee

Comment 5

3 years ago
The patch has already been reviewed by cam, all I you need look at is the one line webidl change.
Yes, I'm explicitly punting all basic "webidl hook requires this" reviews to Peter.
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.
Flags: needinfo?(longsonr)
(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.
Flags: needinfo?(longsonr)
[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.
Assignee

Comment 10

3 years ago
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.
Assignee

Comment 11

3 years ago
where do we go from here?
Flags: needinfo?(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.
Attachment #8709153 - Flags: review?(peterv) → review+
Flags: needinfo?(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.
Attachment #8709153 - Flags: review+ → review-
Assignee

Comment 14

3 years ago
I've filed another bug, but I need your help implementing it. Can I land this as-is in the meantime?
Flags: needinfo?(peterv)
I'm ok with landing this without the Constant annotation.
Flags: needinfo?(peterv)
Blocks: svg2

Comment 17

2 years ago
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/173a14d9e1d2
Support SVG 2 radialGradient fr attribute. r=cam r=peterv (see comment 16)
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
Flags: needinfo?(longsonr)

Comment 20

2 years ago
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eabcbe4f0a1
Support SVG 2 radialGradient fr attribute. r=cam r=peterv (see comment 16)

Comment 23

2 years ago
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c18a711575f5
Support SVG 2 radialGradient fr attribute. r=cam r=peterv (see comment 16)

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c18a711575f5
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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!
You need to log in before you can comment on or make changes to this bug.