Last Comment Bug 801562 - Remove Node.isSupported
: Remove Node.isSupported
Status: RESOLVED FIXED
: dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla22
Assigned To: Aryeh Gregor (:ayg) (next working March 28-April 26)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 773780
Blocks: 801425
  Show dependency treegraph
 
Reported: 2012-10-15 02:42 PDT by Aryeh Gregor (:ayg) (next working March 28-April 26)
Modified: 2013-05-16 11:50 PDT (History)
12 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (147.22 KB, patch)
2012-10-15 03:21 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
bzbarsky: review+
Details | Diff | Splinter Review
Patch, rebased (140.13 KB, patch)
2013-03-14 07:37 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
bzbarsky: review+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-10-15 02:42:08 PDT
DOM says to.  Worth a try?
Comment 1 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-10-15 02:51:25 PDT
This should land after bug 773780 to avoid bitrot.  I'll still write the patch now while I have time, and update it later.
Comment 2 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-10-15 03:21:23 PDT
Created attachment 671362 [details] [diff] [review]
Patch

The diffstat made me an immediate convert:

 42 files changed, 1 insertions(+), 3484 deletions(-)

Granted that it's almost all test files -- but still, they're horribly ugly tests and I'm delighted to be rid of them.  A few more test changes might be needed.  Try: <https://tbpl.mozilla.org/?tree=Try&rev=3f35951f7d8c>

The one line inserted is because of js/xpconnect/crashtests/468552-1.html.  Bug 468552 happened to use isSupported to test for an XPConnect bug -- I switched it to replaceChild instead, on the theory that it should be testing the same basic bug.
Comment 3 Robert Longson 2012-10-15 03:30:24 PDT
For the moment perhaps the SVG mochitest should stay but use hasFeature instead and only test SVG features. That would still allow you to get rid of isSupported wouldn't it?
Comment 4 Olli Pettay [:smaug] 2012-10-15 03:47:54 PDT
Given that even removing navigator.taintEnabled() (Bug 679971) caused major problems, I'd
be very careful removing this stuff.
We need to have at least some data whether it is being used.
So, please add first some telemetry probes.
Comment 5 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-10-15 04:03:57 PDT
(In reply to Robert Longson from comment #3)
> For the moment perhaps the SVG mochitest should stay but use hasFeature
> instead and only test SVG features. That would still allow you to get rid of
> isSupported wouldn't it?

This is written on top of bug 801425, so hasFeature() always returns true.  If bug 801425 changes so that this is no longer the case, what you suggest would make sense.

(In reply to Olli Pettay [:smaug] from comment #4)
> Given that even removing navigator.taintEnabled() (Bug 679971) caused major
> problems, I'd
> be very careful removing this stuff.
> We need to have at least some data whether it is being used.
> So, please add first some telemetry probes.

I did Google Code Search for /isSupported\([^)[]*,/ in JavaScript.  There were around 85 results and I looked at all of them -- none seemed to be real uses.  That implies at least that it's not used in any major JS library, since any major JS library would be included in at least one project indexed by Code Search.  So it's better than navigator.taintEnabled, which it turned out was used in jQuery.

We could add telemetry probes, but if so, please let's decide in advance how we plan to interpret the results.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-10-15 11:44:10 PDT
Comment on attachment 671362 [details] [diff] [review]
Patch

Rev iid, please.  ;)

Seems fine with that.
Comment 7 Aryeh Gregor (:ayg) (next working March 28-April 26) 2013-03-14 07:37:37 PDT
Created attachment 724904 [details] [diff] [review]
Patch, rebased

I had to rewrite the patch to apply on top of the current tree, because IsSupported implementations have moved around a bunch.  It builds locally and passes some tests I tried.  I also adjusted the SVG test to use hasFeature instead of deleting it.  The basic idea is identical to the previous patch.

Try: https://tbpl.mozilla.org/?tree=Try&rev=4c0a12849b8f
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2013-03-14 11:22:48 PDT
Comment on attachment 724904 [details] [diff] [review]
Patch, rebased

r=me especially if other UAs already don't support this...
Comment 9 Aryeh Gregor (:ayg) (next working March 28-April 26) 2013-03-15 03:23:57 PDT
Nope, we're breaking new ground here.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7006217d7d
Comment 10 Phil Ringnalda (:philor) 2013-03-16 15:57:52 PDT
https://hg.mozilla.org/mozilla-central/rev/0e7006217d7d
Comment 11 Tom Schuster [:evilpie] 2013-03-22 13:26:03 PDT
Mentioned on https://developer.mozilla.org/en-US/docs/Firefox_22_for_developers and put an obsolete header on https://developer.mozilla.org/en-US/docs/DOM/Node.isSupported.
Comment 12 Kohei Yoshino [:kohei] 2013-03-29 09:54:33 PDT
I've added this bug to the compatibility doc. Please correct the info if wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22

Note You need to log in before you can comment on or make changes to this bug.