enumerating of model instances

RESOLVED FIXED

Status

Core Graveyard
XForms
--
enhancement
RESOLVED FIXED
12 years ago
a year ago

People

(Reporter: surkov, Assigned: Allan Beaufour)

Tracking

({fixed1.8.0.2, fixed1.8.1})

Trunk
fixed1.8.0.2, fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 years ago
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.
(Reporter)

Comment 2

12 years ago
I guess it is what I need.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → WORKSFORME

Comment 3

12 years ago
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 
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
(Assignee)

Comment 4

12 years ago
(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.
Assignee: aaronr → allan
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 5

12 years ago
*** Bug 309294 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 6

12 years ago
(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);
}
(Reporter)

Comment 7

12 years ago
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?
(Assignee)

Comment 8

12 years ago
(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?
(Assignee)

Comment 9

12 years ago
(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?
(Reporter)

Comment 10

12 years ago
(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.

(Reporter)

Comment 11

12 years ago
(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?
(Reporter)

Comment 12

12 years ago
(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.
(Assignee)

Comment 13

12 years ago
(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.
(Assignee)

Comment 14

12 years ago
(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...
OS: Windows 2000 → All
Hardware: PC → All
(Reporter)

Comment 15

12 years ago
(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. 
(Assignee)

Comment 16

12 years ago
(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.
(Reporter)

Comment 17

12 years ago
(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?
(Assignee)

Comment 18

12 years ago
(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()")?
(Reporter)

Comment 19

12 years ago
(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?
(Assignee)

Comment 20

12 years ago
(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.
(Reporter)

Comment 21

12 years ago
(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)?

(Assignee)

Comment 22

12 years ago
(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.
(Reporter)

Comment 23

12 years ago
(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).
(Assignee)

Comment 24

12 years ago
(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.
(Reporter)

Comment 25

12 years ago
(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.
(Assignee)

Comment 26

12 years ago
(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).
(Assignee)

Comment 27

12 years ago
Created attachment 199790 [details] [diff] [review]
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.
Attachment #199790 - Flags: review?(aaronr)
(Assignee)

Comment 28

12 years ago
Created attachment 199791 [details]
Testcase

Comment 29

12 years ago
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
Attachment #199790 - Flags: review?(aaronr) → review+
(Assignee)

Comment 30

12 years ago
Created attachment 200067 [details]
Testcase for lazy instance documents

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

Comment 31

12 years ago
Created attachment 200068 [details] [diff] [review]
Nit and Item() fix

This fixes your nits, and uses SafeObjectAt() instead of ObjectAt() in Item()
Attachment #199790 - Attachment is obsolete: true
Attachment #200068 - Flags: review?(aaronr)

Updated

12 years ago
Attachment #200068 - Flags: review?(aaronr) → review+
(Assignee)

Updated

12 years ago
Attachment #200068 - Flags: review?(smaug)

Comment 32

12 years ago
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;
Attachment #200068 - Flags: review?(smaug) → review-

Comment 33

12 years ago
Hmm, what happened. I thought I cut the rest of the patch....
(Assignee)

Comment 34

12 years ago
Created attachment 200337 [details] [diff] [review]
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.
Attachment #200068 - Attachment is obsolete: true
Attachment #200337 - Flags: review?(smaug)

Comment 35

12 years ago
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
Attachment #200337 - Flags: review?(smaug) → review+
(Assignee)

Comment 36

12 years ago
Created attachment 200603 [details] [diff] [review]
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
Attachment #200337 - Attachment is obsolete: true
Attachment #200603 - Flags: review?(doronr)
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.
Attachment #200603 - Flags: review?(doronr) → review+
(Assignee)

Comment 38

12 years ago
Checked in to trunk
Status: NEW → ASSIGNED
Whiteboard: xf-to-branch
(Assignee)

Comment 39

12 years ago
Created attachment 200761 [details] [diff] [review]
Used nsIXFormsNSModelElement

This is the version checked in.
Attachment #200603 - Attachment is obsolete: true

Comment 40

12 years ago
Created attachment 201029 [details] [diff] [review]
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
Attachment #201029 - Flags: review?(allan)

Updated

12 years ago
Attachment #201029 - Flags: review?(doronr)

Updated

12 years ago
Attachment #201029 - Flags: review?(allan) → review+
(Assignee)

Updated

12 years ago
Attachment #201029 - Flags: review?(doronr) → review+

Comment 41

12 years ago
(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

12 years ago
checked into MOZILLA_1_8_BRANCH via bug 323691.  Leaving open for now until it gets into 1.8.0
(Assignee)

Updated

12 years ago
Whiteboard: xf-to-branch
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Keywords: fixed1.8.0.2
Resolution: --- → FIXED

Comment 43

11 years ago
verified fixed in MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.