338314 - non-relevant nodes not removed for some types of submission

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

"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]

Attachment: Testcase

Comment 2

[Allan Beaufour]

Hmm, this part was actually the big part. Bug 278762 should be fairly easy, after this.

Comment 3

[Allan Beaufour]

Attachment: 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)]

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]

Attachment: Patch w/1 new line :)

Comment 6

[aaronr]

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]

(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]

Fixed on trunk