Bug 71190 – Document.importNode's 'deep' argument should default to true

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Test at <http://w3c-test.org/webapps/DOMCore/tests/submissions/Ms2ger/Document-importNode.html>.

[Olli Pettay (:smaug)]

See also https://bugzilla.mozilla.org/show_bug.cgi?id=698061

[Vineet Chaudhary (vineetc)]

Currently importNode() is defined in Document.idl as,

[OldStyleObjC, ReturnsNew] Node importNode(in [Optional=CallWithDefaultValue] Node importedNode,
                                                   in [Optional=CallWithDefaultValue] boolean deep)
            raises (DOMException);

Which generates code in JSDocument.cpp as,

bool deep(MAYBE_MISSING_PARAMETER(exec, 1, MissingIsUndefined).toBoolean(exec));

So I thought Document.idl is the correct place to fix this issue but,

on debugging it is found that if there is method in idl with boolean argument as "Optional=CallWithDefaultValue",
CodeGeneratorJS.pm creates code with policy "MissingIsUndefined" and for boolean jsUndefined() means "false". 
So for all booleans args with "Optional=CallWithDefaultValue" default value is set to false.

This contradicts with this bug's requirement "Document.importNode's 'deep' argument should default to true".

Should we have policy like "MissingIsTrue" or extended attribute like "CallWithTrueValue"..?

Please guide me If I am looking in wrong direction.

[Kentaro Hara]

(In reply to comment #2)
> Currently importNode() is defined in Document.idl as,
> 
> [OldStyleObjC, ReturnsNew] Node importNode(in [Optional=CallWithDefaultValue] Node importedNode,
>                                                    in [Optional=CallWithDefaultValue] boolean deep)
>             raises (DOMException);
> 
> Which generates code in JSDocument.cpp as,
> 
> bool deep(MAYBE_MISSING_PARAMETER(exec, 1, MissingIsUndefined).toBoolean(exec));
> 
> So I thought Document.idl is the correct place to fix this issue but,
> 
> on debugging it is found that if there is method in idl with boolean argument as "Optional=CallWithDefaultValue",
> CodeGeneratorJS.pm creates code with policy "MissingIsUndefined" and for boolean jsUndefined() means "false". 
> So for all booleans args with "Optional=CallWithDefaultValue" default value is set to false.
> 
> This contradicts with this bug's requirement "Document.importNode's 'deep' argument should default to true".
> 
> Should we have policy like "MissingIsTrue" or extended attribute like "CallWithTrueValue"..?
> 
> Please guide me If I am looking in wrong direction.

According to the spec (http://www.w3.org/TR/DOM-Level-3-Core/core.html#i-Document), the IDL should be

    Node importNode(in Node importedNode, in boolean deep)

This means that both "importedNode" and "deep" must not be optional, in the first place. So I think that the right fix to this issue is just to make "deep" a mandatory argument. Are there any concerns about backward compatibility for doing it?

[Vineet Chaudhary (vineetc)]

(In reply to comment #3)
 
> According to the spec (http://www.w3.org/TR/DOM-Level-3-Core/core.html#i-Document), the IDL should be
> 
>     Node importNode(in Node importedNode, in boolean deep)
> 
> This means that both "importedNode" and "deep" must not be optional, in the first place. So I think that the right fix to this issue is just to make "deep" a mandatory argument. Are there any concerns about backward compatibility for doing it?

I agree that w3c spec doesn't say "importedNode" and "deep" are optional but same time w3c test http://w3c-test.org/webapps/DOMCore/tests/submissions/Ms2ger/Document-importNode.html expects that if "deep" is missing then it should behave like it is true but not false.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #3)
> (In reply to comment #2)
> > Currently importNode() is defined in Document.idl as,
> > 
> > [OldStyleObjC, ReturnsNew] Node importNode(in [Optional=CallWithDefaultValue] Node importedNode,
> >                                                    in [Optional=CallWithDefaultValue] boolean deep)
> >             raises (DOMException);
> > 
> > Which generates code in JSDocument.cpp as,
> > 
> > bool deep(MAYBE_MISSING_PARAMETER(exec, 1, MissingIsUndefined).toBoolean(exec));
> > 
> > So I thought Document.idl is the correct place to fix this issue but,
> > 
> > on debugging it is found that if there is method in idl with boolean argument as "Optional=CallWithDefaultValue",
> > CodeGeneratorJS.pm creates code with policy "MissingIsUndefined" and for boolean jsUndefined() means "false". 
> > So for all booleans args with "Optional=CallWithDefaultValue" default value is set to false.
> > 
> > This contradicts with this bug's requirement "Document.importNode's 'deep' argument should default to true".
> > 
> > Should we have policy like "MissingIsTrue" or extended attribute like "CallWithTrueValue"..?
> > 
> > Please guide me If I am looking in wrong direction.
> 
> According to the spec (http://www.w3.org/TR/DOM-Level-3-Core/core.html#i-Document), the IDL should be
> 
>     Node importNode(in Node importedNode, in boolean deep)
> 
> This means that both "importedNode" and "deep" must not be optional, in the first place. So I think that the right fix to this issue is just to make "deep" a mandatory argument. Are there any concerns about backward compatibility for doing it?

Please look at the current DOM standard (also in $URL):

http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html

[Kentaro Hara]

> Please look at the current DOM standard (also in $URL):
> 
> http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html

Ah, thank you, I got it.

> Should we have policy like "MissingIsTrue" or extended attribute like "CallWithTrueValue"..?

Are there several use cases for "CallWithTrueValue"? If so, I think that introducing "CallWithTrueValue" is reasonable, but otherwise, you should write a custom method for document.importNode(). (Actually I would like to avoid "MissingIsTrue" or "CallWithTrueValue" if possible, since they are specific to a boolean value.)

[Vineet Chaudhary (vineetc)]

(In reply to comment #6)

> Are there several use cases for "CallWithTrueValue"? If so, I think that introducing "CallWithTrueValue" is reasonable, but otherwise, you should write a custom method for document.importNode(). (Actually I would like to avoid "MissingIsTrue" or "CallWithTrueValue" if possible, since they are specific to a boolean value.)

Probably adding "CallWithTrueValue and CallWithFalseValue" will solve the purpose here but, though currently i don't see any use case I am doubtful (sorry for lack of understanding) what if the other methods which have "CallWithDefaultValue" but expecting  some different default values?
eg. unsigned may expect 0 or 1 as defaultValue.

[Kentaro Hara]

(In reply to comment #7)
> (In reply to comment #6)
> I am doubtful (sorry for lack of understanding) what if the other methods 
> which have "CallWithDefaultValue" but expecting  some different default values?
> eg. unsigned may expect 0 or 1 as defaultValue.

I am not sure but I guess that your case is special and there are few existing cases like that. I would recommend to write a custom method (in WebCore/bindings/v8/custom/ and in WebCore/bindings/JS/).

[Vineet Chaudhary (vineetc)]

See also https://bugs.webkit.org/show_bug.cgi?id=71438 which will also require default parameter as true.

[Adam Barth]

As I mentioned on email, you should just use the [Optional] attribute (without any attribute value).  That will let you use C++ dispatch to set whatever default value you like in the DOM header file.

[Vineet Chaudhary (vineetc)]

Created attachment 113729 [details]
proposed patch

Please find the proposed patch as per the above mentioned comments.

1) Modified importNode() "deep" argument Optional
2) Modified importNode() as Custom
3) Implemented WebCore/bindings/JS/

Do I need to add custom function to V8 binding too?

[Adam Barth]

Comment on attachment 113729 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113729&action=review

> Source/WebCore/dom/Document.idl:53
> +        [OldStyleObjC, ReturnsNew, Custom] Node importNode(in [Optional=CallWithDefaultValue] Node importedNode,
> +                                                   in [Optional] boolean deep)

This shouldn't need to be custom.  If there's something missing from the code generator script, please add it.

[WebKit Review Bot]

Comment on attachment 113729 [details]
proposed patch

Attachment 113729 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10337160

[Vineet Chaudhary (vineetc)]

Created attachment 113739 [details]
another patch

(In reply to comment #12)
> > Source/WebCore/dom/Document.idl:53
> > +        [OldStyleObjC, ReturnsNew, Custom] Node importNode(in [Optional=CallWithDefaultValue] Node importedNode,
> > +                                                   in [Optional] boolean deep)
> 
> This shouldn't need to be custom.  If there's something missing from the code generator script, please add it.

Sorry Adam, I guess I couldn't get your point. Did you wanted something like this new patch..?
If no Could you please clarify more.. here I have added new Policy to code generator script (will update changelog and test once we agree).
Thanks.

[Adam Barth]

> Sorry Adam, I guess I couldn't get your point. Did you wanted something like this new patch..?

The IDL files should say [Optional] without any attribute value and there should be no custom bindings for this method.  In you first patch you did the former but not the latter.  In the second patch, you did the former but not the former.

We eventually want to get down to only using [Optional] without any values anywhere.  Adding more values moves us away from that goal.  Also, all the custom code in the bindings is full of bugs.  We don't want to add any more custom code than absolutely necessary.

I'm surprised this doesn't just work out of the box for you, but maybe there's a bug in the code generator script that's causing problems for you.  You can also remove the value of the Optional attribute for importedNode if that's what's causing you trouble.

[Vineet Chaudhary (vineetc)]

Created attachment 114014 [details]
Patch

Modified "deep" argument of importNode() method as [Optional] in idl.
Also added tests to check this behavior.

[Adam Barth]

Comment on attachment 114014 [details]
Patch

Perfect.  Thanks.

[WebKit Review Bot]

Comment on attachment 114014 [details]
Patch

Clearing flags on attachment: 114014

Committed r99612: <http://trac.webkit.org/changeset/99612>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.