Bug 14591 - Get rid of Range.detach()
Summary: Get rid of Range.detach()
Status: RESOLVED FIXED
Alias: None
Product: WebAppsWG
Classification: Unclassified
Component: DOM (show other bugs)
Version: unspecified
Hardware: All All
: P2 enhancement
Target Milestone: ---
Assignee: Anne
QA Contact: public-webapps-bugzilla
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 14842
  Show dependency treegraph
 
Reported: 2011-10-28 19:35 UTC by Aryeh Gregor
Modified: 2012-12-04 11:45 UTC (History)
8 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Aryeh Gregor 2011-10-28 19:35:39 UTC
It's absolutely useless, and it adds a line to every single Range attribute and method definition.  Skimming Code Search turns up no usage of Range.detach() in the first few pages (jquery/mootools omitted because they define methods by that name that occur a lot):

http://www.google.com/codesearch#search/&q=lang:%5Ejavascript$%20function:%5Edetach$%20-file:(%5E%7C/)jquery.*%5C.js$%20-file:mootools%5C.js$&type=cs

Are implementers willing to drop support here?  Are there sites that actually use it?  I find four crash bugs in Gecko when searching for "range detach", so clearly enough people must be using it for those to get reported.  If that's too big a compat risk, can we make it a no-op?
Comment 1 Olli Pettay 2011-11-07 16:20:44 UTC
It might be worth trying this, but perhaps implementations
should first warn about deprecated method for few releases, then
make detach no-op, and the after few releases remove the method.
Comment 2 Olli Pettay 2011-11-07 16:22:47 UTC
Or, could we make detach more useful, like detaching range automatically
from selection.
Comment 3 Mats Palmgren 2011-11-16 04:01:33 UTC
(In reply to comment #0)
> I find four crash bugs in Gecko when searching for "range detach", so
> clearly enough people must be using it for those to get reported.

I found 5 Gecko crash bugs involving Range.detach(), all of which
were found by fuzz testing tools, so you shouldn't take those as
an indication that detach() is used much on the web.

(In reply to comment #1)
> warn about deprecated method for few releases, then
> make detach no-op, and the after few releases remove the method.

This sounds like a good plan to me.

(In reply to comment #2)
> Or, could we make detach more useful, like detaching range automatically
> from selection.

I would like to remove detach().
Comment 4 Aryeh Gregor 2011-11-16 14:09:50 UTC
Could Mozilla or Google try just removing the method in nightlies/canaries and see if there are any complaints?  It seems very unlikely based on available evidence that any sites are actually relying on it, so making it warn/no-op/etc. seems overly complicated.  The point of nightlies is you can make changes that have a little compat risk, right?
Comment 5 Aryeh Gregor 2011-11-16 14:22:02 UTC
Anne suggested I file a Gecko bug, so I did:

https://bugzilla.mozilla.org/show_bug.cgi?id=702948
Comment 6 Olli Pettay 2011-11-16 15:07:52 UTC
Nightly/Canary builds don't get enough testing, especially for intranet
web apps.
Comment 7 Aryeh Gregor 2012-05-03 11:31:24 UTC
Gecko is experimenting with making Range.detach() a no-op for Firefox 15.  Currently it's in nightlies, and I don't think we've received any complaints of broken pages yet, but it's only been a few days.  If it gets to stable and we still have no or almost no problem reports, I think it's definitely worth speccing it as a no-op.

Removing it entirely is probably a bad idea.  I've seen code (including in Gecko's own non-detach()-related test support code) that pointlessly calls detach() after it's done with the range and doesn't intend to use it again.  Probably this is based on the theory that any associated storage can be freed at that point or something -- although Gecko never actually did that AFAICT.

Ms2ger, Anne, are you okay with us updating the spec now to have an XXX about this?  I'd wait a little longer before changing the spec to match Gecko, in case problems arise.
Comment 8 Anne 2012-05-03 13:18:41 UTC
Just make it a no-op once we know enough. Intermediate update is not necessary.
Comment 9 Anne 2012-11-28 16:28:20 UTC
Given that it's been many Firefox releases without complaints, I removed it since Aryeh is short on time:

https://github.com/whatwg/dom/commit/5fc29db3e55d3b9b5b5ba62b12741636fd05d8ee

Review appreciated:

http://dom.spec.whatwg.org/#ranges
Comment 10 Olli Pettay 2012-11-28 16:32:47 UTC
Gecko does have .detach(). It is just no-op.
Comment 11 Olli Pettay 2012-11-28 16:33:49 UTC
...which is what the spec says it should be doing :)
Comment 12 Anne 2012-11-28 16:34:35 UTC
As does the spec. I meant I removed the detached flag.
Comment 13 Tim Down 2012-11-28 17:11:11 UTC
I'm in favour of getting rid of detach() because its existence is confusing, but I still have some residual confusion. I had assumed that the reason for detach()'s existence was because a range needs to observe the DOM in order to update itself when the DOM changes, which must have some performance implication. Was I mistaken? If not, is the process of detaching a range now handled when the range is garbage collected?
Comment 14 Aryeh Gregor 2012-12-04 11:45:06 UTC
Yes, in theory detaching a range avoids the need for it to be informed of DOM changes, but in practice JS relies on garbage collection for this.  The whole point of a garbage-collected language is that authors shouldn't have to manually release resources.