The node and allowPartialContainment arguments for Selection API should not be optional

The node and allowPartialContainment arguments for Selection API should not be optional

As per spec link
http://w3c.github.io/selection-api/#idl-def-Selection
these arguments should not be optional

Firefox throws TypeError: Not enough arguments to Selection.containsNode,
if arguments are missing.

BUG=460722
Committed: https://crrev.com/ad5085931cd229c91c5d2905b2a67eef23ae442d
git-svn-id: svn://svn.chromium.org/blink/trunk@202043 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[tanay.c, 2015-08-26 13:10:54]

tanay.c@samsung.com changed reviewers:
+ philipj@opera.com

[tanay.c, 2015-08-26 13:10:55]

Please have a look.

[tanay.c, 2015-08-26 13:10:56]

Please have a look.

[philipj_slow, 2015-08-27 09:15:32]

https://codereview.chromium.org/1308663003/diff/20001/LayoutTests/editing/sel...
File LayoutTests/editing/selection/DOMSelection-DocumentType-expected.txt
(right):

https://codereview.chromium.org/1308663003/diff/20001/LayoutTests/editing/sel...
LayoutTests/editing/selection/DOMSelection-DocumentType-expected.txt:12: PASS
sel.containsNode(docType, false) is false
Seeing this I think it would make sense to keep the second argument optional,
and have pinged a spec issue I filed on this:
https://github.com/w3c/selection-api/issues/30

I think you can proceed on the assumption that the second argument will be
optional, but that the first argument will be required. The CL can land as soon
as the spec is changed or the editor agrees in principal to the change.

https://codereview.chromium.org/1308663003/diff/20001/Source/core/editing/Sel...
File Source/core/editing/Selection.idl (right):

https://codereview.chromium.org/1308663003/diff/20001/Source/core/editing/Sel...
Source/core/editing/Selection.idl:63: [MeasureAs=SelectionContainsNode] boolean
containsNode(Node node, boolean allowPartialContainment);
Please also add TypeChecking=Interface so that passing null for the first
argument will throw an exception. It's better to change this once only.

With this change you should also be able to remove the internal null check, as
long as the API isn't called from any other place where node can be null.

[tanay.c, 2015-08-28 08:52:32]

Thanks for the comments. I have made the changes as required. Could you please
take a look.

https://codereview.chromium.org/1308663003/diff/20001/LayoutTests/editing/sel...
File LayoutTests/editing/selection/DOMSelection-DocumentType-expected.txt
(right):

https://codereview.chromium.org/1308663003/diff/20001/LayoutTests/editing/sel...
LayoutTests/editing/selection/DOMSelection-DocumentType-expected.txt:12: PASS
sel.containsNode(docType, false) is false
On 2015/08/27 09:15:32, philipj wrote:
> Seeing this I think it would make sense to keep the second argument optional,
> and have pinged a spec issue I filed on this:
> https://github.com/w3c/selection-api/issues/30
> 
> I think you can proceed on the assumption that the second argument will be
> optional, but that the first argument will be required. The CL can land as
soon
> as the spec is changed or the editor agrees in principal to the change.

Done.

https://codereview.chromium.org/1308663003/diff/20001/Source/core/editing/Sel...
File Source/core/editing/Selection.idl (right):

https://codereview.chromium.org/1308663003/diff/20001/Source/core/editing/Sel...
Source/core/editing/Selection.idl:63: [MeasureAs=SelectionContainsNode] boolean
containsNode(Node node, boolean allowPartialContainment);
On 2015/08/27 09:15:32, philipj wrote:
> Please also add TypeChecking=Interface so that passing null for the first
> argument will throw an exception. It's better to change this once only.
> 
> With this change you should also be able to remove the internal null check, as
> long as the API isn't called from any other place where node can be null.

Done.

[philipj_slow, 2015-08-31 14:48:21]

https://codereview.chromium.org/1308663003/diff/60001/LayoutTests/editing/sel...
File LayoutTests/editing/selection/containsNode.html (left):

https://codereview.chromium.org/1308663003/diff/60001/LayoutTests/editing/sel...
LayoutTests/editing/selection/containsNode.html:36:
shouldBe('s.containsNode(null, false)', false);
Instead of removing these tests, they should check that this throws an exception
instead. Try shouldThrow(...).

https://codereview.chromium.org/1308663003/diff/60001/Source/core/editing/DOM...
File Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/1308663003/diff/60001/Source/core/editing/DOM...
Source/core/editing/DOMSelection.cpp:457: bool DOMSelection::containsNode(const
Node* n, bool allowPartial) const
ASSERT(n) at the top of this method so that it's very clear that !m_frame does
not imply that it's OK to pass null as the node.

https://codereview.chromium.org/1308663003/diff/60001/Source/core/editing/Sel...
File Source/core/editing/Selection.idl (right):

https://codereview.chromium.org/1308663003/diff/60001/Source/core/editing/Sel...
Source/core/editing/Selection.idl:63: [MeasureAs=SelectionContainsNode,
TypeChecking=Interface] boolean containsNode(Node node, [Default=Undefined]
optional boolean allowPartialContainment);
Please remove [Default=Undefined] use "optional boolean allowPartialContainment
= false" instead.

[tanay.c, 2015-09-01 10:58:09]

Modified as per comments. PTAL

https://codereview.chromium.org/1308663003/diff/60001/LayoutTests/editing/sel...
File LayoutTests/editing/selection/containsNode.html (left):

https://codereview.chromium.org/1308663003/diff/60001/LayoutTests/editing/sel...
LayoutTests/editing/selection/containsNode.html:36:
shouldBe('s.containsNode(null, false)', false);
On 2015/08/31 14:48:21, philipj wrote:
> Instead of removing these tests, they should check that this throws an
exception
> instead. Try shouldThrow(...).

Done.

https://codereview.chromium.org/1308663003/diff/60001/Source/core/editing/DOM...
File Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/1308663003/diff/60001/Source/core/editing/DOM...
Source/core/editing/DOMSelection.cpp:457: bool DOMSelection::containsNode(const
Node* n, bool allowPartial) const
On 2015/08/31 14:48:21, philipj wrote:
> ASSERT(n) at the top of this method so that it's very clear that !m_frame does
> not imply that it's OK to pass null as the node.

Done.

https://codereview.chromium.org/1308663003/diff/60001/Source/core/editing/Sel...
File Source/core/editing/Selection.idl (right):

https://codereview.chromium.org/1308663003/diff/60001/Source/core/editing/Sel...
Source/core/editing/Selection.idl:63: [MeasureAs=SelectionContainsNode,
TypeChecking=Interface] boolean containsNode(Node node, [Default=Undefined]
optional boolean allowPartialContainment);
On 2015/08/31 14:48:21, philipj wrote:
> Please remove [Default=Undefined] use "optional boolean
allowPartialContainment
> = false" instead.

Done.

[philipj_slow, 2015-09-01 12:38:53]

LGTM, looks like DOMSelection::containsNode is called only from bindings so the
new ASSERT should be safe.

[tanay.c, 2015-09-10 06:37:31]

The CQ bit was checked by tanay.c@samsung.com

[tanay.c, 2015-09-10 06:37:39]

The CQ bit was unchecked by tanay.c@samsung.com

[philipj_slow, 2015-09-10 08:06:03]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2015-09-10 08:06:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308663003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308663003/80001

[commit-bot: I haz the power, 2015-09-10 09:13:17]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202043

[commit-bot: I haz the power, 2015-09-23 12:09:01]

Patchset 5 (id:??) landed as
https://crrev.com/ad5085931cd229c91c5d2905b2a67eef23ae442d