263008 - Tracking bug for XForms Action Module

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a4) Gecko/20040927
Build Identifier: 

Tracking bug for XForms Action Module

Reproducible: Always
Steps to Reproduce:

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up to date with CVS

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Single node bindings for load and message

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up to date with CVS, support for single node binding attributes in setvalue

This is now something usable, reviews or comments are welcome.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up to date with CVS

Fixes the empty string ("") bug in setvalue.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up to date with trunk

hmm, it took few hours to remove all nsIContents and nsIAtoms. But here it is.
I left also the changes to nsXFormsModel element, so that it can handle
events properly.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: still minor fixes

Does not contain the fix for nsXFormsModelElement (Bug 264365).
Would this be ready for reviews, so that I could move on to something else ;)

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Or let's say so that it is not yet fully ready, but something that could be 
checked in and then making smaller patches to add missing features.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 162096 [details] [diff] [review]
still minor fixes

Bryner, do you want to sr this or should someone else do it?

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 162096 [details] [diff] [review]
still minor fixes

I hate to say this but... the recent changes to the xforms code are going to
require some significant changes to this patch.  There should be better
opportunity for code reuse.  Could you merge this to the current trunk and I'll
review it? Sorry for the delay.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Yes, I'm just merging the code.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up to date with CVS.

No other changes. 
I will open new bugs at least for xf:message and xf:load elements to implement
new features.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 163559 [details] [diff] [review]
up to date with CVS.

Maybe sr is enough for this, because the earlier patch got already +r.

Comment 13

[Brian Ryner (not reading)]

Comment on attachment 163559 [details] [diff] [review]
up to date with CVS.

--- /dev/null	2004-02-23 23:02:56.000000000 +0200
+++ extensions/xforms/nsXFormsActionElement.h	2004-10-13 20:36:35.000000000
+0300
+  nsDataHashtable<nsISupportsHashKey,PRUint32> mDeferredUpdates;

I don't really understand why a single action element needs to be able to deal
with multiple models.  I mean, in theory, it could be observing events that are
tied to more than one model, but is this a common scenario? (the spec doesn't
take this into account in its description of deferred updates)

>--- /dev/null	2004-02-23 23:02:56.000000000 +0200
>+++ extensions/xforms/nsXFormsActionElement.cpp	2004-10-27 15:16:37.519491864 +0300
>+PR_STATIC_CALLBACK(PLDHashOperator) DoDeferredActions(nsISupports * aModel, 
>+                                                      PRUint32 aDeferred,
>+                                                      void * data)
>+{
>+  if (aModel && aDeferred) {
>+    nsCOMPtr<nsIDOMElement> element = NS_STATIC_CAST(nsIDOMElement *, aModel);
>+    nsCOMPtr<nsIDOMDocument> doc;
>+    element->GetOwnerDocument(getter_AddRefs(doc));
>+    if (doc) {
>+      nsCOMPtr<nsIDOMEventTarget> targetEl(do_QueryInterface(element));
>+      nsCOMPtr<nsIDOMDocumentEvent> docEvent(do_QueryInterface(doc));
>+      if (targetEl) {
>+        PRBool cancelled;
>+        if (aDeferred & DEFERRED_REBUILD) {
>+          nsCOMPtr<nsIDOMEvent> event;
>+          docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
>+          event->InitEvent(NS_LITERAL_STRING("xforms-rebuild"), PR_TRUE, PR_TRUE);
>+          targetEl->DispatchEvent(event, &cancelled);
>+        }
>+        if (aDeferred & DEFERRED_RECALCULATE) {
>+          nsCOMPtr<nsIDOMEvent> event;
>+          docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
>+          event->InitEvent(NS_LITERAL_STRING("xforms-recalculate"), PR_TRUE, PR_TRUE);
>+          targetEl->DispatchEvent(event, &cancelled);
>+        }
>+        if (aDeferred & DEFERRED_REVALIDATE) {
>+          nsCOMPtr<nsIDOMEvent> event;
>+          docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
>+          event->InitEvent(NS_LITERAL_STRING("xforms-revalidate"), PR_TRUE, PR_TRUE);
>+          targetEl->DispatchEvent(event, &cancelled);
>+        }
>+        if (aDeferred & DEFERRED_REFRESH) {
>+          nsCOMPtr<nsIDOMEvent> event;
>+          docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
>+          event->InitEvent(NS_LITERAL_STRING("xforms-refresh"), PR_TRUE, PR_TRUE);
>+          targetEl->DispatchEvent(event, &cancelled);
>+        }
>+      }

I think we can get rid of some redundancy here and save on code size.  What do
you think about doing it this way?

const char *eventName = nsnull;
if (aDeferred & DEFERRED_REBUILD) {
  eventName = "xforms-rebuild";
} else ...
...
}

if (eventName) {
  nsCOMPtr<nsIDOMDocumentEvent> docEvent = do_QueryInterface(doc);
  NS_ASSERTION(docEvent, "document must support DocumentEvent interface!");

  nsCOMPtr<nsIDOMEvent> event;
  docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_Addrefs(event));
  if (event) { // check for out-of-memory
    event->InitEvent(NS_ConvertUTF16toUTF8(eventName), PR_TRUE, PR_TRUE);
   targetEl->DispatchEvent(event, &cancelled);
  }
}

As an additional optimization, you could use the 'data' parameter to pass a
pointer to a stack-allocated struct that contains pre-QI'd pointers to
nsIDOMDocumentEvent and nsIDOMEventTarget.  But this is only worthwhile if it's
likely that there would be multiple events to fire.

>--- /dev/null	2004-02-23 23:02:56.000000000 +0200
>+++ extensions/xforms/nsXFormsActionModuleBase.cpp	2004-10-27 15:16:12.071360568 +0300
>+NS_IMETHODIMP
>+nsXFormsActionModuleBase::OnCreated(nsIXTFGenericElementWrapper *aWrapper)
>+{
>+  nsCOMPtr<nsIDOMElement> node;
>+  aWrapper->GetElementNode(getter_AddRefs(node));
>+  mElement = node;

Why not just

aWrapper->GetElementNode(getter_Addrefs(mElement));  ?

>+nsresult
>+nsXFormsActionModuleBase::GetModelElement(nsIDOMElement** aModelElement)
>+{

Can you just reuse nsXFormsUtils::GetModelAndBind() ?  We could add a flag to
this method that says not to look for a bind, in the case of action elements.

>--- /dev/null	2004-02-23 23:02:56.000000000 +0200
>+++ extensions/xforms/nsXFormsDispatchElement.cpp	2004-10-26 22:08:21.000000000 +0300
>+static const EventData sXFormsEventsEntries[] = {
>+  { "xforms-model-construct",      PR_FALSE, PR_TRUE  },
>+  { "xforms-model-construct-done", PR_FALSE, PR_TRUE  },
>+  { "xforms-ready",                PR_FALSE, PR_TRUE  },

Whoa let's not duplicate this.	We need to share this with
nsXFormsModelElement.

>+static nsDataHashtable<nsStringHashKey,PRUint32> sXFormsEvents;
>+
>+static const EventData sEventDefaultsEntries[] = {

There are many more events that we support.  The spec is somewhat silent on
whether "predefined events" refers only to events defined by XForms or also to
events defined by the host language.  We should figure out exactly where we're
getting the list of events we consider "predefined".

>+static nsDataHashtable<nsStringHashKey,PRUint32> sEventDefaults;

I'd skip the lazy initialization of the hash table and do it at module startup.
 I went back on forth on whether I think we actually need the hash table at all
(as opposed to just a linear search).  This does make the event lookup very
fast (I don't know how to make it any faster) so I guess I'm ok with leaving it
in.

>+
>+static PRBool IsXFormsEvent(const nsAString& aEvent, PRBool& aCancelable, PRBool& aBubbles)
>+{
>+  if (!sXFormsEvents.IsInitialized()) {
>+    if (!sXFormsEvents.Init())
>+      return PR_FALSE;
>+    for (unsigned int i = 0; i < NS_ARRAY_LENGTH(sXFormsEventsEntries); ++i) {
>+      PRUint32 flag = 0;
>+      if (sXFormsEventsEntries[i].canCancel)
>+        flag |= CANCELABLE;
>+      if (sXFormsEventsEntries[i].canBubble)
>+        flag |= BUBBLES;
>+      nsAutoString event;
>+      event.Assign(NS_ConvertUTF8toUTF16(sXFormsEventsEntries[i].name));
>+      sXFormsEvents.Put(event, flag);

You should just be able to do:

sXFormsEvents.Put(NS_ConvertUTF8toUTF16(sXFormsEventsEntries[i].name), flag);

>+static void GetEventDefaults(const nsAString& aEvent, PRBool& aCancelable, PRBool& aBubbles)

My preference would be to merge the two event lists and make "cancellable" and
"bubbles" no-ops for _all_ events that we know about.


>+NS_IMETHODIMP
>+nsXFormsDispatchElement::HandleAction(nsIDOMEvent* aEvent,
>+                                      nsIXFormsActionElement *aParentAction)
>+{
>+  if (!mElement)
>+    return NS_OK;
>+  
>+  PRBool hasName;
>+  PRBool hasTarget;
>+  mElement->HasAttribute(NS_LITERAL_STRING("name"), &hasName);
>+  mElement->HasAttribute(NS_LITERAL_STRING("target"), &hasTarget);
>+  if (!hasName || !hasTarget)
>+    return NS_OK;

Don't bother with the HasAttribute checks.  Just get the attributes and check
if the value is non-empty.

>+  PRBool cancelable = PR_TRUE;
>+  PRBool bubbles = PR_TRUE;
>+  if (!IsXFormsEvent(name, cancelable, bubbles)) {
>+    PRBool hasCancelableAttr;
>+    mElement->HasAttribute(NS_LITERAL_STRING("cancelable"), &hasCancelableAttr);
>+    if (hasCancelableAttr) {
>+      nsAutoString cancelableStr;
>+      mElement->GetAttribute(NS_LITERAL_STRING("cancelable"), cancelableStr);
>+      cancelable = cancelableStr.EqualsLiteral("true");
>+    }

Don't bother with the HasAttribute check here either.

>+
>+    PRBool hasBubblesAttr;
>+    mElement->HasAttribute(NS_LITERAL_STRING("bubbles"), &hasBubblesAttr);

Or here.

>--- /dev/null	2004-02-23 23:02:56.000000000 +0200
>+++ extensions/xforms/nsXFormsSendElement.cpp	2004-10-26 22:09:00.000000000 +0300
>+NS_IMETHODIMP
>+nsXFormsSendElement::HandleAction(nsIDOMEvent* aEvent,
>+                                  nsIXFormsActionElement *aParentAction)
>+{
>+  if (!mElement)
>+    return NS_OK;
>+
>+  PRBool hasSubmission;
>+  mElement->HasAttribute(NS_LITERAL_STRING("submission"), &hasSubmission);
>+  if (!hasSubmission)
>+    return NS_OK;
>+
>+  nsAutoString submission;
>+  mElement->GetAttribute(NS_LITERAL_STRING("submission"), submission);

Same comments as above.

The code required here to dispatch the event seems to be a common pattern, you
should factor that out into a helper method on nsXFormsUtils (maybe figure out
how to integrate nsXFormsModelElement::DispatchEvent into that as well).


>--- /dev/null	2004-02-23 23:02:56.000000000 +0200
>+++ extensions/xforms/nsXFormsSetFocusElement.cpp	2004-10-26 22:09:10.000000000 +0300
>+NS_IMETHODIMP
>+nsXFormsSetFocusElement::HandleAction(nsIDOMEvent* aEvent,
>+                                      nsIXFormsActionElement *aParentAction)
>+{

Same as my previous comments.

>--- /dev/null	2004-02-23 23:02:56.000000000 +0200
>+++ extensions/xforms/nsXFormsLoadElement.cpp	2004-10-26 22:08:04.000000000 +0300
>+NS_IMETHODIMP
>+nsXFormsLoadElement::HandleAction(nsIDOMEvent* aEvent,
>+                                  nsIXFormsActionElement *aParentAction)
>+{
>+  if(!mElement)
>+    return NS_OK;
>+  //If the element has 'resource' and single node binding, the
>+  //action has no effect.
>+  PRBool hasResource;
>+  PRBool hasBind;
>+  PRBool hasModel;
>+  PRBool hasRef;
>+  mElement->HasAttribute(NS_LITERAL_STRING("resource"), &hasResource);
>+  mElement->HasAttribute(NS_LITERAL_STRING("bind"), &hasBind);
>+  mElement->HasAttribute(NS_LITERAL_STRING("model"), &hasModel);
>+  mElement->HasAttribute(NS_LITERAL_STRING("ref"), &hasRef);
>+  
>+  
>+  if (hasResource && ((hasBind) || (hasModel) || (hasRef)))
>+    return NS_OK;

Hm, does "model" imply single node binding by itself?

>+
>+  nsAutoString resource;
>+  if (hasResource)
>+    mElement->GetAttribute(NS_LITERAL_STRING("resource"), resource);

Just get this attribute up-front.

>+
>+  if (!hasResource && !nsXFormsUtils::GetSingleNodeBindingValue(mElement, resource))

Hm, how can this ever do anything useful? if |hasResource| is false, |resource|
will always be empty.


>--- /dev/null	2004-02-23 23:02:56.000000000 +0200
>+++ extensions/xforms/nsXFormsMessageElement.cpp	2004-10-26 22:08:30.000000000 +0300

>+NS_IMETHODIMP
>+nsXFormsMessageElement::HandleAction(nsIDOMEvent* aEvent,
>+                                     nsIXFormsActionElement *aParentAction)
>+{
...
>+  else {
>+    nsAutoString src;
>+    mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
>+    if (!src.IsEmpty()) {

I can't find any reference to 'src'.  Isn't the attribute 'resource' ?

>+      //Don't reuse windows for external messages, at least not for now
>+      level.Assign(NS_LITERAL_STRING(""));

level.Truncate(0);

What you're doing below for the modeless window is kind of scary.  One idea for
how to handle the inline content would be to serialize your children and open
it as a data: url.  Then there's no intermediate about:blank document that you
have to clean up.

Another idea (this would require adding an API to Gecko) is to support opening
a new DOMWindow with a Document passed in instead of a URI.

>--- /dev/null	2004-02-23 23:02:56.000000000 +0200
>+++ extensions/xforms/nsXFormsSetValueElement.cpp	2004-10-26 22:09:24.000000000 +0300
>+NS_IMETHODIMP
>+nsXFormsSetValueElement::HandleAction(nsIDOMEvent* aEvent,
>+                                      nsIXFormsActionElement *aParentAction)
>+{
>+  if (!mElement)
>+    return NS_OK;
>+
>+  nsCOMPtr<nsIDOMElement> dommodel;
>+  GetModelElement(getter_AddRefs(dommodel));
>+  if (!dommodel)
>+    return NS_OK;
>+
>+  PRBool hasValue;
>+  mElement->HasAttribute(NS_LITERAL_STRING("value"), &hasValue);
>+  
>+  nsAutoString value;
>+  if (hasValue) {
>+    mElement->GetAttribute(NS_LITERAL_STRING("value"), value);

Use nsXFormsUtils::EvaluateXPath() for the rest of this evaluation?

>+  else {
>+    nsCOMPtr<nsIDOM3Node> n3(do_QueryInterface(mElement));
>+    n3->GetTextContent(value);
>+  }

The spec doesn't really say but it would be good to get clarification on
whether this is defined to be the entire text content or just the first text
node child.


>+    nsCOMPtr<nsIDOMDocument> doc;
>+    mElement->GetOwnerDocument(getter_AddRefs(doc));
>+    nsCOMPtr<nsIDOMEventTarget> targetEl(do_QueryInterface(dommodel));
>+    nsCOMPtr<nsIDOMDocumentEvent> docEvent(do_QueryInterface(doc));
>+    if (!docEvent || !targetEl)
>+      return NS_OK;
>+    nsCOMPtr<nsIDOMEvent> event;
>+    PRBool cancelled;
>+    docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
>+    event->InitEvent(NS_LITERAL_STRING("xforms-recalculate"), PR_TRUE, PR_TRUE);
>+    targetEl->DispatchEvent(event, &cancelled);
>+
>+    docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
>+    event->InitEvent(NS_LITERAL_STRING("xforms-revalidate"), PR_TRUE, PR_TRUE);
>+    targetEl->DispatchEvent(event, &cancelled);
>+
>+    docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
>+    event->InitEvent(NS_LITERAL_STRING("xforms-refresh"), PR_TRUE, PR_TRUE);
>+    targetEl->DispatchEvent(event, &cancelled);

See earlier comments about reusing event dispatch code.

Other than the things I noted above, the logic seems reasonable... just need to
work on not copying and pasting code so much.  It bloats the code and means
when we find a bug it has to be fixed in N places.

Comment 14

[Leigh L. Klotz, Jr.]

Here is something to consider when you are implementing
nsXFormsMessageElement::HandleAction:  It would be really nice if the message
element properly handled XForms and XHTML markup.  Please don't make it a
special window that can display only text.  

XForms 1.0 requires that xforms:message handle xforms:output but leaves it up to
the the host language to decide what other markup is allowed.  Many XForms
processors have decided that this includes XHTML markup, which can recursively
include XForms markup.  

My belief is that XForms 1.1 will clarify this and simply require xforms:message
to handle all xforms controls, but there is nothing incompatible about going
ahead and doing it now.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

thanks for the sr-, comments very reasonable.
And about xf:message, I'm really trying implement it in that
way that it may contain any markup. The xf:message code in the patch was 
more like a template for the coming 'real' implementation. 

New patch coming hopefully later today, xf:message implementation a bit later.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: simpler patch without xf:message

- moved DispatchEvent to nsXFormsUtils
- Event types handled by nsXFormsUtils, initialized at module startup
  (predefined event types are those which are mentioned in DOM3,
   more can be added later.)
- nsXFormsUtils::GetModelAndBind does not try use "bind" attribute, if the
  **aBindElement parameter is nsnull
- a single action still handles multiple models, I think that must be supported

- removed unnecessary "HasAttribute()"s
- etc.

Comment 17

[Brian Ryner (not reading)]

Comment on attachment 164084 [details] [diff] [review]
simpler patch without xf:message

>--- extensions/xforms/nsXFormsUtils.cpp	29 Oct 2004 19:50:31 -0000	1.2
>+++ extensions/xforms/nsXFormsUtils.cpp	31 Oct 2004 11:30:13 -0000
>+/* static */ PRBool
>+nsXFormsUtils::GetSingleNodeBindingValue(nsIDOMElement* aElement,
>+                                         nsAString& aValue)
>+{
>+  if (!aElement)
>+    return PR_FALSE;
>+  nsCOMPtr<nsIDOMNode> model;
>+  nsCOMPtr<nsIDOMElement> bindElement;
>+  nsCOMPtr<nsIDOMXPathResult> result =
>+    EvaluateNodeBinding(aElement, ELEMENT_WITH_MODEL_ATTR,
>+                        nsIDOMXPathResult::STRING_TYPE, getter_AddRefs(model),
>+                        getter_AddRefs(bindElement));
>+  if (!result)
>+    return PR_FALSE;
>+  result->GetStringValue(aValue);

GetStringValue isn't really correct, because the XPath definition of the
string() function doesn't match up with the XForms definition of reading from
instance data (see XPath section 4.2, and XForms section 8.1.1).  I'm ok with
it for now as a placeholder, but I'll want to change this to use GetNodeValue()
when I have that checked in.  See the first patch on bug 267612.

>+/* static */ PRBool
>+nsXFormsUtils::IsXFormsEvent(const nsAString& aEvent,
>+                             PRBool& aCancelable,
>+                             PRBool& aBubbles)
>+{
>+  PRUint32 flag = 0;
>+  if (!sXFormsEvents.Get(aEvent, &flag))
>+    return PR_FALSE;
>+  aCancelable = (flag & CANCELABLE);
>+  aBubbles = (flag & BUBBLES);

PRBools must be 1 or 0.  This code will set aBubbles to 2 if the bubble flag is
set.

>+/* static */ void
>+nsXFormsUtils::GetEventDefaults(const nsAString& aEvent,
>+                                PRBool& aCancelable,
>+                                PRBool& aBubbles)
>+{
>+  PRUint32 flag = 0;
>+  if (!sEventDefaults.Get(aEvent, &flag))
>+    return;
>+  aCancelable = (flag & CANCELABLE);
>+  aBubbles = (flag & BUBBLES);
>+}

Same.


>--- /dev/null	2004-02-23 23:02:56.000000000 +0200
>+++ extensions/xforms/nsXFormsActionModuleBase.h	2004-10-31 12:54:03.000000000 +0200
>+class nsXFormsActionModuleBase : public nsIDOMEventListener,
>+                                 public nsXFormsStubElement,
>+                                 public nsIXFormsActionModuleElement
>+{
>+public:
>+  nsXFormsActionModuleBase();
>+  virtual ~nsXFormsActionModuleBase();
>+  NS_DECL_ISUPPORTS

Don't use NS_DECL_ISUPPORTS when you inherit from nsXFormsStubElement; you'll
have a duplicate refcount.  Use NS_DECL_ISUPPORTS_INHERITED.

>--- /dev/null	2004-02-23 23:02:56.000000000 +0200
>+++ extensions/xforms/nsXFormsActionModuleBase.cpp	2004-10-31 12:54:17.000000000 +0200
>+NS_IMPL_ISUPPORTS4(nsXFormsActionModuleBase,
>+                   nsIXFormsActionModuleElement,
>+                   nsIDOMEventListener,
>+                   nsIXTFElement,
>+                   nsIXTFGenericElement)
>+

Same here... do this instead:

NS_IMPL_ISUPPORTS_INHERITED2(nsXFormsActionModuleBase,
			     nsXFormsStubElement,
			     nsIXFormsActionModuleElement,
			     nsIDOMEventListener)

>--- /dev/null	2004-02-23 23:02:56.000000000 +0200
>+++ extensions/xforms/nsXFormsLoadElement.cpp	2004-10-31 14:13:44.355042032 +0200
>+NS_IMETHODIMP
>+nsXFormsLoadElement::HandleAction(nsIDOMEvent* aEvent,
>+                                  nsIXFormsActionElement *aParentAction)
>+{
...
>+  nsCOMPtr<nsIDOMDocumentView> dview(do_QueryInterface(doc));
>+  if (!dview)
>+    return NS_OK;
>+
>+  nsCOMPtr<nsIDOMAbstractView> aview;
>+  dview->GetDefaultView(getter_AddRefs(aview));
>+
>+  nsCOMPtr<nsIDOMWindowInternal> internal(do_QueryInterface(aview));
>+  if (!internal)

We should assert if this is null.

>--- /dev/null	2004-02-23 23:02:56.000000000 +0200
>+++ extensions/xforms/nsXFormsSetValueElement.cpp	2004-10-31 14:30:14.518514304 +0200
>+NS_IMETHODIMP
>+nsXFormsSetValueElement::HandleAction(nsIDOMEvent* aEvent,
>+                                      nsIXFormsActionElement *aParentAction)
>+{
...
>+  nsCOMPtr<nsIDOMNode> model;
>+  nsCOMPtr<nsIDOMElement> bindElement;
>+  nsCOMPtr<nsIDOMXPathResult> result =
>+    nsXFormsUtils:: EvaluateNodeBinding(mElement,
>+                                        nsXFormsUtils::ELEMENT_WITH_MODEL_ATTR,
>+                                        nsIDOMXPathResult::FIRST_ORDERED_NODE_TYPE,
>+                                        getter_AddRefs(model),
>+                                        getter_AddRefs(bindElement));
>+
>+  if (!result)
>+    return NS_OK;
>+
>+  nsCOMPtr<nsIDOMNode> singleNode;
>+  result->GetSingleNodeValue(getter_AddRefs(singleNode));
>+

Null check singleNode here.  Also, this should use nsXFormsUtils::SetNodeValue
once that's finished (see above).  I don't want to make you wait for my patch
so this is fine until then.

sr=me with those comments addressed.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up to date with CVS + Comments

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up to date

Comment 20

[Brian Ryner (not reading)]

checked in... not sure if you wanted this to remain open for xf:message so i'll
leave it for now.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Closing this one. I'll open a new bug for xf:message.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 164084 [details] [diff] [review]
simpler patch without xf:message

clearing obsolete review request