297097 - xforms:submission can't submit partial instance data

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Doron Rosenberg (IBM)]

ref attribute on submission is handled wrongly right now.

Comment 1

[Doron Rosenberg (IBM)]

Attachment: patch

Comment 2

[aaronr]

Comment on attachment 185695 [details] [diff] [review]
patch


>+  } else {
>+    // if we got a document, we need to create a new
>+    rv = CreateSubmissionDoc(data, encoding, attachments, getter_AddRefs(newDoc));
>+  }

Nit: Please fix the comment, doesn't make sense/incomplete.  Also please point
out in the comment how here data is potentially a subset of the instance
document instead of the whole instance document or use that as an example so
that the reader can more easily see why it might be a document or a node.

With that, r=me

Comment 3

[Allan Beaufour]

A testcase or a better description than "is handled wrongly" would have been nice.

Comment 4

[Allan Beaufour]

Comment on attachment 185695 [details] [diff] [review]
patch

Hmmm, why is it that the submission files do not follow the (argument) code
style of
the rest of the xforms impl.?
- arguments are not aXXX
- arguments are not aligned


> Index: extensions/xforms/nsXFormsSubmissionElement.cpp
> ===================================================================
> -  nsCOMPtr<nsIDOMDocument> doc;
> +  nsCOMPtr<nsIDOMDocument> doc, newDoc;
>    data->GetOwnerDocument(getter_AddRefs(doc));
> -  // owner doc is null when the data node is the document (e.g., ref="/")
> -  if (!doc)
> +
> +  nsresult rv;
> +  if (!doc) {
> +    // owner doc is null when the data node is the document (e.g., ref="/")
> +    // so we can just get the document via QI.
>      doc = do_QueryInterface(data);
> -  NS_ENSURE_TRUE(doc, NS_ERROR_UNEXPECTED);
> +    NS_ENSURE_TRUE(doc, NS_ERROR_UNEXPECTED);
> +
> +    rv = CreateSubmissionDoc(doc, encoding, attachments, getter_AddRefs(newDoc));

split line

> +  } else {
> +    // if we got a document, we need to create a new
> +    rv = CreateSubmissionDoc(data, encoding, attachments, getter_AddRefs(newDoc));

split line

> +  }

But what exactly is the purpose of the above? If there's no owner doc you cast
from nsIDOMNode -> nsIDOMDocument -> nsIDOMNode? And you never use 'doc' later.
Isn't
|rv = CreateSubmissionDoc(data, encoding, attachments,
getter_AddRefs(newDoc));| all there is needed?

>    // recursively walk the source document, copying nodes as appropriate
> +  nsCOMPtr<nsIDOMNode> startNode(source);
> +  // if it is a document, get the root element
> +  if (sourceDoc) {
> +    nsCOMPtr<nsIDOMElement> elm;
> +    sourceDoc->GetDocumentElement(getter_AddRefs(elm));
> +    startNode = elm;
> +  }

How about |else { startNode = source; }| instead, so it's not set twice when
|sourceDoc|?

> -  nsresult rv = CopyChildren(source, doc, doc, attachments, cdataElements, indent, 0);
> +  nsresult rv = CopyChildren(startNode, doc, doc, attachments, cdataElements, indent, 0);

split line

>          // recurse
> -        nsresult rv = CopyChildren(sourceChild, destChild, destDoc, attachments,
> +        nsCOMPtr<nsIDOMNode> startNode(source);
> +        currentNode->GetFirstChild(getter_AddRefs(startNode));

initialized to source?

> Index: extensions/xforms/nsXFormsSubmissionElement.h
> ===================================================================
> -  NS_HIDDEN_(nsresult) CreateSubmissionDoc(nsIDOMDocument *source, const nsString &encoding, SubmissionAttachmentArray *, nsIDOMDocument **result);
> +  NS_HIDDEN_(nsresult) CreateSubmissionDoc(nsIDOMNode *source, const nsString &encoding, SubmissionAttachmentArray *, nsIDOMDocument **result);

Why is encoding not nsAString?

And could please state what the problem you are fixing is, eventually include a
testcase?

Comment 5

[Doron Rosenberg (IBM)]

Attachment: testcase

To test this, save locally and use the patch from bug 296231.

The issue is that ref on submission should make submission submit the partial
instance document pointed to in ref, but that doesn't work right now, we submit
everything.

Comment 6

[Doron Rosenberg (IBM)]

(In reply to comment #4)
> (From update of attachment 185695 [details] [diff] [review] [edit])
> Hmmm, why is it that the submission files do not follow the (argument) code
> style of
> the rest of the xforms impl.?
> - arguments are not aXXX
> - arguments are not aligned
I blame the previous owner :)

> 
> 
> > Index: extensions/xforms/nsXFormsSubmissionElement.cpp
> > ===================================================================
> > -  nsCOMPtr<nsIDOMDocument> doc;
> > +  nsCOMPtr<nsIDOMDocument> doc, newDoc;
> >    data->GetOwnerDocument(getter_AddRefs(doc));
> > -  // owner doc is null when the data node is the document (e.g., ref="/")
> > -  if (!doc)
> > +
> > +  nsresult rv;
> > +  if (!doc) {
> > +    // owner doc is null when the data node is the document (e.g., ref="/")
> > +    // so we can just get the document via QI.
> >      doc = do_QueryInterface(data);
> > -  NS_ENSURE_TRUE(doc, NS_ERROR_UNEXPECTED);
> > +    NS_ENSURE_TRUE(doc, NS_ERROR_UNEXPECTED);
> > +
> > +    rv = CreateSubmissionDoc(doc, encoding, attachments,
getter_AddRefs(newDoc));
> 
> split line
> 
> > +  } else {
> > +    // if we got a document, we need to create a new
> > +    rv = CreateSubmissionDoc(data, encoding, attachments,
getter_AddRefs(newDoc));
> 
> split line
> 
> > +  }
> 
> But what exactly is the purpose of the above? If there's no owner doc you cast
> from nsIDOMNode -> nsIDOMDocument -> nsIDOMNode? And you never use 'doc' later.
> Isn't
> |rv = CreateSubmissionDoc(data, encoding, attachments,
> getter_AddRefs(newDoc));| all there is needed?
> 

I thought so too, but it crashes.  You need the QI.

 
> > Index: extensions/xforms/nsXFormsSubmissionElement.h
> > ===================================================================
> > -  NS_HIDDEN_(nsresult) CreateSubmissionDoc(nsIDOMDocument *source, const
nsString &encoding, SubmissionAttachmentArray *, nsIDOMDocument **result);
> > +  NS_HIDDEN_(nsresult) CreateSubmissionDoc(nsIDOMNode *source, const
nsString &encoding, SubmissionAttachmentArray *, nsIDOMDocument **result);
> 
> Why is encoding not nsAString?

Darin used nsString for all the methods, not sure why.

Comment 7

[Doron Rosenberg (IBM)]

Attachment: with allan's comments addressed

Comment 8

[Allan Beaufour]

Comment on attachment 186110 [details] [diff] [review]
with allan's comments addressed

(In reply to comment #6)
> > But what exactly is the purpose of the above? If there's no owner doc you cast
> > from nsIDOMNode -> nsIDOMDocument -> nsIDOMNode? And you never use 'doc' later.
> > Isn't
> > |rv = CreateSubmissionDoc(data, encoding, attachments,
> > getter_AddRefs(newDoc));| all there is needed?
> > 
> 
> I thought so too, but it crashes.  You need the QI.

Weird. Add a comment about it.

With the comment r=me

Comment 9

[Doron Rosenberg (IBM)]

checked in.