698384 - Document.createNodeIterator's 2nd and 3rd parameter should be optional, and remove the 4th (entityReferenceExpansion)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 1

[Matthew Schranz [:mjschranz]]

Attachment: Not intended to landing or anything, just wanted to check if I'm along the right path

I simply want to check if the changes made are in the right direction of what is intended.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 586619 [details] [diff] [review]
Not intended to landing or anything, just wanted to check if I'm along the right path

There is also the feedback flag ;)

> 
> NS_IMETHODIMP
> nsDocument::CreateNodeIterator(nsIDOMNode *aRoot,
>-                               PRUint32 aWhatToShow,
>-                               nsIDOMNodeFilter *aFilter,
>-                               bool aEntityReferenceExpansion,
>-                               nsIDOMNodeIterator **_retval)
>+                               PRUint32 aWhatToShow = nsIDOMNodeFilter::SHOW_ALL,
>+                               nsIDOMNodeFilter *aFilter = nsnull)

How can you remove the _retval? Don't put default values to method implementation (does that even compile?).

Comment 3

[Matthew Schranz [:mjschranz]]

(In reply to Olli Pettay [:smaug] from comment #2)
> Comment on attachment 586619 [details] [diff] [review]
> Not intended to landing or anything, just wanted to check if I'm along the
> right path
> 
> There is also the feedback flag ;)

*Feels pretty stupid right now*
 
> How can you remove the _retval? Don't put default values to method
> implementation (does that even compile?).
_retval was one that I wasn't sure about and figured it shouldn't be removed but figured there can't be much harm in doing so. No it didn't compile but I wanted to get some sort of feedback on what I was doing thusfar. The compiler complained about it not being able to find a matching method with that definition.

So if I can't do the defaulting that way as is described by the spec (which I didn't think I could), how can I know if the optional parmeters were provided or not (I'm sure it might be a dumb question but I honestly don't know)?

I know you mentioned to me before that boolean parameters were often defaulted to false, but what about the cases for the ones we are using? My hunch is aFilter would be null but I don't have the slightest clue for aWhatToShow. I just didn't know how to check/know if they provided said parameters or not.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Numeric values default to 0.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Looks like per spec whatToShow should default to SHOW_ALL, 0xFFFFFFFF, so you need to look at
whether the parameter was passed.
See how addEventListener is implemented.
The interface is http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/events/nsIDOMEventTarget.idl#104

Comment 6

[Matthew Schranz [:mjschranz]]

One thing I didn't think of when on IRC earlier today was the situation where I'd only have 1 of the two optional arguments provided. With [optional_argc] being provided in the IDL, would it be correct to assume that it literally just gives me a count of how many were passed? So in the case of this method having two the possible values could be 0, 1 or 2?

The only thing that has me stumped at this point is figuring out which parameter was the one passed in the event they pass only 1. Since aFilter defaults to null, if they pass null for it but nothing for aWhatToShow I can't just check if it's false.

Otherwise I think I have this one figured out. Simple for most of you I'm sure but oh well :P

Comment 7

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

If aOptionalArgc is 1, that means the first argument was given; if it's 2, the first 2 were given, etc.

Comment 8

[Matthew Schranz [:mjschranz]]

Attachment: Patch V2

As it stands the code changes can't compile but as far as I can tell this is simply because of changes that need to be made to CreateTreeWalker at the same time but I imagine that I should be keeping them separate.

My main worry is how I handled expandEntityReferences. If I understood the comments in 698303 correctly we wanted to keep a getter around for it but to simply return a falsy value.

Let me know though!

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 589075 [details] [diff] [review]
Patch V2

> #include "nsGUIEvent.h"
> #include "nsAsyncDOMEvent.h"
>+#include "nsIDOMNodeFilter.h"
I wonder if this is really needed.
 
>   nsNodeIterator *iterator = new nsNodeIterator(root,
>                                                 aWhatToShow,
>-                                                aFilter,
>-                                                aEntityReferenceExpansion);
>+                                                aFilter);
>   NS_ENSURE_TRUE(iterator, NS_ERROR_OUT_OF_MEMORY);
>-
>+  
No unneeded whitespace changes, please
>--- a/dom/interfaces/core/nsIDOMDocument.idl
>+++ b/dom/interfaces/core/nsIDOMDocument.idl
>@@ -113,21 +113,20 @@ interface nsIDOMDocument : nsIDOMNode
You should update the uuid when changing an interface.

I you need to make CreateTreeWalker changes at the same time, just do it.

And this all needs some tests, which ensure that optional paramater handling is correct.

But starting to look good!

Comment 10

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

Comment on attachment 589075 [details] [diff] [review]
Patch V2

>--- a/content/base/src/nsDocument.cpp
>+++ b/content/base/src/nsDocument.cpp
> nsDocument::CreateNodeIterator(nsIDOMNode *aRoot,
>                                PRUint32 aWhatToShow,
>                                nsIDOMNodeFilter *aFilter,
>-                               bool aEntityReferenceExpansion,
>+                               PRUint8 aOptionalArgc,
>                                nsIDOMNodeIterator **_retval)
> {
>   *_retval = nsnull;
>-
>+  
>+  if(!aOptionalArgc)
>+    aWhatToShow = nsIDOMNodeFilter::SHOW_ALL;

Space between 'if' and '(', and {} around the block.

>--- a/dom/interfaces/traversal/nsIDOMDocumentTraversal.idl
>+++ b/dom/interfaces/traversal/nsIDOMDocumentTraversal.idl

Looks like I forgot to remove this file, could you hg remove it?

Comment 11

[Matthew Schranz [:mjschranz]]

Attachment: Patch V3

(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 589075 [details] [diff] [review]
> Patch V2
> 
> > #include "nsGUIEvent.h"
> > #include "nsAsyncDOMEvent.h"
> >+#include "nsIDOMNodeFilter.h"
> I wonder if this is really needed.

It would seem to be needed. When I don't include it I get a specific compile error related to it.

mozilla-central/content/base/src/nsDocument.cpp:5033:19: error: incomplete type 'nsIDOMNodeFilter' named in nested name specifier
    aWhatToShow = nsIDOMNodeFilter::SHOW_ALL;

 
> >   nsNodeIterator *iterator = new nsNodeIterator(root,
> >                                                 aWhatToShow,
> >-                                                aFilter,
> >-                                                aEntityReferenceExpansion);
> >+                                                aFilter);
> >   NS_ENSURE_TRUE(iterator, NS_ERROR_OUT_OF_MEMORY);
> >-
> >+  
> No unneeded whitespace changes, please

Maybe I'm missing something but where is the unneeded whitespace? Is it between the NS_ENSURE_TRUE and the NS_ADDREF below it?

> >--- a/dom/interfaces/core/nsIDOMDocument.idl
> >+++ b/dom/interfaces/core/nsIDOMDocument.idl
> >@@ -113,21 +113,20 @@ interface nsIDOMDocument : nsIDOMNode
> You should update the uuid when changing an interface.
> 
Done and done.

> I you need to make CreateTreeWalker changes at the same time, just do it.
> 

Mmmk. I didn't at first because I wasn't sure if I should be leaving the changes for each specific one in their appropriate bug.

> And this all needs some tests, which ensure that optional paramater handling
> is correct.
> 

Will do!

Anyway, this patch should cover what you mentioned smaug and Ms2ger. I'll start making a test for these changes.

Comment 12

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

Comment on attachment 589388 [details] [diff] [review]
Patch V3

> nsDocument::CreateNodeIterator(nsIDOMNode *aRoot,
>                                PRUint32 aWhatToShow,
>                                nsIDOMNodeFilter *aFilter,
>-                               bool aEntityReferenceExpansion,
>+                               PRUint8 aOptionalArgc,
>                                nsIDOMNodeIterator **_retval)
> {
>   *_retval = nsnull;
>-
>+  
>+  if (!aOptionalArgc) {
>+    aWhatToShow = nsIDOMNodeFilter::SHOW_ALL;
>+  }
>+    

There are four spaces on this line; there should be none.

>   nsNodeIterator *iterator = new nsNodeIterator(root,
>                                                 aWhatToShow,
>-                                                aFilter,
>-                                                aEntityReferenceExpansion);
>+                                                aFilter);
>   NS_ENSURE_TRUE(iterator, NS_ERROR_OUT_OF_MEMORY);
>-
>+  

And you added two spaces on this line that shouldn't be there.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 589388 [details] [diff] [review]
Patch V3

Fix the issues Ms2ger mentioned and add some tests and ask review.

Comment 14

[Matthew Schranz [:mjschranz]]

Attachment: Patch V4

Patch includes changes, previous whitespace errors, changes to the all of the current uses of the old createNodeIterator definition and a test file checking proper setting of default values.

Let me know of any issues!

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 589996 [details] [diff] [review]
Patch V4


> # Split files arbitrarily in two groups to not run into too-long command lines
> # which break on Windows (see bug 563151)
> _TEST_FILES1 = \
>+		test_bug698384.html \
Nit, usually new tests are added to the end of the list


> 
>   var iterator = document.createNodeIterator(document,
>                                              NodeFilter.SHOW_ALL,
>-                                             null, false);
>+                                             null);
No need for this change

>   iterator = document.createNodeIterator(document, NodeFilter.SHOW_ALL,
>-                                         filter, false);
>+                                         filter);
Nor this

>   function checkBadFilter(method, n) {
>     var iterator =
>       document.createNodeIterator(document, NodeFilter.SHOW_ALL,
>                                   function() {
>                                     if (n < 0)
>                                       iterator.detach();
>                                     return NodeFilter.FILTER_ACCEPT;
>-                                  }, false);
>+                                  });
or this


>       return document.createNodeIterator(grandparent,
>                                          NodeFilter.SHOW_ALL,
>-                                         filter,
>-                                         false);
>+                                         filter);
Or this

>     var iterator = document.createNodeIterator(root, NodeFilter.SHOW_ALL,
>-					       null, false);
>+					       null);
Or this

>     var iterator = document.createNodeIterator(root, NodeFilter.SHOW_ALL,
>-					       null, false);
>+					       null);
Or this

>     var iterator = document.createNodeIterator(root, NodeFilter.SHOW_ALL,
>-					       null, false);
>+					       null);
Or this


>     var iterator = document.createNodeIterator(root, NodeFilter.SHOW_ALL,
>-					       null, false);
>+					       null);
Or this, or other similar cases. We want to test that existing javascript code does still work.

>+++ b/content/base/test/test_bug698384.html
>@@ -0,0 +1,62 @@
>+<!DOCTYPE HTML>
>+<html>
>+  <!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=698384
>+-->
>+  <head>
>+    <title>Test for Bug 698384</title>
>+    <script type="text/javascript"
>+            src="/tests/SimpleTest/SimpleTest.js"></script>
>+    <script src="/tests/SimpleTest/EventUtils.js"
>+            type="text/javascript"></script>
>+    <link rel="stylesheet" type="text/css"
>+          href="/tests/SimpleTest/test.css" />
>+  </head>
>+  <body onload="runTests();">
>+    <a target="_blank"
>+       href="https://bugzilla.mozilla.org/show_bug.cgi?id=698384">
>+      Mozilla Bug 698384</a>
>+    <p id="display"></p>
>+    <div id="content" style="display: none">
>+    </div>
>+    <canvas id="canvas" width="150" height="150"></canvas>
>+    
>+    <pre id="test">
>+      <script type="text/javascript">
>+        /*
>+          Checks to see if default parameter handling is correct when none, 1
>+          or second parameters only are passed.
s/default/optional/
when 0, 1 or 2...

>+        var content = $('content'), 
>+            ni;
>+        content.innerHTML = ('<span id="A"><\/span><span id="B"><\/span>'
>+			    + '<span id="C"><\/span>');
>+			  
>+			  function runTests() {
>+			    ni = document.createNodeIterator(content);
>+			    is(ni.whatToShow, "0xFFFFFFFF", "whatToShow should be " +
>+			      "NodeFilter.SHOW_ALL (Hex value of '0xFFFFFFFF') when both optionals" +
>+			      " are not given");
>+			    is(ni.filter, null, "filter should be defaulted to null when both " +
>+			      " optionals are not given");
>+			   
>+			    // Test NodeIterator when first optional is passed
>+			    ni = document.createNodeIterator(content, NodeFilter.SHOW_ELEMENT);
>+			    is(ni.filter, null, "filter should be defaulted to null when only " +
>+			      " first argument is passed");
>+			    is(ni.whatToShow, "0x00000001", "whatToShow should properly be set to " +
>+			      "NodeFilter.SHOW_ELEMENT (0x00000001) when whatToShow is provided " +
>+			      "and filter is not");
>+			    			    
>+			    SimpleTest.finish();
>+			  }
Please use spaces, not tabs. Indentation looks strange here.


almost there :)

Comment 16

[Matthew Schranz [:mjschranz]]

Maybe I'm missing something here but isn't my code going to break all of those tests? That was the main reason why I went and changed them since there obviously won't be a four argument constructor anymore since expandEntityReferences isn't present anymore.

Comment 17

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

No; if you pass too many arguments, they are silently ignored.

Comment 18

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

(And note that aOptionalArgc is capped to the number of IDL-declared arguments.)

Comment 19

[Matthew Schranz [:mjschranz]]

Attachment: Patch V5

Removed all the previous changes to tests since they weren't needed. I'm hoping the spacing in my own test file should be fine now as I literally went back and repositioned everything with spaces.

I'm just wondering, what should be done with Bug 698385 since the fixes for it are present in this patch. Is there some sort of appropriate status I should change it to?

Anyway, I'm hoping I didn't miss anything here this time.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

It is ok to fix Bug 698385 while fixing this bug too.
When the patch lands, need to just mention in Bug 698385 that it was fixed here.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 590487 [details] [diff] [review]
Patch V5



>+          // Test NodeIterator when no optional arguments are given
>+          ni = document.createNodeIterator(content);
>+          is(ni.whatToShow, "0xFFFFFFFF", "whatToShow should be " +
>+            "NodeFilter.SHOW_ALL (Hex value of '0xFFFFFFFF') when both " +
>+            " optionals are not given");

Could you use NodeFilter,SHOW_ALL and not 0xFFFFFFFF, and it shouldn't be inside ""


>+          is(ni.filter, null, "filter should be defaulted to null when both " +
>+            " optionals are not given");
>+
>+          // Test NodeIterator when first optional is passed
>+          ni = document.createNodeIterator(content, NodeFilter.SHOW_ELEMENT);
>+          is(ni.filter, null, "filter should be defaulted to null when only " +
>+            " first argument is passed");
>+          is(ni.whatToShow, "0x00000001", "whatToShow should properly be " +
>+            " set to NodeFilter.SHOW_ELEMENT (0x00000001) when whatToShow is " +
>+            " provided and filter is not");
Use NodeFilter.SHOW_ELEMENT

Comment 22

[Matthew Schranz [:mjschranz]]

Attachment: Patch V5

Fixed those 2 silly mistakes. Carrying over the review+ from smaug.

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3807634fedb7

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3807634fedb7

Comment 25

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/DOM/Document.createNodeIterator

Listed on Firefox 12 for developers.