W3C bug: http://www.w3.org/Bugs/Public/show_bug.cgi?id=14591 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.
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.
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.
5 years ago
Autoland Patchset: Patches: 617838 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=ba191aafc3c8 Try run started, revision ba191aafc3c8. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=ba191aafc3c8
Try run for ba191aafc3c8 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=ba191aafc3c8 Results (out of 216 total builds): exception: 1 success: 181 warnings: 34 Builds (or logs if builds failed) available at: http://firstname.lastname@example.org
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: https://developer.mozilla.org/en/DOM/range#Other_methods And the doc explains this is now a no-op: https://developer.mozilla.org/en/DOM/range.detach 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.