Last Comment Bug 698381 - Node.cloneNode's deep parameter should be optional (default to true)
: Node.cloneNode's deep parameter should be optional (default to true)
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Matthew Schranz [:mjschranz]
:
Mentors:
Depends on: 745568
Blocks: 698303 730213
  Show dependency treegraph
 
Reported: 2011-10-31 04:47 PDT by Olli Pettay [:smaug] (TPAC)
Modified: 2012-04-29 10:24 PDT (History)
9 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Current Changes (7.19 KB, patch)
2012-02-03 07:29 PST, Matthew Schranz [:mjschranz]
no flags Details | Diff | Splinter Review
Proposed patch making aDeep in Node.cloneNode optional (13.99 KB, patch)
2012-02-10 19:22 PST, Matthew Schranz [:mjschranz]
bugs: review+
Details | Diff | Splinter Review
Proposed patch making aDeep in Node.cloneNode optional (16.75 KB, patch)
2012-02-16 12:04 PST, Matthew Schranz [:mjschranz]
schranz.m: review+
Details | Diff | Splinter Review

Comment 1 Matthew Schranz [:mjschranz] 2012-01-12 09:36:50 PST
Once again I'm doing all the other bugs associated with 698303 so I'm grabbing this as well.
Comment 2 Matthew Schranz [:mjschranz] 2012-01-24 12:45:16 PST
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 4 Olli Pettay [:smaug] (TPAC) 2012-01-24 14:29:52 PST
Though, looks like that dxr search isn't too useful. But there can be better parameters for it.
Comment 5 Matthew Schranz [:mjschranz] 2012-02-03 07:29:20 PST
Created attachment 594180 [details] [diff] [review]
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] 2012-02-10 19:22:04 PST
Created attachment 596259 [details] [diff] [review]
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] (TPAC) 2012-02-11 04:51:13 PST
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] 2012-02-16 12:04:41 PST
Created attachment 597942 [details] [diff] [review]
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] 2012-02-16 12:50:18 PST
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) (away until October 25) 2012-02-20 09:08:02 PST
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) (away until October 25) 2012-02-20 09:41:20 PST
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] 2012-02-22 17:39:18 PST
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 14 Richard Newman [:rnewman] 2012-02-23 18:52:09 PST
https://hg.mozilla.org/mozilla-central/rev/7eb9acce263a
Comment 15 Eric Shepherd [:sheppy] 2012-04-29 10:24:15 PDT
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

Note You need to log in before you can comment on or make changes to this bug.