Last Comment Bug 338314 - non-relevant nodes not removed for some types of submission
: non-relevant nodes not removed for some types of submission
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Allan Beaufour
: Stephen Pride
Mentors:
http://www.w3.org/TR/xforms/
Depends on: 278762
Blocks: 263086 326372
  Show dependency treegraph
 
Reported: 2006-05-17 09:24 PDT by Allan Beaufour
Modified: 2006-06-06 07:05 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (2.31 KB, application/xhtml+xml)
2006-05-17 09:27 PDT, Allan Beaufour
no flags Details
Patch (47.00 KB, patch)
2006-05-18 09:11 PDT, Allan Beaufour
doronr: review+
Details | Diff | Review
Patch w/1 new line :) (46.86 KB, patch)
2006-05-18 09:33 PDT, Allan Beaufour
aaronr: review+
Details | Diff | Review

Description Allan Beaufour 2006-05-17 09:24:20 PDT
"A node from the instance data is selected, based on attributes on the submission element. The indicated node and all nodes for which it is an ancestor are considered for the remainder of the submit process. Any node which is considered not relevant as defined in 6.1.4 The relevant Property is removed."
http://www.w3.org/TR/2006/REC-xforms-20060314/slice11.html#submit-event

We currently fail to do that for: get, form-data, and url-encoded submits.
Comment 1 Allan Beaufour 2006-05-17 09:27:00 PDT
Created attachment 222368 [details]
Testcase
Comment 2 Allan Beaufour 2006-05-18 09:09:04 PDT
Hmm, this part was actually the big part. Bug 278762 should be fairly easy, after this.
Comment 3 Allan Beaufour 2006-05-18 09:11:21 PDT
Created attachment 222507 [details] [diff] [review]
Patch

This patch streamlines the submission process some, so we always start by creating a submission XML document, and then serialize that to whatever format is specified.

It also removes some unnecessary includes from the element factory. I got tired of the unneeded recompiles.

(schema validation should be fairly easy after this, but I the schema validator complained when I tried. So one step at a time :) )
Comment 4 Doron Rosenberg (IBM) 2006-05-18 09:28:26 PDT
Comment on attachment 222507 [details] [diff] [review]
Patch

>diff -X patch-excludes -uprN -U8 xforms/nsXFormsSubmissionElement.cpp xforms.submitschema/nsXFormsSubmissionElement.cpp
>--- xforms/nsXFormsSubmissionElement.cpp	2006-05-16 10:17:29.000000000 +0200
>+++ xforms.submitschema/nsXFormsSubmissionElement.cpp	2006-05-18 18:04:26.000000000 +0200
>@@ -590,77 +590,95 @@ nsXFormsSubmissionElement::LoadReplaceAl
> 
>   channel->GetURI(getter_AddRefs(uri));
>   channel->GetContentType(contentType);
>   channel->GetContentCharset(contentCharset);
> 
>   return docshell->LoadStream(mPipeIn, uri, contentType, contentCharset, nsnull);
> }
> 
>+
nit: extra newline

The rest actually looks good.  Mostly code cleanup/movement, looks much better than before.
Comment 5 Allan Beaufour 2006-05-18 09:33:59 PDT
Created attachment 222510 [details] [diff] [review]
Patch w/1 new line :)
Comment 6 aaronr 2006-05-21 21:20:21 PDT
Comment on attachment 222510 [details] [diff] [review]
Patch w/1 new line :)

>diff -X patch-excludes -uprN -U8 xforms/nsXFormsSubmissionElement.cpp xforms.submitschema/nsXFormsSubmissionElement.cpp
>--- xforms/nsXFormsSubmissionElement.cpp	2006-05-16 10:17:29.000000000 +0200
>+++ xforms.submitschema/nsXFormsSubmissionElement.cpp	2006-05-18 18:33:03.000000000 +0200

>@@ -1140,22 +1028,137 @@ nsXFormsSubmissionElement::GetIncludeNSP
>       (*aHash)->Put(p);
>     }
>   }
> 
>   return NS_OK;
> }
> 
> nsresult
>-nsXFormsSubmissionElement::CreateSubmissionDoc(nsIDOMNode *source,
>-                                               const nsString &encoding,
>-                                               SubmissionAttachmentArray *attachments,
>-                                               nsIDOMDocument **result)
>+nsXFormsSubmissionElement::CreateSubmissionDoc(nsIDOMNode      *aRoot,
>+                                               nsIDOMDocument **aReturnDoc)
>+{
>+  NS_ENSURE_ARG_POINTER(aRoot);
>+  NS_ENSURE_ARG_POINTER(aReturnDoc);
>+
>+  nsCOMPtr<nsIDOMDocument> instDoc, submDoc;
>+  aRoot->GetOwnerDocument(getter_AddRefs(instDoc));
>+  nsresult rv;
>+
>+  if (!instDoc) {
>+    // owner doc is null when the aRoot node is the document (e.g., ref="/")
>+    // so we can just get the document via QI.
>+    instDoc = do_QueryInterface(aRoot);
>+    NS_ENSURE_STATE(instDoc);
>+
>+    rv = CreatePurgedDoc(instDoc, getter_AddRefs(submDoc));
>+  } else {
>+    rv = CreatePurgedDoc(aRoot, getter_AddRefs(submDoc));
>+  }
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ENSURE_STATE(submDoc);
>+
>+  // We now need to add namespaces to the submission document.  We get them
>+  // from 3 sources - the main document's documentElement, the model and the
>+  // xforms:instance that contains the submitted instance data node.
>+
>+  nsCOMPtr<nsIDOMNode> instanceNode;
>+  rv = nsXFormsUtils::GetInstanceNodeForData(aRoot,
>+                                             getter_AddRefs(instanceNode));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // add namespaces from the main document to the submission document, but only
>+  // if the instance data is local, not remote.
>+  PRBool serialize = PR_FALSE;
>+  nsCOMPtr<nsIDOMElement> instanceElement(do_QueryInterface(instanceNode));
>+
>+  // make sure that this is a DOMElement.  It won't be if it was lazy
>+  // authored.  Lazy authored instance documents don't inherit namespaces
>+  // from parent nodes or the original document (in formsPlayer and Novell,
>+  // at least).
>+  if (instanceElement) {
>+    PRBool hasSrc = PR_FALSE;
>+    instanceElement->HasAttribute(NS_LITERAL_STRING("src"), &hasSrc);
>+    serialize = !hasSrc;
>+  }
>+
>+  if (serialize) {
>+    // Handle "includenamespaceprefixes" attribute, if present
>+    nsStringHashSet* prefixHash = nsnull;
>+    PRBool hasPrefixAttr = PR_FALSE;
>+    mElement->HasAttribute(kIncludeNamespacePrefixes, &hasPrefixAttr);
>+    if (hasPrefixAttr) {
>+      rv = GetIncludeNSPrefixesAttr(&prefixHash);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+
>+    // get the document element of the document we are going to submit
>+    nsCOMPtr<nsIDOMElement> submDocElm;
>+    submDoc->GetDocumentElement(getter_AddRefs(submDocElm));
>+    NS_ENSURE_STATE(submDocElm);
>+
>+    // if submission document has empty default namespace attribute and if
>+    // @includenamespaceprefixes attribute doesn't contain "#default" value then
>+    // we should remove default namespace attribute (see the specs 11.3).
>+    nsAutoString XMLNSAttrValue;
>+    submDocElm->GetAttributeNS(kXMLNSNameSpaceURI, NS_LITERAL_STRING("xmlns"),
>+                               XMLNSAttrValue);
>+
>+    if (XMLNSAttrValue.IsEmpty() && (!prefixHash ||
>+        !prefixHash->Contains(NS_LITERAL_STRING("#default")))) {
>+      submDocElm->RemoveAttributeNS(kXMLNSNameSpaceURI,
>+                                    NS_LITERAL_STRING("xmlns"));
>+    }
>+
>+    // handle namespaces on the root element of the instance document
>+    nsCOMPtr<nsIDOMElement> instDocElm;
>+    instDoc->GetDocumentElement(getter_AddRefs(instDocElm));
>+    nsCOMPtr<nsIDOMNode> instDocNode(do_QueryInterface(instDocElm));
>+    NS_ENSURE_STATE(instDocNode);
>+    rv = AddNameSpaces(submDocElm, instDocNode, prefixHash);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // handle namespaces on the xforms:instance
>+    rv = AddNameSpaces(submDocElm, instanceNode, prefixHash);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // handle namespaces on the model
>+    nsCOMPtr<nsIModelElementPrivate> model = GetModel();
>+    nsCOMPtr<nsIDOMNode> modelNode(do_QueryInterface(model));
>+    NS_ENSURE_STATE(modelNode);
>+    rv = AddNameSpaces(submDocElm, modelNode, prefixHash);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // handle namespace on main document
>+    nsCOMPtr<nsIDOMDocument> mainDoc;
>+    mElement->GetOwnerDocument(getter_AddRefs(mainDoc));
>+    NS_ENSURE_STATE(mainDoc);
>+
>+    nsCOMPtr<nsIDOMElement> mainDocElm;
>+    mainDoc->GetDocumentElement(getter_AddRefs(mainDocElm));
>+    nsCOMPtr<nsIDOMNode> mainDocNode(do_QueryInterface(mainDocElm));
>+    NS_ENSURE_STATE(mainDocNode);
>+
>+    rv = AddNameSpaces(submDocElm, mainDocNode, prefixHash);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    if (prefixHash)
>+      delete prefixHash;
>+  }

If we really need to delete the prefixHash, then there are about 10 returns above this where we wouldn't delete it (NS_ENSURE_*) and would end up leaking.

>+
>+  NS_ADDREF(*aReturnDoc = submDoc);
>+
>+  return NS_OK;
>+}
>+
>+
>+nsresult
>+nsXFormsSubmissionElement::CreatePurgedDoc(nsIDOMNode *source,
>+                                           nsIDOMDocument **result)
> {
>-  PRBool indent = GetBooleanAttr(NS_LITERAL_STRING("indent"), PR_FALSE);

Looks like you removed our support for 'indent'.  Don't forget to open a new bug to cover this.  Could probably just grab the testcase(s) from the bug we put it in on.

>@@ -1205,231 +1213,200 @@ nsXFormsSubmissionElement::CreateSubmiss
>     sourceDoc->GetDocumentElement(getter_AddRefs(elm));
>     startNode = elm;
>   } else {
>     startNode = source;
>   }
> 
>   nsCOMPtr<nsIModelElementPrivate> model = GetModel();
>   NS_ENSURE_STATE(model);
>-  nsresult rv = CopyChildren(model, startNode, doc, doc, attachments,
>-                             cdataElements, indent, 0);
>+  nsresult rv = CopyChildren(model, startNode, doc, doc, cdataElements, 0);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   NS_ADDREF(*result = doc);
>   return NS_OK;
> }
> 
> nsresult
>-nsXFormsSubmissionElement::CopyChildren(nsIModelElementPrivate* model,
>-                                        nsIDOMNode *source, nsIDOMNode *dest,
>-                                        nsIDOMDocument *destDoc,
>-                                        SubmissionAttachmentArray *attachments,
>-                                        const nsString &cdataElements,
>-                                        PRBool indent, PRUint32 depth)
>+nsXFormsSubmissionElement::CreateAttachments(nsIModelElementPrivate *aModel,
>+                                             nsIDOMNode* aNode,
>+                                             SubmissionAttachmentArray *aAttachments)

nit: I know, lots of places in this file don't, but since this is a new function, might as well do it right from here on out.  Of course, don't bother if they are beyond 80 chars.

Rest looks good.  With these, r=me
Comment 7 Allan Beaufour 2006-05-22 01:38:35 PDT
(In reply to comment #6)
> >+    rv = AddNameSpaces(submDocElm, mainDocNode, prefixHash);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+    if (prefixHash)
> >+      delete prefixHash;
> >+  }
> 
> If we really need to delete the prefixHash, then there are about 10 returns
> above this where we wouldn't delete it (NS_ENSURE_*) and would end up leaking.

(Not my code, I just refactored). But yes, good catch. I'll change it to use an nsAutoPtr instead.
 
> >+nsresult
> >+nsXFormsSubmissionElement::CreatePurgedDoc(nsIDOMNode *source,
> >+                                           nsIDOMDocument **result)
> > {
> >-  PRBool indent = GetBooleanAttr(NS_LITERAL_STRING("indent"), PR_FALSE);
> 
> Looks like you removed our support for 'indent'.  Don't forget to open a new
> bug to cover this.  Could probably just grab the testcase(s) from the bug we
> put it in on.

Heh. Well our "support" for indent _is exactly_ that line :-)

We get the indent attribute and pass it around in various functions, but it is never used for anything. So I just killed it, until somebody actually implements handling for it. It is bug 278761.
 
> > nsresult
> >-nsXFormsSubmissionElement::CopyChildren(nsIModelElementPrivate* model,
> >-                                        nsIDOMNode *source, nsIDOMNode *dest,
> >-                                        nsIDOMDocument *destDoc,
> >-                                        SubmissionAttachmentArray *attachments,
> >-                                        const nsString &cdataElements,
> >-                                        PRBool indent, PRUint32 depth)
> >+nsXFormsSubmissionElement::CreateAttachments(nsIModelElementPrivate *aModel,
> >+                                             nsIDOMNode* aNode,
> >+                                             SubmissionAttachmentArray *aAttachments)
> 
> nit: I know, lots of places in this file don't, but since this is a new
> function, might as well do it right from here on out.  Of course, don't bother
> if they are beyond 80 chars.

You want me to align the parameters? I'll assume that
Comment 8 Allan Beaufour 2006-05-22 01:41:55 PDT
Fixed on trunk

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