Closed Bug 698384 Opened 13 years ago Closed 12 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: smaug, Assigned: mjschranz)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 5 obsolete files)

Assignee: nobody → schranz.m
I simply want to check if the changes made are in the right direction of what is intended.
Attachment #586619 - Flags: review?(bugs)
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?).
Attachment #586619 - Flags: review?(bugs) → review-
(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.
Numeric values default to 0.
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
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
Status: NEW → ASSIGNED
If aOptionalArgc is 1, that means the first argument was given; if it's 2, the first 2 were given, etc.
Attached patch Patch V2 (obsolete) — Splinter Review
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!
Attachment #586619 - Attachment is obsolete: true
Attachment #589075 - Flags: feedback?(bugs)
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!
Attachment #589075 - Flags: feedback?(bugs) → feedback+
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?
Attached patch Patch V3 (obsolete) — Splinter Review
(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.
Attachment #589075 - Attachment is obsolete: true
Attachment #589388 - Flags: feedback?(bugs)
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 on attachment 589388 [details] [diff] [review]
Patch V3

Fix the issues Ms2ger mentioned and add some tests and ask review.
Attachment #589388 - Flags: feedback?(bugs) → feedback+
Attached patch Patch V4 (obsolete) — Splinter Review
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!
Attachment #589388 - Attachment is obsolete: true
Attachment #589996 - Flags: review?(bugs)
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 :)
Attachment #589996 - Flags: review?(bugs) → review-
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.
No; if you pass too many arguments, they are silently ignored.
(And note that aOptionalArgc is capped to the number of IDL-declared arguments.)
Attached patch Patch V5 (obsolete) — Splinter Review
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.
Attachment #589996 - Attachment is obsolete: true
Attachment #590487 - Flags: review?(bugs)
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 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
Attachment #590487 - Flags: review?(bugs) → review+
Attached patch Patch V5Splinter Review
Fixed those 2 silly mistakes. Carrying over the review+ from smaug.
Attachment #590487 - Attachment is obsolete: true
Attachment #591115 - Flags: checkin?(bugs)
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/3807634fedb7
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/3807634fedb7
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
Hardware: x86_64 → All
Summary: Document.createNodeIterator's 2nd and 3rd parameter should be optional → Document.createNodeIterator's 2nd and 3rd parameter should be optional, and remove the 4th (entityReferenceExpansion)
Version: unspecified → Trunk
Attachment #591115 - Flags: checkin?(bugs)
Documentation updated:

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

Listed on Firefox 12 for developers.
Depends on: 838518
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.