Closed
Bug 1174097
Opened 9 years ago
Closed 2 years ago
remove SVGSVGElement.useCurrentView
Categories
(Core :: SVG, task)
Core
SVG
Tracking
()
RESOLVED
FIXED
107 Branch
People
(Reporter: heycam, Assigned: longsonr)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, site-compat)
Attachments
(1 file, 1 obsolete file)
(deleted),
text/x-phabricator-request
|
Details |
SVGSVGElement.currentView and useCurrentView have been dropped from SVG 2. The former we never implemented.
https://github.com/w3c/svgwg/commit/4c26fd36937a65192024208d85c144a21071b057
r?longsonr for the SVG change, r?smaug for the IDL change.
Attachment #8621481 -
Flags: review?(longsonr)
Attachment #8621481 -
Flags: review?(bugs)
Reporter | ||
Comment 1•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → cam
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
Comment on attachment 8621481 [details] [diff] [review]
patch
There's no point in having a valid field if you're removing the code that checks it.
Nor is there much point in syntax checking fragments since we can't tell if they are valid without this interface. I'm not sure how much the test_fragments.html test amounts to without that. I can't think of any other way to test fragments though.
Attachment #8621481 -
Flags: review?(longsonr) → review-
Assignee | ||
Comment 3•9 years ago
|
||
I'm guess (reluctantly since I don't see what harm there is in having the method) I'm OK with removing test_fragments altogether if this is the way we're going, since there won't really be any way to test whether they do anything except to see if there's a visual change.
What do you think Daniel?
Flags: needinfo?(dholbert)
Reporter | ||
Comment 4•9 years ago
|
||
mUseCurrentView seems to be used internally still (in SVGFragmentIdentifier.cpp), so I think we should keep it.
Assignee | ||
Comment 5•9 years ago
|
||
My previous comments were only about the test_fragments.html testcase. I agree that mUseCurrentView needs to stay.
Comment 6•9 years ago
|
||
Comment on attachment 8621481 [details] [diff] [review]
patch
Do we have any evidence we can remove the property?
Other browsers seem to have this and supporting useCurrentView doesn't seem
to cost us much.
So, is it ok to remove it?
Reporter | ||
Comment 7•9 years ago
|
||
I have never personally seen any pages that use this property (and it's not a particularly useful feature IMO), but I haven't measured anything. You are right that it doesn't cost much but if we can remove a feature nobody uses then I'd prefer to. Other vendors in the F2F meeting this week agreed. IE doesn't support this.
Once bug 968923 is done, we could measure this, if you think we should.
Comment 8•9 years ago
|
||
Comment 9•9 years ago
|
||
Comment on attachment 8621481 [details] [diff] [review]
patch
I think we should wait for some usage data before removing this property.
Attachment #8621481 -
Flags: review?(bugs)
Comment 10•9 years ago
|
||
(In reply to Robert Longson from comment #3)
> I'm guess (reluctantly since I don't see what harm there is in having the
> method) I'm OK with removing test_fragments altogether if this is the way
> we're going, since there won't really be any way to test whether they do
> anything except to see if there's a visual change.
>
> What do you think Daniel?
I think this makes sense (modulo comment 9). Though if we end up removing this test, it sounds like we should replace it with some reftests, to test for the visual change that we're expecting. (or make sure we already have such reftests)
Flags: needinfo?(dholbert)
Updated•9 years ago
|
Keywords: dev-doc-needed
Comment 11•9 years ago
|
||
Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/getelementbyid-and-several-properties-will-be-removed-from-svgsvgelement/
Keywords: site-compat
Comment 12•7 years ago
|
||
This was removed in Chrome 56
Intent to deprecate and Remove: currentView, useCurrentView and SVGViewSpec
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Dfqld7wjHJM
Updated•5 years ago
|
Type: defect → task
Reporter | ||
Comment 13•4 years ago
|
||
Not working on this, but I am certain this is safe to remove.
Assignee: cam → nobody
Status: ASSIGNED → NEW
Updated•2 years ago
|
Severity: normal → S3
Assignee | ||
Comment 14•2 years ago
|
||
Updated•2 years ago
|
Assignee: nobody → longsonr
Status: NEW → ASSIGNED
Assignee | ||
Comment 15•2 years ago
|
||
Comment 16•2 years ago
|
||
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5f736523258b
remove useCurrentView r=emilio
Comment 17•2 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox107:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 107 Branch
Comment 18•2 years ago
|
||
The (minimal) FF107 docs work for this can be tracked in https://github.com/mdn/content/issues/21539
Assignee | ||
Updated•2 years ago
|
Attachment #8621481 -
Attachment is obsolete: true
Updated•2 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•2 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•