Last Comment Bug 326452 - Required bind only works with post method
: Required bind only works with post method
Status: RESOLVED FIXED
: fixed1.8.0.4, fixed1.8.1, testcase
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Steve Speicher
: Stephen Pride
Mentors:
Depends on:
Blocks: 332853
  Show dependency treegraph
 
Reported: 2006-02-08 12:18 PST by Charles Brunet
Modified: 2006-04-07 04:42 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.35 KB, application/xhtml+xml)
2006-02-08 12:19 PST, Charles Brunet
no flags Details
test case will all submission types, separated (1.62 KB, application/xml)
2006-02-22 08:27 PST, Steve Speicher
no flags Details
proposed patch to nsXFormsSubmitElement.* (4.12 KB, patch)
2006-03-07 10:22 PST, Steve Speicher
allan: review-
Details | Diff | Review
patch take 2 (5.37 KB, patch)
2006-03-08 12:45 PST, Steve Speicher
allan: review+
Details | Diff | Review
updated with Allan's review comments (6.20 KB, patch)
2006-03-13 05:29 PST, Steve Speicher
doronr: review+
allan: review-
Details | Diff | Review
fixed: return value, indents, eol (5.38 KB, patch)
2006-03-14 05:28 PST, Steve Speicher
allan: review+
Details | Diff | Review

Description Charles Brunet 2006-02-08 12:18:24 PST
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; fr; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; fr; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

If the submission method is post, the form isn't sent until all required fields are filled.
If the submission method is get or urlencoded-post, the form is sent even if a required field is empty.

Reproducible: Always

Steps to Reproduce:
Comment 1 Charles Brunet 2006-02-08 12:19:11 PST
Created attachment 211170 [details]
testcase
Comment 2 Allan Beaufour 2006-02-10 03:11:31 PST
Unless I'm sleeping or something this is quite amazing :) Because it seems to be correct. Amazing amazing.

NB. It does also work for put and multipart-post. Not that it makes it so much better though...
Comment 3 aaronr 2006-02-10 12:04:55 PST
sounds like a good starter bug.  Assigning to Steve.
Comment 4 Steve Speicher 2006-02-22 08:27:00 PST
Created attachment 212757 [details]
test case will all submission types, separated

Oddly post,multipart,put work and get,url-post,form-data don't
Comment 5 Steve Speicher 2006-03-07 07:36:05 PST
After some debugging, here's the issue:

Since post,multipart,put cause nsXFormsSubmissionElement::SerializeDataXML() to be invoked, which indirectly invokes nsXFormsModelElement::HandleInstanceDataNode() (which actually does the validity, relevant and requiredness checking).

HandleInstanceDataNode() is not invoked with the other submission method types.

I'll investigate a way to iterate over the instance nodes for the other method types.
Comment 6 Steve Speicher 2006-03-07 10:22:46 PST
Created attachment 214330 [details] [diff] [review]
proposed patch to nsXFormsSubmitElement.*

Notes to reviewers:
- Should the arg to CanSubmit() be const?
- Ok to 'borrow' code form CopyChildren(), or is there a better tree traversal approach?
- Should the test be moved to SerializeData* methods? I liked it in in Submit() as it prevents further processing and can be used for other tests (like cross domain?)
Comment 7 Allan Beaufour 2006-03-08 04:20:26 PST
Comment on attachment 214330 [details] [diff] [review]
proposed patch to nsXFormsSubmitElement.*

Your comments are too long, and maybe use a slightly short variable name. Check:
http://beaufour.dk/jst-review/?patch=214330

(In reply to comment #6)
> Notes to reviewers:
> - Should the arg to CanSubmit() be const?

People (incl. me) are very bad at using const where it should be used. It would be nice if it was, yes.

> - Ok to 'borrow' code form CopyChildren(), or is there a better tree traversal
> approach?

It's ok to keep the style of the code you are changing. I'm not sure we are heading the right path though... more about that.

> - Should the test be moved to SerializeData* methods? I liked it in in Submit()
> as it prevents further processing and can be used for other tests (like cross
> domain?)

It should be moved. You end up doing the same work twice for XML submissions.

>+/*
>+ * This walks the DOM tree, starting at topNode and invokes model->HandleInstanceDataNode()
>+ * that ensures that the instance is valid, required conditions met...if so, then can submit.
>+ */

Good style with a comment! But, please move it to the header file, and use "/**", and comment the argument, just like EndSubmit() is.

>+nsresult
>+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *topNode)

Arguments should start with an "a", so:
nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode)

>+{
>+  nsCOMPtr<nsIDOMNode> currentNode(topNode), node;
>+  nsCOMPtr<nsIModelElementPrivate> model = GetModel();

You need to make sure that you actually got a model here. It could be null
NS_ENSURE_STATE(model);

>+
>+  while (currentNode)
>+  {

(I can see that this file has it's own "style", compared to the rest of XForms code.... hmmm, but:)
please move the "{" up on the prev. line.

>+     PRUint16 handleNodeResult;
>+     model->HandleInstanceDataNode(currentNode, &handleNodeResult);
>+
>+    /*
>+     *  SUBMIT_SERIALIZE_NODE   - node is to be serialized
>+     *  SUBMIT_SKIP_NODE        - node is not to be serialized
>+     *  SUBMIT_ABORT_SUBMISSION - abort submission (invalid node or empty required node)
>+     */
>+    if (handleNodeResult == nsIModelElementPrivate::SUBMIT_SKIP_NODE) {
>+       // skip node and subtree
>+       currentNode->GetNextSibling(getter_AddRefs(node));
>+       currentNode.swap(node);
>+       continue;
>+    } else if (handleNodeResult == nsIModelElementPrivate::SUBMIT_ABORT_SUBMISSION) {
>+      // abort
>+      return NS_ERROR_ILLEGAL_VALUE;

Since you are aborting, please use NS_ERROR_ABORT.

>+    }

If you want to be really nice :), you can do a:
NS_ASSERTION(handleNodeResult == SUBMIT_SERIALIZE_NODE)
here, just in case.

>+    // recurse
>+    nsCOMPtr<nsIDOMNode> startNode;

I know this is copy'n'paste, but please call it "firstChild", since that is what it is.

>Index: nsXFormsSubmissionElement.h
>===================================================================
>+  nsresult CanSubmit(nsIDOMNode *source);

Here you call the argument something else than in the cpp file.

Something is wrong in the submission handling. I think we need a common "CreateInstancePurgedForNonRelevantNodes" function, that is called for all serialization possibilities and that will also check for required nodes. We need that to do a full schema-check before submission. BUT, that is a different bug.
Comment 8 Steve Speicher 2006-03-08 12:45:59 PST
Created attachment 214460 [details] [diff] [review]
patch take 2

Can't find a good solution to const args (at least in this case), as I get into all sorts of nsCOMPtr contructor type conflicts (they want non-const *).  So, not fixing that.

Regarding the comment:
  we need a common "CreateInstancePurgedForNonRelevantNodes" function
I assume that wasn't directed at this patch but towards the forward referenced full schema check bug.
Comment 9 Allan Beaufour 2006-03-13 01:29:37 PST
Comment on attachment 214460 [details] [diff] [review]
patch take 2

>Index: nsXFormsSubmissionElement.cpp
>===================================================================
>-  // XXX call nsISchemaValidator::validate on each node
>-
>-
>+  // XXX This is handled by the SerializeData() method
>+  

No need for XXX here, if there's no bug.

>+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode)
>+    } else if (handleNodeResult ==
>+        nsIModelElementPrivate::SUBMIT_ABORT_SUBMISSION) {
>+      // abort
>+      return NS_ERROR_ILLEGAL_VALUE;

Ahem. I remember saying something about this ;-)

>Index: nsXFormsSubmissionElement.h
>===================================================================
>   /**
>+   * This walks the DOM tree, starting at aTopNode and invokes
>+   * model->HandleInstanceDataNode()that ensures that the instance is valid,
>+   * required conditions met...if so, then can submit.
>+   *
>+   * @param aTopNode Root instance node of tree to be evaluated
>+   * @return NS_ERROR_ABORT if instance is not valid and empty required nodes,
>+   * otherwise return NS_OK.
>+   *
>+   */
>+  nsresult CanSubmit(nsIDOMNode *aTopNode);

Please indent the comment properly, that is, something like this:
>+   * @param        aTopNode Root instance node of tree to be evaluated
>+   * @return       NS_ERROR_ABORT if instance is not valid and empty required
>+   *                nodes, otherwise return NS_OK.

With the above changes, r=me
Comment 10 Steve Speicher 2006-03-13 05:29:43 PST
Created attachment 214896 [details] [diff] [review]
updated with Allan's review comments
Comment 11 Doron Rosenberg (IBM) 2006-03-13 08:14:30 PST
Comment on attachment 214896 [details] [diff] [review]
updated with Allan's review comments

Whoever checks this in, nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode) has some 4 space indentation (or tab chars perhaps) that should be 2 spaces only.
Comment 12 Allan Beaufour 2006-03-14 00:55:43 PST
(In reply to comment #11)
> (From update of attachment 214896 [details] [diff] [review] [edit])
> Whoever checks this in, nsXFormsSubmissionElement::CanSubmit(nsIDOMNode
> *aTopNode) has some 4 space indentation (or tab chars perhaps) that should be 2
> spaces only.

Not only that, it also has Windows line-endings. Steve: Please convert file to Unix line-endings before submitting.
Comment 13 Allan Beaufour 2006-03-14 01:04:15 PST
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 214896 [details] [diff] [review] [edit] [edit])
> > Whoever checks this in, nsXFormsSubmissionElement::CanSubmit(nsIDOMNode
> > *aTopNode) has some 4 space indentation (or tab chars perhaps) that should be 2
> > spaces only.
> 
> Not only that, it also has Windows line-endings. Steve: Please convert file to
> Unix line-endings before submitting.

I also changes the return value in CopyChildren() and not in CanSubmit()...
Comment 14 Allan Beaufour 2006-03-14 01:05:02 PST
Comment on attachment 214896 [details] [diff] [review]
updated with Allan's review comments

Please fix:
* return value in correct function
* indentation
* unix line endings
Comment 15 Steve Speicher 2006-03-14 05:28:00 PST
Created attachment 215005 [details] [diff] [review]
fixed: return value, indents, eol
Comment 16 Allan Beaufour 2006-03-15 04:25:09 PST
Comment on attachment 215005 [details] [diff] [review]
fixed: return value, indents, eol

*sigh* still, Windows line-endings...

Whoever checks this in, be SURE to change this.
Comment 17 Allan Beaufour 2006-03-15 09:55:05 PST
(In reply to comment #16)
> (From update of attachment 215005 [details] [diff] [review] [edit])
> *sigh* still, Windows line-endings...
> 
> Whoever checks this in, be SURE to change this. 

jst-review v2 now warns about this too...
Comment 18 aaronr 2006-03-22 11:28:53 PST
fixed line endings.  Checked into trunk for sspeiche.
Comment 19 Allan Beaufour 2006-03-28 00:39:11 PST
Comment on attachment 215005 [details] [diff] [review]
fixed: return value, indents, eol

>+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode)
>+{
>+  nsCOMPtr<nsIDOMNode> currentNode(aTopNode), node;
>+  nsCOMPtr<nsIModelElementPrivate> model = GetModel();
>+  NS_ENSURE_STATE(model);

Ouch. I just saw this. The recursive CanSubmit() function calls GetModel() on each call. Very expensive. We need to change this.

I'm suspecting something else is wrong too...
Comment 20 Allan Beaufour 2006-03-28 03:19:38 PST
(In reply to comment #19)
> (From update of attachment 215005 [details] [diff] [review] [edit])
> >+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode)
> >+{
> >+  nsCOMPtr<nsIDOMNode> currentNode(aTopNode), node;
> >+  nsCOMPtr<nsIModelElementPrivate> model = GetModel();
> >+  NS_ENSURE_STATE(model);
> 
> Ouch. I just saw this. The recursive CanSubmit() function calls GetModel() on
> each call. Very expensive. We need to change this.

CopyChildren does the same... :(

> I'm suspecting something else is wrong too...

Still trying.
Comment 21 aaronr 2006-03-28 09:39:33 PST
(In reply to comment #19)
> (From update of attachment 215005 [details] [diff] [review] [edit])
> >+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode)
> >+{
> >+  nsCOMPtr<nsIDOMNode> currentNode(aTopNode), node;
> >+  nsCOMPtr<nsIModelElementPrivate> model = GetModel();
> >+  NS_ENSURE_STATE(model);
> 
> Ouch. I just saw this. The recursive CanSubmit() function calls GetModel() on
> each call. Very expensive. We need to change this.
> 
> I'm suspecting something else is wrong too...
> 

Not THAT expensive.  Gets the parent node and QI's it.  But you are right.  No reason we can't pass model through as a parameter since it is recursive.
Comment 22 Allan Beaufour 2006-03-28 11:06:54 PST
(In reply to comment #21)
> Not THAT expensive.  Gets the parent node and QI's it.  But you are right.  No
> reason we can't pass model through as a parameter since it is recursive.

Well, it's unnecessary and being done for every tree level in an instance document... that's expensive in my world.

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