Make several DOMImplementation API arguments mandatory

Make several DOMImplementation API arguments mandatory

Make the arguments of the following DOMImplementation API arguments mandatory:
- createDocumentType()
- createDocument()

This matches the specification:
http://dom.spec.whatwg.org/#interface-domimplementation

The arguments are already mandatory in Firefox and IE10 so the compatibility
risk should be low.

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

[haraken, 2013-08-14 15:05:40]

Given the spec and other browsers' behavior, this change looks good, but one
comment:

https://codereview.chromium.org/22875013/diff/1/LayoutTests/fast/dom/DOMImple...
File
LayoutTests/fast/dom/DOMImplementation/script-tests/createDocumentType-err.js
(left):

https://codereview.chromium.org/22875013/diff/1/LayoutTests/fast/dom/DOMImple...
LayoutTests/fast/dom/DOMImplementation/script-tests/createDocumentType-err.js:95:
// Moz throws a "Not enough arguments" exception in these, we don't:

As far as I see this test and ^^^ this comment, it looks like we have kept the
wrong behavior intentionally. Maybe should we git-blame and ask the patch author
about the intention?

[arv (Not doing code reviews), 2013-08-14 15:08:22]

https://codereview.chromium.org/22875013/diff/1/Source/core/dom/DOMImplementa...
File Source/core/dom/DOMImplementation.idl (right):

https://codereview.chromium.org/22875013/diff/1/Source/core/dom/DOMImplementa...
Source/core/dom/DOMImplementation.idl:27: boolean hasFeature(DOMString feature,
[TreatNullAs=NullString] DOMString version);
These changes make little sense. A nullable param in tail position is ripe for
being optional.

I'll file dom spec bugs

https://codereview.chromium.org/22875013/diff/1/Source/core/dom/DOMImplementa...
Source/core/dom/DOMImplementation.idl:34: [RaisesException] Document
createDocument([TreatNullAs=NullString] DOMString namespaceURI,
I'm concerned about this one. I've seen people call this with less than 3
arguments (in webkit specific code but still).

[do-not-use, 2013-08-14 15:43:01]

https://codereview.chromium.org/22875013/diff/1/Source/core/dom/DOMImplementa...
File Source/core/dom/DOMImplementation.idl (right):

https://codereview.chromium.org/22875013/diff/1/Source/core/dom/DOMImplementa...
Source/core/dom/DOMImplementation.idl:34: [RaisesException] Document
createDocument([TreatNullAs=NullString] DOMString namespaceURI,
On 2013/08/14 15:08:22, arv wrote:
> I'm concerned about this one. I've seen people call this with less than 3
> arguments (in webkit specific code but still).

I agree that the last argument could be optional since it is already nullable.

However, I believe the first argument only makes sense if the second one is
given. Also note that currently, if you omit the second argument
(qualifiedName), it will create a documentElement for the document with the
"undefined" string as tagName. This is unlikely to be what the caller wants IMO.

If we don't want to comply with the spec, I think we should add least add
[TreatUndefinedAs=NullString] for the second argument to avoid this weird
behavior.

[do-not-use, 2013-08-14 15:46:14]

Hopefully, someone else agrees that the following is weird:

doc = document.implementation.createDocument()
-> #document
doc.documentElement
-> <undefined>​</undefined>​
doc.documentElement.namespaceURI
-> "undefined"

[arv (Not doing code reviews), 2013-08-15 14:21:59]

On the dom spec bug, https://www.w3.org/Bugs/Public/show_bug.cgi?id=22959

"--- Comment #8 from Olli Pettay <bugs@pettay.fi> ---
I would not change hasFeature, just because it shouldn't be used at all.
But making the last param of createDocument optional sounds ok to me."

So let's do that.

[do-not-use, 2013-08-16 07:34:37]

On 2013/08/15 14:21:59, arv wrote:
> On the dom spec bug, https://www.w3.org/Bugs/Public/show_bug.cgi?id=22959
> 
> "--- Comment #8 from Olli Pettay <mailto:bugs@pettay.fi> ---
> I would not change hasFeature, just because it shouldn't be used at all.
> But making the last param of createDocument optional sounds ok to me."
> 
> So let's do that.

I updated the CL accordingly, thanks for checking with w3c.

[arv (Not doing code reviews), 2013-08-16 13:33:27]

https://codereview.chromium.org/22875013/diff/17001/LayoutTests/fast/css/zoom...
File LayoutTests/fast/css/zoom-on-unattached.html (right):

https://codereview.chromium.org/22875013/diff/17001/LayoutTests/fast/css/zoom...
LayoutTests/fast/css/zoom-on-unattached.html:6: d =
document.implementation.createDocument("", "");
FYI, (null, "") is a better default

https://codereview.chromium.org/22875013/diff/17001/LayoutTests/fast/dom/HTML...
File LayoutTests/fast/dom/HTMLLinkElement/prefetch-detached.html (right):

https://codereview.chromium.org/22875013/diff/17001/LayoutTests/fast/dom/HTML...
LayoutTests/fast/dom/HTMLLinkElement/prefetch-detached.html:14: var doc =
document.implementation.createDocument('application/html+xml', 'html');
I'm not sure what the crash was but the first argument should be the namespace
and not the mime type.

I don't think it is worth changing

https://codereview.chromium.org/22875013/diff/17001/Source/core/dom/DOMImplem...
File Source/core/dom/DOMImplementation.idl (right):

https://codereview.chromium.org/22875013/diff/17001/Source/core/dom/DOMImplem...
Source/core/dom/DOMImplementation.idl:37: [Default=Undefined] optional
DocumentType doctype);
Why is [Default=Undefined] needed?

[do-not-use, 2013-08-16 13:47:03]

https://codereview.chromium.org/22875013/diff/17001/LayoutTests/fast/css/zoom...
File LayoutTests/fast/css/zoom-on-unattached.html (right):

https://codereview.chromium.org/22875013/diff/17001/LayoutTests/fast/css/zoom...
LayoutTests/fast/css/zoom-on-unattached.html:6: d =
document.implementation.createDocument("", "");
On 2013/08/16 13:33:27, arv wrote:
> FYI, (null, "") is a better default

Right, my bad, I misread the spec.

https://codereview.chromium.org/22875013/diff/17001/Source/core/dom/DOMImplem...
File Source/core/dom/DOMImplementation.idl (right):

https://codereview.chromium.org/22875013/diff/17001/Source/core/dom/DOMImplem...
Source/core/dom/DOMImplementation.idl:37: [Default=Undefined] optional
DocumentType doctype);
On 2013/08/16 13:33:27, arv wrote:
> Why is [Default=Undefined] needed?

It did not compile otherwise because the implementation looks like:
PassRefPtr<Document> createDocument(const String& namespaceURI, const String&
qualifiedName, DocumentType*, ExceptionState&);

There is no such overload:
PassRefPtr<Document> createDocument(const String& namespaceURI, const String&
qualifiedName, ExceptionState&);

[arv (Not doing code reviews), 2013-08-16 13:59:09]

LGTM

I'm just confused about the IDL annotation. It is clear that the behavior is
correct.

https://codereview.chromium.org/22875013/diff/17001/Source/core/dom/DOMImplem...
File Source/core/dom/DOMImplementation.idl (right):

https://codereview.chromium.org/22875013/diff/17001/Source/core/dom/DOMImplem...
Source/core/dom/DOMImplementation.idl:37: [Default=Undefined] optional
DocumentType doctype);
On 2013/08/16 13:47:03, Christophe Dumez wrote:
> On 2013/08/16 13:33:27, arv wrote:
> > Why is [Default=Undefined] needed?
> 
> It did not compile otherwise because the implementation looks like:
> PassRefPtr<Document> createDocument(const String& namespaceURI, const String&
> qualifiedName, DocumentType*, ExceptionState&);
> 
> There is no such overload:
> PassRefPtr<Document> createDocument(const String& namespaceURI, const String&
> qualifiedName, ExceptionState&);

Why not [Default=Null]?

[do-not-use, 2013-08-16 14:02:20]

On 2013/08/16 13:59:09, arv wrote:
> LGTM
> 
> I'm just confused about the IDL annotation. It is clear that the behavior is
> correct.
> 
>
https://codereview.chromium.org/22875013/diff/17001/Source/core/dom/DOMImplem...
> File Source/core/dom/DOMImplementation.idl (right):
> 
>
https://codereview.chromium.org/22875013/diff/17001/Source/core/dom/DOMImplem...
> Source/core/dom/DOMImplementation.idl:37: [Default=Undefined] optional
> DocumentType doctype);
> On 2013/08/16 13:47:03, Christophe Dumez wrote:
> > On 2013/08/16 13:33:27, arv wrote:
> > > Why is [Default=Undefined] needed?
> > 
> > It did not compile otherwise because the implementation looks like:
> > PassRefPtr<Document> createDocument(const String& namespaceURI, const
String&
> > qualifiedName, DocumentType*, ExceptionState&);
> > 
> > There is no such overload:
> > PassRefPtr<Document> createDocument(const String& namespaceURI, const
String&
> > qualifiedName, ExceptionState&);
> 
> Why not [Default=Null]?

There is not [Default=Null], there is only:
- [Default=Undefined]
- [Default=NullString]

according to
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Defaul...

I fixed the arguments for createDocument() in the tests.

[do-not-use, 2013-08-16 14:06:40]

On 2013/08/16 14:02:20, Christophe Dumez wrote:
> On 2013/08/16 13:59:09, arv wrote:
> > LGTM
> > 
> > I'm just confused about the IDL annotation. It is clear that the behavior is
> > correct.
> > 
> >
>
https://codereview.chromium.org/22875013/diff/17001/Source/core/dom/DOMImplem...
> > File Source/core/dom/DOMImplementation.idl (right):
> > 
> >
>
https://codereview.chromium.org/22875013/diff/17001/Source/core/dom/DOMImplem...
> > Source/core/dom/DOMImplementation.idl:37: [Default=Undefined] optional
> > DocumentType doctype);
> > On 2013/08/16 13:47:03, Christophe Dumez wrote:
> > > On 2013/08/16 13:33:27, arv wrote:
> > > > Why is [Default=Undefined] needed?
> > > 
> > > It did not compile otherwise because the implementation looks like:
> > > PassRefPtr<Document> createDocument(const String& namespaceURI, const
> String&
> > > qualifiedName, DocumentType*, ExceptionState&);
> > > 
> > > There is no such overload:
> > > PassRefPtr<Document> createDocument(const String& namespaceURI, const
> String&
> > > qualifiedName, ExceptionState&);
> > 
> > Why not [Default=Null]?
> 
> There is not [Default=Null], there is only:
> - [Default=Undefined]
> - [Default=NullString]
> 
> according to
>
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Defaul...

As per the doc of [Default=Undefined], "A JavaScript undefined is converted to
0, following the value conversion rule in the Web IDL spec", which is why it
works.

[arv (Not doing code reviews), 2013-08-16 14:26:05]

Thanks, that makes some sense.

Still LGTM

On Fri, Aug 16, 2013 at 10:06 AM,  <ch.dumez@sisa.samsung.com> wrote:
> On 2013/08/16 14:02:20, Christophe Dumez wrote:
>>
>> On 2013/08/16 13:59:09, arv wrote:
>> > LGTM
>> >
>> > I'm just confused about the IDL annotation. It is clear that the
>> > behavior is
>> > correct.
>> >
>> >
>
>
>
https://codereview.chromium.org/22875013/diff/17001/Source/core/dom/DOMImplem...
>>
>> > File Source/core/dom/DOMImplementation.idl (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/22875013/diff/17001/Source/core/dom/DOMImplem...
>>
>> > Source/core/dom/DOMImplementation.idl:37: [Default=Undefined] optional
>> > DocumentType doctype);
>> > On 2013/08/16 13:47:03, Christophe Dumez wrote:
>> > > On 2013/08/16 13:33:27, arv wrote:
>> > > > Why is [Default=Undefined] needed?
>> > >
>> > > It did not compile otherwise because the implementation looks like:
>> > > PassRefPtr<Document> createDocument(const String& namespaceURI, const
>> String&
>> > > qualifiedName, DocumentType*, ExceptionState&);
>> > >
>> > > There is no such overload:
>> > > PassRefPtr<Document> createDocument(const String& namespaceURI, const
>> String&
>> > > qualifiedName, ExceptionState&);
>> >
>> > Why not [Default=Null]?
>
>
>> There is not [Default=Null], there is only:
>> - [Default=Undefined]
>> - [Default=NullString]
>
>
>> according to
>
>
>
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Defaul...
>
> As per the doc of [Default=Undefined], "A JavaScript undefined is converted
> to
> 0, following the value conversion rule in the Web IDL spec", which is why it
> works.
>
>
>
> https://codereview.chromium.org/22875013/



-- 
erik

[jochen (gone - plz use gerrit), 2013-08-19 08:22:02]

lgtm

[commit-bot: I haz the power, 2013-08-19 08:37:46]

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

[commit-bot: I haz the power, 2013-08-19 11:02:50]

Change committed as 156303