Closed
Bug 1240275
Opened 8 years ago
Closed 6 years ago
Support radialGradient fr attribute
Categories
(Core :: SVG, enhancement)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: longsonr, Assigned: longsonr)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 1 obsolete file)
11.79 KB,
patch
|
peterv
:
review-
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee: nobody → longsonr
Attachment #8708672 -
Flags: review?(cam)
Comment 2•8 years ago
|
||
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+
Updated•8 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 3•8 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 4•8 years ago
|
||
Comment on attachment 8709153 [details] [diff] [review] with more tests Punting to Peter.
Attachment #8709153 -
Flags: review?(bzbarsky) → review?(peterv)
Assignee | ||
Comment 5•8 years ago
|
||
The patch has already been reviewed by cam, all I you need look at is the one line webidl change.
![]() |
||
Comment 6•8 years ago
|
||
Yes, I'm explicitly punting all basic "webidl hook requires this" reviews to Peter.
Comment 7•8 years ago
|
||
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.
Updated•8 years ago
|
Flags: needinfo?(longsonr)
Comment 8•8 years ago
|
||
(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)
![]() |
||
Comment 9•8 years ago
|
||
[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•8 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.
Comment 12•8 years ago
|
||
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+
Updated•8 years ago
|
Flags: needinfo?(peterv)
Comment 13•8 years ago
|
||
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•7 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)
Assignee | ||
Comment 15•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=935c829ce276
Comment 16•7 years ago
|
||
I'm ok with landing this without the Constant annotation.
Flags: needinfo?(peterv)
Comment 17•6 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)
![]() |
||
Comment 18•6 years ago
|
||
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)
Assignee | ||
Comment 19•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d20bf7dee89e05ca4fe205ffcc197aea275e077
Flags: needinfo?(longsonr)
Comment 20•6 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)
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
Flags: needinfo?(longsonr)
Assignee | ||
Comment 22•6 years ago
|
||
added fuzzy-if for windows https://treeherder.mozilla.org/#/jobs?repo=try&revision=6be9ccac5f1e04fa29e7edb3d574549021f5a800
Flags: needinfo?(longsonr)
Comment 23•6 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c18a711575f5
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox55:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 25•6 years ago
|
||
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!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•