Bug 71190 - Document.importNode's 'deep' argument should default to true
Summary: Document.importNode's 'deep' argument should default to true
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://dvcs.w3.org/hg/domcore/raw-fil...
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-30 11:04 PDT by Ms2ger (he/him; ⌚ UTC+1/+2)
Modified: 2011-11-08 13:02 PST (History)
9 users (show)

See Also:


Attachments
proposed patch (5.70 KB, patch)
2011-11-04 16:25 PDT, Vineet Chaudhary (vineetc)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
another patch (4.32 KB, patch)
2011-11-04 17:21 PDT, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
Patch (5.23 KB, patch)
2011-11-08 01:47 PST, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ms2ger (he/him; ⌚ UTC+1/+2) 2011-10-30 11:04:01 PDT
Test at <http://w3c-test.org/webapps/DOMCore/tests/submissions/Ms2ger/Document-importNode.html>.
Comment 1 Olli Pettay (:smaug) 2011-10-30 13:23:02 PDT
See also https://bugzilla.mozilla.org/show_bug.cgi?id=698061
Comment 2 Vineet Chaudhary (vineetc) 2011-11-02 01:11:28 PDT
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.
Comment 3 Kentaro Hara 2011-11-02 01:23:21 PDT
(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?
Comment 4 Vineet Chaudhary (vineetc) 2011-11-02 01:48:15 PDT
(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.
Comment 5 Ms2ger (he/him; ⌚ UTC+1/+2) 2011-11-02 02:18:20 PDT
(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
Comment 6 Kentaro Hara 2011-11-02 08:07:14 PDT
> 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.)
Comment 7 Vineet Chaudhary (vineetc) 2011-11-02 20:26:25 PDT
(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.
Comment 8 Kentaro Hara 2011-11-03 10:27:15 PDT
(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/).
Comment 9 Vineet Chaudhary (vineetc) 2011-11-03 22:38:21 PDT
See also https://bugs.webkit.org/show_bug.cgi?id=71438 which will also require default parameter as true.
Comment 10 Adam Barth 2011-11-04 00:22:57 PDT
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.
Comment 11 Vineet Chaudhary (vineetc) 2011-11-04 16:25:32 PDT
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?
Comment 12 Adam Barth 2011-11-04 16:35:43 PDT
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.
Comment 13 WebKit Review Bot 2011-11-04 17:14:29 PDT
Comment on attachment 113729 [details]
proposed patch

Attachment 113729 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10337160
Comment 14 Vineet Chaudhary (vineetc) 2011-11-04 17:21:41 PDT
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.
Comment 15 Adam Barth 2011-11-04 17:29:33 PDT
> 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.
Comment 16 Vineet Chaudhary (vineetc) 2011-11-08 01:47:43 PST
Created attachment 114014 [details]
Patch

Modified "deep" argument of importNode() method as [Optional] in idl.
Also added tests to check this behavior.
Comment 17 Adam Barth 2011-11-08 11:34:28 PST
Comment on attachment 114014 [details]
Patch

Perfect.  Thanks.
Comment 18 WebKit Review Bot 2011-11-08 13:02:16 PST
Comment on attachment 114014 [details]
Patch

Clearing flags on attachment: 114014

Committed r99612: <http://trac.webkit.org/changeset/99612>
Comment 19 WebKit Review Bot 2011-11-08 13:02:24 PST
All reviewed patches have been landed.  Closing bug.