Last Comment Bug 702948 - Get rid of Range.detach()
: Get rid of Range.detach()
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla15
Assigned To: Aryeh Gregor (:ayg) (next working March 28-April 26)
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2011-11-16 06:21 PST by Aryeh Gregor (:ayg) (next working March 28-April 26)
Modified: 2013-04-04 13:53 PDT (History)
12 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch - Add telemetry to count how many ranges are detached (2.88 KB, patch)
2012-04-17 07:41 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
bugs: review+
Details | Diff | Splinter Review
Patch v1 -- Make Range.detach() a no-op (11.32 KB, patch)
2012-04-24 04:23 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
bugs: review+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (next working March 28-April 26) 2011-11-16 06:21:44 PST
W3C bug:

This is a useless feature and it would be great if we could get rid of it.  It adds needless complication to the Range spec and implementations, and has caused at least four crash bugs in Gecko: bug 62796, bug 336381, bug 529670, bug 594808.  What happens if you detach() a Range that's part of a Selection is currently undefined by the spec.  It's extremely unlikely that any sites are actually using the method; a cursory skim of Google Code Search found none.  Could we try removing this from nightlies and see if there are any complaints?
Comment 1 Olli Pettay [:smaug] 2011-11-16 07:09:17 PST
Nightly/Canary builds don't get enough testing, especially for intranet
web apps. So I wouldn't remove it without warning first.
Comment 2 Aryeh Gregor (:ayg) (next working March 28-April 26) 2011-11-16 08:55:48 PST
Are people really going to notice warnings?  Also, isn't the idea that if there are no complaints on nightly it can be pushed out to Aurora for more testing, then beta, then stable?  I thought the whole point of the tiered system is that things you're not sure about can be pushed to progressively broader audiences, so once they've been tested by a smaller audience you're confident enough to push it out to a somewhat larger audience.  No one has yet come up with an actual site that uses detach(), nor a reason why any would . . .
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-11-16 16:58:36 PST
We still see *vastly* more testing on the release channel than on any other channel. So I would not expect to get enough feedback here until a warning reaches the release channel.

And like you say, I would even be worried that being on the release channel won't give enough feedback since this is a function that has been around for a very long time.

I wonder if the first step here would be to gather telemetry data instead.
Comment 4 Aryeh Gregor (:ayg) (next working March 28-April 26) 2011-11-16 19:03:14 PST
Telemetry is obviously ideal, if it can be set up easily.  It would be really cool if we had a list of usage frequencies for *all* JS-exposed methods and attributes, actually -- that would be really useful for all kinds of things.  But that's a separate discussion, I guess.
Comment 5 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-03-19 13:23:22 PDT
Could we make it a no-op first?  If that causes no regression on the release channel for a while, we could remove it at that point.  It could only cause a regression if a site called detach() and then relied on the fact that some later action threw an exception.
Comment 6 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-04-17 07:41:08 PDT
Created attachment 615712 [details] [diff] [review]
Patch - Add telemetry to count how many ranges are detached

I think it's safe to try making this a no-op with no telemetry, but here's a patch to add telemetry as you requested.  This will tell us what percentage of Range objects were ever detached.  I think the percentage will be basically zero, and what will be interesting is how many users have the percentage exactly zero -- any such users would have been unaffected by removing detach() during the sample period.
Comment 7 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-04-17 07:54:42 PDT
Comment 8 Ed Morley [:emorley] 2012-04-18 05:23:06 PDT
Comment 9 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-04-24 04:23:58 PDT
Created attachment 617838 [details] [diff] [review]
Patch v1 -- Make Range.detach() a no-op

This will probably cause test failures somewhere, so feel free to ignore it until try results come in.
Comment 10 Mozilla RelEng Bot 2012-04-24 04:27:56 PDT
Autoland Patchset:
	Patches: 617838
	Branch: mozilla-central => try
Try run started, revision ba191aafc3c8. To cancel or monitor the job, see:
Comment 11 Mozilla RelEng Bot 2012-04-24 09:00:59 PDT
Try run for ba191aafc3c8 is complete.
Detailed breakdown of the results available here:
Results (out of 216 total builds):
    exception: 1
    success: 181
    warnings: 34
Builds (or logs if builds failed) available at:
Comment 12 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-04-25 05:05:28 PDT
Comment 13 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-25 20:41:53 PDT
Comment 14 Ryosuke Niwa 2012-05-03 12:40:45 PDT
Where can I see the telemetry statistics?
Comment 15 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-05-04 02:59:20 PDT
It's password-protected, but it's not very interesting.  The Range destructor just records whether it was ever detached or not.  So far it's 1,709,643,535 Ranges that were never detached, and 230,713 that were, or about 0.013% of Ranges detached.  But that doesn't really tell us anything very useful.  If a user visits one page that creates 999,999 ranges and doesn't detach any, and they visit a second page that creates one range and detaches it and breaks because it doesn't work, then 99.9999% of Ranges weren't detached, but half the pages broke.  IIUC, there were 140,593 submissions, so in the absolute worst case each submission might have involved close to two broken pages on average, which would probably not be acceptable.  But most likely there are no or almost no broken pages.

The interesting thing will be if we get any regression reports.  So far we haven't, but it hasn't been long, and it's only on the nightlies so far.

(It might have been more interesting to measure how many times ranges had operations called that would have formerly thrown and no longer do, so that we ignored ranges that were detached and then never used again.  Well, whatever.)
Comment 16 Eric Shepherd [:sheppy] 2012-05-07 10:19:09 PDT
Do we have any expectation as a timeline on a decision whether or not to outright remove this method? Trying to plan docs around it.
Comment 17 Eric Shepherd [:sheppy] 2012-05-07 10:29:05 PDT
For now, detach() is listed as deprecated:

And the doc explains this is now a no-op:

Mentioned on Firefox 15 for developers.
Comment 18 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-05-07 23:13:59 PDT
I think we probably don't want to ever completely remove it.  I've seen too many scripts that pointlessly call it when they're done using a range.  Making it a no-op doesn't affect them, but removing it would break them.

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