312956 - Expose XForms node state directly on instance data nodes

Status: RESOLVED FIXED

(Core Graveyard :: XForms, enhancement)

Description

[alexander :surkov (:asurkov)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

This proposal is from bug 307093. The mentioned bug is about model instances
enumerating. I think we should leave it as it is. Here I want to discuss
interface for managing instance data. 

I can think of two ways to modify data:
1) by modifying instance document (DOM methods use) and updating model.
2) by using xforms controls (nsIXFormsDelegate method)

I can think of two ways for reading MIPs
1) by reading xforms:bind element attributes (I get result as xpath expression)
2) by using nsIXFormsDelegate interface (I get result as boolean).

I can see only one way to change MIPs wich is to change xforms:bind element
attributes and then to update model.

Thus I can only count two approaches total: use DOM methods, use the
nsIXFormsDelegate interface.

Sometimes I want to do it without having to use xforms:controls. Then I should
work with DOM. I guess it is not very comfortable. Threafore I want to have
special interface to do it.

Reproducible: Always

Comment 1

[Allan Beaufour]

My understanding of this, is the possibility to do:
node = model.getInstanceDocument('i1').getNode("/data/x/y/z")
and then node would expose nsIXFormsDelegate-like properties like
node.isReadonly(), node.isRelevant, and node.set/.getValue().

I think that it could be a useful feature, but it needs some research.

Alexander also wants some way of _modifying_ the MIPs, as discussed on bug
307093. And I haven't changed my mind on that since
https://bugzilla.mozilla.org/show_bug.cgi?id=307093#c26.
So for modifying MIPs through another interface than through <bind/> elements,
please create another bug.

Comment 2

[Allan Beaufour]

I'm thinking that with bug 306764, we could do an nsXFormsAccessors with no
delegate, which takes an XPath expression on construction. So we would have
interface nsIXFormsModelElement2 : nsISupports {
  nsIXFormsAccessors getAccessorsForNode(in DOMString xpathExpr);
}
where |xpathExpr| would be evaluated like a @ref (so only single node). F.x:
acc = model.getAccessor("instance('inst')/x/y[@b = 1]/z")
and you could then do acc.get/setValue(), and ask on acc.isReadonly() etc.

It would need to do one of these:
1) be staticly bound to the same node
2) re-evaluate the expression on each access
3) be bound to the model like a delegate, so it knows when to "bind()" to a new
node.

I'm mostly for 3), but it might need some adjustments to our model framework.

Just some thoughts...

Comment 3

[alexander :surkov (:asurkov)]

I'd like to get accessors by xpath expression as Allan said and by dom node too. Something like this:

interface nsIXFormsModelElement2 : nsISupports {
  nsIXFormsAccessors getAccessorsForNode(in DOMString xpathExpr);
  nsIXFormsAccessors getAccessorsForNode2(in DOMNode node);
}

Getting accessors by dom node is useful when you traverse instance document. Getting accessors by xpath expressiont is useful when you work in global environment.
Thus I don't know is there a way to realize it? Probably MDG can?

Comment 4

[Allan Beaufour]

(In reply to comment #3)
> I'd like to get accessors by xpath expression as Allan said and by dom node
> too. Something like this:
> 
> interface nsIXFormsModelElement2 : nsISupports {
>   nsIXFormsAccessors getAccessorsForNode(in DOMString xpathExpr);
>   nsIXFormsAccessors getAccessorsForNode2(in DOMNode node);
> }

Good point, let's have both. How about:
getAccessorsForRef(in DOMString aXPathExpr);
getAccessorsForNode(in DOMNode node);

> Getting accessors by dom node is useful when you traverse instance document.
> Getting accessors by xpath expressiont is useful when you work in global
> environment.
> Thus I don't know is there a way to realize it? Probably MDG can?

There should be no problem in supporting both of these, and as I wrote I think getAccessorsForRef() should be dynamic, that is proposal 3 in comment 2.

At least, I need it for my XForms Buddy :-D

Comment 5

[aaronr]

(In reply to comment #4)
> (In reply to comment #3)

> Good point, let's have both. How about:
> getAccessorsForRef(in DOMString aXPathExpr);
> getAccessorsForNode(in DOMNode node);
> 

yeah, this would be useful to have.  But I'm not big on the getAccessorsForRef as the method name.  Since we don't know that the user would necessarily be pulling the xpath expression from a ref.  Can't we call them both getAccessorsForNode?  The parameter signatures are different.  Otherwise maybe getAccessorsByNode and getAccessorsByExpr?  I know that it is nitpicky....

Comment 6

[Allan Beaufour]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> 
> > Good point, let's have both. How about:
> > getAccessorsForRef(in DOMString aXPathExpr);
> > getAccessorsForNode(in DOMNode node);
> > 
> 
> yeah, this would be useful to have.  But I'm not big on the getAccessorsForRef
> as the method name.  Since we don't know that the user would necessarily be
> pulling the xpath expression from a ref.  Can't we call them both
> getAccessorsForNode?

Hmmm, I'd rather have two different names. That's less confusing.

>  The parameter signatures are different.  Otherwise maybe
> getAccessorsByNode and getAccessorsByExpr?  I know that it is nitpicky....

I'm mr. nitpicker, so no worries :) Well, I'll live with both versions, but my point in choosing "Ref" was that it acts as @ref, in that it is a "live" reference: rebound if the instance data changes, MIP states change, etc. It should not refer to a control, but have an xpath expression, just as a standard control.

Comment 7

[alexander :surkov (:asurkov)]

I have some thoughts about nsXFormsAccessors with no delegate. The first is nsXFormsAccessors should be intialized by model and instnace node. The second isReadonly() and etc methods realization should ask for new method getState() from a model. And I think bug 324096 can be helpful for this.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: rough patch [accessors without delegate]

Comment 9

[alexander :surkov (:asurkov)]

There is a question. When nsXFormsAccessors is created in dynamical memory via operator new and it saved in nsRefPtr<nsXFormsAccessors> then memory will be free after nsRefPtr<nsXFormsAccessors> destroying?

Comment 10

[Allan Beaufour]

(In reply to comment #8)
> Created an attachment (id=215383) [edit]
> rough patch [accessors without delegate]

I've just skimmed this. But
1) Accessors is getting a bit heavy I think (saving too much stuff)
2) How will this fit with comment 4?
3) If the bound node changes for a control, you will still return the states for the old node will you not?

Comment 11

[Allan Beaufour]

Comment on attachment 215383 [details] [diff] [review]
rough patch [accessors without delegate]

r- until I get answers to comment 10 ... got tired of  looking at it in my request queue :)

Comment 12

[alexander :surkov (:asurkov)]

Attachment: testcase

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch

(In reply to comment #10)
> 
> I've just skimmed this. But
> 1) Accessors is getting a bit heavy I think (saving too much stuff)

Fixed.

> 2) How will this fit with comment 4?

Model has methods for accessors getting by node and by xpath expression.

> 3) If the bound node changes for a control, you will still return the states
> for the old node will you not?
> 
Fixed.

Comment 14

[Allan Beaufour]

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

A couple of bad things:
* getter functions not returning addref'ed pointers (and some doing)
* Use of plain pointers. For example: If somebody creates an accessor via a model, and the model is destroyed, the accessor will end up with a dangling pointer.

Catch me on IRC

Comment 15

[alexander :surkov (:asurkov)]

Attachment: patch2

(In reply to comment #14)
> (From update of attachment 220224 [details] [diff] [review] [edit])
> A couple of bad things:
> * getter functions not returning addref'ed pointers (and some doing)
> * Use of plain pointers. For example: If somebody creates an accessor via a
> model, and the model is destroyed, the accessor will end up with a dangling
> pointer.
> 
They should be fixed.

Comment 16

[Allan Beaufour]

Comment on attachment 220900 [details] [diff] [review]
patch2

An "insane" number of nits. It takes a couple of iterations to get through the beaufour-nitpicking-extravaganza... :)

> diff -uNrap mozilla.orig/extensions/xforms/nsIModelElementPrivate.idl mozilla/extensions/xforms/nsIModelElementPrivate.idl
> --- mozilla.orig/extensions/xforms/nsIModelElementPrivate.idl	2006-03-30 14:45:56.000000000 +0900
> +++ mozilla/extensions/xforms/nsIModelElementPrivate.idl	2006-05-05 17:28:55.000000000 +0900
> @@ -133,6 +133,12 @@ interface nsIModelElementPrivate : nsIXF
>    void setStates(in nsIXFormsControl aControl, in nsIDOMNode aBoundNode);
 
>    /**
> +    Return states for bound node in terms of NS_EVENT_STATE constants
> +    @param aBoundNode         The instance node
> +   */
> +  long getStates(in nsIDOMNode aBoundNode);

Use the same comment styling as the rest of the file, that is use "*" in front of each line.

> +
> +  /**
>     * Add an instance element to the model's instance list
>     * @param aInstanceElement        The instance element to add to the list
>     */
> diff -uNrap mozilla.orig/extensions/xforms/nsIXFormsControl.idl mozilla/extensions/xforms/nsIXFormsControl.idl
> --- mozilla.orig/extensions/xforms/nsIXFormsControl.idl	2006-02-24 12:46:49.000000000 +0800
> +++ mozilla/extensions/xforms/nsIXFormsControl.idl	2006-05-05 17:28:55.000000000 +0900
> @@ -37,6 +37,7 @@
>   * ***** END LICENSE BLOCK ***** */
 
>  #include "nsIXFormsContextControl.idl"
> +#include "nsIModelElementPrivate.idl"
 
>  interface nsIDOMNode;
>  interface nsIDOMElement;
> @@ -58,6 +59,11 @@ interface nsIXFormsControl : nsIXFormsCo
>    boolean tryFocus();
 
>    /**
> +   * The model that the control is referenced on.
> +   */
> +  readonly attribute nsIModelElementPrivate model;

The control is bound to the model too in some sense:
"The model that the control is bound to."

> diff -uNrap mozilla.orig/extensions/xforms/nsIXFormsNSModelElement.idl mozilla/extensions/xforms/nsIXFormsNSModelElement.idl
> --- mozilla.orig/extensions/xforms/nsIXFormsNSModelElement.idl	2006-02-06 12:49:26.000000000 +0800
> +++ mozilla/extensions/xforms/nsIXFormsNSModelElement.idl	2006-05-05 17:28:55.000000000 +0900
> @@ -39,6 +39,7 @@
>  #include "domstubs.idl"
 
>  interface nsIDOMNodeList;
> +interface nsIXFormsAccessors;
 
>  /**
>   * Defines additional post-spec. functions for the \<model\> element.
> @@ -46,5 +47,18 @@ interface nsIDOMNodeList;
>  [scriptable, uuid(85fd60c7-1db7-40c0-ae8d-f723fdd1eea8)]
>  interface nsIXFormsNSModelElement : nsISupports
>  {
> +  /*
> +   * Return nsIDOMNodeList of instance documents
> +   */
>    nsIDOMNodeList getInstanceDocuments();

In general there is no need to include parameter types in comment, it's pretty obivous from the function signature.

"Returns a list of the instance documents for the model."

> +
> +  /*
> +   * Return nsIXFormsAccessors by instance node
> +   */

"Returns an accessor interface to the given instance node".

> +  nsIXFormsAccessors getAccessorsByNode(in nsIDOMNode node);
> +
> +  /*
> +   * Return nsIXFormsAccessors by xpath expression
> +   */

"Evaluates the XPath expression as was it an XForms single node binding
expression for an outer-most control and returns an accessor interface to the
resulting node.

@note The expression is only evaluated once.
"

The comment should also say what happens if the XPath expression is invalid, or the expression points to void.

> +  nsIXFormsAccessors getAccessorsByRef(in DOMString xpath);

And I believe the function should be called getAccessorsByExpr(). Ref in my ears hint to the expression being "live". I think that would be quite a useful feature too, but let's start with this.

>  };
> diff -uNrap mozilla.orig/extensions/xforms/nsXFormsAccessors.cpp mozilla/extensions/xforms/nsXFormsAccessors.cpp
> --- mozilla.orig/extensions/xforms/nsXFormsAccessors.cpp	2006-03-16 09:50:12.000000000 +0800
> +++ mozilla/extensions/xforms/nsXFormsAccessors.cpp	2006-05-05 17:28:55.000000000 +0900
>  NS_IMETHODIMP
> -nsXFormsAccessors::GetValue(nsAString &aValue)
> +nsXFormsAccessorsBase::GetValue(nsAString &aValue)
>  {
> -  if (mDelegate) {
> -    mDelegate->GetValue(aValue);
> -  } else {
> -    SetDOMStringToNull(aValue);
> -  }
> +  nsCOMPtr<nsIModelElementPrivate> model;
> +  GetModel(getter_AddRefs(model));
> +  NS_ENSURE_STATE(model);
> +
> +  nsCOMPtr<nsIDOMNode> instNode;
> +  GetInstanceNode(getter_AddRefs(instNode));
> +  NS_ENSURE_STATE(instNode);

I think you should do the same as for a controlaccessor, that is set the value to null if there is no instance node.

> +
> +  model->GetNodeValue(instNode, aValue);
>    return NS_OK;
>  }
 
>  NS_IMETHODIMP
> -nsXFormsAccessors::SetValue(const nsAString & aValue)
> +nsXFormsAccessorsBase::SetValue(const nsAString & aValue)

"& aValue" -> " &aValue"


>  NS_IMETHODIMP
> -nsXFormsAccessors::HasBoundNode(PRBool *aHasBoundNode)
> +nsXFormsAccessorsBase::HasBoundNode(PRBool *aHasBoundNode)
>  {
>    NS_ENSURE_ARG_POINTER(aHasBoundNode);
> -  *aHasBoundNode = PR_FALSE;
> -  return mDelegate ? mDelegate->GetHasBoundNode(aHasBoundNode) : NS_OK;
> +
> +  nsCOMPtr<nsIDOMNode> instNode;
> +  GetInstanceNode(getter_AddRefs(instNode));
> +  NS_ENSURE_STATE(instNode);
> +
> +  *aHasBoundNode = instNode ? PR_TRUE : PR_FALSE;

And when will you return PR_FALSE here? :)

>  NS_IMETHODIMP
> -nsXFormsAccessors::GetBoundNode(nsIDOMNode **aBoundNode)
> +nsXFormsAccessorsBase::GetBoundNode(nsIDOMNode **aBoundNode)
>  {
>    NS_ENSURE_ARG_POINTER(aBoundNode);
> -  if (mDelegate) {
> -    nsCOMPtr<nsIXFormsControl> control = do_QueryInterface(mDelegate);
> -    return control->GetBoundNode(aBoundNode);
> -  }
> +
> +  nsCOMPtr<nsIDOMNode> instNode;
> +  GetInstanceNode(getter_AddRefs(instNode));
> +  NS_ENSURE_STATE(instNode);

Now we return OK if there is no bound node, you return an error. Having an accessor with no bound node is maybe slightly weird, but I still think we should return null and ok.

> +
> +  NS_IF_ADDREF(*aBoundNode = instNode);

(and since you actually checked instNode, there is no reason for using the safe version of addref)


> +nsXFormsAccessors::nsXFormsAccessors(
> +    nsIModelElementPrivate* aModel, nsIDOMNode* aInstanceNode) :
> +  mModel(aModel), mInstanceNode(aInstanceNode)

This is how we (I :) ), like it:
nsXFormsAccessors::nsXFormsAccessors(nsIModelElementPrivate *aModel,
                                     nsIDOMNode *aInstanceNode)
  : mModel(aModel), mInstanceNode(aInstanceNode)

> +{
> +}
> +
> +nsresult
> +nsXFormsAccessors::GetModel(nsIModelElementPrivate** aModel)
"** aModel" -> " **aModel"

Fix argument styling in rest of the patch too (that includes the .h files too :) ).

> +{
NS_IF_ADDREF only safeguard that the result is non-null, not that *aModel makes sense. You need to check that.
> +  NS_IF_ADDREF(*aModel = mModel);
> +  return NS_OK;
> +}
> +
> +nsresult
> +nsXFormsAccessors::GetInstanceNode(nsIDOMNode** aInstanceNode)
> +{

Same here.

> +  NS_IF_ADDREF(*aInstanceNode = mInstanceNode);
> +  return NS_OK;
> +}
> +
> +// nsXFormsControlAccessors implementation
> +
> +nsXFormsControlAccessors::nsXFormsControlAccessors(
> +  nsIDelegateInternal *aControl) : mDelegate(aControl)

fix styling as above.

> +{
> +}
> +
> +nsresult
> +nsXFormsControlAccessors::GetModel(nsIModelElementPrivate** aModel)
> +{
> +  NS_ENSURE_STATE(mDelegate);
> +
> +  nsCOMPtr<nsIXFormsControl> control = do_QueryInterface(mDelegate);
> +  control->GetModel(aModel);
> +  return NS_OK;

return control->GetModel(aModel);

or an error will get "lost".

> +}
> +
> +nsresult
> +nsXFormsControlAccessors::GetInstanceNode(nsIDOMNode** aInstanceNode)
> +{
> +  NS_ENSURE_STATE(mDelegate);
> +
> +  nsCOMPtr<nsIXFormsControl> control = do_QueryInterface(mDelegate);
> +  control->GetBoundNode(aInstanceNode);
> +  return NS_OK;
> +}

Same here.

> +
> +NS_IMETHODIMP
> +nsXFormsControlAccessors::GetValue(nsAString &aValue)
> +{
> +  if (mDelegate) {
> +    mDelegate->GetValue(aValue);

Also catch errors here.

> diff -uNrap mozilla.orig/extensions/xforms/nsXFormsAccessors.h mozilla/extensions/xforms/nsXFormsAccessors.h
> --- mozilla.orig/extensions/xforms/nsXFormsAccessors.h	2006-02-06 12:49:26.000000000 +0800
> +++ mozilla/extensions/xforms/nsXFormsAccessors.h	2006-05-05 17:28:55.000000000 +0900
> +/**
> + * Implementation of the nsIXFormsAccessors object for instance node.

"An nsIXFormsAccessor bound to an instance data node."

> + */
> +
> +class nsXFormsAccessors : public nsXFormsAccessorsBase
> +{
> +public:
> +  nsXFormsAccessors(nsIModelElementPrivate* aModel, nsIDOMNode* aInstanceNode);
 
> -  /** The delegate owning us */
> -  nsIDelegateInternal*   mDelegate;
> +protected:
> +  virtual nsresult GetModel(nsIModelElementPrivate** aModel);
> +  virtual nsresult GetInstanceNode(nsIDOMNode** aInstanceNode);
> +
> +private:
> +  /* The model */
> +  nsCOMPtr<nsIModelElementPrivate> mModel;
> +
> +  /* The instance node */
> +  nsCOMPtr<nsIDOMNode> mInstanceNode;

indent the variable names so that the m's are at the same line.

> +class nsXFormsControlAccessors : public nsXFormsAccessorsBase
> +{
> +public:
> +  nsXFormsControlAccessors(nsIDelegateInternal* aControl);
> +
> +  // nsIXFormsAccessors
> +  NS_IMETHOD nsXFormsControlAccessors::GetValue(nsAString &aValue);
> +  NS_IMETHOD nsXFormsControlAccessors::SetValue(const nsAString & aValue);

Good old cut'n'paste... remove "nsXFormsControlAccessors::"

> +
> +protected:
> +  virtual nsresult GetModel(nsIModelElementPrivate** aModel);
> +  virtual nsresult GetInstanceNode(nsIDOMNode** aInstanceNode);
 
> -  /** The control DOM element */
> -  nsIDOMElement*         mElement;
> +private:
> +  /* The xforms control binded to instance data */

/* The xforms control the accessor is bound to */

> +  nsCOMPtr<nsIDelegateInternal> mDelegate;
>  };
 
>  #endif
> diff -uNrap mozilla.orig/extensions/xforms/nsXFormsControlStub.cpp mozilla/extensions/xforms/nsXFormsControlStub.cpp
> --- mozilla.orig/extensions/xforms/nsXFormsControlStub.cpp	2006-04-20 11:51:00.000000000 +0900
> +++ mozilla/extensions/xforms/nsXFormsControlStub.cpp	2006-05-05 17:28:55.000000000 +0900
> @@ -105,6 +105,13 @@ nsXFormsHintHelpListener::HandleEvent(ns
>  }
 
>  NS_IMETHODIMP
> +nsXFormsControlStubBase::GetModel(nsIModelElementPrivate **aModel)
> +{

Check the argument

> +  NS_IF_ADDREF(*aModel = mModel);
> +  return NS_OK;
> +}

> --- mozilla.orig/extensions/xforms/nsXFormsDelegateStub.cpp	2006-04-19 17:28:01.000000000 +0900
> +++ mozilla/extensions/xforms/nsXFormsDelegateStub.cpp	2006-05-05 17:28:56.000000000 +0900
> @@ -91,7 +91,7 @@ nsXFormsDelegateStub::OnDestroyed()
>  {
>    nsXFormsModelElement::CancelPostRefresh(this);
>    if (mAccessor) {
> -    mAccessor->Destroy();
> +    mAccessor = nsnull;
>    }

skip the if clause

>    return nsXFormsBindableControlStub::OnDestroyed();
>  }
> @@ -263,7 +263,7 @@ NS_IMETHODIMP
>  nsXFormsDelegateStub::GetXFormsAccessors(nsIXFormsAccessors **aAccessor)
>  {
>    if (!mAccessor) {
> -    mAccessor = new nsXFormsAccessors(this, mElement);
> +    mAccessor = new nsXFormsControlAccessors(this);
>      if (!mAccessor) {
>        return NS_ERROR_OUT_OF_MEMORY;
>      }
> diff -uNrap mozilla.orig/extensions/xforms/nsXFormsDelegateStub.h mozilla/extensions/xforms/nsXFormsDelegateStub.h
> --- mozilla.orig/extensions/xforms/nsXFormsDelegateStub.h	2006-02-06 12:49:26.000000000 +0800
> +++ mozilla/extensions/xforms/nsXFormsDelegateStub.h	2006-05-05 17:28:56.000000000 +0900
> @@ -112,7 +112,7 @@ protected:
>    nsRepeatState mRepeatState;
 
>    /** The accessors object for this delegate */
> -  nsRefPtr<nsXFormsAccessors> mAccessor;
> +  nsRefPtr<nsXFormsAccessorsBase> mAccessor;

Why use the base, when you actually instantiate the controlAccessors?

>  };
 
>  #endif
> diff -uNrap mozilla.orig/extensions/xforms/nsXFormsModelElement.cpp mozilla/extensions/xforms/nsXFormsModelElement.cpp
> --- mozilla.orig/extensions/xforms/nsXFormsModelElement.cpp	2006-04-27 18:13:34.000000000 +0900
> +++ mozilla/extensions/xforms/nsXFormsModelElement.cpp	2006-05-05 17:28:56.000000000 +0900

>  NS_IMETHODIMP
> +nsXFormsModelElement::GetAccessorsByNode(nsIDOMNode *aInstanceNode,
> +                                         nsIXFormsAccessors **aAccessors)
> +{
> +  NS_ENSURE_ARG_POINTER(aInstanceNode);
> +  NS_ENSURE_ARG_POINTER(aAccessors);
> +
> +  nsCOMPtr<nsIDOMDocument> instanceDOMDoc = nsnull;

nsCOMPtr's are null at creation.

> +  aInstanceNode->GetOwnerDocument(getter_AddRefs(instanceDOMDoc));
> +  NS_ENSURE_STATE(instanceDOMDoc);
> +
> +  nsCOMPtr<nsIDocument> instanceDoc = do_QueryInterface(instanceDOMDoc);
> +  NS_ENSURE_STATE(instanceDoc);
> +
> +  nsISupports *instanceDocOwner = NS_STATIC_CAST(nsISupports*,
> +    instanceDoc->GetProperty(nsXFormsAtoms::instanceDocumentOwner));
> +  NS_ENSURE_STATE(instanceDocOwner);
> +
> +  nsCOMPtr<nsIDOMNode> instanceDocNode = do_QueryInterface(instanceDocOwner);
> +  NS_ENSURE_STATE(instanceDocNode);

Use nsXFormsUtils::GetInstanceNodeForData()

> +
> +  nsCOMPtr<nsIDOMNode> modelNode = nsnull;
> +  instanceDocNode->GetParentNode(getter_AddRefs(modelNode));
> +  NS_ENSURE_STATE(modelNode);
> +
> +  nsCOMPtr<nsIDOM3Node> theModelNode(do_QueryInterface(mElement)); 
> +  PRBool isSameNode = PR_FALSE;
> +  theModelNode->IsSameNode(modelNode, &isSameNode);
> +  if (!isSameNode)
> +    return NS_ERROR_UNEXPECTED;
> +
> +  NS_ADDREF(*aAccessors = new nsXFormsAccessors(this, aInstanceNode));

new might fail.

> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsXFormsModelElement::GetAccessorsByRef(const nsAString& aXPathExpr,
> +                                        nsIXFormsAccessors **aAccessors)
> +{
> +  NS_ENSURE_ARG_POINTER(aAccessors);
> +
> +  nsCOMPtr<nsIDOMDocument> firstInstanceDoc =
> +    FindInstanceDocument(EmptyString());
> +  NS_ENSURE_STATE(firstInstanceDoc);
> +
> +  nsCOMPtr<nsIDOMElement> defaultContextNode;
> +  firstInstanceDoc->GetDocumentElement(getter_AddRefs(defaultContextNode));
> +
> +  nsCOMPtr<nsIDOMXPathResult> xpRes;
> +  nsXFormsUtils::EvaluateXPath(aXPathExpr, defaultContextNode, mElement,
> +                               nsIDOMXPathResult::FIRST_ORDERED_NODE_TYPE,
> +                               getter_AddRefs(xpRes));
> +  NS_ENSURE_STATE(xpRes);
> +
> +  nsCOMPtr<nsIDOMNode> instanceNode;
> +  xpRes->GetSingleNodeValue(getter_AddRefs(instanceNode));
> +  NS_ENSURE_STATE(instanceNode);
> +
> +  NS_ADDREF(*aAccessors = new nsXFormsAccessors(this, instanceNode));

new might fail.

> +  return NS_OK;
> +}

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch3

> > +{
> NS_IF_ADDREF only safeguard that the result is non-null, not that *aModel 
> makes sense. You need to check that.
> > +  NS_IF_ADDREF(*aModel = mModel);
> > +  return NS_OK;
> > +}
> > +
> > +nsresult
> > +nsXFormsAccessors::GetInstanceNode(nsIDOMNode** aInstanceNode)
> > +{

> Same here.

I'm not sure because I don't check model and instance data in consturctor, I guess they might be null.

Thanks for good review (too much nits!!).

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch4

> > +{
> NS_IF_ADDREF only safeguard that the result is non-null, not that *aModel 
> makes sense. You need to check that.
> > +  NS_IF_ADDREF(*aModel = mModel);
> > +  return NS_OK;
> > +}
> > +
> > +nsresult
> > +nsXFormsAccessors::GetInstanceNode(nsIDOMNode** aInstanceNode)
> > +{

> Same here.

Fixed

Comment 19

[alexander :surkov (:asurkov)]

isReadonly() and e.t.c methods return NS_OK currently, but after patch applying they can fail. It involves that select/select1 without bind are broken. Should we fix select/select1 or isReadonly() and e.t.c should be more friednly? :)

Comment 20

[Allan Beaufour]

(In reply to comment #19)
> isReadonly() and e.t.c methods return NS_OK currently, but after patch applying
> they can fail. It involves that select/select1 without bind are broken. Should
> we fix select/select1 or isReadonly() and e.t.c should be more friednly? :)

I always think that we should not hide errors, so we should fix select/select1. That said, if we have to jump through 1000 hoops to get it working, it might not be feasible.

Comment 21

[alexander :surkov (:asurkov)]

Attachment: patch5

(In reply to comment #20)
> (In reply to comment #19)
> > isReadonly() and e.t.c methods return NS_OK currently, but after patch applying
> > they can fail. It involves that select/select1 without bind are broken. Should
> > we fix select/select1 or isReadonly() and e.t.c should be more friednly? :)
> 
> I always think that we should not hide errors, so we should fix select/select1.
> That said, if we have to jump through 1000 hoops to get it working, it might
> not be feasible.
> 

The patch checks bound node in xbl part of xforms before accessors methods using.

Comment 22

[Allan Beaufour]

Comment on attachment 221689 [details] [diff] [review]
patch5

(please fix the W-error: http://beaufour.dk/jst-review/?patch=221689)

> +++ mozilla/extensions/xforms/nsIXFormsNSModelElement.idl	2006-05-11 19:16:58.000000000 +0900

> +  /*
> +   * Evaluates the XPath expression as was it an XForms single node binding
> +   * expression for an outer-most control and returns an accessor interface to
> +   * the resulting node. If XPath expression is invalid then method dispatches
> +   * "xforms-compute-exception" event to the model.
> +   * @note The expression is only evaluated once.
> +   */
> +  nsIXFormsAccessors getAccessorsByExpr(in DOMString xpath);

Please also state what is _returned_ when 1) expression is invalid or 2) expression is valid, but does not point to a node.

> +++ mozilla/extensions/xforms/nsXFormsModelElement.cpp	2006-05-11 19:16:59.000000000 +0900

> +nsXFormsModelElement::GetAccessorsByNode(nsIDOMNode *aInstanceNode,
> +                                         nsIXFormsAccessors **aAccessors)
> +{
> +  NS_ENSURE_ARG_POINTER(aInstanceNode);
> +  NS_ENSURE_ARG_POINTER(aAccessors);
> +
> +  nsCOMPtr<nsIDOMNode> instanceDocNode;
> +  nsresult rv;
> +  nsXFormsUtils::GetInstanceNodeForData(aInstanceNode,
> +                                         getter_AddRefs(instanceDocNode));
> +  NS_ENSURE_SUCCESS(rv, rv);

Actually storing something in "rv" would help the check here :)

> +++ mozilla/extensions/xforms/resources/content/xforms.xml	2006-05-11 19:17:00.000000000 +0900
> @@ -154,7 +154,10 @@
 
>        <property name="stringValue" readonly="true">
>          <getter>
> -          var value = this.accessors.getValue();
> +          var value = null;
> +          if (this.accessors.hasBoundNode())
> +            value = this.accessors.getValue();
> +

Wouldn't these fixes keep a control from clearing an old value, if it goes from "having a bound node" to "not having a bound node"?

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> 
> Wouldn't these fixes keep a control from clearing an old value, if it goes from
> "having a bound node" to "not having a bound node"?
> 

Right. I think it makes sense to drop value/state on default when control goes from "having a bound node" to "not having a bound node".

Comment 24

[Allan Beaufour]

(In reply to comment #23)
> (In reply to comment #22)
> > 
> > Wouldn't these fixes keep a control from clearing an old value, if it goes from
> > "having a bound node" to "not having a bound node"?
> > 
> 
> Right. I think it makes sense to drop value/state on default when control goes
> from "having a bound node" to "not having a bound node".

? What I meant was that if an output was bound to a node and displaying "Value XX", and then it is refreshed and not pointing to a node, then it will still display "Value XX", will it not? It should display "".

Comment 25

[alexander :surkov (:asurkov)]

Attachment: patch6

Now if control hans't a bind then it has default value and not readonly state.

Comment 26

[Allan Beaufour]

Comment on attachment 222155 [details] [diff] [review]
patch6

http://localhost/xftst/Model/nonbound-output-state.xml.show

Comment 27

[Allan Beaufour]

(In reply to comment #26)
> (From update of attachment 222155 [details] [diff] [review] [edit])
> http://localhost/xftst/Model/nonbound-output-state.xml.show

Aïe. Something's playing tricks on me :(

The correct links were of course:
http://beaufour.dk/xftst/Model/nonbound-control-state.xml.show
http://beaufour.dk/xftst/Model/nonbound-output-state.xml.show

Both end up with an exception when using the patch.

Comment 28

[alexander :surkov (:asurkov)]

Attachment: patch7

accessors for delegate control doesn't fail for isReadonly()/etc methods even if delegate doesn't have instance node bound. Therefore I removed from the patch all changes in xbl bindings (they are not actual more).

Comment 29

[alexander :surkov (:asurkov)]

Attachment: testcase2

Comment 30

[Allan Beaufour]

Comment on attachment 222995 [details] [diff] [review]
patch7

Aie, this dispappeared in my bugmail, and now it is not up-to-date. Could you please update it to current trunk and re-request review?

Sorry for that.

Comment 31

[alexander :surkov (:asurkov)]

Attachment: patch8

(In reply to comment #30)
> (From update of attachment 222995 [details] [diff] [review] [edit])
> Aie, this dispappeared in my bugmail, and now it is not up-to-date. Could you
> please update it to current trunk and re-request review?

Updated :)

Comment 32

[alexander :surkov (:asurkov)]

Comment on attachment 224210 [details] [diff] [review]
patch8

Since Allan hasn't enough time for review right now then Aaron, can you do review?

Comment 33

[aaronr]

(In reply to comment #32)
> (From update of attachment 224210 [details] [diff] [review] [edit])
> Since Allan hasn't enough time for review right now then Aaron, can you do
> review?
> 

I'll get to it eventually, but it may be after I get the libxul build issues fixed.  I assume that you aren't dependent on this patch to do any other patches?

Comment 34

[alexander :surkov (:asurkov)]

(In reply to comment #33)
> (In reply to comment #32)
> > (From update of attachment 224210 [details] [diff] [review] [edit] [edit])
> > Since Allan hasn't enough time for review right now then Aaron, can you do
> > review?
> > 
> 
> I'll get to it eventually, but it may be after I get the libxul build issues
> fixed.  I assume that you aren't dependent on this patch to do any other
> patches?

For sure, when you'll get enough time for this. I just like to see the bug in next version of xforms, there are no any patch dependences.

Comment 35

[aaronr]

Comment on attachment 224210 [details] [diff] [review]
patch8

The patch contains window line endings.  Remember to run dos2unix on the patch before uploading it (or at least verifying that your patch doesn't contain windows line endings).


>Index: mozilla/extensions/xforms/nsIModelElementPrivate.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIModelElementPrivate.idl,v
>retrieving revision 1.22
>diff -p -u -8 -r1.22 nsIModelElementPrivate.idl
>--- mozilla/extensions/xforms/nsIModelElementPrivate.idl	24 May 2006 07:29:53 -0000	1.22
>+++ mozilla/extensions/xforms/nsIModelElementPrivate.idl	2 Jun 2006 16:53:03 -0000
>@@ -128,16 +128,22 @@ interface nsIModelElementPrivate : nsIXF
>   const unsigned short SUBMIT_ABORT_SUBMISSION = 3;
> 
>   /**
>    * Handles an instance data node and returns one of the above 3 values.
>    */
>   unsigned short handleInstanceDataNode(in nsIDOMNode aInstanceDataNode);
> 
>   /**
>+   * Return states for bound node in terms of NS_EVENT_STATE constants
>+   * @param aBoundNode         The instance node
>+   */
>+  long getStates(in nsIDOMNode aBoundNode);
>+
>+  /**
>    * Set MIP states for given control bound to the given bound node.
>    * @param aControl          The control
>    * @param aBoundNode        The node the control is bound to
>    */
>   void setStates(in nsIXFormsControl aControl, in nsIDOMNode aBoundNode);

You are changing the interface, so you need to generate a new uuid for the interface

>Index: mozilla/extensions/xforms/nsIXFormsControl.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIXFormsControl.idl,v
>retrieving revision 1.12
>diff -p -u -8 -r1.12 nsIXFormsControl.idl
>--- mozilla/extensions/xforms/nsIXFormsControl.idl	24 May 2006 03:45:45 -0000	1.12
>+++ mozilla/extensions/xforms/nsIXFormsControl.idl	2 Jun 2006 16:53:04 -0000

>@@ -53,16 +54,21 @@ interface nsIDOMElement;
> interface nsIXFormsControl : nsIXFormsContextControl
> {
>   /**
>    * Tries to move focus to form control and returns true if succeeded.
>    */
>   boolean tryFocus();
> 
>   /**
>+   * The model that the control is bound to.
>+   */
>+  readonly attribute nsIModelElementPrivate model;
>+
>+  /**
>    * The instance node that the control is bound to.
>    */
>   readonly attribute nsIDOMNode boundNode;

You are changing the interface, so you need to generate a new uuid for the interface

>Index: mozilla/extensions/xforms/nsIXFormsNSModelElement.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIXFormsNSModelElement.idl,v
>retrieving revision 1.1
>diff -p -u -8 -r1.1 nsIXFormsNSModelElement.idl
>--- mozilla/extensions/xforms/nsIXFormsNSModelElement.idl	25 Oct 2005 17:29:56 -0000	1.1
>+++ mozilla/extensions/xforms/nsIXFormsNSModelElement.idl	2 Jun 2006 16:53:06 -0000
>@@ -34,17 +34,36 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "domstubs.idl"
> 
> interface nsIDOMNodeList;
>+interface nsIXFormsAccessors;
> 
> /**
>  * Defines additional post-spec. functions for the \<model\> element.
>  */
> [scriptable, uuid(85fd60c7-1db7-40c0-ae8d-f723fdd1eea8)]
> interface nsIXFormsNSModelElement : nsISupports
> {
>+  /*
>+   * Returns a list of the instance documents for the model.
>+   */
>   nsIDOMNodeList getInstanceDocuments();
>+
>+  /*
>+   * Returns an accessor interface to the given instance node.
>+   */
>+  nsIXFormsAccessors getAccessorsByNode(in nsIDOMNode node);
>+
>+  /*
>+   * Evaluates the XPath expression as was it an XForms single node binding
>+   * expression for an outer-most control and returns an accessor interface to
>+   * the resulting node. If XPath expression is invalid then method dispatches
>+   * "xforms-compute-exception" event to the model and rises an exception. If
>+   * XPath doesn't point to a node then method rises an exception.
>+   * @note The expression is only evaluated once.
>+   */
>+  nsIXFormsAccessors getAccessorsByExpr(in DOMString xpath);

You are changing the interface, so you need to generate a new uuid for the interface

I would also suggest this phrasing for the opening of the comment: "Evaluates the XPath expression as if it were an XForms single node binding expression for an outer-most control..."

Also, there are a couple of things in the accessors interface that are for controls.  Like hasBoundNode() and getBoundNode().  How are those functions going to work?

> };
>Index: mozilla/extensions/xforms/nsXFormsAccessors.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsAccessors.cpp,v
>retrieving revision 1.5
>diff -p -u -8 -r1.5 nsXFormsAccessors.cpp
>--- mozilla/extensions/xforms/nsXFormsAccessors.cpp	22 May 2006 08:51:05 -0000	1.5
>+++ mozilla/extensions/xforms/nsXFormsAccessors.cpp	2 Jun 2006 16:53:08 -0000
>@@ -42,166 +42,273 @@
> #include "nsXFormsUtils.h"
> #include "nsXFormsAccessors.h"
> #include "nsDOMString.h"
> #include "nsIEventStateManager.h"
> #include "nsIContent.h"
> #include "nsIXFormsControl.h"
> #include "nsIProgrammingLanguage.h"
> 
>-NS_IMPL_ISUPPORTS2(nsXFormsAccessors, nsIXFormsAccessors, nsIClassInfo)
>+// nsXFormsAccessors object implementation
> 
>-void
>-nsXFormsAccessors::Destroy()
>-{
>-  mElement = nsnull;
>-  mDelegate = nsnull;
>-}

Don't you still need this for nsXFormsControlAccessors?  I mention below why I think we should keep mElement and I'd think that you'd still want to null them out.

>+NS_IMPL_ISUPPORTS2(nsXFormsAccessorsBase, nsIXFormsAccessors, nsIClassInfo)
> 
> nsresult
>-nsXFormsAccessors::GetState(PRInt32 aState, PRBool *aStateVal)
>+nsXFormsAccessorsBase::GetState(PRInt32 aState, PRBool *aStateVal)
> {
>   NS_ENSURE_ARG_POINTER(aStateVal);
>-  nsCOMPtr<nsIContent> content(do_QueryInterface(mElement));
>-  *aStateVal = (content && (content->IntrinsicState() & aState));
>+
>+  nsCOMPtr<nsIModelElementPrivate> model;
>+  GetModel(getter_AddRefs(model));
>+  NS_ENSURE_STATE(model);
>+
>+  nsCOMPtr<nsIDOMNode> instNode;
>+  GetInstanceNode(getter_AddRefs(instNode));
>+  NS_ENSURE_STATE(instNode);
>+
>+  PRInt32 states;
>+  model->GetStates(instNode, &states);
>+  *aStateVal = states & aState;
> 

maybe I missed this somewhere when reading the bug, but why are you getting the state this way rather than converting instNode to nsIContent and using IntrinsicState()?  If I have a xforms control and get the nsxformsaccessor from that and call GetState that way, shouldn't that return the exact same values as if I get the @ref from the control and then call GetAccessorsByExpr and then call GetState on that accessor?  Right now there is a chance that they won't be the same value, isn't there?

> NS_IMETHODIMP
>-nsXFormsAccessors::GetHelperForLanguage(PRUint32 language,
>+nsXFormsAccessorsBase::GetHelperForLanguage(PRUint32 language,
>                                         nsISupports **_retval)
> {

nit: please line up the parameters.

>+
>+// nsXFormsControlAccessors implementation
>+
>+nsXFormsControlAccessors::nsXFormsControlAccessors(nsIDelegateInternal *aControl)
>+  : mDelegate(aControl)
>+{
>+}
>+
>+nsresult
>+nsXFormsControlAccessors::GetModel(nsIModelElementPrivate **aModel)
>+{
>+  NS_ENSURE_ARG_POINTER(aModel);
>+  NS_ENSURE_STATE(mDelegate);
>+
>+  nsCOMPtr<nsIXFormsControl> control = do_QueryInterface(mDelegate);
>+  return control->GetModel(aModel);
>+}
>+
>+nsresult
>+nsXFormsControlAccessors::GetInstanceNode(nsIDOMNode **aInstanceNode)
>+{
>+  NS_ENSURE_ARG_POINTER(aInstanceNode);
>+  NS_ENSURE_STATE(mDelegate);
>+
>+  nsCOMPtr<nsIXFormsControl> control = do_QueryInterface(mDelegate);
>+  return control->GetBoundNode(aInstanceNode);
>+}
>+
>+nsresult
>+nsXFormsControlAccessors::GetState(PRInt32 aState, PRBool *aStateVal)
>+{
>+  NS_ENSURE_ARG_POINTER(aStateVal);
>+
>+  if (mDelegate) {
>+    nsCOMPtr<nsIXFormsControl> control(do_QueryInterface(mDelegate));
>+
>+    nsCOMPtr<nsIDOMElement> element;
>+    nsresult rv = control->GetElement(getter_AddRefs(element));
>+    NS_ENSURE_SUCCESS(rv, rv);

I would go back to storing and using mElement.  GetState gets called a lot, I believe, so I personally think that it is worth that small amount of memory to save a QI and function call.


>Index: mozilla/extensions/xforms/nsXFormsAccessors.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsAccessors.h,v
>retrieving revision 1.2
>diff -p -u -8 -r1.2 nsXFormsAccessors.h
>--- mozilla/extensions/xforms/nsXFormsAccessors.h	14 Nov 2005 08:42:36 -0000	1.2
>+++ mozilla/extensions/xforms/nsXFormsAccessors.h	2 Jun 2006 16:53:09 -0000
>@@ -34,52 +34,91 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef __NSXFORMSACCESSORS_H__
> #define __NSXFORMSACCESSORS_H__
> 
>+#include "nsCOMPtr.h"
>+
> #include "nsIClassInfo.h"
> #include "nsIXFormsAccessors.h"
> #include "nsIDelegateInternal.h"
>-
>-class nsIDOMElement;
>+#include "nsIModelElementPrivate.h"
> 
> /**
>- * Implementation of the nsIXFormsAccessors object. It is always owned by a
>- * nsIXFormsDelegate.
>+ * Abstract class for nsIXFormsAccessors objects.

nit: Maybe say explicitly that nsXFormsAccessorsBase isn't meant to be instantiated, but instead a user should instantiate nsXFormsAccessors or nsXFormsControlAccessors.  Just to make it really clear.

>  */
>-class nsXFormsAccessors : public nsIXFormsAccessors,
>-                          public nsIClassInfo
>+class nsXFormsAccessorsBase : public nsIXFormsAccessors,
>+                              public nsIClassInfo
> {
> public:
>   NS_DECL_ISUPPORTS
>   NS_DECL_NSICLASSINFO
>   NS_DECL_NSIXFORMSACCESSORS
> 
>-  /** Constructor */
>-  nsXFormsAccessors(nsIDelegateInternal* aDelegate, nsIDOMElement* aElement)
>-    : mDelegate(aDelegate), mElement(aElement) 
>-  {
>-  }
>-
>-  /** Called by the owning delegate when it itself is destroyed */
>-  void Destroy();
>-
> protected:
>+  virtual nsresult GetModel(nsIModelElementPrivate **aModel) = 0;
>+  virtual nsresult GetInstanceNode(nsIDOMNode **aInstanceNode) = 0;
>+

Maybe you should make a comment here about why GetModel and GetInstanceNode need to be implemented in both subclasses (because the implementations of nsIXFormsAccessor functions like setValue and setContent in the base class need them).  I finally figured it out, but boy it made my brain hurt! :-)

>   /**
>    * Checks the status of the model item properties
>    *
>    * @param aState       The state to check
>    * @para  aStateVal    The returned state
>    */
>-  nsresult GetState(PRInt32 aState, PRBool *aStateVal);
>+  virtual nsresult GetState(PRInt32 aState, PRBool *aStateVal);
>+};
> 
>-  /** The delegate owning us */
>-  nsIDelegateInternal*   mDelegate;
> 
>-  /** The control DOM element */
>-  nsIDOMElement*         mElement;
>+/**
>+ * Implementation of the nsIXFormsAccessors object for instance node.
>+ */
>+
>+class nsXFormsAccessors : public nsXFormsAccessorsBase
>+{
>+public:
>+  nsXFormsAccessors(nsIModelElementPrivate *aModel, nsIDOMNode *aInstanceNode);
>+
>+protected:
>+  virtual nsresult GetModel(nsIModelElementPrivate **aModel);
>+  virtual nsresult GetInstanceNode(nsIDOMNode **aInstanceNode);
>+
>+private:
>+  /* The model */
>+  nsCOMPtr<nsIModelElementPrivate> mModel;
>+
>+  /* The instance node */
>+  nsCOMPtr<nsIDOMNode>             mInstanceNode;
>+};
>+
>+/**
>+ * Implementation of the nsIXFormsAccessors object for nsIXFormsDelegate
>+ * controls.
>+ *
>+ * Some nsIXFormsDelegate controls have a value different from its
>+ * bound node value (like xforms:label and xforms:output).
>+ * nsXFormsControlAccessors object serves for redirection of
>+ * getValue()/setValue() calls to nsIXFormsDelegate control.
>+ */

That's not true, is it?  How can a control have a value different from its bound node?  A control could have a value even if it doesn't HAVE a bound node.  Is that what you meant?  Or am I missing something?

>+
>+class nsXFormsControlAccessors : public nsXFormsAccessorsBase
>+{
>+public:
>+  nsXFormsControlAccessors(nsIDelegateInternal *aControl);
>+
>+  // nsIXFormsAccessors
>+  NS_IMETHOD GetValue(nsAString &aValue);
>+  NS_IMETHOD SetValue(const nsAString &aValue);
>+
>+protected:
>+  virtual nsresult GetModel(nsIModelElementPrivate **aModel);
>+  virtual nsresult GetInstanceNode(nsIDOMNode **aInstanceNode);
>+  virtual nsresult GetState(PRInt32 aState, PRBool *aStateVal);
>+
>+private:
>+  /* The xforms control the accessor is bound to */
>+  nsCOMPtr<nsIDelegateInternal> mDelegate;
> };
> 
> #endif

This is as far as I've gotten so far.  I'll try to finish my review tonight or tomorrow.

Comment 36

[aaronr]

(In reply to comment #35)
> (From update of attachment 224210 [details] [diff] [review] [edit])

> >Index: mozilla/extensions/xforms/nsIXFormsNSModelElement.idl
> >===================================================================
> >RCS file: /cvsroot/mozilla/extensions/xforms/nsIXFormsNSModelElement.idl,v
> >retrieving revision 1.1
> >diff -p -u -8 -r1.1 nsIXFormsNSModelElement.idl
> >--- mozilla/extensions/xforms/nsIXFormsNSModelElement.idl	25 Oct 2005 17:29:56 -0000	1.1
> >+++ mozilla/extensions/xforms/nsIXFormsNSModelElement.idl	2 Jun 2006 16:53:06 -0000
> >@@ -34,17 +34,36 @@
> >  * the provisions above, a recipient may use your version of this file under
> >  * the terms of any one of the MPL, the GPL or the LGPL.
> >  *
> >  * ***** END LICENSE BLOCK ***** */
> > 
> > #include "domstubs.idl"
> > 
> > interface nsIDOMNodeList;
> >+interface nsIXFormsAccessors;
> > 
> > /**
> >  * Defines additional post-spec. functions for the \<model\> element.
> >  */
> > [scriptable, uuid(85fd60c7-1db7-40c0-ae8d-f723fdd1eea8)]
> > interface nsIXFormsNSModelElement : nsISupports
> > {
> >+  /*
> >+   * Returns a list of the instance documents for the model.
> >+   */
> >   nsIDOMNodeList getInstanceDocuments();
> >+
> >+  /*
> >+   * Returns an accessor interface to the given instance node.
> >+   */
> >+  nsIXFormsAccessors getAccessorsByNode(in nsIDOMNode node);
> >+
> >+  /*
> >+   * Evaluates the XPath expression as was it an XForms single node binding
> >+   * expression for an outer-most control and returns an accessor interface to
> >+   * the resulting node. If XPath expression is invalid then method dispatches
> >+   * "xforms-compute-exception" event to the model and rises an exception. If
> >+   * XPath doesn't point to a node then method rises an exception.
> >+   * @note The expression is only evaluated once.
> >+   */
> >+  nsIXFormsAccessors getAccessorsByExpr(in DOMString xpath);
> 
> You are changing the interface, so you need to generate a new uuid for the
> interface
> 
> I would also suggest this phrasing for the opening of the comment: "Evaluates
> the XPath expression as if it were an XForms single node binding expression for
> an outer-most control..."
> 
> Also, there are a couple of things in the accessors interface that are for
> controls.  Like hasBoundNode() and getBoundNode().  How are those functions
> going to work?
> 

You don't have to reply to me.  I can see it after I read the code.  Just make a comment in the code somewhere.


> >+NS_IMPL_ISUPPORTS2(nsXFormsAccessorsBase, nsIXFormsAccessors, nsIClassInfo)
> > 
> > nsresult
> >-nsXFormsAccessors::GetState(PRInt32 aState, PRBool *aStateVal)
> >+nsXFormsAccessorsBase::GetState(PRInt32 aState, PRBool *aStateVal)
> > {
> >   NS_ENSURE_ARG_POINTER(aStateVal);
> >-  nsCOMPtr<nsIContent> content(do_QueryInterface(mElement));
> >-  *aStateVal = (content && (content->IntrinsicState() & aState));
> >+
> >+  nsCOMPtr<nsIModelElementPrivate> model;
> >+  GetModel(getter_AddRefs(model));
> >+  NS_ENSURE_STATE(model);
> >+
> >+  nsCOMPtr<nsIDOMNode> instNode;
> >+  GetInstanceNode(getter_AddRefs(instNode));
> >+  NS_ENSURE_STATE(instNode);
> >+
> >+  PRInt32 states;
> >+  model->GetStates(instNode, &states);
> >+  *aStateVal = states & aState;
> > 
> 
> maybe I missed this somewhere when reading the bug, but why are you getting the
> state this way rather than converting instNode to nsIContent and using
> IntrinsicState()?  If I have a xforms control and get the nsxformsaccessor from
> that and call GetState that way, shouldn't that return the exact same values as
> if I get the @ref from the control and then call GetAccessorsByExpr and then
> call GetState on that accessor?  Right now there is a chance that they won't be
> the same value, isn't there?

Bah!  Never mind this comment.  I must have been loopy at the time.  None of the state information stuff is stored on the instance node. 

Comment 37

[alexander :surkov (:asurkov)]

> >-NS_IMPL_ISUPPORTS2(nsXFormsAccessors, nsIXFormsAccessors, nsIClassInfo)
> >+// nsXFormsAccessors object implementation
> > 
> >-void
> >-nsXFormsAccessors::Destroy()
> >-{
> >-  mElement = nsnull;
> >-  mDelegate = nsnull;
> >-}
> 
> Don't you still need this for nsXFormsControlAccessors?  I mention below why I
> think we should keep mElement and I'd think that you'd still want to null them
> out.
> 
> I would go back to storing and using mElement.  GetState gets called a lot, I
> believe, so I personally think that it is worth that small amount of memory to
> save a QI and function call.
> 

It seems to me Allan asked to reduce amount of stored variables. I think GetState()
is not often be called, so it looks like it shouldn't be performance issue.

Since variables use nsCOMPtr then I guess I shouldn't null them on destroy.

> That's not true, is it?  How can a control have a value different from its
> bound node?  A control could have a value even if it doesn't HAVE a bound node.
>  Is that what you meant?  Or am I missing something?

You're absolutely right. That is the example where there is a difference between
what I said and what I had in view.

A new patch is going to be a bit later.

Comment 38

[alexander :surkov (:asurkov)]

Attachment: patch9

Comment 39

[aaronr]

Comment on attachment 224210 [details] [diff] [review]
patch8


>Index: mozilla/extensions/xforms/nsXFormsModelElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.cpp,v
>retrieving revision 1.126
>diff -p -u -8 -r1.126 nsXFormsModelElement.cpp
>--- mozilla/extensions/xforms/nsXFormsModelElement.cpp	31 May 2006 07:47:58 -0000	1.126
>+++ mozilla/extensions/xforms/nsXFormsModelElement.cpp	2 Jun 2006 16:53:53 -0000
>@@ -57,16 +57,17 @@
> #include "nsIDOMXPathResult.h"
> #include "nsIXFormsXPathEvaluator.h"
> #include "nsIDOMXPathNSResolver.h"
> #include "nsIDOMNSXPathExpression.h"
> #include "nsIContent.h"
> #include "nsIURL.h"
> #include "nsNetUtil.h"
> #include "nsIXFormsControl.h"
>+#include "nsXFormsAccessors.h"
> #include "nsXFormsTypes.h"
> #include "nsXFormsXPathParser.h"
> #include "nsXFormsXPathAnalyzer.h"
> #include "nsIInstanceElementPrivate.h"
> #include "nsXFormsUtils.h"
> #include "nsXFormsSchemaValidator.h"
> #include "nsIXFormsUIWidget.h"
> #include "nsIAttribute.h"
>@@ -1003,28 +1004,91 @@ nsXFormsModelElement::OnCreated(nsIXTFGe
>     PRInt32 val;
>     if (NS_SUCCEEDED(pref->GetIntPref("xforms.modelLoopMax", &val)))
>       mLoopMax = val;
>   }
> 
>   return NS_OK;
> }
> 
>-// nsIXFormsModelElement
>+// nsIXFormsNSModelElement
> 
> NS_IMETHODIMP
> nsXFormsModelElement::GetInstanceDocuments(nsIDOMNodeList **aDocuments)
> {
>   NS_ENSURE_STATE(mInstanceDocuments);
>   NS_ENSURE_ARG_POINTER(aDocuments);
>   NS_ADDREF(*aDocuments = mInstanceDocuments);
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
>+nsXFormsModelElement::GetAccessorsByNode(nsIDOMNode *aInstanceNode,
>+                                         nsIXFormsAccessors **aAccessors)
>+{
>+  NS_ENSURE_ARG_POINTER(aInstanceNode);

nit: Most places in Mozilla seem to use NS_ENSURE_ARG_POINTER for parameters that are return buffers and NS_ENSURE_ARG for input parameters (unless they are in/out parameters)

>+  NS_ENSURE_ARG_POINTER(aAccessors);

nit: How about setting the value of *aAccessors to nsnull here?  Then we'll always return nsnull for this value if there is an error.

>+
>+  nsCOMPtr<nsIDOMNode> instanceDocNode;
>+  nsresult rv = nsXFormsUtils::
>+    GetInstanceNodeForData(aInstanceNode, getter_AddRefs(instanceDocNode));


nit: please move GetInstanceNodeForData up a line so that it is still with nsXFormsUtils and if you need to, put the parameters on the second line.

>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIDOMNode> modelNode;
>+  instanceDocNode->GetParentNode(getter_AddRefs(modelNode));
>+  NS_ENSURE_STATE(modelNode);
>+
>+  nsCOMPtr<nsIDOM3Node> theModelNode(do_QueryInterface(mElement));
>+  PRBool isSameNode = PR_FALSE;
>+  theModelNode->IsSameNode(modelNode, &isSameNode);
>+  if (!isSameNode)
>+    return NS_ERROR_UNEXPECTED;
>+

nit: what about using NS_ENSURE_TRUE here?  Like NS_ENSURE_TRUE(isSameNode, NS_ERROR_UNEXPECTED).  Just to keep it looking consistent with the rest of our code.

>+  *aAccessors = new nsXFormsAccessors(this, aInstanceNode);
>+  if (!*aAccessors)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+

nit: NS_ENSURE_TRUE here, too.

>+  NS_ADDREF(*aAccessors);
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsModelElement::GetAccessorsByExpr(const nsAString& aXPathExpr,
>+                                         nsIXFormsAccessors **aAccessors)

nit: looks like the style in this file is to line up the a's if the parameters are on seperate lines.

>+{
>+  NS_ENSURE_ARG_POINTER(aAccessors);

nit: set *aAccessors to nsnull so that it is null if there is an error below.

>+
>+  nsCOMPtr<nsIDOMDocument> firstInstanceDoc =
>+    FindInstanceDocument(EmptyString());
>+  NS_ENSURE_STATE(firstInstanceDoc);
>+
>+  nsCOMPtr<nsIDOMElement> defaultContextNode;
>+  firstInstanceDoc->GetDocumentElement(getter_AddRefs(defaultContextNode));
>+
>+  nsCOMPtr<nsIDOMXPathResult> xpRes;
>+  nsXFormsUtils::EvaluateXPath(aXPathExpr, defaultContextNode, mElement,
>+                               nsIDOMXPathResult::FIRST_ORDERED_NODE_TYPE,
>+                               getter_AddRefs(xpRes));
>+  NS_ENSURE_STATE(xpRes);
>+
>+  nsCOMPtr<nsIDOMNode> instanceNode;
>+  xpRes->GetSingleNodeValue(getter_AddRefs(instanceNode));
>+  NS_ENSURE_STATE(instanceNode);
>+
>+  *aAccessors = new nsXFormsAccessors(this, instanceNode);
>+  if (!*aAccessors)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  NS_ADDREF(*aAccessors);
>+  return NS_OK;
>+} 
>+

This doesn't look right.  Doing a new and an addref?  I'm no XPCOM expert, but I've never seen this done.  I don't know that the memory will ever be freed.  Usually you have a nsCOMPtr, then you initialize it with values, then swap it to the return buffer.  You might want to verify that this memory gets freed or get Olli, AaronL or someone who knows XPCOM better to say that this is correct.

>+// nsIXFormsModelElement
>+
>+NS_IMETHODIMP
> nsXFormsModelElement::GetInstanceDocument(const nsAString& aInstanceID,
>                                           nsIDOMDocument **aDocument)
> {
>   NS_ENSURE_ARG_POINTER(aDocument);
> 
>   *aDocument = FindInstanceDocument(aInstanceID).get();  // transfer reference
> 
>   if (*aDocument) {
>@@ -1147,16 +1211,26 @@ nsXFormsModelElement::SetStates(nsIXForm
>   if (ns->ShouldDispatchValueChanged()) {
>     nsXFormsUtils::DispatchEvent(element, eEvent_ValueChanged);
>   }
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
>+nsXFormsModelElement::GetStates(nsIDOMNode *aBoundNode, PRInt32 *aStates)
>+{

nit: NS_ENSURE_ARG_POINTER(aStates) needed.

The rest of the code looked ok, though the mElement constructor might have to go back in if you concur with my opinion about storing off the element in the controlaccessor

r- to remove it from my queue and to see the review changes.  Also we need to get that question about 'new' and 'addref' figured out.

Comment 40

[aaronr]

(In reply to comment #37)
> > >-NS_IMPL_ISUPPORTS2(nsXFormsAccessors, nsIXFormsAccessors, nsIClassInfo)
> > >+// nsXFormsAccessors object implementation
> > > 
> > >-void
> > >-nsXFormsAccessors::Destroy()
> > >-{
> > >-  mElement = nsnull;
> > >-  mDelegate = nsnull;
> > >-}
> > 
> > Don't you still need this for nsXFormsControlAccessors?  I mention below why I
> > think we should keep mElement and I'd think that you'd still want to null them
> > out.
> > 
> > I would go back to storing and using mElement.  GetState gets called a lot, I
> > believe, so I personally think that it is worth that small amount of memory to
> > save a QI and function call.
> > 
> 
> It seems to me Allan asked to reduce amount of stored variables. I think
> GetState()
> is not often be called, so it looks like it shouldn't be performance issue.

I was just thinking that GetState gets called on every refresh of almost every control, right?  It gets called any time the xbl code tests isReadonly, isValid, etc. on a control.  And you removed mElement on controlaccessor, but then you re-introduce it in RangeConditionAccessors?  I dunno, not that big of a deal to me.  If no one else minds, I can live without it.

Comment 41

[aaronr]

Comment on attachment 238645 [details] [diff] [review]
patch9

removing the review request since I assume another patch will be coming.  If not, please re-request the review.  Thanks.

Comment 42

[aaronr]

(In reply to comment #39)
> (From update of attachment 224210 [details] [diff] [review] [edit])

> >+
> >+  nsCOMPtr<nsIDOMDocument> firstInstanceDoc =
> >+    FindInstanceDocument(EmptyString());
> >+  NS_ENSURE_STATE(firstInstanceDoc);
> >+
> >+  nsCOMPtr<nsIDOMElement> defaultContextNode;
> >+  firstInstanceDoc->GetDocumentElement(getter_AddRefs(defaultContextNode));
> >+
> >+  nsCOMPtr<nsIDOMXPathResult> xpRes;
> >+  nsXFormsUtils::EvaluateXPath(aXPathExpr, defaultContextNode, mElement,
> >+                               nsIDOMXPathResult::FIRST_ORDERED_NODE_TYPE,
> >+                               getter_AddRefs(xpRes));
> >+  NS_ENSURE_STATE(xpRes);
> >+
> >+  nsCOMPtr<nsIDOMNode> instanceNode;
> >+  xpRes->GetSingleNodeValue(getter_AddRefs(instanceNode));
> >+  NS_ENSURE_STATE(instanceNode);
> >+
> >+  *aAccessors = new nsXFormsAccessors(this, instanceNode);
> >+  if (!*aAccessors)
> >+    return NS_ERROR_OUT_OF_MEMORY;
> >+
> >+  NS_ADDREF(*aAccessors);
> >+  return NS_OK;
> >+} 
> >+
> 
> This doesn't look right.  Doing a new and an addref?  I'm no XPCOM expert, but
> I've never seen this done.  I don't know that the memory will ever be freed. 
> Usually you have a nsCOMPtr, then you initialize it with values, then swap it
> to the return buffer.  You might want to verify that this memory gets freed or
> get Olli, AaronL or someone who knows XPCOM better to say that this is correct.
> 

I verified that the memory does get freed and bz says that what you are doing is perfectly fine, so ignore me.  Hopefully golf will clear my cobwebs!

Comment 43

[alexander :surkov (:asurkov)]

Attachment: patch10

> >+NS_IMETHODIMP
> >+nsXFormsModelElement::GetAccessorsByExpr(const nsAString& aXPathExpr,
> >+                                         nsIXFormsAccessors **aAccessors)

> nit: looks like the style in this file is to line up the a's if the parameters
> are on seperate lines.

Sorry?

> I was just thinking that GetState gets called on every refresh of almost every
> control, right?  It gets called any time the xbl code tests isReadonly,
> isValid, etc. on a control.  And you removed mElement on controlaccessor, but
> then you re-introduce it in RangeConditionAccessors?  I dunno, not that big of
> a deal to me.  If no one else minds, I can live without it.

I returned dom element reference to controlaccessors. Really I haven't preferences
here. If you like, so let it be.

> removing the review request since I assume another patch will be coming.  If
> not, please re-request the review.  Thanks.

I'm trying to care about reviewers :).

Comment 44

[aaronr]

(In reply to comment #43)
> Created an attachment (id=238757) [edit]
> patch10
> 
> > >+NS_IMETHODIMP
> > >+nsXFormsModelElement::GetAccessorsByExpr(const nsAString& aXPathExpr,
> > >+                                         nsIXFormsAccessors **aAccessors)
> 
> > nit: looks like the style in this file is to line up the a's if the parameters
> > are on seperate lines.
> 
> Sorry?
> 

I just meant that the a's in aXPathExpr and aAccessors should line up like the parameters do in other parts of the file, that's all.

With that, r=me

Comment 45

[alexander :surkov (:asurkov)]

Comment on attachment 238757 [details] [diff] [review]
patch10

> I just meant that the a's in aXPathExpr and aAccessors should line up like the
> parameters do in other parts of the file, that's all.

I'll fix it with Olli's comment.

Comment 46

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

Comment on attachment 238757 [details] [diff] [review]
patch10


>-  /** The delegate owning us */
>-  nsIDelegateInternal*   mDelegate;


>+class nsXFormsControlAccessors : public nsXFormsAccessorsBase
>+{
...
>+private:
>+  /* The XTF element for xforms control the accessor is bound to. */
>+  nsCOMPtr<nsIDelegateInternal> mDelegate;
> };


The patch causes new leaks, probably because of the change from 
nsIDelegateInternal* to nsCOMPtr<nsIDelegateInternal>

Comment 47

[alexander :surkov (:asurkov)]

(In reply to comment #46)

> The patch causes new leaks, probably because of the change from 
> nsIDelegateInternal* to nsCOMPtr<nsIDelegateInternal>
> 

Can you give me example? I tried to refresh several times the page that contains couple of xforms elements. After each refresh seamonkey took some memory. But I can see it too without my patch.

Comment 48

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

I tested the patch using XPCOM_MEM_LEAK_LOG (http://www.mozilla.org/performance/leak-tutorial.html)
and a very simple xforms document (containing only one model and one 
control).
It showed that there were new leaks, for example nsXFormsAccessorsBase were leaked. Though, perhaps I should re-test.

Comment 49

[alexander :surkov (:asurkov)]

(In reply to comment #48)
> I tested the patch using XPCOM_MEM_LEAK_LOG
> (http://www.mozilla.org/performance/leak-tutorial.html)
> and a very simple xforms document (containing only one model and one 
> control).
> It showed that there were new leaks, for example nsXFormsAccessorsBase were
> leaked. Though, perhaps I should re-test.
> 

I tried to use XPCOM_MEM_LEAK_LOG=1 (my os is windows). I got only one message related with leaking:

WARNING: LEAKED 399 nsIJSEventListeners
nsStringStats
 => mAllocCount:          68724
 => mReallocCount:         2246
 => mFreeCount:           61690  --  LEAKED 7034 !!!
 => mShareCount:          77691
 => mAdoptCount:           3678
 => mAdoptFreeCount:       3621  --  LEAKED 57 !!!

I can't imagine myself where leaking can happen. I have only one assumption, probably desctructor is called not always and binding doesn't release accessors objects since I think code with nsCOMPtr should be safe. Currently accessors has method Destroy() that is called when control is destroyed. My patch removes Destroy(), probably that's a reason.

Comment 50

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

(In reply to comment #49)
> 
> WARNING: LEAKED 399 nsIJSEventListeners
> nsStringStats
>  => mAllocCount:          68724
>  => mReallocCount:         2246
>  => mFreeCount:           61690  --  LEAKED 7034 !!!
>  => mShareCount:          77691
>  => mAdoptCount:           3678
>  => mAdoptFreeCount:       3621  --  LEAKED 57 !!!
> 
That is not the output of XPCOM_MEM_LEAK_LOG.
And I re-tested with and without the patch and I do see new leaks.
Is it somehow so that nsXFormsAccessorsBase owns the Delegate and Delegate owns the Accessors, so there is a cycle.

Comment 51

[alexander :surkov (:asurkov)]

(In reply to comment #50)

> And I re-tested with and without the patch and I do see new leaks.
> Is it somehow so that nsXFormsAccessorsBase owns the Delegate and Delegate owns
> the Accessors, so there is a cycle.
> 

So, there is idea how to avoid this? Probably I should return Destroy() method that will null delegate's nsCOMPtr?

Comment 52

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

What was the reason to change raw pointer to nsCOMPtr?
Delegate should own the Accessors, but Accessors should not own
Delegate, right?

Comment 53

[alexander :surkov (:asurkov)]

(In reply to comment #52)
> What was the reason to change raw pointer to nsCOMPtr?
> Delegate should own the Accessors, but Accessors should not own
> Delegate, right?
> 

Right. I just liked to remove Destroy() method. I think now that wrongly.

Comment 54

[alexander :surkov (:asurkov)]

(In reply to comment #53)
> (In reply to comment #52)
> > What was the reason to change raw pointer to nsCOMPtr?
> > Delegate should own the Accessors, but Accessors should not own
> > Delegate, right?
> > 
> 
> Right. I just liked to remove Destroy() method. I think now that wrongly.
> 

Or probably should I use desctructor instead of Destroy()?

Comment 55

[alexander :surkov (:asurkov)]

Attachment: patch11

I get back to raw pointers and Destroy() method.

Comment 56

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

Attachment: a testcase for memleaks

With this testcase I still see new memleaks when using the patch.
I don't quite understand why.

Comment 57

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

Comment on attachment 239953 [details]
a testcase for memleaks

No need to click anything. Just load the page and close the browser.

Comment 58

[aaronr]

after using http://www.mozilla.org/performance/refcnt-balancer.html, I found that we are leaking xf:label's with the patch that we are not leaking w/o the patch.  But I still don't know why.

Comment 59

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

Comment on attachment 239911 [details] [diff] [review]
patch11

Clearing the review request, until some information about mem leaks are given.

Comment 60

[alexander :surkov (:asurkov)]

Attachment: patch12

Comment 61

[alexander :surkov (:asurkov)]

Moving to 0.8

Comment 62

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

Comment on attachment 242182 [details] [diff] [review]
patch12

This does still cause some (XForms related) leaks which I don't see without the patch.

For example these are leaked (can be all the same object perhaps):
nsXFormsControlStub
nsXFormsDelegateStub        nsXFormsLabelElement
nsXFormsStubElement 
nsXTFElementWrapper

Comment 63

[aaronr]

moving to 0.9.  Can't checkin until leaks can be identified.

Comment 64

[alexander :surkov (:asurkov)]

Attachment: patch13

updated to trunk
added xpcom cycle collector support
memory leaks are still here, no idea what's wrong

Comment 65

[alexander :surkov (:asurkov)]

Attachment: patch14

getting back to  raw pointers, no memory leaks

Comment 66

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

Comment on attachment 333890 [details] [diff] [review]
patch14

> NS_IMETHODIMP
>-nsXFormsAccessors::GetValue(nsAString &aValue)
>+nsXFormsAccessorsBase::GetValue(nsAString &aValue)
> {
>-  if (mDelegate) {
>-    mDelegate->GetValue(aValue);
>-  } else {
>-    aValue.SetIsVoid(PR_TRUE);
>-  }
>+  nsCOMPtr<nsIDOMNode> instNode;
>+  GetInstanceNode(getter_AddRefs(instNode));
>+  NS_ENSURE_STATE(instNode);
>+
>+  nsXFormsUtils::GetNodeValue(instNode, aValue);
>+
There was some reason to return Void. Are you sure this doesn't regress
anything?

>+nsXFormsAccessorsBase::GetBoundNode(nsIDOMNode **aBoundNode)
> {
>   NS_ENSURE_ARG_POINTER(aBoundNode);
>-  if (mDelegate) {
>-    nsCOMPtr<nsIXFormsControl> control = do_QueryInterface(mDelegate);
>-    return control->GetBoundNode(aBoundNode);
>-  }
>+
>+  nsCOMPtr<nsIDOMNode> instNode;
>+  GetInstanceNode(getter_AddRefs(instNode));
>+  NS_IF_ADDREF(*aBoundNode = instNode);
Just call GetInstanceNode(aBoundNode)

With those addressed, r=me

Comment 67

[alexander :surkov (:asurkov)]

checked in with Olli's comments

Checking in extensions/xforms/nsIModelElementPrivate.idl;
/cvsroot/mozilla/extensions/xforms/nsIModelElementPrivate.idl,v  <--  nsIModelElementPrivate.idl
new revision: 1.29; previous revision: 1.28
done
Checking in extensions/xforms/nsIXFormsControl.idl;
/cvsroot/mozilla/extensions/xforms/nsIXFormsControl.idl,v  <--  nsIXFormsControl.idl
new revision: 1.15; previous revision: 1.14
done
Checking in extensions/xforms/nsIXFormsNSModelElement.idl;
/cvsroot/mozilla/extensions/xforms/nsIXFormsNSModelElement.idl,v  <--  nsIXFormsNSModelElement.idl
new revision: 1.2; previous revision: 1.1
done
Checking in extensions/xforms/nsXFormsAccessors.cpp;
/cvsroot/mozilla/extensions/xforms/nsXFormsAccessors.cpp,v  <--  nsXFormsAccessors.cpp
new revision: 1.7; previous revision: 1.6
done
Checking in extensions/xforms/nsXFormsAccessors.h;
/cvsroot/mozilla/extensions/xforms/nsXFormsAccessors.h,v  <--  nsXFormsAccessors.h
new revision: 1.3; previous revision: 1.2
done
Checking in extensions/xforms/nsXFormsControlStub.cpp;
/cvsroot/mozilla/extensions/xforms/nsXFormsControlStub.cpp,v  <--  nsXFormsControlStub.cpp
new revision: 1.65; previous revision: 1.64
done
Checking in extensions/xforms/nsXFormsControlStub.h;
/cvsroot/mozilla/extensions/xforms/nsXFormsControlStub.h,v  <--  nsXFormsControlStub.h
new revision: 1.48; previous revision: 1.47
done
Checking in extensions/xforms/nsXFormsDelegateStub.cpp;
/cvsroot/mozilla/extensions/xforms/nsXFormsDelegateStub.cpp,v  <--  nsXFormsDelegateStub.cpp
new revision: 1.22; previous revision: 1.21
done
Checking in extensions/xforms/nsXFormsDelegateStub.h;
/cvsroot/mozilla/extensions/xforms/nsXFormsDelegateStub.h,v  <--  nsXFormsDelegateStub.h
new revision: 1.10; previous revision: 1.9
done
Checking in extensions/xforms/nsXFormsModelElement.cpp;
/cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.cpp,v  <--  nsXFormsModelElement.cpp
new revision: 1.164; previous revision: 1.163
done
Checking in extensions/xforms/nsXFormsRangeAccessors.h;
/cvsroot/mozilla/extensions/xforms/nsXFormsRangeAccessors.h,v  <--  nsXFormsRangeAccessors.h
new revision: 1.4; previous revision: 1.3
done
Checking in extensions/xforms/nsXFormsRangeConditionAccessors.cpp;
/cvsroot/mozilla/extensions/xforms/nsXFormsRangeConditionAccessors.cpp,v  <--  nsXFormsRangeConditionAccessors.cpp
new revision: 1.2; previous revision: 1.1
done
Checking in extensions/xforms/nsXFormsRangeConditionAccessors.h;
/cvsroot/mozilla/extensions/xforms/nsXFormsRangeConditionAccessors.h,v  <--  nsXFormsRangeConditionAccessors.h
new revision: 1.2; previous revision: 1.1
done