Closed Bug 698381 Opened 11 years ago Closed 11 years ago

Node.cloneNode's deep parameter should be optional (default to true)


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

Not set





(Reporter: smaug, Assigned: mjschranz)



(Keywords: dev-doc-complete)


(1 file, 2 obsolete files)

Hardware: x86_64 → All
Once again I'm doing all the other bugs associated with 698303 so I'm grabbing this as well.
Assignee: nobody → schranz.m
Is there a better way of looking for where this has been implemented by each inheriting interface other than manually searching each of the classes using said interfaces? At this point it's getting pretty overwhelming in the sense that I have no idea if where I make a change is actually affecting it.
Though, looks like that dxr search isn't too useful. But there can be better parameters for it.
Attached patch Current Changes (obsolete) — Splinter Review
As it stands this is where I get stuck. I originally made changes in all files except nsGenericDOMDataNode.h and nsGenericElement.h which left me getting compile errors like these:

/Users/matthewschranz/Sites/repos/mozilla-central/content/html/content/src/nsHTMLOptionElement.h:70:3: error:
      too many arguments to function call, expected 2, have 3
../../../dist/include/nsIDOMNode.h:225:135: note: instantiated from:
  ...* *_retval NS_OUTPARAM) { return _to CloneNode(deep, _argc, _retval); } \
/Users/matthewschranz/Sites/repos/mozilla-central/content/base/src/nsGenericElement.h:437:3: note:
      'CloneNode' declared here
  nsresult CloneNode(bool aDeep, nsIDOMNode **aResult)

I wasn't sure how to really track where calls were those calls go to (Probably should try using GDB sometime) and went with making changes in the previous two files.

From here my compile errors are related to calls in the code looking for methods with 2 arguments but only finding the versions with three defined. 

I hate to be a bother but is there something I'm missing here?
Attachment #594180 - Flags: feedback?(bugs)
I feel pretty dumb for not just doing this all in the first place before, but doing Bug 725289 made me see that actually just putting the numerical value for how many arguments were passed was probably the right way to do it.

Was hoping to throw this one up on the try server but for some reason it's giving me fits right now.
Attachment #594180 - Attachment is obsolete: true
Attachment #594180 - Flags: feedback?(bugs)
Attachment #596259 - Flags: review?(bugs)
Comment on attachment 596259 [details] [diff] [review]
Proposed patch making aDeep in Node.cloneNode optional

Please write some tests to check that default handling is correct.
Attachment #596259 - Flags: review?(bugs) → review+
Tests included now. Assuming dev-doc-needed flag should be added so I'll do that.
Attachment #596259 - Attachment is obsolete: true
Attachment #597942 - Flags: review+
Keywords: dev-doc-needed already describes the DOM4 version of this method. It only needs to be updated when this lands, i.e. to mark the first Gecko version in which this is supported.
Both IE and WebKit default to false in my testing, not true.  I just changed the spec to match them:

See WebKit bug:
Summary: Node.cloneNode's deep parameter should be optional (default to true) → Node.cloneNode's deep parameter should be optional (default to false)
Ms2ger and Anne objected to my change, so I reverted it for now.  Implementer feedback requested in the www-dom thread:
Summary: Node.cloneNode's deep parameter should be optional (default to false) → Node.cloneNode's deep parameter should be optional (default to true)
Assuming that the spec is still going to be the same as it was when this bug was filed, I'm going to set it to checkin-needed.
Keywords: checkin-needed
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla13
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 745568
Someone already documented this; I cleaned up a bit and added a mention to Firefox 13 for developers.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.