Make Range.detach() a no-op

Make Range.detach() a no-op

Make Range.detach() a no-op as per DOM4:
http://dom.spec.whatwg.org/#dom-range-detach

This makes the code a lot simpler, as it avoids a lot of null checks and
exception throwing. Also add a UseCounter and a deprecation message to see
if this can be removed entirely from the specification.

Firefox already does this since version 15:
https://bugzilla.mozilla.org/show_bug.cgi?id=702948

However, detach() is not a no-op in IE11.

R=arv@chromium.org, tkent@chromium.org
BUG=367322
TEST=fast/dom/Range/detach-no-op.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173010

[tkent, 2014-04-28 01:36:21]

> However, detach() is not a no-op in IE11. 

It sounds risky.  We should be very careful to unsupport old DOM features.
I'd like just adding UseCounters for Range creation, Range.detach call, and
exceptions in Range after detach.

[Inactive, 2014-04-28 01:55:26]

On 2014/04/28 01:36:21, tkent wrote:
> > However, detach() is not a no-op in IE11. 
> 
> It sounds risky.  We should be very careful to unsupport old DOM features.
> I'd like just adding UseCounters for Range creation, Range.detach call, and
> exceptions in Range after detach.

Firefox has been doing this for a very long time (since version 15), without
trouble. I do think the risk is limited. Adding a UseCounter will tell us that
it is used (which is why it is still in the spec). However, the issue is that it
is called unnecessarily.

My understanding is that this change could only break sites if they use the
range after calling detach () and expect it to throw, right? If so, adding a
usecounter for such exception throwing would tell us if this happens in real
world. This feels unlikely but I'll probably upload a patch to add the
usecounter tomorrow since there are concerns.

[tkent, 2014-04-28 02:20:15]

I found Range.detach has been supported since IE9.  The risk is very very low
actually.  I'll review the code.

http://msdn.microsoft.com/en-us/library/ie/ff975442(v=vs.85).aspx

[tkent, 2014-04-28 08:45:45]

+yosin, yutak

[tkent, 2014-04-28 08:50:49]

https://codereview.chromium.org/252783002/diff/40001/Source/core/dom/Range.idl
File Source/core/dom/Range.idl (right):

https://codereview.chromium.org/252783002/diff/40001/Source/core/dom/Range.id...
Source/core/dom/Range.idl:33: readonly attribute Node commonAncestorContainer;
nit: remove an extra whitespace

https://codereview.chromium.org/252783002/diff/40001/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/252783002/diff/40001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:720: return "'Range.detach' is now a no-op, as
per DOM4 (http://dom.spec.whatwg.org/#dom-range-detach).";
It's not DOM*4*.

[Yuta Kitamura, 2014-04-28 09:27:37]

This is a web-visible change but I think this is low-risk.

Code change looks mostly sane to me.

https://codereview.chromium.org/252783002/diff/40001/LayoutTests/fast/dom/Ran...
File LayoutTests/fast/dom/Range/detach-no-op.html (right):

https://codereview.chromium.org/252783002/diff/40001/LayoutTests/fast/dom/Ran...
LayoutTests/fast/dom/Range/detach-no-op.html:12:
shouldBe("range.startContainer", "document");
Could you check the invariance of all attributes related to
boundary positions (i.e. {start,end}{Container,Offset})?

Also, could you set some non-trivial boundary positions
before calling detach()? I'd like to make sure detach()
is *not* something that just resets its internal state.

https://codereview.chromium.org/252783002/diff/40001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/252783002/diff/40001/Source/core/dom/Range.cp...
Source/core/dom/Range.cpp:997: UseCounter::countDeprecation(ownerDocument(),
UseCounter::RangeDetach);
Why don't you use "DeprecateAs=" IDL attribute here?

[Inactive, 2014-04-28 14:44:18]

https://codereview.chromium.org/252783002/diff/40001/LayoutTests/fast/dom/Ran...
File LayoutTests/fast/dom/Range/detach-no-op.html (right):

https://codereview.chromium.org/252783002/diff/40001/LayoutTests/fast/dom/Ran...
LayoutTests/fast/dom/Range/detach-no-op.html:12:
shouldBe("range.startContainer", "document");
On 2014/04/28 09:27:38, Yuta Kitamura wrote:
> Could you check the invariance of all attributes related to
> boundary positions (i.e. {start,end}{Container,Offset})?
> 
> Also, could you set some non-trivial boundary positions
> before calling detach()? I'd like to make sure detach()
> is *not* something that just resets its internal state.

Done.

https://codereview.chromium.org/252783002/diff/40001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/252783002/diff/40001/Source/core/dom/Range.cp...
Source/core/dom/Range.cpp:997: UseCounter::countDeprecation(ownerDocument(),
UseCounter::RangeDetach);
On 2014/04/28 09:27:38, Yuta Kitamura wrote:
> Why don't you use "DeprecateAs=" IDL attribute here?

Done.

https://codereview.chromium.org/252783002/diff/40001/Source/core/dom/Range.idl
File Source/core/dom/Range.idl (right):

https://codereview.chromium.org/252783002/diff/40001/Source/core/dom/Range.id...
Source/core/dom/Range.idl:33: readonly attribute Node commonAncestorContainer;
On 2014/04/28 08:50:50, tkent wrote:
> nit: remove an extra whitespace

Done.

https://codereview.chromium.org/252783002/diff/40001/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/252783002/diff/40001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:720: return "'Range.detach' is now a no-op, as
per DOM4 (http://dom.spec.whatwg.org/#dom-range-detach).";
On 2014/04/28 08:50:50, tkent wrote:
> It's not DOM*4*.

Done.

[ojan, 2014-04-28 22:12:03]

I'll let tkent finish the review, but I just want to say I <3 this change. :)

[tkent, 2014-04-29 22:45:40]

lgtm

[Inactive, 2014-04-29 22:58:01]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-04-29 22:59:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/252783002/120001

[commit-bot: I haz the power, 2014-04-30 05:06:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 05:06:43]

Failed to apply patch for Source/core/frame/UseCounter.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/frame/UseCounter.h
  Hunk #1 FAILED at 370.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/frame/UseCounter.h.rej

Patch:       Source/core/frame/UseCounter.h
Index: Source/core/frame/UseCounter.h
diff --git a/Source/core/frame/UseCounter.h b/Source/core/frame/UseCounter.h
index
7784a8d2ff7bd6dad5e7c58f356747fd6a8cb5f3..3655cf0333ba04f0813d1cd0a0d4cebfa6ca7108
100644
--- a/Source/core/frame/UseCounter.h
+++ b/Source/core/frame/UseCounter.h
@@ -370,6 +370,7 @@ public:
         EventGetReturnValueFalse = 351,
         EventSetReturnValueTrue = 352,
         EventSetReturnValueFalse = 353,
+        RangeDetach = 354,
         // Add new features immediately above this line. Don't change assigned
         // numbers of any item, and don't reuse removed slots.
         NumberOfFeatures, // This enum value must be last.

[Inactive, 2014-04-30 17:07:58]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-04-30 17:08:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/252783002/160001

[commit-bot: I haz the power, 2014-04-30 17:17:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 17:17:39]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink

[Inactive, 2014-04-30 17:21:01]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-04-30 17:21:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/252783002/180001

[commit-bot: I haz the power, 2014-04-30 18:28:12]

Change committed as 173010