Test at <http://w3c-test.org/webapps/DOMCore/tests/submissions/Ms2ger/Document-importNode.html>.
See also https://bugzilla.mozilla.org/show_bug.cgi?id=698061
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.
(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?
(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.
(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
> 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.)
(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.
(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/).
See also https://bugs.webkit.org/show_bug.cgi?id=71438 which will also require default parameter as true.
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.
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 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 on attachment 113729 [details] proposed patch Attachment 113729 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10337160
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.
> 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.
Created attachment 114014 [details] Patch Modified "deep" argument of importNode() method as [Optional] in idl. Also added tests to check this behavior.
Comment on attachment 114014 [details] Patch Perfect. Thanks.
Comment on attachment 114014 [details] Patch Clearing flags on attachment: 114014 Committed r99612: <http://trac.webkit.org/changeset/99612>
All reviewed patches have been landed. Closing bug.