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

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#dom-node-clonenode

Comment 1

[Matthew Schranz [:mjschranz]]

Once again I'm doing all the other bugs associated with 698303 so I'm grabbing this as well.

Comment 2

[Matthew Schranz [:mjschranz]]

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.

Comment 3

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

The following mxr queries should give you quite good information
http://mxr.mozilla.org/mozilla-central/search?string=cloneNode&find=\.h%24&findi=\.h%24&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
http://mxr.mozilla.org/mozilla-central/search?string=cloneNode&find=\.c&findi=\.c&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

You may also want to try dxr.
http://dxr.mozilla.org/mozilla/search.cgi?tree=mozilla-central&derived=nsIDOMNode%3A%3ACloneNode

Comment 4

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

Though, looks like that dxr search isn't too useful. But there can be better parameters for it.

Comment 5

[Matthew Schranz [:mjschranz]]

Attachment: Current Changes

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
  NS_FORWARD_NSIDOMNODE(nsGenericHTMLElement::) 
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../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?

Comment 6

[Matthew Schranz [:mjschranz]]

Attachment: Proposed patch making aDeep in Node.cloneNode optional

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.

Comment 7

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

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.

Comment 8

[Matthew Schranz [:mjschranz]]

Attachment: Proposed patch making aDeep in Node.cloneNode optional

Tests included now. Assuming dev-doc-needed flag should be added so I'll do that.

Comment 9

[Marek Stępień [:marcoos, inactive]]

https://developer.mozilla.org/En/DOM/Node.cloneNode 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.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Both IE and WebKit default to false in my testing, not true.  I just changed the spec to match them: http://dvcs.w3.org/hg/domcore/rev/f2d97e6f5cf1

See WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=78887

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Ms2ger and Anne objected to my change, so I reverted it for now.  Implementer feedback requested in the www-dom thread: http://lists.w3.org/Archives/Public/www-dom/2012JanMar/0084.html

Comment 12

[Matthew Schranz [:mjschranz]]

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.

Comment 13

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7eb9acce263a

Comment 14

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/7eb9acce263a

Comment 15

[Eric Shepherd [:sheppy]]

Someone already documented this; I cleaned up a bit and added a mention to Firefox 13 for developers.

https://developer.mozilla.org/en/DOM/Node.cloneNode