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

RESOLVED FIXED in mozilla13

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: smaug, Assigned: mjschranz)

Tracking

({dev-doc-complete})

unspecified
mozilla13
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#dom-node-clonenode
Hardware: x86_64 → All
(Assignee)

Comment 1

6 years ago
Once again I'm doing all the other bugs associated with 698303 so I'm grabbing this as well.
Assignee: nobody → schranz.m
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
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.
(Reporter)

Comment 3

6 years ago
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
(Reporter)

Comment 4

6 years ago
Though, looks like that dxr search isn't too useful. But there can be better parameters for it.
(Assignee)

Comment 5

6 years ago
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?
Attachment #594180 - Flags: feedback?(bugs)
(Assignee)

Comment 6

6 years ago
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.
Attachment #594180 - Attachment is obsolete: true
Attachment #594180 - Flags: feedback?(bugs)
Attachment #596259 - Flags: review?(bugs)
(Reporter)

Comment 7

6 years ago
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+
(Assignee)

Comment 8

6 years ago
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.
Attachment #596259 - Attachment is obsolete: true
Attachment #597942 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed
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.
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
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: http://lists.w3.org/Archives/Public/www-dom/2012JanMar/0084.html
Summary: Node.cloneNode's deep parameter should be optional (default to false) → Node.cloneNode's deep parameter should be optional (default to true)
(Assignee)

Comment 12

6 years ago
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.
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/7eb9acce263a
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/7eb9acce263a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 730213

Updated

5 years ago
Depends on: 745568
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.