cloneNode() does not work for XTF elements

RESOLVED FIXED

Status

RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: allan, Assigned: alex)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; rv:1.7.3) Gecko/20040913 Firefox/0.10
Build Identifier: 

cloneNode does not seem to work for XTF elements.

Reproducible: Always
Steps to Reproduce:
(Reporter)

Comment 1

14 years ago
Created attachment 163279 [details]
cloneNode test form

Here's an example of cloneNode for various elements, including a non-working
XForms <input> element.
(Reporter)

Updated

14 years ago
Assignee: general → alex
Blocks: 264329
Comment on attachment 163568 [details] [diff] [review]
implement CloneNode for XTF wrapper elements

oops, I should handle also that case when xtf element is an attribute handler.
Attachment #163568 - Flags: superreview?(jst)
Attachment #163568 - Flags: review?(bryner)
(Reporter)

Comment 4

14 years ago
Attribute handler or not, the patch fixes my problem :) Thanks.
Created attachment 163570 [details] [diff] [review]
Copying also the attributes from original attribute handler
Attachment #163568 - Attachment is obsolete: true
Attachment #163570 - Flags: superreview?(jst)
Attachment #163570 - Flags: superreview?(bryner)
Attachment #163570 - Flags: review?(bryner)
Attachment #163570 - Flags: review?(alex)
Comment on attachment 163570 [details] [diff] [review]
Copying also the attributes from original attribute handler

r=me (at some point we'll need to consider whether content node properties
should be copied as well)
Attachment #163570 - Flags: review?(bryner) → review+
Comment on attachment 163570 [details] [diff] [review]
Copying also the attributes from original attribute handler

>Index: content/xtf/src/nsXTFElementWrapper.cpp
>+  nsContentUtils::GetXTFServiceWeakRef()->CreateElement(&it, mNodeInfo);

This addrefs "it".

>+  nsCOMPtr<nsIDOMNode> kungFuDeathGrip = NS_STATIC_CAST(nsIDOMNode*, wrapper);

This addrefs "it" again.

>+  if (NS_SUCCEEDED(rv)) {
>+    kungFuDeathGrip.swap(*aResult);
>+  }

This does NOT release "it".

So this code leaks the new node, since it comes out of here with a refcount of
2, not 1.
Comment on attachment 164012 [details] [diff] [review]
This should not leak.

- In nsXTFElementWrapper::CloneNode():

+  nsXTFElementWrapper* wrapper =
+    NS_STATIC_CAST(nsXTFElementWrapper*, NS_STATIC_CAST(nsIContent*,
it.get()));
+  nsresult rv = CopyInnerTo(wrapper, aDeep);
+
+  if (NS_SUCCEEDED(rv)) {
+    if (mAttributeHandler) {
+      PRUint32 innerCount = 0;
+      mAttributeHandler->GetAttributeCount(&innerCount);
+      for (PRUint32 i = 0; i < innerCount; ++i) {
+	 nsCOMPtr<nsIAtom> attrName;
+	 mAttributeHandler->GetAttributeNameAt(i, getter_AddRefs(attrName));
+	 if (attrName) {
+	   nsAutoString value;
+	   if (NS_SUCCEEDED(mAttributeHandler->GetAttribute(attrName, value)));
+	     it->SetAttr(kNameSpaceID_None, attrName, value, PR_TRUE);
+	 }
+      }
+    }
+    kungFuDeathGrip.swap(*aResult);
+  }

I'd flip that around to do the deep copy after copletely cloning the node
that's passed in, i.e. set attributes first to complete the root node, and then
copy children etc.

sr=jst either way though.
Attachment #164012 - Flags: superreview+
(Reporter)

Updated

14 years ago
Blocks: 269132
(Reporter)

Updated

14 years ago
No longer blocks: 264329
checked in.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
Comment on attachment 163570 [details] [diff] [review]
Copying also the attributes from original attribute handler

clearing obsolete sr request
Attachment #163570 - Flags: superreview?(jst)
You need to log in before you can comment on or make changes to this bug.