1174097 - remove SVGSVGElement.useCurrentView

Status: RESOLVED FIXED

(Core :: SVG, task)

Description

[Cameron McCormack (:heycam)]

Attachment: 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.

Comment 1

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2787c2978f80

Comment 2

[Robert Longson [:longsonr]]

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.

Comment 3

[Robert Longson [:longsonr]]

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?

Comment 4

[Cameron McCormack (:heycam)]

mUseCurrentView seems to be used internally still (in SVGFragmentIdentifier.cpp), so I think we should keep it.

Comment 5

[Robert Longson [:longsonr]]

My previous comments were only about the test_fragments.html testcase. I agree that mUseCurrentView needs to stay.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 7

[Cameron McCormack (:heycam)]

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

[Olli Pettay [:smaug][[e-mail]]]

http://logs.glob.uno/?c=freenode%23whatwg&s=12%20Jun%202015&e=12%20Jun%202015#c950747

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8621481 [details] [diff] [review]
patch

I think we should wait for some usage data before removing this property.

Comment 10

[Daniel Holbert [:dholbert]]

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

Comment 11

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/getelementbyid-and-several-properties-will-be-removed-from-svgsvgelement/

Comment 12

[Florian Scholz (Open Web Docs)]

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

Comment 13

[Cameron McCormack (:heycam)]

Not working on this, but I am certain this is safe to remove.

Comment 14

[Robert Longson [:longsonr]]

Attachment: Bug 1174097 - remove useCurrentView r=emilio

Comment 15

[Robert Longson [:longsonr]]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f736523258b
remove useCurrentView r=emilio

Comment 17

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/5f736523258b

Comment 18

[Hamish Willee]

The (minimal) FF107 docs work for this can be tracked in https://github.com/mdn/content/issues/21539