320081 - dynamical instance isn't handled

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1

var model = document.getElementById('model');
var instance = document.createElementNS(XFORMS_NS, "instance");
instance.setAttribute('id', 'lazy2');
model.appendChild(instance);
model.getInstanceDocument('lazy2');

throws an exception NS_ERROR_FAILURE.

What behaviour should be presented? Should I update model or not? Should rebuild() method force getInstanceDocument() to make working?

Reproducible: Always

Comment 1

[aaronr]

Supporting this wouldn't be part of XForms compliance.  I doubt any other processor would support it.  Here is some background:

What happens currently is that getInstanceDocument keys off of the model's internally maintained list of instance elements.  This list is only ever added to when the model is fully populated the first time.  No matter whether you do a rebuild or not, it won't change that fact.

We could probably alter this so that maybe the instance elements add themselves to the list during ::ParentChanged instead, which would allow for your case.  So when the instance is appended as a child of a node, it will add itself to that node's internal list.  We'll just have to make sure that it won't cause any regressions to do that.  We should probably also handle the opposite...if an instance element is removed as a child of the model it should remove itself from the list.

Or we could key off of nsXFormsModelElement::ChildAppended.  That would be better as long as it doesn't fire for subchildren.

What do the rest of you think?  We obviously don't need to support this to be XForms compliant but might be nice to have.

Comment 2

[alexander :surkov (:asurkov)]

I like the case when model catches events about child modifying. This case allows to support instances and binds modifying (bug 316967). I guess support of instance removing would be as well. I cannot find spec information about dynamic behaviour. Probably spec doesn't say anything about it. Or am I wrong? Probably the proposition should be maked clear with w3c members if it broks XForms compliance. What do you think? But I belive dynamic xforms model modifying is a good possiblity.

Comment 3

[aaronr]

(In reply to comment #2)
> I like the case when model catches events about child modifying. This case
> allows to support instances and binds modifying (bug 316967). I guess support
> of instance removing would be as well. I cannot find spec information about
> dynamic behaviour. Probably spec doesn't say anything about it. Or am I wrong?
> Probably the proposition should be maked clear with w3c members if it broks
> XForms compliance. What do you think? But I belive dynamic xforms model
> modifying is a good possiblity.
> 

The spec does not address dynamic updates at all, apart from exposing the model interface so that instance data can be updated by the author and allows the author to rebuild, refresh, etc.  I doubt that dynamic modifications will ever be made part of the spec since scripting support is not a requirement of the host language for XForms.  It will be left to the processors to be compatible with each other with respect to dynamic behavior.  And as far as I know, we are the only ones with even limited support.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3) 
> The spec does not address dynamic updates at all, apart from exposing the model
> interface so that instance data can be updated by the author and allows the
> author to rebuild, refresh, etc.  I doubt that dynamic modifications will ever
> be made part of the spec since scripting support is not a requirement of the
> host language for XForms.  It will be left to the processors to be compatible
> with each other with respect to dynamic behavior.  And as far as I know, we are
> the only ones with even limited support.
> 

Yes, I don't see the dynamic XForms utility if XForms are used instead of web forms. But I see in XForms a technology of data separation from its presentation and I want to use XForms in application. Here dynamic XForms is a great thing.

I agree it's cool if Mozilla XForms is compatible with other XForms processors. But if other processors don't realize dynamic XForms yet (and probably will not) then how can compliance be achived?

Comment 5

[aaronr]

(In reply to comment #4)
> (In reply to comment #3) 
> > The spec does not address dynamic updates at all, apart from exposing the model
> > interface so that instance data can be updated by the author and allows the
> > author to rebuild, refresh, etc.  I doubt that dynamic modifications will ever
> > be made part of the spec since scripting support is not a requirement of the
> > host language for XForms.  It will be left to the processors to be compatible
> > with each other with respect to dynamic behavior.  And as far as I know, we are
> > the only ones with even limited support.
> > 
> 
> Yes, I don't see the dynamic XForms utility if XForms are used instead of web
> forms. But I see in XForms a technology of data separation from its
> presentation and I want to use XForms in application. Here dynamic XForms is a
> great thing.
> 
> I agree it's cool if Mozilla XForms is compatible with other XForms processors.
> But if other processors don't realize dynamic XForms yet (and probably will
> not) then how can compliance be achived?
> 

For example, formsPlayer came up with a really great way to interact with web services and their 1.x processor can do this.  Now this isn't required by the W3C to be XForms 1.0 compliant.  But since this is a useful thing that many people could potentially use, I can see other XForms 1.0 processors, like ourselves, probably supporting this very capability in our 1.0 processors because it will also benefit OUR customers.  And since we are trying to draw as many people to XForms as possible, we'll implement it in such a way that it is compliant with formsPlayer.  This is the way that I see XForms being enhanced as we go forward.  Not every processor may offer every extension to XForms that exists in every other processor, but if two or more processors are trying to achieve the same capability, I think that they will do it so that they will be compatible with the other processors rather than trying to compete with the other processors.  After all, I don't think that anyone is trying to make money off of their processors, but rather off of XForms in general, so it is in everyone's best interest to be compatible with each other as much as they can.

But you must also understand that XForms is meant to be embedded in other host languages and meant to run on all kinds of devices.  XHTML and JavaScript interacting with XForms is just one possible scenario.  Many XForms processors, especially on small devices, may not want or even be able to support JavaScript so this in turn limits the kinds of ideas that the W3C can have in the spec.  For example, a client side XForms processor in a browser like Mozilla may very well want to encourage XForms authors to use a lot of JavaScript interaction with an XForms and allow for dynamic updates when DOM changes take place.  Yet this might not make sense for formsPlayer since they may be limited by the Active-X capabilities in IE which might not notify the plugin that these DOM changes have occurred.  And it might not make sense for server side processors like Chiba to support much JS interaction at all because that might mean a lot more server round trips which hurts their performance.

That is why the W3C has taken the stance that if the author wants to modify the instance data they can, and the W3C even went so far as to produce a model interface to help along that path.  So the author can query the instance data and modify it.  But they also require the author to notify the XForms processor when they are done (by calling rebuild(), revalidate(), etc.) so that the processor doesn't have to support dynamic changes.  This way, processors that can't detect dynamic changes like formsPlayer, will know when to rebuild the instance data.  And this way Chiba can tell the server that it needs to update the instance data and the server can make the required changes and send them back down to the client.

And like I mentioned in a previous reply to this bug, we'll probably fix this bug since it is a requirement from a customer and it can be achieved without a huge impact to our performance.  But be aware that your XForm may only ever work in our processor for the reasons I just mentioned.

Comment 6

[Allan Beaufour]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3) 
> And like I mentioned in a previous reply to this bug, we'll probably fix this
> bug since it is a requirement from a customer and it can be achieved without a
> huge impact to our performance.  But be aware that your XForm may only ever
> work in our processor for the reasons I just mentioned.

Ahem, so IBM customers decides what goes in? :) (sorry, had to react to that)

Nevertheless, I agree with Aaron's novel. I do not see us breaking anything by supporting dynamics, but users can then create forms that only work in Mozilla/Firefox -- but that's up to the form author.

Comment 7

[aaronr]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3) 
> > And like I mentioned in a previous reply to this bug, we'll probably fix this
> > bug since it is a requirement from a customer and it can be achieved without a
> > huge impact to our performance.  But be aware that your XForm may only ever
> > work in our processor for the reasons I just mentioned.
> 
> Ahem, so IBM customers decides what goes in? :) (sorry, had to react to that)
> 

Sorry, when I said that we'd fix it since it is a requirement from a customer, I was using 'customer' in the corporate-speak sense.  That is the IBMer in me :) I meant Mr. Surkov, the user of the Mozilla XForms processor who took the time to open this bug and argue for it.  I wasn't referring to an IBM customer.  Those probably won't come out of the woodwork for a little while, yet.  Not so quick to try the bleeding edge stuff :)

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> 
> And like I mentioned in a previous reply to this bug, we'll probably fix this
> bug since it is a requirement from a customer and it can be achieved without a
> huge impact to our performance.  
> 

That's very good news for me :).

> But be aware that your XForm may only ever
> work in our processor for the reasons I just mentioned.

Yes, we cannot avoid this. But it isn't so important for me because I want to use xforms in applications which implies mozilla using.

Comment 9

[alexander :surkov (:asurkov)]

I guess dynamic instance isssue should include additional methods for instance managing. Something like this:

interface nsIXFormsNSModelElement : nsISupports {
  nsIDOMNodeList getInstanceDocuments();

  nsIDOMDocument appendInstance(in DOMString instanceID, in nsIDOMDocument instanceDoc);
  void removeInstance(in DOMString instanceID);
}

If instanceDoc argument is null then behaviour should be so it will be defined in bug 321876.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: first patch

patch supports "add new instance" feature

Comment 11

[alexander :surkov (:asurkov)]

Attachment: first testcase

Comment 12

[alexander :surkov (:asurkov)]

Attachment: simple patch

patch handles adding/removing instance elements.

When instance is removed then model is not rebuilded automatically. I guess it should.

Comment 13

[alexander :surkov (:asurkov)]

Attachment: simple patch

Comment 14

[alexander :surkov (:asurkov)]

Attachment: simple testcase

Comment 15

[aaronr]

Comment on attachment 208532 [details] [diff] [review]
simple patch

>diff -uNra mozilla.orig/extensions/xforms/nsXFormsInstanceElement.cpp mozilla/extensions/xforms/nsXFormsInstanceElement.cpp
>--- mozilla.orig/extensions/xforms/nsXFormsInstanceElement.cpp	2006-01-13 12:16:18.000000000 +0800
>+++ mozilla/extensions/xforms/nsXFormsInstanceElement.cpp	2006-01-15 14:58:54.000000000 +0800
>@@ -139,7 +139,10 @@
> nsXFormsInstanceElement::OnCreated(nsIXTFGenericElementWrapper *aWrapper)
> {
>   aWrapper->SetNotificationMask(nsIXTFElement::NOTIFY_ATTRIBUTE_SET |
>-                                nsIXTFElement::NOTIFY_ATTRIBUTE_REMOVED);
>+                                nsIXTFElement::NOTIFY_ATTRIBUTE_REMOVED |
>+                                nsIXTFElement::NOTIFY_WILL_CHANGE_PARENT |
>+                                nsIXTFElement::NOTIFY_PARENT_CHANGED |
>+                                nsIXTFElement::NOTIFY_DONE_ADDING_CHILDREN);
> 
>   nsCOMPtr<nsIDOMElement> node;
>   aWrapper->GetElementNode(getter_AddRefs(node));
>@@ -389,6 +392,43 @@
> }
> 
> NS_IMETHODIMP
>+nsXFormsInstanceElement::WillChangeParent(nsIDOMElement *newParent)
>+{
>+  if (!newParent) {
>+    nsCOMPtr<nsIModelElementPrivate> model = GetModel();
>+    if (model != nsnull) {
>+      model->RemoveInstanceElement(this);
>+    }
>+  }
>+
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsInstanceElement::ParentChanged(nsIDOMElement *newParent)
>+{
>+  nsCOMPtr<nsIModelElementPrivate> model = GetModel();
>+  if (model != nsnull) {
>+    return Initialize();
>+  }
>+
>+  return NS_OK; 
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsInstanceElement::DoneAddingChildren()
>+{
>+  nsAutoString src;
>+  mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
>+
>+  if (src.IsEmpty()) {
>+    return CloneInlineInstance();
>+  }
>+
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
> nsXFormsInstanceElement::Initialize()
> {
>   if (mInitialized || !mElement) {
>@@ -451,7 +491,10 @@
>   
>     if (src.IsEmpty()) {
>       // If we don't have a linked external instance, use our inline data.
>-      CloneInlineInstance();
>+      PRBool hasChildren = PR_FALSE;
>+      mElement->HasChildNodes(&hasChildren);
>+      if (hasChildren)
>+        CloneInlineInstance();
>     } else {
>       LoadExternalInstance(src);
>     }

Ok, here are my thoughts about these changes to the nsXFormsInstanceElement class.  I would suggest that we leave the call to Initialize as it currently is -> the responsibility of the model.  However, I think that it would be fine to put a check for @src in nsXFormsInstanceElement::ParentChanged and call LoadExternalInstance(src) from there if @src exists.  Just make sure that you also set mInitialized there if that is the case.  Your change to add ::WillChangeParent is fine, too.  We should remove the instance document from the model in that case.  

I also suggest that we should continue to follow our rule as to dynamic updates to instance elements...inserting nodes UNDER an instance element in the xforms document should NOT be allowed dynamically.  And if it happens, we'll ignore such changes (though we should probably throw an error in the JS console).  If the author wants to insert an instance element dynamically, ok.  But the author should initialize the instance document via model's getInstanceDocument and normal DOM methods and then call rebuild when they are done.  So I don't think that we need the code that you have for nsXFormsInstanceElement::DoneAddingChildren.


>diff -uNra mozilla.orig/extensions/xforms/nsXFormsInstanceElement.h mozilla/extensions/xforms/nsXFormsInstanceElement.h
>--- mozilla.orig/extensions/xforms/nsXFormsInstanceElement.h	2005-11-02 17:37:12.000000000 +0800
>+++ mozilla/extensions/xforms/nsXFormsInstanceElement.h	2006-01-15 14:58:54.000000000 +0800
>@@ -75,6 +75,9 @@
>   NS_IMETHOD OnDestroyed();
>   NS_IMETHOD AttributeSet(nsIAtom *aName, const nsAString &aNewValue);
>   NS_IMETHOD AttributeRemoved(nsIAtom *aName);
>+  NS_IMETHOD WillChangeParent(nsIDOMElement *newParent);
>+  NS_IMETHOD ParentChanged(nsIDOMElement *newParent);
>+  NS_IMETHOD DoneAddingChildren();
>   NS_IMETHOD OnCreated(nsIXTFGenericElementWrapper *aWrapper);
> 

I don't think that you'll need DoneAddingChildren.

>   nsXFormsInstanceElement() NS_HIDDEN;
>diff -uNra mozilla.orig/extensions/xforms/nsXFormsModelElement.cpp mozilla/extensions/xforms/nsXFormsModelElement.cpp
>--- mozilla.orig/extensions/xforms/nsXFormsModelElement.cpp	2005-12-23 09:27:14.000000000 +0800
>+++ mozilla/extensions/xforms/nsXFormsModelElement.cpp	2006-01-15 14:58:55.000000000 +0800
>@@ -370,25 +370,6 @@
> 
>   mInstancesInitialized = PR_TRUE;
> 
>-  nsCOMPtr<nsIDOMNodeList> children;
>-  mElement->GetChildNodes(getter_AddRefs(children));
>-
>-  PRUint32 childCount = 0;
>-  if (children) {
>-    children->GetLength(&childCount);
>-  }
>-
>-  for (PRUint32 i = 0; i < childCount; ++i) {
>-    nsCOMPtr<nsIDOMNode> child;
>-    children->Item(i, getter_AddRefs(child));
>-    if (nsXFormsUtils::IsXFormsElement(child, NS_LITERAL_STRING("instance"))) {
>-      nsCOMPtr<nsIInstanceElementPrivate> instance(do_QueryInterface(child));
>-      if (instance) {
>-        instance->Initialize();
>-      }
>-    }
>-  }
>-
>   // (XForms 4.2.1)
>   // 1. load xml schemas
> 

I think this code should stay in model and not be removed.


Please reply if my comments don't make sense or won't handle the scenarios that you have in mind.  Thanks.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)

> I also suggest that we should continue to follow our rule as to dynamic updates
> to instance elements...inserting nodes UNDER an instance element in the xforms
> document should NOT be allowed dynamically. And if it happens, we'll ignore
> such changes (though we should probably throw an error in the JS console).  If
> the author wants to insert an instance element dynamically, ok. 

I agree. All elements is placed inside of instance element are used only for instance document initialization (as default value).

>  But the author
> should initialize the instance document via model's getInstanceDocument and
> normal DOM methods and then call rebuild when they are done.  So I don't think
> that we need the code that you have for
> nsXFormsInstanceElement::DoneAddingChildren.

DoneAddingChildren() method is not called when instance element children are changed dynamically. There is one reason why I use it. I wanted to move all intializing of instance element to nsXFormsInstanceElement object.

DoneAddingChildren() is called when xforms model is loading only. ParentChanged() method is called before DoneAddingChildren() method i.e. when I try to initialize instance then children of instance element is not appended yet. Although I don't like a much DoneAddingChildren() in this case.


> Ok, here are my thoughts about these changes to the nsXFormsInstanceElement
> class.  I would suggest that we leave the call to Initialize as it currently is
> -> the responsibility of the model.  

I see the one way to leave initial instance intializing in nsXFormsModelElement. nsXFormsInstanceElement::ParentChanged() should initialize instance as it does and nsXFormsModelElement::DoneAddingChildren() should call CloneInlineInstance() when it is needed.

If you see other way then please let me know. If above way is what do you have in view then let me know and I create new patch.

I have one more question. When instance element is removed from model then how should bound controls and bind elements behave?

Comment 17

[aaronr]

> >  But the author
> > should initialize the instance document via model's getInstanceDocument and
> > normal DOM methods and then call rebuild when they are done.  So I don't think
> > that we need the code that you have for
> > nsXFormsInstanceElement::DoneAddingChildren.
> 
> DoneAddingChildren() method is not called when instance element children are
> changed dynamically. There is one reason why I use it. I wanted to move all
> intializing of instance element to nsXFormsInstanceElement object.
> 

Why move initialization of the instance? Initialize() does 3 things:
1) handles lazy instance
2) clones inline content
3) handles @src

I'm sorry, I don't see where it makes sense to handle 1 or 2 under ::ParentChanged().  And neither of them are impacted by dynamic update.  So why not leave them under model, where they can be handled more efficiently?  Let's assume that we use my proposal of leaving instance initialization the way it is and we add handling in ::ParentChanged() that will call LoadExternalInstance() if @src is detected.  What scenario do you have in mind that won't work in this situation?

> I have one more question. When instance element is removed from model then how
> should bound controls and bind elements behave?
> 

You need to make sure that a rebuild, recalculate, revalidate, refresh happens on the model.  This will cause the controls to rebind.  But you'll have to verify that xf:bind's will also behave properly.  I believe that they will, though.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)

> Why move initialization of the instance? Initialize() does 3 things:
> 1) handles lazy instance
> 2) clones inline content
> 3) handles @src
> 
> I'm sorry, I don't see where it makes sense to handle 1 or 2 under
> ::ParentChanged().  And neither of them are impacted by dynamic update.

I guess it makes sence to handle 2 for dynamic instances. For example:

var instance = document.createElementNS(XFORMS_NS, "instance");
var root = document.createElementNS("", "rootElm");
var child = document.createElementNS("", "child");
root.appendChild(child);
instance.appendChild(root);
model.appendChild(instance);

>  So why
> not leave them under model, where they can be handled more efficiently?  Let's
> assume that we use my proposal of leaving instance initialization the way it is
> and we add handling in ::ParentChanged() that will call LoadExternalInstance()
> if @src is detected.  What scenario do you have in mind that won't work in this
> situation?
> 

I guess all scenaios should work. So let's model handles the 1 thing.


> > I have one more question. When instance element is removed from model then how
> > should bound controls and bind elements behave?
> > 
> 
> You need to make sure that a rebuild, recalculate, revalidate, refresh happens
> on the model.  This will cause the controls to rebind.  But you'll have to
> verify that xf:bind's will also behave properly.  I believe that they will,
> though.
> 

Who should a rebuild and e.t.c do: the patch or user?

Comment 19

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 20

[aaronr]

(In reply to comment #18)
> (In reply to comment #17)
> 
> > Why move initialization of the instance? Initialize() does 3 things:
> > 1) handles lazy instance
> > 2) clones inline content
> > 3) handles @src
> > 
> > I'm sorry, I don't see where it makes sense to handle 1 or 2 under
> > ::ParentChanged().  And neither of them are impacted by dynamic update.
> 
> I guess it makes sence to handle 2 for dynamic instances. For example:
> 
> var instance = document.createElementNS(XFORMS_NS, "instance");
> var root = document.createElementNS("", "rootElm");
> var child = document.createElementNS("", "child");
> root.appendChild(child);
> instance.appendChild(root);
> model.appendChild(instance);
> 

But how does the processor know when to clone?  Once the instance is inserted into the model?  What if an empty instance is inserted into the model and then the script author inserts children under the instance?  Do we ignore that?  I personally think that we should ignore ALL children added under instance elements in the document.  We should allow the script author to add an instance element, then the script author can use getInstanceDocument() to get the newly created instance document and populate the instance document that way.  Then if they want to restore the original values in the instance document, they can listen for the xforms-reset event on the model and reset the instance by script.  But that is just my own opinion.  What does everyone else think?

What should we do if there is no model in the original document at all and then during xforms-model-construct-done the script inserts a model and instance into the document...should we still do the lazy authoring of model and instance that xforms-model-construct-done processing suggests?

> >  So why
> > not leave them under model, where they can be handled more efficiently?  Let's
> > assume that we use my proposal of leaving instance initialization the way it is
> > and we add handling in ::ParentChanged() that will call LoadExternalInstance()
> > if @src is detected.  What scenario do you have in mind that won't work in this
> > situation?
> > 
> 
> I guess all scenaios should work. So let's model handles the 1 thing.
> 
> 
> > > I have one more question. When instance element is removed from model then how
> > > should bound controls and bind elements behave?
> > > 
> > 
> > You need to make sure that a rebuild, recalculate, revalidate, refresh happens
> > on the model.  This will cause the controls to rebind.  But you'll have to
> > verify that xf:bind's will also behave properly.  I believe that they will,
> > though.
> > 
> 
> Who should a rebuild and e.t.c do: the patch or user?
> 

If the instance is removed by script, then I would say that the script author should do the rebuild.  Basically if the script author changes the instance data in ANY way, then the script author is responsible for making sure that rebuild, recalculate, revalidate and refresh get called as needed.  That way they can do it as efficiently as possible, running them just once when he is all done with his changes to that model.  If the processor had to try to anticipate when to do all of these, we would end up doing way too many.

Comment 21

[aaronr]

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

I'm removing the request for me to review this patch until we make a final decision how this is supposed to work.  We need some feedback from smaug and allan, at least, to get their opinion.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> 
> But how does the processor know when to clone?  Once the instance is inserted
> into the model?  

If instance has children while it is inserting into model then why shouldn't children be used as default instance document?

> What if an empty instance is inserted into the model and then
> the script author inserts children under the instance?  Do we ignore that?

Surely dynamical insetions of children is not default children of instance.

>  I
> personally think that we should ignore ALL children added under instance
> elements in the document.  We should allow the script author to add an instance
> element, then the script author can use getInstanceDocument() to get the newly
> created instance document and populate the instance document that way.  Then if
> they want to restore the original values in the instance document, they can
> listen for the xforms-reset event on the model and reset the instance by
> script.  But that is just my own opinion.  What does everyone else think?

Thus I don't get one's way and probably we should ignore children when instance is appended dynamically.

> What should we do if there is no model in the original document at all and then
> during xforms-model-construct-done the script inserts a model and instance into
> the document...should we still do the lazy authoring of model and instance that
> xforms-model-construct-done processing suggests?

Why not? I guess we should. But when model is dynamically inserted then I think we shouldn't.

Comment 23

[alexander :surkov (:asurkov)]

There is a question. Why when lazy instance document is created then it is backuped (http://lxr.mozilla.org/mozilla/source/extensions/xforms/nsXFormsInstanceElement.cpp#439)?  It will be backuped in any way when "xforms-ready" event will be handled.

Also lazy instance document creating code dublicates nsXFormsInstanceElement::CreateInstanceDocument() method. Should lazy instance document creating use the nsXFormsInstanceElement::CreateInstanceDocument() method?

Comment 24

[aaronr]

(In reply to comment #23)
> There is a question. Why when lazy instance document is created then it is
> backuped
> (http://lxr.mozilla.org/mozilla/source/extensions/xforms/nsXFormsInstanceElement.cpp#439)?
>  It will be backuped in any way when "xforms-ready" event will be handled.
> 

Lazy instance isn't 'backed up' when it is created.  When the lazy instance is created the document which will store the backup is created, just like a normal instance.  Then during xforms-ready handling the data in the instance document will be cloned into the backup document.

> Also lazy instance document creating code dublicates
> nsXFormsInstanceElement::CreateInstanceDocument() method. Should lazy instance
> document creating use the nsXFormsInstanceElement::CreateInstanceDocument()
> method?
> 

Yeah, I guess the lazy instance code should probably call CreateInstanceDocument() and THEN append the instanceData element as the root of the instance document.  Good catch.

Comment 25

[aaronr]

(In reply to comment #22)
> (In reply to comment #20)
> > 
> > But how does the processor know when to clone?  Once the instance is inserted
> > into the model?  
> 
> If instance has children while it is inserting into model then why shouldn't
> children be used as default instance document?
> 

I agree with yout, I think that it does make sense.  But my concern is that a script author will get the backup/restore capability if they do dynamic instance data update BEFORE the instance element is inserted into the model but not if they do it AFTER the instance element is inserted into the model.  This is a big difference that might not be noticed as a comment in an interface.  But it is ok with me if we are willing to have this limitation.

> > What should we do if there is no model in the original document at all and then
> > during xforms-model-construct-done the script inserts a model and instance into
> > the document...should we still do the lazy authoring of model and instance that
> > xforms-model-construct-done processing suggests?
> 
> Why not? I guess we should. But when model is dynamically inserted then I think
> we shouldn't.
> 

I disagree here.  Lazy instance is meant to be a convenience for form authors so that they don't have to set up instance structure beforehand and alter it as they further define the form.  I think that if an author is advanced enough and the form complex enough to have script creating dynamic instance elements, then they lose lazy authoring.  So I think that lazy authoring should be kept as it is...if an instance document exists when we go to bind controls, then they better fit into the structure of the instance document or they will generate binding exceptions.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)

> I agree with yout, I think that it does make sense.  But my concern is that a
> script author will get the backup/restore capability if they do dynamic
> instance data update BEFORE the instance element is inserted into the model but
> not if they do it AFTER the instance element is inserted into the model.  This
> is a big difference that might not be noticed as a comment in an interface. 
> But it is ok with me if we are willing to have this limitation.

Is it mean instance element children will be used for current instance document when instance element is inserted into model? When thease children will be used for backup document creating? Should the sctipt author fire "xforms-ready" event? If the answer is yes then can I ask you for review after I'll file a patch taking into account the comment 23 and comment 24?

> 
> > > What should we do if there is no model in the original document at all and then
> > > during xforms-model-construct-done the script inserts a model and instance into
> > > the document...should we still do the lazy authoring of model and instance that
> > > xforms-model-construct-done processing suggests?
> > 
> > Why not? I guess we should. But when model is dynamically inserted then I think
> > we shouldn't.
> > 
> 
> I disagree here.  Lazy instance is meant to be a convenience for form authors
> so that they don't have to set up instance structure beforehand and alter it as
> they further define the form.  I think that if an author is advanced enough and
> the form complex enough to have script creating dynamic instance elements, then
> they lose lazy authoring.  So I think that lazy authoring should be kept as it
> is...if an instance document exists when we go to bind controls, then they
> better fit into the structure of the instance document or they will generate
> binding exceptions.
> 

In any way I guess this issue is bug 313858 target.

Comment 27

[aaronr]

So I think what the outcome Alexander and I agreed to is that if the dynamically inserted instance has children BEFORE it is inserted into the model, then this instance should be backed up as the default instance.

No, I don't think that the script author should EVER fire xforms-ready and if they do, our processor shouldn't ever handle the event.  It should be for notification only and should only be generated by the processor.  If the form author wants automatic backup and restore of the dynamic instance, then the form author needs to get their instance in the model before model-construct-done processing happens on that model OR the patch needs to setup the backup and restore stuff for the instance document if it gets created dynamically after that point.  But again, only the child nodes that are in the instance element when it is inserted into the model will be backed up.  Please make sure that you comment this in your code and in the interface.

And before you put together the patch, we might want beaufour or smaug to sign off on this new behavior, too.  Just to make sure that they can't come up with some issues that I might have missed.

beaufour or smaug, please mark this bug as 'new' or 'assigned' (to surkov) if/when you agree that this new behavior is ok.  Thanks.

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #27)
> If the form
> author wants automatic backup and restore of the dynamic instance, then the
> form author needs to get their instance in the model before
> model-construct-done processing happens on that model OR the patch needs to
> setup the backup and restore stuff for the instance document if it gets created
> dynamically after that point.  

How the form author can make non automaitc backup and restore?

Though I prefer the second way - patch should setup the backup and restore stuff. But I can't see a way how it can be achived. Should I use "xforms-ready" event for backup stuff for dynamic instance?

The case if I should is next. The patch should handle internal instances only since "xforms-ready" event is fired when data of external instance is loaded. When I should dispatch "xforms-ready" event for dynamic internal instance?

The case if I shouldn't is next. "xforms-ready" event shouldn't be fired when instance data for external dynamic instance is loaded. The problem is we should differentiate external dynamic instance from external no dynamic instance.

What the way should I choose for backup stuff of dynamic instance?

Comment 29

[aaronr]

(In reply to comment #28)
> (In reply to comment #27)
> > If the form
> > author wants automatic backup and restore of the dynamic instance, then the
> > form author needs to get their instance in the model before
> > model-construct-done processing happens on that model OR the patch needs to
> > setup the backup and restore stuff for the instance document if it gets created
> > dynamically after that point.  
> 
> How the form author can make non automaitc backup and restore?
>
The form author knows what the original data is because they authored the form and are the ones populating the instance document.  To restore, they just need to put a listener on the model for the 'xforms-reset' event and then set the instance document back to the original data.
> 
> Though I prefer the second way - patch should setup the backup and restore
> stuff. But I can't see a way how it can be achived. Should I use "xforms-ready"
> event for backup stuff for dynamic instance?

No, you should never fire xforms-ready outside of the process that we already use to fire xforms-ready.  xforms-ready should only ever fire once on a model in a document.  In your patch, if you detect that a dynamic instance is added to the model after the xforms-ready event has been fired, then you'll need to call the backup on the instance via nsXFormsInstanceElement::BackupOriginalDocument().  As long as you do this and your patch adds the dynamic instance to the model's list of instances, then restore should work automatically.  Of course, test this to be sure.

> 
> The case if I should is next. The patch should handle internal instances only
> since "xforms-ready" event is fired when data of external instance is loaded.
> When I should dispatch "xforms-ready" event for dynamic internal instance?
> 
> The case if I shouldn't is next. "xforms-ready" event shouldn't be fired when
> instance data for external dynamic instance is loaded. The problem is we should
> differentiate external dynamic instance from external no dynamic instance.
> 
> What the way should I choose for backup stuff of dynamic instance?
> 

Up to this point, I've only ever considered dynamic internal instance.  A dynamic external instance is another big problem.  Because you've got to make sure that the backup doesn't happen until the instance is populated from the external source.  You also have to make sure that no controls try to bind to the new dynamic instance until after it is all set up (loaded from the external resource) otherwise you'll create xforms-binding-exception's.  And keep in mind that if controls in the form try to bind to an instance that isn't already there prior to xforms-model-construct-done's default processing (when the controls in the form try to bind for the first time), then this will generate xforms-binding-exceptions which are fatal errors.  Once this happens, xforms-ready may NEVER fire.  Maybe we should not process any dynamic instances until after xforms-ready to make sure that we don't introduce timing problems that could keep xforms-ready from properly firing.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #29)

> And keep in mind
> that if controls in the form try to bind to an instance that isn't already
> there prior to xforms-model-construct-done's default processing (when the
> controls in the form try to bind for the first time), then this will generate
> xforms-binding-exceptions which are fatal errors.  Once this happens,
> xforms-ready may NEVER fire. Maybe we should not process any dynamic instances
> until after xforms-ready to make sure that we don't introduce timing problems
> that could keep xforms-ready from properly firing.

I like the approach: dynamic instances are ignored until model is ready.

> You also have to make sure that no controls try to bind to
> the new dynamic instance until after it is all set up (loaded from the external
> resource) otherwise you'll create xforms-binding-exception's.  

I prefer the way if dynamic external instance isn't loaded then control should wait for instance loading. Why is exception needed?

> 
> Up to this point, I've only ever considered dynamic internal instance.  A
> dynamic external instance is another big problem.  Because you've got to make
> sure that the backup doesn't happen until the instance is populated from the
> external source. 

Probably should we create another bug for external dynamic instnaces supporting?
Now I'm getting exigency in internal dynamic instances only.

Comment 31

[alexander :surkov (:asurkov)]

I have a question. When nsXFormsInstanceElement::LoadExternalInstance() is called then nsModelElementPrivate::InstanceLoadStarted() is called after nsIChannel::AsyncOpen(). I guess there is theoretical case when AsyncOpen() will executed before then InstanceLoadStarted() is called. If it is right then InstanceLoadStarted() should be called before AsynOpen() call. Is there such theoretical case?

Comment 32

[aaronr]

(In reply to comment #31)
> I have a question. When nsXFormsInstanceElement::LoadExternalInstance() is
> called then nsModelElementPrivate::InstanceLoadStarted() is called after
> nsIChannel::AsyncOpen(). I guess there is theoretical case when AsyncOpen()
> will executed before then InstanceLoadStarted() is called. If it is right then
> InstanceLoadStarted() should be called before AsynOpen() call. Is there such
> theoretical case?
> 

biesi said that AsyncOpen should definitely return before OnStopRequest is called.  And that if it doesn't happen like that, then it is a bug.  But he also said that such bug have been known to exist :=)

Comment 33

[aaronr]

(In reply to comment #30)
> (In reply to comment #29)
> 
> > And keep in mind
> > that if controls in the form try to bind to an instance that isn't already
> > there prior to xforms-model-construct-done's default processing (when the
> > controls in the form try to bind for the first time), then this will generate
> > xforms-binding-exceptions which are fatal errors.  Once this happens,
> > xforms-ready may NEVER fire. Maybe we should not process any dynamic instances
> > until after xforms-ready to make sure that we don't introduce timing problems
> > that could keep xforms-ready from properly firing.
> 
> I like the approach: dynamic instances are ignored until model is ready.

Make sure that you document this limitation clearly in your patch so that we always remember that we did this on purpose and why we limit it this way.

> 
> > You also have to make sure that no controls try to bind to
> > the new dynamic instance until after it is all set up (loaded from the external
> > resource) otherwise you'll create xforms-binding-exception's.  
> 
> I prefer the way if dynamic external instance isn't loaded then control should
> wait for instance loading. Why is exception needed?

That is per spec.  If a control binds to an instance node that isn't there, then we throw a binding exception.  What save this from producing unnessessary binding exceptions is lazy authoring and the fact that controls don't try to bind until xforms-model-construct-done default processing.  But with dynamic instances, neither of these apply.  I guess that if you determine that a control is trying to bind to a dynamic external instance that hasn't finished loading yet, then you could queue them up to bind after the dynamic external instance is loaded, but you'll have to make sure that you don't lose track of them.  They need to generate the binding exceptions if they still can't bind after the instance is done loading.

> 
> > 
> > Up to this point, I've only ever considered dynamic internal instance.  A
> > dynamic external instance is another big problem.  Because you've got to make
> > sure that the backup doesn't happen until the instance is populated from the
> > external source. 
> 
> Probably should we create another bug for external dynamic instnaces
> supporting?
> Now I'm getting exigency in internal dynamic instances only.
> 

that is fine with me if you want to handle it in a seperate bug.  Just make sure that you clearly mark with XXX or whatever to show that we won't support dynamic external instances, yet.

Comment 34

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 35

[Allan Beaufour]

(In reply to comment #34)
> Created an attachment (id=210556) [edit]
> patch
> 

I'll do a proper review soon, but please do not compare pointers with "nsnull". Check your patch with:
http://beaufour.dk/jst-review/?patch=210556

Comment 36

[alexander :surkov (:asurkov)]

Attachment: patch

with allan comment

Comment 37

[Allan Beaufour]

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

> +++ mozilla/extensions/xforms/nsIModelElementPrivate.idl	2006-02-04 12:19:01.000000000 +0800
> +  /**
> +   * Return true if xforms-ready event has been fired.
> +   */
> +  PRBool isReady();

you should use "boolean" in IDL files, and with something like that you should
use an attribute:
readonly attribute boolean isReady;

> +++ mozilla/extensions/xforms/nsXFormsInstanceElement.cpp	2006-02-04 12:19:02.000000000 +0800
> +nsXFormsInstanceElement::WillChangeParent(nsIDOMElement *newParent)

Arguments should start with 'a', so "newParent" => "aNewParent"

> +nsXFormsInstanceElement::ParentChanged(nsIDOMElement *newParent)

aNewParent

 
> +  // If model isn't loaded entirely (It means xforms-ready event hasn't been
> +  // fired) then the instance will be initialized by model and will backed up
"will be backup up"

> +  // when xforms-ready event is fired.
> +  PRBool isready;
> +  model->IsReady(&isready);

Something could go wrong in the method call, and isready might not be initialized then, so you need to initialize isready (to PR_FALSE)

> +  if (!isready) return NS_OK;

Please but "return NS_OK;" on a new line

> +
> +  // If model is loaded then the instance is inserted dynamically. The instance
"If the model is loaded and ready then ..."

> +  // should create instance document and back up it.

"should then create the instance document and back it up."

> +
> +  // Probably dynamic instances should be handled too when model isn't loaded
> +  // entirely (for more information see a comment 29 of bug 320081
> +  // https://bugzilla.mozilla.org/show_bug.cgi?id=320081#c29).

I do not understand this comment.

> +
> +  nsAutoString src;
> +  mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
> +

**

> +  if (src.IsEmpty()) {
> +    // If we don't have a linked external instance, use our inline data.
> +    PRBool hasChildren = PR_FALSE;
> +    mElement->HasChildNodes(&hasChildren);
> +    if (hasChildren) {
> +    nsresult rv = CloneInlineInstance();
> +    if (NS_FAILED(rv))
> +      return rv;

the indentation is wrong in the above.

> +    return BackupOriginalDocument();
> +    }
> +  } else {
> +    // XXX: external dynamic instances isn't handled (see a bug 325684
> +    // https://bugzilla.mozilla.org/show_bug.cgi?id=325684)
> +  }
> +
> +  return NS_OK;

Do an "early return" for this. That is, do this at ** above:

if (!src.IsEmpty()) {
  // XXX: external dynamic instances isn't handled (see a bug 325684
  // https://bugzilla.mozilla.org/show_bug.cgi?id=325684)
  return NS_OK;
}
and then continue with the "src.IsEmpty()" handling

That makes the code easier to read.

> +nsXFormsInstanceElement::Initialize()
> +{
>    mElement->HasAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_MOZ_XFORMS_LAZY),
>                             NS_LITERAL_STRING("lazy"), &mLazy);

> -  // Lazy instance
>    if (mLazy) {
> -    nsCOMPtr<nsIDOMDocument> domDoc;
> -    mElement->GetOwnerDocument(getter_AddRefs(domDoc));
> -    NS_ENSURE_STATE(domDoc);
> -  
> -    nsCOMPtr<nsIDOMDOMImplementation> domImpl;
> -    nsresult rv = domDoc->GetImplementation(getter_AddRefs(domImpl));
> -    NS_ENSURE_SUCCESS(rv, rv);
> -  
> -    nsCOMPtr<nsIDOMDocument> newDoc;
> -    rv = domImpl->CreateDocument(EmptyString(), EmptyString(), nsnull,
> -                                 getter_AddRefs(newDoc));
> -    NS_ENSURE_SUCCESS(rv, rv);
> -  
> -    rv = SetDocument(newDoc);
> -    NS_ENSURE_SUCCESS(rv, rv);
> -  
> -    // Lazy authored instance documents have a root named "instanceData"
> -    nsCOMPtr<nsIDOMElement> instanceDataElement;
> -    nsCOMPtr<nsIDOMNode> childReturn;
> -    rv = mDocument->CreateElementNS(EmptyString(), 
> -                                    NS_LITERAL_STRING("instanceData"),
> -                                    getter_AddRefs(instanceDataElement));
> -    NS_ENSURE_SUCCESS(rv, rv);
> -    rv = mDocument->AppendChild(instanceDataElement, getter_AddRefs(childReturn));
> -    NS_ENSURE_SUCCESS(rv, rv);
> -  
> -    // I don't know if not being able to create a backup document is worth
> -    // failing this function.  Since it probably won't be used often, we'll
> -    // let it slide.  But it probably does mean that things are going south
> -    // with the browser.
> -    domImpl->CreateDocument(EmptyString(), EmptyString(), nsnull,
> -                            getter_AddRefs(mOriginalDocument));
> -    NS_WARN_IF_FALSE(mOriginalDocument, "Couldn't create mOriginalDocument!!");
> +    // Lazy instance
> +    nsresult rv = CreateInstanceDocument(NS_LITERAL_STRING("instanceData"));
> +    if (NS_FAILED(rv))
> +      return rv; // don't warn, we might just not be in the document yet
>
>    } else {

this else is mostly useless. With an early return, you could rewrite the entire function to this:

  mElement->HasAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_MOZ_XFORMS_LAZY),
                           NS_LITERAL_STRING("lazy"), &mLazy);
  if (mLazy) // Lazy instance
    return CreateInstanceDocument(NS_LITERAL_STRING("instanceData"));

  // Normal instance
  nsAutoString src;
  mElement->GetAttribute(NS_LITERAL_STRING("src"), src);

  return src.IsEmpty() ? CloneInlineInstance() : LoadExternalInstance(src);

>      // Normal instance
> -
> -    // By the time this is called, we should be inserted in the document and
> -    // have all of our child elements, so this is our first opportunity to
> -    // create the instance document.
> -  
>      nsAutoString src;
>      mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
> -  
> +
>      if (src.IsEmpty()) {
> -      // If we don't have a linked external instance, use our inline data.
> -      CloneInlineInstance();
> +      return CloneInlineInstance();
>      } else {
>        LoadExternalInstance(src);
>      }

> +++ mozilla/extensions/xforms/nsXFormsInstanceElement.h	2006-02-04 12:19:02.000000000 +0800

> +  NS_IMETHOD WillChangeParent(nsIDOMElement *newParent);
> +  NS_IMETHOD ParentChanged(nsIDOMElement *newParent);

Remember to change the argument names here too.

> +++ mozilla/extensions/xforms/nsXFormsModelElement.cpp	2006-02-04 12:19:02.000000000 +0800
 
>  already_AddRefed<nsIDOMDocument>
> @@ -1474,7 +1481,8 @@
>    for (i = 0; i < models->Count(); ++i) {
>      nsXFormsModelElement *model =
>          NS_STATIC_CAST(nsXFormsModelElement *, models->ElementAt(i));
> -    nsXFormsUtils::DispatchEvent(model->mElement, eEvent_Ready);  
> +    nsXFormsUtils::DispatchEvent(model->mElement, eEvent_Ready);
> +    model->mIsReady = PR_TRUE;

I would rather see that the model sets this itself, like Aaron does in bug 300255 (mReadyHandled) -- which will clash with this anyhow, so we need to sync it :)

> +++ mozilla/extensions/xforms/nsXFormsModelElement.h	2006-02-04 12:19:02.000000000 +0800
 
> +  // This flag indicates whether xfors-ready event has been fired

xfors -> xforms
Need to be synced with Aaron's patch though...

Comment 38

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 39

[alexander :surkov (:asurkov)]

In reply (In reply to comment #37)

> > +
> > +  // Probably dynamic instances should be handled too when model isn't loaded
> > +  // entirely (for more information see a comment 29 of bug 320081
> > +  // https://bugzilla.mozilla.org/show_bug.cgi?id=320081#c29).

> I do not understand this comment.

Aaron in comment#29 wrote:

"And keep in mind
that if controls in the form try to bind to an instance that isn't already
there prior to xforms-model-construct-done's default processing (when the
controls in the form try to bind for the first time), then this will generate
xforms-binding-exceptions which are fatal errors.  Once this happens,
xforms-ready may NEVER fire.  Maybe we should not process any dynamic instances
until after xforms-ready to make sure that we don't introduce timing problems
that could keep xforms-ready from properly firing."

I just want to say: if somebody needs to support dynamic instances before xforms-ready event then he can do it because the current behaviour is not solely right.

Comment 40

[Allan Beaufour]

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

Only two notices that you can happily ignore for this patch. Just information from my part :)

>+  PRBool isready;
>+  model->GetIsReady(&isready);
>+  if (!isready)
>+    return NS_OK;

Strictly speaking you should check the return value of the function call, or at least initialize isready -- if not, isready could end up being uninitialized. But with GetIsReady(), that is not a problem :) Just remember for next time.

>+  nsresult rv = CloneInlineInstance();
>+  if (NS_FAILED(rv))
>+    return rv;

Didn't notice this the first time. Only use "if (NS_FAILED(rv)) return (RV)" if you really do not want warnings on the console. Else use "NS_ENSURE_SUCCESS(rv, rv)".

r=me, good and quick work Alexander!

Comment 41

[Allan Beaufour]

Attachment: checked in patch

Checked this in. Only difference to attachment 211107 [details] [diff] [review] is the order of member var. initialization to avoid compiler warning.