Closed Bug 702948 Opened 8 years ago Closed 8 years ago

Get rid of Range.detach()


(Core :: DOM: Core & HTML, enhancement)

Not set





(Reporter: ayg, Assigned: ayg)


(Keywords: dev-doc-complete)


(2 files)

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?
Nightly/Canary builds don't get enough testing, especially for intranet
web apps. So I wouldn't remove it without warning first.
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 . . .
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.
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.
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.
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.
Attachment #615712 - Flags: review?(bugs)
Attachment #615712 - Flags: review?(bugs) → review+
Assignee: nobody → ayg
Target Milestone: --- → mozilla14
This will probably cause test failures somewhere, so feel free to ignore it until try results come in.
Attachment #617838 - Flags: review?(bugs)
Whiteboard: [Leave open after merge] → [autoland-try:617838:-u all]
Whiteboard: [autoland-try:617838:-u all] → [autoland-in-queue]
Autoland Patchset:
	Patches: 617838
	Branch: mozilla-central => try
Try run started, revision ba191aafc3c8. To cancel or monitor the job, see:
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:
Whiteboard: [autoland-in-queue]
Attachment #617838 - Flags: review?(bugs) → review+
Flags: in-testsuite-
Target Milestone: mozilla14 → mozilla15
Where can I see the telemetry statistics?
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.)
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.
For now, detach() is listed as deprecated:

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

Mentioned on Firefox 15 for developers.
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.
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.