xforms:submission can't submit partial instance data

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
13 years ago
a year ago

People

(Reporter: Doron Rosenberg (IBM), Assigned: Doron Rosenberg (IBM))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

13 years ago
ref attribute on submission is handled wrongly right now.
(Assignee)

Comment 1

13 years ago
Created attachment 185695 [details] [diff] [review]
patch
Attachment #185695 - Flags: review?(aaronr)

Comment 2

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

Updated

13 years ago
Attachment #185695 - Flags: review?(allan)

Comment 3

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

Comment 4

13 years ago
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?
Attachment #185695 - Flags: review?(allan) → review-
(Assignee)

Comment 5

13 years ago
Created attachment 186107 [details]
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.
(Assignee)

Comment 6

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

Comment 7

13 years ago
Created attachment 186110 [details] [diff] [review]
with allan's comments addressed
Attachment #186110 - Flags: review?(allan)

Comment 8

13 years ago
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
Attachment #186110 - Flags: review?(allan) → review+
(Assignee)

Comment 9

13 years ago
checked in.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.