279552 - handle all xforms-binding-exception cases

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

We are correctly setting xforms-binding-exception sometimes, but missing 3 cases:

1) @model doesn't point to a xforms:model
2) @bind doesn't point to a xforms:bind
3) @submission doesn't point to a xforms:submission

Comment 1

[aaronr]

Attachment: testcase that shows the problem

Run the testcase.  Should produce 3 exceptions.

Comment 2

[Allan Beaufour]

(In reply to comment #1)
> Created an attachment (id=172245) [edit]
> testcase that shows the problem

Not quite. Quoting the spec.:
"It is an exception (4.5.1 The xforms-binding-exception Event) if the XForms
Processor encounters a model IDREF value that refers to an ID not on a model
element, or a bind IDREF value that refers to an ID not on a bind element."
Your testcase refers to non-existing IDs, the spec. talks about refering to
existing IDs. What to do if the IDs are non-existant is up to us I think. 

Nevertheless, I would mean that it makes sense to dispatch the event for
non-existant IDs too.

Comment 3

[Allan Beaufour]

Attachment: Patch for bind and model

Here's a larger than hoped patch for cases 1 and 2. The reason it affect so
many files is that I have changed all nsXFormsUtils-functions that handle the
model tag to use a pointer to nsIModelElementPrivate instead of nsIDOMNode.

Comment 4

[aaronr]

Comment on attachment 172258 [details] [diff] [review]
Patch for bind and model

One comment: in nsXFormsUtils::GetNodeContext() you might want to put the
NS_ENSURE_ARG_POINTER(aBindElement) before we set *aBindElement to nsnull four
lines above it :=)

With that change, r=aaronr

Comment 5

[Allan Beaufour]

(In reply to comment #4)
> (From update of attachment 172258 [details] [diff] [review] [edit])
> One comment: in nsXFormsUtils::GetNodeContext() you might want to put the
> NS_ENSURE_ARG_POINTER(aBindElement) before we set *aBindElement to nsnull four
> lines above it :=)

*doh*! Yessir, it's changed.

Comment 6

[Allan Beaufour]

We still need to figure out what to do when we dispatch such events, because
after an exception we cannot really trust the rest of the form....

Comment 7

[Allan Beaufour]

Attachment: + Aaron's comment

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 172335 [details] [diff] [review]
+ Aaron's comment

>--- xforms/nsXFormsUtils.cpp	2005-01-24 09:56:08.000000000 +0100
>+++ xforms.binding-exception/nsXFormsUtils.cpp	2005-01-25 10:34:22.424555784 +0100
>@@ -183,18 +183,18 @@ nsXFormsUtils::Init()
>       flag |= BUBBLES;
>     sEventDefaults.Put(NS_ConvertUTF8toUTF16(sEventDefaultsEntries[i].name),
>                                              flag);
>   }
>   return NS_OK;
> }
> 
> /* static */ PRBool
>-nsXFormsUtils::GetParentModel(nsIDOMElement *aBindElement,
>-                              nsIDOMNode   **aModel)
>+nsXFormsUtils::GetParentModel(nsIDOMElement           *aBindElement,
>+                              nsIModelElementPrivate **aModel)
> {

I'm not sure about the pros/cons of returning a ModelElementPrivate vs. a
DOMNode.  You're adding some QI's and removing others.	If this comes up in any
performance-sensitive spots (i.e. anything that would come up during document
parsing), it may be worthwhile to reconsider or even return both pointers.

>@@ -211,55 +211,64 @@ nsXFormsUtils::GetParentModel(nsIDOMElem
>     temp.swap(modelWrapper);
>     temp->GetParentNode(getter_AddRefs(modelWrapper));
> 
>     // Model is not the immediate parent, this is a reference to a nested
>     // (invalid) bind
>     res = PR_FALSE;
>   }
>   *aModel = nsnull;
>-  modelWrapper.swap(*aModel);
>+  nsCOMPtr<nsIModelElementPrivate> model = do_QueryInterface(modelWrapper);
>+  if (!model) {
>+    DispatchEvent(aBindElement, eEvent_BindingException);
>+    return NS_ERROR_ABORT;

This returns a PRBool, not a nsresult.	I'm not sure what you intended here...
you should check how this result is used.

Looks ok otherwise.

Comment 9

[Allan Beaufour]

(In reply to comment #8)
> (From update of attachment 172335 [details] [diff] [review] [edit])
> I'm not sure about the pros/cons of returning a ModelElementPrivate vs. a
> DOMNode.  You're adding some QI's and removing others.	If this comes up in any
> performance-sensitive spots (i.e. anything that would come up during document
> parsing), it may be worthwhile to reconsider or even return both pointers.

I had the same thoughts, but except for event dispatching I think
ModelElementPrivate is the interface that we need. The patch bug 278370 saves
the model as a member variable -- using more storage, saving some lookups and QI's.

> This returns a PRBool, not a nsresult. I'm not sure what you intended here...
> you should check how this result is used.

Argh, will do.

Comment 10

[Allan Beaufour]

Checked in the patch.

We still need to handle case 3, should I do a patch for that too?

Comment 11

[aaronr]

Sure, if you have time, fix case 3 as well.  Otherwise I'll try to get to it
tomorrow.

Comment 12

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Attachment: 3) @submission doesn't point to a xforms:submission

Comment 13

[aaronr]

Comment on attachment 172704 [details] [diff] [review]
3) @submission doesn't point to a xforms:submission

Perfect!  Now we'll behave just like Novell and fP.

Comment 14

[Brian Ryner (not reading)]

checked in