The default bug view has changed. See this FAQ.

non-relevant nodes not removed for some types of submission

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
11 years ago
8 months ago

People

(Reporter: Allan Beaufour, Assigned: Allan Beaufour)

Tracking

({fixed1.8.0.5, fixed1.8.1})

Trunk
fixed1.8.0.5, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

2.31 KB, application/xhtml+xml
Details
46.86 KB, patch
aaronr
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Updated

11 years ago
Summary: non-revant nodes not removed for some types of submission → non-relevant nodes not removed for some types of submission
(Assignee)

Comment 1

11 years ago
Created attachment 222368 [details]
Testcase
Assignee: aaronr → allan
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Depends on: 278762
(Assignee)

Comment 2

11 years ago
Hmm, this part was actually the big part. Bug 278762 should be fairly easy, after this.
(Assignee)

Comment 3

11 years ago
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 :) )
Attachment #222507 - Flags: review?(doronr)

Comment 4

11 years ago
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.
Attachment #222507 - Flags: review?(doronr) → review+
(Assignee)

Comment 5

11 years ago
Created attachment 222510 [details] [diff] [review]
Patch w/1 new line :)
Attachment #222507 - Attachment is obsolete: true
Attachment #222510 - Flags: review?(aaronr)

Comment 6

11 years ago
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
Attachment #222510 - Flags: review?(aaronr) → review+
(Assignee)

Comment 7

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

Comment 8

11 years ago
Fixed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.5
(Assignee)

Updated

11 years ago
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.