Last Comment Bug 702948 - Get rid of Range.detach()
: Get rid of Range.detach()
Status: RESOLVED FIXED
: 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) (away until October 25)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-16 06:21 PST by Aryeh Gregor (:ayg) (away until October 25)
Modified: 2013-04-04 13:53 PDT (History)
12 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch - Add telemetry to count how many ranges are detached (2.88 KB, patch)
2012-04-17 07:41 PDT, Aryeh Gregor (:ayg) (away until October 25)
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) (away until October 25)
bugs: review+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (away until October 25) 2011-11-16 06:21:44 PST
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?
Comment 1 Olli Pettay [:smaug] (TPAC) 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) (away until October 25) 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) (away until October 25) 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) (away until October 25) 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) (away until October 25) 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) (away until October 25) 2012-04-17 07:54:42 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/df9ea73ec1f4
Comment 8 Ed Morley [:emorley] 2012-04-18 05:23:06 PDT
https://hg.mozilla.org/mozilla-central/rev/df9ea73ec1f4
Comment 9 Aryeh Gregor (:ayg) (away until October 25) 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
	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
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:
    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://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-ba191aafc3c8
Comment 12 Aryeh Gregor (:ayg) (away until October 25) 2012-04-25 05:05:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1e2504f48fd
Comment 13 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-25 20:41:53 PDT
https://hg.mozilla.org/mozilla-central/rev/c1e2504f48fd
Comment 14 Ryosuke Niwa 2012-05-03 12:40:45 PDT
Where can I see the telemetry statistics?
Comment 15 Aryeh Gregor (:ayg) (away until October 25) 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:

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.
Comment 18 Aryeh Gregor (:ayg) (away until October 25) 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.