Add [TypeChecking=Interface] on various methods in Document.idl

Add [TypeChecking=Interface] on various methods in Document.idl

This adds type checks on some Node arguments in the generated bindings
code, and lets us drop the corresponding manual null checks and exception
throwing code in the implementations.

The type of exception thrown changes from DOMException (NOT_SUPPORTED_ERR)
to TypeError as a side-effect. This matches the Web IDL specification.

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

[Jens Widell, 2014-06-27 12:01:31]

PTAL

I also experimented with adding [TypeChecking=Interface] on the whole interface,
but decided not to in the end, since the effects of that was a bit bigger than I
had hoped.

The new exception types match Firefox's behavior, while IE11's throws
DOMException in some cases and Error in others.

[sof, 2014-06-27 12:11:08]

https://codereview.chromium.org/360463005/diff/1/LayoutTests/fast/dom/NodeIte...
File LayoutTests/fast/dom/NodeIterator/NodeIterator-basic.html (right):

https://codereview.chromium.org/360463005/diff/1/LayoutTests/fast/dom/NodeIte...
LayoutTests/fast/dom/NodeIterator/NodeIterator-basic.html:44: assert_throws(new
TypeError(), function () { document.createNodeIterator(null); });
It's consistent with the above, but the 'new' is redundant.

[haraken, 2014-06-27 12:39:33]

https://codereview.chromium.org/360463005/diff/1/Source/core/dom/Document.idl
File Source/core/dom/Document.idl (right):

https://codereview.chromium.org/360463005/diff/1/Source/core/dom/Document.idl...
Source/core/dom/Document.idl:74: [RaisesException, TypeChecking=Interface]
NodeIterator createNodeIterator(Node root,

Just help me understand: When do we need to use [TypeChecking=Interface] instead
of [TypeChecking=Interface|Nullable]? For example, what happens if we specify
[TypeChecking=Interface|Nullable] here? (I'm asking this since NodeFilter? has
"?".)

[Jens Widell, 2014-06-27 12:53:31]

https://codereview.chromium.org/360463005/diff/1/Source/core/dom/Document.idl
File Source/core/dom/Document.idl (right):

https://codereview.chromium.org/360463005/diff/1/Source/core/dom/Document.idl...
Source/core/dom/Document.idl:74: [RaisesException, TypeChecking=Interface]
NodeIterator createNodeIterator(Node root,
On 2014/06/27 12:39:33, haraken wrote:
> 
> Just help me understand: When do we need to use [TypeChecking=Interface]
instead
> of [TypeChecking=Interface|Nullable]? For example, what happens if we specify
> [TypeChecking=Interface|Nullable] here? (I'm asking this since NodeFilter? has
> "?".)

Nothing happens if I add "|Nullable" here.

AFAICT, [TypeChecking=Nullable] is only meaningful on nullable attributes.

[haraken, 2014-06-27 13:15:04]

https://codereview.chromium.org/360463005/diff/1/Source/core/dom/Document.idl
File Source/core/dom/Document.idl (right):

https://codereview.chromium.org/360463005/diff/1/Source/core/dom/Document.idl...
Source/core/dom/Document.idl:74: [RaisesException, TypeChecking=Interface]
NodeIterator createNodeIterator(Node root,
On 2014/06/27 12:53:31, Jens Lindström wrote:
> On 2014/06/27 12:39:33, haraken wrote:
> > 
> > Just help me understand: When do we need to use [TypeChecking=Interface]
> instead
> > of [TypeChecking=Interface|Nullable]? For example, what happens if we
specify
> > [TypeChecking=Interface|Nullable] here? (I'm asking this since NodeFilter?
has
> > "?".)
> 
> Nothing happens if I add "|Nullable" here.
> 
> AFAICT, [TypeChecking=Nullable] is only meaningful on nullable attributes.

Isn't it (or shouldn't it be) meaningful on nullable parameters?

I expect that if I use [TypeChecking=Interface] here, a null value is not
allowed for the |filter| parameter. In order to allow a null value, I expect we
need to use [TypeChecking=Interface|Nullable].

(I haven't looked at the current implementation, and am just thinking about what
should happen.)

[Jens Widell, 2014-06-27 13:18:14]

On 2014/06/27 13:15:04, haraken wrote:
> https://codereview.chromium.org/360463005/diff/1/Source/core/dom/Document.idl
> File Source/core/dom/Document.idl (right):
> 
>
https://codereview.chromium.org/360463005/diff/1/Source/core/dom/Document.idl...
> Source/core/dom/Document.idl:74: [RaisesException, TypeChecking=Interface]
> NodeIterator createNodeIterator(Node root,
> On 2014/06/27 12:53:31, Jens Lindström wrote:
> > On 2014/06/27 12:39:33, haraken wrote:
> > > 
> > > Just help me understand: When do we need to use [TypeChecking=Interface]
> > instead
> > > of [TypeChecking=Interface|Nullable]? For example, what happens if we
> specify
> > > [TypeChecking=Interface|Nullable] here? (I'm asking this since NodeFilter?
> has
> > > "?".)
> > 
> > Nothing happens if I add "|Nullable" here.
> > 
> > AFAICT, [TypeChecking=Nullable] is only meaningful on nullable attributes.
> 
> Isn't it (or shouldn't it be) meaningful on nullable parameters?
> 
> I expect that if I use [TypeChecking=Interface] here, a null value is not
> allowed for the |filter| parameter. In order to allow a null value, I expect
we
> need to use [TypeChecking=Interface|Nullable].
> 
> (I haven't looked at the current implementation, and am just thinking about
what
> should happen.)

I'm not sure I understand why we should need to declare via [TypeChecking] that
null is an allowed value for a nullable argument. It is per spec.

[haraken, 2014-06-27 13:22:32]

On 2014/06/27 13:18:14, Jens Lindström wrote:
> On 2014/06/27 13:15:04, haraken wrote:
> >
https://codereview.chromium.org/360463005/diff/1/Source/core/dom/Document.idl
> > File Source/core/dom/Document.idl (right):
> > 
> >
>
https://codereview.chromium.org/360463005/diff/1/Source/core/dom/Document.idl...
> > Source/core/dom/Document.idl:74: [RaisesException, TypeChecking=Interface]
> > NodeIterator createNodeIterator(Node root,
> > On 2014/06/27 12:53:31, Jens Lindström wrote:
> > > On 2014/06/27 12:39:33, haraken wrote:
> > > > 
> > > > Just help me understand: When do we need to use [TypeChecking=Interface]
> > > instead
> > > > of [TypeChecking=Interface|Nullable]? For example, what happens if we
> > specify
> > > > [TypeChecking=Interface|Nullable] here? (I'm asking this since
NodeFilter?
> > has
> > > > "?".)
> > > 
> > > Nothing happens if I add "|Nullable" here.
> > > 
> > > AFAICT, [TypeChecking=Nullable] is only meaningful on nullable attributes.
> > 
> > Isn't it (or shouldn't it be) meaningful on nullable parameters?
> > 
> > I expect that if I use [TypeChecking=Interface] here, a null value is not
> > allowed for the |filter| parameter. In order to allow a null value, I expect
> we
> > need to use [TypeChecking=Interface|Nullable].
> > 
> > (I haven't looked at the current implementation, and am just thinking about
> what
> > should happen.)
> 
> I'm not sure I understand why we should need to declare via [TypeChecking]
that
> null is an allowed value for a nullable argument. It is per spec.

I just want to understand what [TypeChecking=Nullable] means or should mean.

For example, we have:

[TypeChecking=Interface|Nullable, RaisesException] void
removeStream(MediaStream? stream);
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Why do we need [TypeChecking=Nullable] for removeStream(MediaStream? stream),
but we don't need [TypeChecking=Nullable] for Document.createNodeIterator(...,
optional NodeFilter? filter = null)? Is it related to whether we have "optional"
or not?

[Jens Widell, 2014-06-27 13:25:53]

On 2014/06/27 13:22:32, haraken wrote:
> On 2014/06/27 13:18:14, Jens Lindström wrote:
> > On 2014/06/27 13:15:04, haraken wrote:
> > >
> https://codereview.chromium.org/360463005/diff/1/Source/core/dom/Document.idl
> > > File Source/core/dom/Document.idl (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/360463005/diff/1/Source/core/dom/Document.idl...
> > > Source/core/dom/Document.idl:74: [RaisesException, TypeChecking=Interface]
> > > NodeIterator createNodeIterator(Node root,
> > > On 2014/06/27 12:53:31, Jens Lindström wrote:
> > > > On 2014/06/27 12:39:33, haraken wrote:
> > > > > 
> > > > > Just help me understand: When do we need to use
[TypeChecking=Interface]
> > > > instead
> > > > > of [TypeChecking=Interface|Nullable]? For example, what happens if we
> > > specify
> > > > > [TypeChecking=Interface|Nullable] here? (I'm asking this since
> NodeFilter?
> > > has
> > > > > "?".)
> > > > 
> > > > Nothing happens if I add "|Nullable" here.
> > > > 
> > > > AFAICT, [TypeChecking=Nullable] is only meaningful on nullable
attributes.
> > > 
> > > Isn't it (or shouldn't it be) meaningful on nullable parameters?
> > > 
> > > I expect that if I use [TypeChecking=Interface] here, a null value is not
> > > allowed for the |filter| parameter. In order to allow a null value, I
expect
> > we
> > > need to use [TypeChecking=Interface|Nullable].
> > > 
> > > (I haven't looked at the current implementation, and am just thinking
about
> > what
> > > should happen.)
> > 
> > I'm not sure I understand why we should need to declare via [TypeChecking]
> that
> > null is an allowed value for a nullable argument. It is per spec.
> 
> I just want to understand what [TypeChecking=Nullable] means or should mean.
> 
> For example, we have:
> 
> [TypeChecking=Interface|Nullable, RaisesException] void
> removeStream(MediaStream? stream);
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Why do we need [TypeChecking=Nullable] for removeStream(MediaStream? stream),
> but we don't need [TypeChecking=Nullable] for Document.createNodeIterator(...,
> optional NodeFilter? filter = null)? Is it related to whether we have
"optional"
> or not?

Short answer: we don't need [TypeChecking=Nullable] for any method at all.

See also: https://codereview.chromium.org/356843007/ :-)

[haraken, 2014-06-27 13:53:43]

> > Why do we need [TypeChecking=Nullable] for removeStream(MediaStream?
stream),
> > but we don't need [TypeChecking=Nullable] for
Document.createNodeIterator(...,
> > optional NodeFilter? filter = null)? Is it related to whether we have
> "optional"
> > or not?
> 
> Short answer: we don't need [TypeChecking=Nullable] for any method at all.
> 
> See also: https://codereview.chromium.org/356843007/ :-)

Thanks for the investigation. The last question: Then why do we need
[TypeChecking=Nullable] for attributes? Can't we infer the information from the
fact that the attribute type is nullable as well?

[Jens Widell, 2014-06-27 13:59:33]

On 2014/06/27 13:53:43, haraken wrote:
> > > Why do we need [TypeChecking=Nullable] for removeStream(MediaStream?
> stream),
> > > but we don't need [TypeChecking=Nullable] for
> Document.createNodeIterator(...,
> > > optional NodeFilter? filter = null)? Is it related to whether we have
> > "optional"
> > > or not?
> > 
> > Short answer: we don't need [TypeChecking=Nullable] for any method at all.
> > 
> > See also: https://codereview.chromium.org/356843007/ :-)
> 
> Thanks for the investigation. The last question: Then why do we need
> [TypeChecking=Nullable] for attributes? Can't we infer the information from
the
> fact that the attribute type is nullable as well?

No, we don't, and yes, we can.

See also: https://codereview.chromium.org/352643005/ :-)

[haraken, 2014-06-27 14:00:35]

On 2014/06/27 13:59:33, Jens Lindström wrote:
> On 2014/06/27 13:53:43, haraken wrote:
> > > > Why do we need [TypeChecking=Nullable] for removeStream(MediaStream?
> > stream),
> > > > but we don't need [TypeChecking=Nullable] for
> > Document.createNodeIterator(...,
> > > > optional NodeFilter? filter = null)? Is it related to whether we have
> > > "optional"
> > > > or not?
> > > 
> > > Short answer: we don't need [TypeChecking=Nullable] for any method at all.
> > > 
> > > See also: https://codereview.chromium.org/356843007/ :-)
> > 
> > Thanks for the investigation. The last question: Then why do we need
> > [TypeChecking=Nullable] for attributes? Can't we infer the information from
> the
> > fact that the attribute type is nullable as well?
> 
> No, we don't, and yes, we can.
> 
> See also: https://codereview.chromium.org/352643005/ :-)

Thanks, now everything makes sense. We don't need [TypeChecking=Nullable] at
all.

LGTM.

[Jens Widell, 2014-06-27 14:11:01]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-06-27 14:11:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/360463005/1

[commit-bot: I haz the power, 2014-06-27 15:17:07]

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

[commit-bot: I haz the power, 2014-06-27 15:17:08]

Failed to apply patch for Source/core/dom/Document.idl:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/dom/Document.idl
  Hunk #1 FAILED at 42.
  1 out of 5 hunks FAILED -- saving rejects to file
Source/core/dom/Document.idl.rej

Patch:       Source/core/dom/Document.idl
Index: Source/core/dom/Document.idl
diff --git a/Source/core/dom/Document.idl b/Source/core/dom/Document.idl
index
3887a624acfcf4dfa67744f3bbb37d1281c0ec18..3c36ffd937c4771256493ec4e045718d32e61ab9
100644
--- a/Source/core/dom/Document.idl
+++ b/Source/core/dom/Document.idl
@@ -42,7 +42,7 @@ typedef (CanvasRenderingContext2D or WebGLRenderingContext)
RenderingContext;
 
     // Introduced in DOM Level 2:
 
-    [CustomElementCallbacks, LogActivity, RaisesException] Node importNode(Node
node, optional boolean deep);
+    [CustomElementCallbacks, LogActivity, RaisesException,
TypeChecking=Interface] Node importNode(Node node, optional boolean deep);
     [CustomElementCallbacks, LogActivity, RaisesException] Element
createElementNS([TreatNullAs=NullString] DOMString namespaceURI, DOMString
qualifiedName);
     [RaisesException, DeprecateAs=DocumentCreateAttributeNS] Attr
createAttributeNS([TreatNullAs=NullString,Default=Undefined] optional DOMString
namespaceURI,
                                                                                
  [TreatNullAs=NullString,Default=Undefined] optional DOMString qualifiedName);
// Removed from DOM4.
@@ -57,7 +57,7 @@ typedef (CanvasRenderingContext2D or WebGLRenderingContext)
RenderingContext;
     [TreatReturnedNullStringAs=Null, TreatNullAs=NullString,
RaisesException=Setter, MeasureAs=DocumentXMLVersion] attribute DOMString
xmlVersion; // Removed from DOM4.
     [RaisesException=Setter, MeasureAs=DocumentXMLStandalone] attribute boolean
xmlStandalone; // Removed from DOM4.
 
-    [RaisesException, CustomElementCallbacks] Node adoptNode(Node node);
+    [RaisesException, CustomElementCallbacks, TypeChecking=Interface] Node
adoptNode(Node node);
 
     [TreatReturnedNullStringAs=Null, ImplementedAs=url] readonly attribute
DOMString documentURI;
 
@@ -71,12 +71,12 @@ typedef (CanvasRenderingContext2D or WebGLRenderingContext)
RenderingContext;
 
     // DOM Level 2 Traversal and Range (DocumentTraversal interface)
 
-    [RaisesException] NodeIterator createNodeIterator(Node root,
-                                                      optional unsigned long
whatToShow = 0xFFFFFFFF,
-                                                      optional NodeFilter?
filter = null);
-    [RaisesException] TreeWalker createTreeWalker(Node root,
-                                                  optional unsigned long
whatToShow = 0xFFFFFFFF,
-                                                  optional NodeFilter? filter =
null);
+    [RaisesException, TypeChecking=Interface] NodeIterator
createNodeIterator(Node root,
+                                                                             
optional unsigned long whatToShow = 0xFFFFFFFF,
+                                                                             
optional NodeFilter? filter = null);
+    [RaisesException, TypeChecking=Interface] TreeWalker createTreeWalker(Node
root,
+                                                                         
optional unsigned long whatToShow = 0xFFFFFFFF,
+                                                                         
optional NodeFilter? filter = null);
 
     // DOM Level 2 Abstract Views (DocumentView interface)
 
@@ -88,8 +88,8 @@ typedef (CanvasRenderingContext2D or WebGLRenderingContext)
RenderingContext;
 
     // DOM Level 2 Style (DocumentCSS interface)
 
-     CSSStyleDeclaration getOverrideStyle([Default=Undefined] optional Element
element,
-                                                        [Default=Undefined]
optional DOMString pseudoElement);
+    CSSStyleDeclaration getOverrideStyle([Default=Undefined] optional Element
element,
+                                         [Default=Undefined] optional DOMString
pseudoElement);
 
     // DOM 4
     readonly attribute DOMString contentType;
@@ -192,16 +192,16 @@ typedef (CanvasRenderingContext2D or
WebGLRenderingContext) RenderingContext;
     attribute EventHandler onwheel;
 
     [RuntimeEnabled=Touch] Touch createTouch([Default=Undefined] optional
Window window,
-                                               [Default=Undefined] optional
EventTarget target,
-                                               [Default=Undefined] optional
long identifier,
-                                               [Default=Undefined] optional
double pageX,
-                                               [Default=Undefined] optional
double pageY,
-                                               [Default=Undefined] optional
double screenX,
-                                               [Default=Undefined] optional
double screenY,
-                                               [Default=Undefined] optional
double webkitRadiusX,
-                                               [Default=Undefined] optional
double webkitRadiusY,
-                                               [Default=Undefined] optional
float webkitRotationAngle,
-                                               [Default=Undefined] optional
float webkitForce);
+                                             [Default=Undefined] optional
EventTarget target,
+                                             [Default=Undefined] optional long
identifier,
+                                             [Default=Undefined] optional
double pageX,
+                                             [Default=Undefined] optional
double pageY,
+                                             [Default=Undefined] optional
double screenX,
+                                             [Default=Undefined] optional
double screenY,
+                                             [Default=Undefined] optional
double webkitRadiusX,
+                                             [Default=Undefined] optional
double webkitRadiusY,
+                                             [Default=Undefined] optional float
webkitRotationAngle,
+                                             [Default=Undefined] optional float
webkitForce);
     [RuntimeEnabled=Touch] TouchList createTouchList(Touch... touches);
 
     [CallWith=ScriptState, CustomElementCallbacks, RaisesException,
MeasureAs=DocumentRegisterElement] CustomElementConstructor
registerElement(DOMString name, optional Dictionary options);

[Jens Widell, 2014-06-27 15:53:03]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-06-27 15:53:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/360463005/20001

[Jens Widell, 2014-06-27 15:59:04]

On 2014/06/27 12:11:08, sof wrote:
>
https://codereview.chromium.org/360463005/diff/1/LayoutTests/fast/dom/NodeIte...
> File LayoutTests/fast/dom/NodeIterator/NodeIterator-basic.html (right):
> 
>
https://codereview.chromium.org/360463005/diff/1/LayoutTests/fast/dom/NodeIte...
> LayoutTests/fast/dom/NodeIterator/NodeIterator-basic.html:44:
assert_throws(new
> TypeError(), function () { document.createNodeIterator(null); });
> It's consistent with the above, but the 'new' is redundant.

It's also quite consistent with other uses of assert_throws() in LayoutTests/
that check for TypeError. The patterns in use seem to be

  assert_throws(new TypeError(), ...)
  assert_throws('SOMETHING_ERR', ...)
  assert_throws('SomethingError', ...)
  assert_throws({ name: 'TypeError' }, ...)

while there wasn't a single "assert_throws(TypeError(), ...)" anywhere. I left
these as they were.

You are right that it is redundant, of course.

[commit-bot: I haz the power, 2014-06-27 20:30:28]

Change committed as 177125