remove SVGSVGElement.useCurrentView

ASSIGNED
Assigned to

Status

()

Core
SVG
ASSIGNED
3 years ago
7 days ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Blocks: 1 bug, {dev-doc-needed, site-compat})

Trunk
dev-doc-needed, site-compat
Points:
---

Firefox Tracking Flags

(firefox41 affected)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8621481 [details] [diff] [review]
patch

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)
(Assignee)

Updated

3 years ago
Assignee: nobody → cam
Status: NEW → ASSIGNED
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-
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)
(Assignee)

Comment 4

3 years ago
mUseCurrentView seems to be used internally still (in SVGFragmentIdentifier.cpp), so I think we should keep it.
My previous comments were only about the test_fragments.html testcase. I agree that mUseCurrentView needs to stay.

Comment 6

3 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?
(Assignee)

Comment 7

3 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 9

3 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)
(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)
Keywords: dev-doc-needed
Blocks: 1328534
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
You need to log in before you can comment on or make changes to this bug.