307093 - enumerating of model instances

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.9a1) Gecko/20050821 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050821 Firefox/1.6a1

Since model can have a number of instances then nsIXFormsModelElement inteface
should be able to enumerate instances. In instance, it can be achived by adding
method like getInstanceDocumentList() or using nsIEnumerator interface.

Reproducible: Always

Comment 1

[aaronr]

Changes to nsIXFormsModelElement interface need to come from the W3C.  We do
have a method in the nsIModelElementPrivate interface to get a list of instances
contained by the model in a nsCOMArray.  Please check it out and let us know if
it meets your needs.

Comment 2

[alexander :surkov (:asurkov)]

I guess it is what I need.

Comment 3

[aaronr]

I'm reopening this bug for a couple of reasons:

1) Gotta keep track and somehow note interfaces that users are going to use so
that we can 'freeze' them at some point or at least make sure they are
appropriately marked so that they don't suddenly go away.

2) Allan, this might be something to bring up with the working group as a 1.1
enhancement to the model interface.  While using DOM a user can get all the
instance elements that were authored, there is no way to currently get ALL of
the instance elements since lazy authored instance elements will probably not
appear in the DOM for all implementations (they wont' for us).

After these items are addressed, we can close this 

Comment 4

[Allan Beaufour]

(In reply to comment #3)
> 2) Allan, this might be something to bring up with the working group as a 1.1
> enhancement to the model interface.  While using DOM a user can get all the
> instance elements that were authored, there is no way to currently get ALL of
> the instance elements since lazy authored instance elements will probably not
> appear in the DOM for all implementations (they wont' for us).

I brought it up at the f2f last week, and got the action item for it. So I have
to define how it should work.

Comment 5

[Allan Beaufour]

*** Bug 309294 has been marked as a duplicate of this bug. ***

Comment 6

[alexander :surkov (:asurkov)]

(In replay to comment 1 of bug 309294)

> If you have any suggestions to
> exactly how the functions should look like / behave, please write it on 
> that bug.

the first approach
interface nsIXFormsModelElement: dom::Element {
  //specs methods
  dom::Document getFirstInstanceDocument();
  dom::Document getNextInstanceDocument();
}

the second approach
interface nsIXFormsModelElement: dom::Element {
  //specs methods
  dom::NodeList getInstanceDocumentsList();
}
NodeList can be replaced by some DocumentList interface (something like)
interface DocumentList: dom::NodeList{
  dom::Document item(in unsigned long index);
}

Comment 7

[alexander :surkov (:asurkov)]

I guess nsIXFormsModelElement should give a way to control instance data. I want
have interface to modify model item properties like it do nsIXFormsDelegate
interface. In instance:

interface nsIXFormsModelElement: dom::Element {
  //specs methods
  nsIXFormsModelItem getModelItem(in string xpath);
  
}

interface nsIXFormsModelItem: dom::Node (?){
  attribute string type;
  attribute string (boolean?) readonly;
  //others model item properties

  attribute string value;
  readonly attribute boolean valid;
}

Now if I want to change instance data value then I should it do by using some
XForms control (by nsIXFormsDelegate interface). If I want to change some model
item property then I should have xforms:bind element and change it attributes
and then update model. I belive it's not right. What do you think?

Comment 8

[Allan Beaufour]

(In reply to comment #7)
> I guess nsIXFormsModelElement should give a way to control instance data. I want
> have interface to modify model item properties like it do nsIXFormsDelegate
> interface. In instance:
> 
> interface nsIXFormsModelElement: dom::Element {
>   //specs methods
>   nsIXFormsModelItem getModelItem(in string xpath);
>   
> }
> 
> interface nsIXFormsModelItem: dom::Node (?){
>   attribute string type;
>   attribute string (boolean?) readonly;
>   //others model item properties
> 
>   attribute string value;
>   readonly attribute boolean valid;
> }
> 
> Now if I want to change instance data value then I should it do by using some
> XForms control (by nsIXFormsDelegate interface). If I want to change some model
> item property then I should have xforms:bind element and change it attributes
> and then update model. I belive it's not right. What do you think?

What you want is the interface described/revised in bug 306764, but just
directly on the instance data node instead of the UI control, right?

Comment 9

[Allan Beaufour]

(In reply to comment #6)
> (In replay to comment 1 of bug 309294)
> 
> > If you have any suggestions to
> > exactly how the functions should look like / behave, please write it on 
> > that bug.
> 
> the first approach
> interface nsIXFormsModelElement: dom::Element {
>   //specs methods
>   dom::Document getFirstInstanceDocument();
>   dom::Document getNextInstanceDocument();
> }

I'm not too fond of that approach, the DOM interfaces I've seen so far have
stateless approaches.

> the second approach
> interface nsIXFormsModelElement: dom::Element {
>   //specs methods
>   dom::NodeList getInstanceDocumentsList();
> }

That seems like the natural way to do it, but I guess we still need to have a
way to figure out the id of the instance document you have actually retrieved?
So we should also have an nsIXFormsInstanceDocument.getInstanceId() function, or
just go through the .getInstanceElement() function from bug 305763, and get the
id from the returned element. What do you say?

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #8)

> What you want is the interface described/revised in bug 306764, but just
> directly on the instance data node instead of the UI control, right?

Right. Thease properties are properties of model. I guess should be possibility
to change them from model. I think sometimes I may not know whay exactly
controls are attached to specified data. It means if I want to change datas then
I should it can do by model.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> That seems like the natural way to do it, but I guess we still need to have a
> way to figure out the id of the instance document you have actually retrieved?
> So we should also have an nsIXFormsInstanceDocument.getInstanceId() function, or
> just go through the .getInstanceElement() function from bug 305763, and get the
> id from the returned element. What do you say?

The first approach. If you have in view thease methods should returns some
nsIXFormsInstanceDocument interface then I agree with you. Something like this:

interface nsIXFormsModelElement: dom::Element {
  //specs methods
  nsIXFormsInstanceDocument getFirstInstanceDocument();
  nsIXFormsInstanceDocument getNextInstanceDocument();
}

interface nsIXFormsInstanceDocument: dom::Document {
  string getInstanceId();
}

What will be looked the second approach?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> What you want is the interface described/revised in bug 306764, but just
> directly on the instance data node instead of the UI control, right?

Yes, like nsIXFormsAccessors but wider. nsIXFormsAccessors provides model
properties as readonly. I guess resembling interface for model should allow to
set properties too.

Comment 13

[Allan Beaufour]

(In reply to comment #12)
> (In reply to comment #8)
> > What you want is the interface described/revised in bug 306764, but just
> > directly on the instance data node instead of the UI control, right?
> 
> Yes, like nsIXFormsAccessors but wider. nsIXFormsAccessors provides model
> properties as readonly. I guess resembling interface for model should allow to
> set properties too.

Model Item Properties are controlled by the model, so they can only be read only.

Comment 14

[Allan Beaufour]

(In reply to comment #11)
> interface nsIXFormsModelElement: dom::Element {
>   //specs methods
>   nsIXFormsInstanceDocument getFirstInstanceDocument();
>   nsIXFormsInstanceDocument getNextInstanceDocument();
> }

I do not like this state-full approach, but yes, the instance document should
implement functionality.

[scriptable]
interface nsIXFormsModelElement : nsISupports
{
  nsIDOMNodeList getInstanceDocuments();
  nsIDOMDocument getInstanceDocument(in DOMString instanceID);
                                          // raises(DOMException)

  void rebuild();
  void recalculate();
  void revalidate();
  void refresh();
};

And instance documents should implement:
[scriptable]
interface nsIXFormsInstanceDocument : nsISupports
{
  readonly attribute DOMString instanceId;
}

That would do it I believe. I do not know if we can easily implement that
attribute on the document though...

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #8)
> > > What you want is the interface described/revised in bug 306764, but just
> > > directly on the instance data node instead of the UI control, right?
> > 
> > Yes, like nsIXFormsAccessors but wider. nsIXFormsAccessors provides model
> > properties as readonly. I guess resembling interface for model should allow to
> > set properties too.
> 
> Model Item Properties are controlled by the model, so they can only be read only.

But I propose interface for model element. Now I should change correspond
attribute for xforms:bind and then update model. I guess such interface will be
useful. 

Comment 16

[Allan Beaufour]

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Yes, like nsIXFormsAccessors but wider. nsIXFormsAccessors provides model
> > > properties as readonly. I guess resembling interface for model should allow to
> > > set properties too.
> > 
> > Model Item Properties are controlled by the model, so they can only be read
only.
> 
> But I propose interface for model element. Now I should change correspond
> attribute for xforms:bind and then update model. I guess such interface will be
> useful. 

IMHO: No way. binds control the MIPs, that's how XForms work.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)

> IMHO: No way. binds control the MIPs, that's how XForms work.

I don't see a contradiction. Please take a look at comment #7. I propose
nsIXFormsModelItem interface like a bind element. Now I don't see a simple way
to change values of MIP. Such interface can. What do you see ugly in that?

Comment 18

[Allan Beaufour]

(In reply to comment #17)
> (In reply to comment #16)
> 
> > IMHO: No way. binds control the MIPs, that's how XForms work.
> 
> I don't see a contradiction. Please take a look at comment #7. I propose
> nsIXFormsModelItem interface like a bind element. Now I don't see a simple way
> to change values of MIP. Such interface can. What do you see ugly in that?

I think we are talking about two different things, or you have some concepts
mixed up.

<bind id="bindid" nodeset="x" readonly="true()"/>, sets the "readonly" Model
Item Property for 1 or more instance data nodes, so that those instance data
nodes will (in theory) have .isReadonly == true.

So what's wrong with document.getElementById("bindid").setAttribute("readonly",
"false()")?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)

> I think we are talking about two different things, or you have some concepts
> mixed up.
> 
> <bind id="bindid" nodeset="x" readonly="true()"/>, sets the "readonly" Model
> Item Property for 1 or more instance data nodes, so that those instance data
> nodes will (in theory) have .isReadonly == true.
> 
> So what's wrong with document.getElementById("bindid").setAttribute("readonly",
> "false()")?

Yes. What's wrong with dynamicaly changing of MIPs?

Comment 20

[Allan Beaufour]

(In reply to comment #19)
> (In reply to comment #18)
> 
> > I think we are talking about two different things, or you have some concepts
> > mixed up.
> > 
> > <bind id="bindid" nodeset="x" readonly="true()"/>, sets the "readonly" Model
> > Item Property for 1 or more instance data nodes, so that those instance data
> > nodes will (in theory) have .isReadonly == true.
> > 
> > So what's wrong with document.getElementById("bindid").setAttribute("readonly",
> > "false()")?
> 
> Yes. What's wrong with dynamicaly changing of MIPs?

Nothing, if you do the above and do it through the binds. The model bindings are
applied to the data. You cannot set properties directly on data.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> Nothing, if you do the above and do it through the binds. The model bindings are
> applied to the data. You cannot set properties directly on data.

So what about interface for MIPs setting through the binds (something like
comment #7)?

Comment 22

[Allan Beaufour]

(In reply to comment #21)
> (In reply to comment #20)
> > Nothing, if you do the above and do it through the binds. The model bindings are
> > applied to the data. You cannot set properties directly on data.
> 
> So what about interface for MIPs setting through the binds (something like
> comment #7)?

It's not needed. You can set the properties by changing the attributes through
the DOM.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> 
> It's not needed. You can set the properties by changing the attributes through
> the DOM.

Will be controls state are changed when I'll change attribute of bind element? 

There is have a sense too when bind element is absent for some instance data. 

I guess it's more comfortable to set some MIPs values as bolean values (not as
xpath string).

Comment 24

[Allan Beaufour]

(In reply to comment #23)
> (In reply to comment #22)
> > 
> > It's not needed. You can set the properties by changing the attributes through
> > the DOM.
> 
> Will be controls state are changed when I'll change attribute of bind element? 

Nope. Binds are static, you need to do a rebuild, etc.
 
> There is have a sense too when bind element is absent for some instance data. 

?

> I guess it's more comfortable to set some MIPs values as bolean values (not as
> xpath string).

Possibly, but it's not XForms.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #24)

Interface is assigned for straight working with data. I can set/get MIPs don't
using bind element obviously. I'm able to set/get data value don't using xforms
controls. Advantages during work with MIPs: 
1) I shouldn't rebuild model.

> > There is have a sense too when bind element is absent for some instance data. 
> ?
2) That means I shouldn't have bind element to set some property


> > I guess it's more comfortable to set some MIPs values as bolean values (not as
> > xpath string).

> Possibly, but it's not XForms.

nsIXFormsDelegate does it.

Comment 26

[Allan Beaufour]

(In reply to comment #25)
> (In reply to comment #24)
> 
> Interface is assigned for straight working with data. I can set/get MIPs don't
> using bind element obviously. I'm able to set/get data value don't using xforms
> controls. Advantages during work with MIPs: 
> 1) I shouldn't rebuild model.
> 
> > > There is have a sense too when bind element is absent for some instance data. 
> > ?
> 2) That means I shouldn't have bind element to set some property

That's possible, but unfortunately that's not XForms.

> > > I guess it's more comfortable to set some MIPs values as bolean values (not as
> > > xpath string).
> 
> > Possibly, but it's not XForms.
> 
> nsIXFormsDelegate does it.

Nope. Delegate returns the state for a MIP, which naturally is boolean. It
cannot set it (it's readonly attributes).

Comment 27

[Allan Beaufour]

Attachment: Patch

This patch creates an nsXFormsInstanceDocuments class that maintains the list
of instances for the model (instead of the nsCOMArray in the model) and
implemen ts nsIDOMNodeList, so it can be returned by the new
getInstanceDocuments(). I modified existing users of model->GetInstanceList to
use this instead.

I'm not happy about the mInstanceDocuments.AddRef()/Release() in
nsXFormsModelElement, but using a nsCOMPtr keeps giving me "ambiguous
nsISupport" errors.

Comment 28

[Allan Beaufour]

Attachment: Testcase

Comment 29

[aaronr]

Comment on attachment 199790 [details] [diff] [review]
Patch


>diff -X patch-excludes -uprN -U8 xforms/nsXFormsModelElement.h xforms.instances/nsXFormsModelElement.h
>--- xforms/nsXFormsModelElement.h	2005-08-05 15:26:39.000000000 +0200
>+++ xforms.instances/nsXFormsModelElement.h	2005-10-12 15:58:36.000000000 +0200

>@@ -203,20 +251,20 @@ private:
>   // This flag indicates whether or not the document fired DOMContentLoaded
>   PRBool mDocumentLoaded;
> 
>   // This flag indicates whether a xforms-rebuild has been called, but no
>   // xforms-revalidate yet
>   PRBool mNeedsRefresh;
> 
>   /**
>-   * List of instance elements contained by this model, including lazy-authored
>+   * All instance documents contained by this model, including lazy-authored
>    * instance elements.
>    */

nit: should probably be, "...including lazy-authored instance documents."

You should probably add a testcase to show that this works with lazy instance,
too.

Patch looks good, though. r=me

Comment 30

[Allan Beaufour]

Attachment: Testcase for lazy instance documents

Includes illegal index testing which triggers a bug in the patch.

Comment 31

[Allan Beaufour]

Attachment: Nit and Item() fix

This fixes your nits, and uses SafeObjectAt() instead of ObjectAt() in Item()

Comment 32

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

Comment on attachment 200068 [details] [diff] [review]
Nit and Item() fix

>diff -X patch-excludes -uprN -U8 xforms/nsIXFormsModelElement.idl xforms.instances/nsIXFormsModelElement.idl
>--- xforms/nsIXFormsModelElement.idl	2005-08-05 15:26:39.000000000 +0200
>+++ xforms.instances/nsIXFormsModelElement.idl	2005-10-11 15:16:10.000000000 +0200
>@@ -34,30 +34,32 @@
>  * 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 nsIDOMDocument;
>+interface nsIDOMNodeList;
> interface nsIDOMDOMException;
> 
> /**
>  * Defines scriptable methods for manipulating instance data
>  * and updating computed and displayed values.
>  *
>  * For more information on this interface please see
>  * http://www.w3.org/TR/xforms/
>  *
>  * @status FROZEN
>  */
>-[scriptable, uuid(e891d132-46f8-464c-b93b-1fb883fa03da)]
>+[scriptable, uuid(85fd60c7-1db7-40c0-ae8d-f723fdd1eea8)]
> interface nsIXFormsModelElement : nsISupports
> {
>+  nsIDOMNodeList getInstanceDocuments();
>   nsIDOMDocument getInstanceDocument(in DOMString instanceID);
>                                           // raises(DOMException)
> 
>   void rebuild();
>   void recalculate();
>   void revalidate();
>   void refresh();
> };

nsIXformsModelElement.idl is FROZEN, you can't change that.
You should create nsIXFormsModelElement2 (or what to call it) interface 
which has nsIDOMNodeList getInstanceDocuments() method



>   /**
>-   * List of instance elements contained by this model, including lazy-authored
>-   * instance elements.
>+   * All instance documents contained by this model, including lazy-authored
>+   * instance documents.
>    */
>-  nsCOMArray<nsIInstanceElementPrivate>    mInstanceList;
>+  nsXFormsModelInstanceDocuments mInstanceDocuments;

I'd prefer nsRefPtr<nsXFormsModelInstanceDocuments> mInstanceDocuments;
Then create the instance document list when it is first time needed.
nsRefPtr addrefs automatically.





> 
>   /** Indicates whether the model's instance was built by lazy authoring */
>   PRBool mLazyModel;
> };
> 
> /**
>  * nsPostRefresh is needed by the UI Controls, which are implemented in
>  * XBL and used inside \<repeat\>. It is needed to refresh the controls,
>diff -X patch-excludes -uprN -U8 xforms/nsXFormsUtilityService.cpp xforms.instances/nsXFormsUtilityService.cpp
>--- xforms/nsXFormsUtilityService.cpp	2005-08-05 15:26:39.000000000 +0200
>+++ xforms.instances/nsXFormsUtilityService.cpp	2005-10-12 13:52:59.000000000 +0200
>@@ -108,70 +108,43 @@ nsXFormsUtilityService::GetModelFromNode
>  * xforms:repeat.
>  */
> NS_IMETHODIMP
> nsXFormsUtilityService::IsNodeAssocWithModel( nsIDOMNode *aNode, 
>                                               nsIDOMNode *aModel,
>                                               PRBool     *aModelAssocWithNode)
> {
> 
>-  nsCOMPtr<nsIDOMElement> element = do_QueryInterface(aNode);
>+  nsCOMPtr<nsIDOMNode> modelNode;
> 
>   nsAutoString namespaceURI;
>   aNode->GetNamespaceURI(namespaceURI);
>-
>   // If the node is in the XForms namespace and XTF based, then it should
>   //   be able to be handled by GetModel.  Otherwise it is probably an instance
>   //   node in a instance document.
>   if (namespaceURI.EqualsLiteral(NS_NAMESPACE_XFORMS)) {
>+    nsCOMPtr<nsIDOMElement> element = do_QueryInterface(aNode);
>     nsCOMPtr<nsIModelElementPrivate> modelPriv = nsXFormsUtils::GetModel(element);
>-    nsCOMPtr<nsIDOMNode> modelNode = do_QueryInterface(modelPriv);
>-
>-    if (modelNode && (modelNode == aModel)) {
>-      *aModelAssocWithNode = PR_TRUE;
>-    } else { 
>-      *aModelAssocWithNode = PR_FALSE;
>-    }
>+    modelNode = do_QueryInterface(modelPriv);
>   } else { 
>     // We are assuming that if the node coming in isn't a proper XForms element,
>     //   then it is an instance element in an instance doc.  Now we just have
>     //   to determine if the given model contains this instance document.
>-    nsCOMPtr<nsIDOMDocument> document;
>-    aNode->GetOwnerDocument(getter_AddRefs(document));
>-    *aModelAssocWithNode = PR_FALSE;
>-
>-    // Guess that we'd better make sure that it is a model
>-    nsCOMPtr<nsIXFormsModelElement> modelEle = do_QueryInterface(aModel);
>-    if (modelEle) {
>-      // OK, we know that this is a model element.  So now we have to go
>-      //   instance element by instance element and find the associated
>-      //   document.  If it is equal to the document that contains aNode,
>-      //   then aNode is associated with this aModel element and we can return
>-      //   true.
>-      nsCOMPtr<nsIModelElementPrivate>model = do_QueryInterface(modelEle);
>-      nsCOMPtr<nsIInstanceElementPrivate> instElement;
>-      nsCOMPtr<nsIDOMDocument> instDocument;
>-
>-      nsCOMArray<nsIInstanceElementPrivate> *instList = nsnull;
>-      model->GetInstanceList(&instList);
>-      NS_ENSURE_TRUE(instList, NS_ERROR_FAILURE);
>-
>-      for (int i = 0; i < instList->Count(); ++i) {
>-        instElement = instList->ObjectAt(i);
>-    
>-        instElement->GetDocument(getter_AddRefs(instDocument));
>-        if (instDocument) {
>-          if (instDocument == document) {
>-            *aModelAssocWithNode = PR_TRUE;
>-            break;
>-          }
>-        }
>-      }
>+    nsCOMPtr<nsIDOMNode> instNode;
>+    nsresult rv =
>+      nsXFormsUtils::GetInstanceNodeForData(aNode, getter_AddRefs(instNode));
>+    if (NS_SUCCEEDED(rv) && instNode) {
>+      instNode->GetParentNode(getter_AddRefs(modelNode));
>     }
>+  }
> 
>+  if (modelNode && (modelNode == aModel)) {
>+    *aModelAssocWithNode = PR_TRUE;
>+  } else {
>+    *aModelAssocWithNode = PR_FALSE;
>   }
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsXFormsUtilityService::GetInstanceDocumentRoot(const      nsAString& aID,
>                                                 nsIDOMNode *aModelNode,
>diff -X patch-excludes -uprN -U8 xforms/nsXFormsUtils.cpp xforms.instances/nsXFormsUtils.cpp
>--- xforms/nsXFormsUtils.cpp	2005-10-19 09:26:19.000000000 +0200
>+++ xforms.instances/nsXFormsUtils.cpp	2005-10-19 13:18:44.000000000 +0200
>@@ -560,20 +560,20 @@ nsXFormsUtils::EvaluateNodeBinding(nsIDO
>               if (colon) {
>                 nsCOMPtr<nsIDOM3Node> dom3node = do_QueryInterface(aElement);
>                 rv = dom3node->LookupNamespaceURI(Substring(expr.get(), colon), 
>                                                   namespaceURI);
>                 NS_ENSURE_SUCCESS(rv, rv);
>               }
> 
>               if (NS_SUCCEEDED(rv)) {
>-                nsCOMArray<nsIInstanceElementPrivate> *instList = nsnull;
>-                (*aModel)->GetInstanceList(&instList);
>-                nsCOMPtr<nsIInstanceElementPrivate> instance = 
>-                  instList->ObjectAt(0);
>+                nsCOMPtr<nsIInstanceElementPrivate> instance;
>+                rv = (*aModel)->FindInstanceElement(EmptyString(),
>+                                                    getter_AddRefs(instance));
>+                NS_ENSURE_SUCCESS(rv, rv);
>                 nsCOMPtr<nsIDOMDocument> domdoc;
>                 instance->GetDocument(getter_AddRefs(domdoc));
>                 nsCOMPtr<nsIDOMElement> instanceDataEle;
>                 nsCOMPtr<nsIDOMNode> childReturn;
>                 rv = domdoc->CreateElementNS(namespaceURI, expr,
>                                              getter_AddRefs(instanceDataEle));
>                 NS_ENSURE_SUCCESS(rv, rv);
>                 nsCOMPtr<nsIDOMElement> instanceRoot;

Comment 33

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

Hmm, what happened. I thought I cut the rest of the patch....

Comment 34

[Allan Beaufour]

Attachment: With Olli's comments

(In reply to comment #32)
> (From update of attachment 200068 [details] [diff] [review] [edit])
> nsIXformsModelElement.idl is FROZEN, you can't change that.
> You should create nsIXFormsModelElement2 (or what to call it) interface 
> which has nsIDOMNodeList getInstanceDocuments() method

You are absolutely right! Done.
 
> >+  nsXFormsModelInstanceDocuments mInstanceDocuments;
> 
> I'd prefer nsRefPtr<nsXFormsModelInstanceDocuments> mInstanceDocuments;
> Then create the instance document list when it is first time needed.
> nsRefPtr addrefs automatically.

That was the solution. Done.

Comment 35

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

Comment on attachment 200337 [details] [diff] [review]
With Olli's comments

> 
> nsXFormsModelElement::nsXFormsModelElement()
>   : mElement(nsnull),
>     mSchemaCount(0),
>     mSchemaTotal(0),
>     mPendingInstanceCount(0),
>     mDocumentLoaded(PR_FALSE),
>     mNeedsRefresh(PR_FALSE),
>-    mInstanceList(16),
>     mLazyModel(PR_FALSE)
> {
>+  mInstanceDocuments = new nsXFormsModelInstanceDocuments();
>+  NS_ASSERTION(mInstanceDocuments, "could not create mInstanceDocuments?!");
> }


Hmm, using |new| in a constructor.
You can't really handle OOM this way.
Maybe create mInstanceDocuments in OnCreate().
Because of OOM you should always check whether 
mInstanceDocuments is null.

With OOM handling,  r=me

Comment 36

[Allan Beaufour]

Attachment: Patch w/proper OOM handling

(In reply to comment #35)
> (From update of attachment 200337 [details] [diff] [review] [edit])
> > 
> > nsXFormsModelElement::nsXFormsModelElement()
> >   : mElement(nsnull),
> >     mSchemaCount(0),
> >     mSchemaTotal(0),
> >     mPendingInstanceCount(0),
> >     mDocumentLoaded(PR_FALSE),
> >     mNeedsRefresh(PR_FALSE),
> >-    mInstanceList(16),
> >     mLazyModel(PR_FALSE)
> > {
> >+  mInstanceDocuments = new nsXFormsModelInstanceDocuments();
> >+  NS_ASSERTION(mInstanceDocuments, "could not create mInstanceDocuments?!");
> > }
> 
> 
> Hmm, using |new| in a constructor.
> You can't really handle OOM this way.
> Maybe create mInstanceDocuments in OnCreate().
> Because of OOM you should always check whether 
> mInstanceDocuments is null.
> 
> With OOM handling,  r=me

I should listen to myself... I wrote it, and thought ... "hmmm, is this really good?". Sorry. Fixed

Comment 37

[Doron Rosenberg (IBM)]

Comment on attachment 200603 [details] [diff] [review]
Patch w/proper OOM handling

Ugh, nsIXFormsModelElement2.

Perhaps nsIXFormsNSModelElement :) (NS being non standard), but 2 is fine as well.

Comment 38

[Allan Beaufour]

Checked in to trunk

Comment 39

[Allan Beaufour]

Attachment: Used nsIXFormsNSModelElement

This is the version checked in.

Comment 40

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

Attachment: Fix for the QI

The patch had wrong QI implementation, and (I think) because of that I get
###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file ../../../dist/include/xpcom/nsCOMPtr.h, line 594
Break: at file ../../../dist/include/xpcom/nsCOMPtr.h, line 594
when testing https://bugzilla.mozilla.org/attachment.cgi?id=200906

Comment 41

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

(In reply to comment #40)
> Created an attachment (id=201029) [edit]
> Fix for the QI
> 
> The patch had wrong QI implementation, and (I think) because of that I get
> ###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file
> ../../../dist/include/xpcom/nsCOMPtr.h, line 594
> Break: at file ../../../dist/include/xpcom/nsCOMPtr.h, line 594
> when testing https://bugzilla.mozilla.org/attachment.cgi?id=200906
> 

Checked in

Comment 42

[aaronr]

checked into MOZILLA_1_8_BRANCH via bug 323691.  Leaving open for now until it gets into 1.8.0

Comment 43

[aaronr]

verified fixed in MOZILLA_1_8_BRANCH