305060 - (UIdep) Changes to context node of parent do not refresh children

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect, P1)

Description

[aaronr]

If a group's mBoundNode is changed, it will be rebound just fine.  But contained
nodes that use the group's bound node as the context node do not reflect this
change.

Comment 1

[aaronr]

Attachment: testcase

Comment 2

[Allan Beaufour]

(In reply to comment #0)
> If a group's mBoundNode is changed, it will be rebound just fine.  But contained
> nodes that use the group's bound node as the context node do not reflect this
> change.

Yeah :( I thought about that yesterday too. It's not just group I believe, it's
any control setting context for children.

Comment 3

[Allan Beaufour]

Just some thoughts.

If a control is refreshed, it should call a "conditional refresh" on any
immediate XForms-children controls. The "conditional refresh" should refresh the
control if it's binding expression is not entirely absolute (and thus trigger
cond. refresh of its own children). That should do the trick.

Challenges:
* is it trivial to inspect whether an expression is entirely absolute
* a control should not be refreshed by a parent if it has just been refreshed by
the MDG
* repeat and selects(?) create/manage their own content, so we will need some
special handling there.

The easy solution is just to ignore the "conditional", and refresh every
child... but that's not exactly efficient.

Comment 4

[aaronr]

(In reply to comment #3)
> Just some thoughts.
> 
> If a control is refreshed, it should call a "conditional refresh" on any
> immediate XForms-children controls. The "conditional refresh" should refresh the
> control if it's binding expression is not entirely absolute (and thus trigger
> cond. refresh of its own children). That should do the trick.
> 
> Challenges:
> * is it trivial to inspect whether an expression is entirely absolute
> * a control should not be refreshed by a parent if it has just been refreshed by
> the MDG
> * repeat and selects(?) create/manage their own content, so we will need some
> special handling there.
> 
> The easy solution is just to ignore the "conditional", and refresh every
> child... but that's not exactly efficient.

Talked to sicking about this a little.  He said that we can't even count on an xpath expression starting with '/' as being absolute due to union expressions.  He said that in the code that they are doing now, the 'optimized xpath' stuff, that this kind of determination would be possible.  But that will go into the 1.9 tree.

Comment 5

[aaronr]

Being a bear of very little brain, I've thunk and thunk and this is what I've come up with:

What if we modify nsXFormsUtils::FindParentContext.  When a control successfully finds its context node by going up the parent chain until it finds a nsIXFormsContextControl, then we add our control to a member data list on that nsIXFormsContextControl called something like mBindChangeNotifyList.  And also store that nsIXFormsContextControl as a member of the control, like mContextControl.  So that if the control ever determines that its mContextControl needs to change, then we'd have to remove the control from the mBindChangeNotifyList of the previous nsIXFormsContextControl.

Then we change nsXFormsControlStub::ResetBoundNode to compare mBoundNode coming in against the one that is found.  If they are different and the control is a nsIXFormsContextControl, then it marks each control on its mBindChangeNotifyList as needing to rebind.  We'd also have to change nsXFormsModelElement::Refresh, to add a test for the above set needs-rebinding condition.

Anyone have any comment as to whether they think this would work or not?  Allan, what is the best way to mark the control as 'dirty' and requiring a rebind?

Comment 6

[aaronr]

(In reply to comment #5)
> Being a bear of very little brain, I've thunk and thunk and this is what I've
> come up with:
> 
> What if we modify nsXFormsUtils::FindParentContext.  When a control
> successfully finds its context node by going up the parent chain until it finds
> a nsIXFormsContextControl, then we add our control to a member data list on
> that nsIXFormsContextControl called something like mBindChangeNotifyList.  And
> also store that nsIXFormsContextControl as a member of the control, like
> mContextControl.  So that if the control ever determines that its
> mContextControl needs to change, then we'd have to remove the control from the
> mBindChangeNotifyList of the previous nsIXFormsContextControl.
> 
> Then we change nsXFormsControlStub::ResetBoundNode to compare mBoundNode coming
> in against the one that is found.  If they are different and the control is a
> nsIXFormsContextControl, then it marks each control on its
> mBindChangeNotifyList as needing to rebind.  We'd also have to change
> nsXFormsModelElement::Refresh, to add a test for the above set needs-rebinding
> condition.
> 
> Anyone have any comment as to whether they think this would work or not? 
> Allan, what is the best way to mark the control as 'dirty' and requiring a
> rebind?
> 

Doron pointed out that this is pretty much the same as just marking all child controls with @ref to rebind when the parent nsIXFormsContextControl's mBoundNode has changed.  So I'll try that out instead.  Maybe I'll add a mForceRebindList hashtable to model and refer to that inside nsXFormsModelElement::Refresh.  And if in the course of normal refresh processing it comes across the control and it exists on the list then it will do the rebind at that time.  If we get to the end of the normal refresh processing and there are still items to be rebound, then we'd do it at that time.

Comment 7

[aaronr]

Attachment: testcase dynamically causing rebind

another testcase.  This one will add and remove @bind on a group which will force the group to rebind and should cause contents to rebind.

Comment 8

[aaronr]

Attachment: first attempt

this patch will work for the attached testcases.  But I doubt that it will properly handle select, select1 or repeat since they have generated content in the DOM to worry about.  I'll work on this more after xmas break.

Comment 9

[Allan Beaufour]

Here's my go at it (discussed briefly with smaug yesterday):

We change mFormControls into a tree: storing (control, nextSibling, and firstChild) no each node and "duplicate" the DOM structure. When a node rebinds, it returns whether it changes its context node, and we would then mark all immediate children as "needing bind".

Using the parent as lookup when doing AddFormControl, we would actually do better than now perf.wise. We would need some extra space pr. node, but with other optimizations in queue, we should remain at status quo in the end. At least, AddFormControl should behave more intelligent. Instead of removing and then re-adding the control from the model on each change binding change, it should only do so on model change.

Comment 10

[aaronr]

(In reply to comment #9)
> Here's my go at it (discussed briefly with smaug yesterday):
> 
> We change mFormControls into a tree: storing (control, nextSibling, and
> firstChild) no each node and "duplicate" the DOM structure. When a node
> rebinds, it returns whether it changes its context node, and we would then mark
> all immediate children as "needing bind".
> 

I don't see why change mFormControls.  The second part seems like all that you'd need -> see if context node changed during ::Bind() and if so, force all immediate children to rebind and make the same check, etc.  Is it to cache those that need to be notified so that we don't have to call GetFirstChild and GetNextSibling?  A noble goal, but I think it will prove problematic if the form author screws around with the form's DOM (dynamically adding/deleting child nodes).  Even if we expose an interface to allow the form author to notify the model when he adds/removes items from the model, you have to also worry about issues like having xhtml/xul/svg elements as children of XForms elements.  They won't have a Bind() method to keep the ball rolling to their sibling elements down the line who might be xforms elements.

Comment 11

[Allan Beaufour]

(In reply to comment #10)
> (In reply to comment #9)
> > Here's my go at it (discussed briefly with smaug yesterday):
> > 
> > We change mFormControls into a tree: storing (control, nextSibling, and
> > firstChild) no each node and "duplicate" the DOM structure. When a node
> > rebinds, it returns whether it changes its context node, and we would then mark
> > all immediate children as "needing bind".
> > 
> 
> I don't see why change mFormControls.  The second part seems like all that
> you'd need -> see if context node changed during ::Bind() and if so, force all
> immediate children to rebind and make the same check, etc.  Is it to cache
> those that need to be notified so that we don't have to call GetFirstChild and
> GetNextSibling?

Yes, I think it will be rather expensive doing the search for every rebind that changes a context node. The problem is that you have to search down the DOM a lot. In fact down to each and every leaf node for every branch with no child xforms element. You also have to do the same "is-same-model" checking for each XForms control. That is, the same work that is already being perfomed by GetNodeContext(). Why not save us from that task with 2 pointers and a bit of housekeeping.

> A noble goal, but I think it will prove problematic if the
> form author screws around with the form's DOM (dynamically adding/deleting
> child nodes).  Even if we expose an interface to allow the form author to
> notify the model when he adds/removes items from the model, you have to also
> worry about issues like having xhtml/xul/svg elements as children of XForms
> elements.  They won't have a Bind() method to keep the ball rolling to their
> sibling elements down the line who might be xforms elements.

I'm not quite following you here, I would only worry about XForms elements -- that's the only ones that matters. Why should I care about anything else?

And secondly, "if a form author screws around with the form's DOM"... yes, what is the difference from today? When (if) the control figures out that it has been moved it does ProcessNodeBinding()/GetNodeContext()/whatever, just as it does today.

Comment 12

[aaronr]

> > A noble goal, but I think it will prove problematic if the
> > form author screws around with the form's DOM (dynamically adding/deleting
> > child nodes).  Even if we expose an interface to allow the form author to
> > notify the model when he adds/removes items from the model, you have to also
> > worry about issues like having xhtml/xul/svg elements as children of XForms
> > elements.  They won't have a Bind() method to keep the ball rolling to their
> > sibling elements down the line who might be xforms elements.
> 
> I'm not quite following you here, I would only worry about XForms elements --
> that's the only ones that matters. Why should I care about anything else?
> 

Ok, at least I am on the same page with what you are trying to do.  This is the kind of thing that I think that you could have a problem with using the approach that you outlined:

<xf:group ref="/root/nodes[//count]">
  <xf:label>problematic group</xf:label>
  <svg:circle cx="30" cy="30" r="30">
  <xf:input ref="//count">
    <xf:label>current count: </xf:label>
  </xf:input>
  <xf:hint ref="./hinttext"/>
</xf:group>

So if I understand what you are trying to do, then if '//count' changes, we'll go into group's Bind(), it'll see that it changed the bound node, so will call Bind() on its first child (as pulled from mFormControls).  This resolves to label's Bind().  Label's bind will see that its context node changed, so will call Bind() on its first child (none in this case) and next sibling.  Next sibling is an SVG element which won't have a Bind().

So I guess what you are thinking is that nextSibling and firstChild as stored in  mFormControl list is the nextSibling and firstChild that implement the nsIXFormsControlBase interface, and not strictly the nextSibling and firstChild like I thought.  Ok, now we are on the same page :)

Comment 13

[Allan Beaufour]

(In reply to comment #12)
> > > A noble goal, but I think it will prove problematic if the
> > > form author screws around with the form's DOM (dynamically adding/deleting
> > > child nodes).  Even if we expose an interface to allow the form author to
> > > notify the model when he adds/removes items from the model, you have to also
> > > worry about issues like having xhtml/xul/svg elements as children of XForms
> > > elements.  They won't have a Bind() method to keep the ball rolling to their
> > > sibling elements down the line who might be xforms elements.
> > 
> > I'm not quite following you here, I would only worry about XForms elements --
> > that's the only ones that matters. Why should I care about anything else?
> > 
> 
> Ok, at least I am on the same page with what you are trying to do.  This is the
> kind of thing that I think that you could have a problem with using the
> approach that you outlined:
> 
> <xf:group ref="/root/nodes[//count]">
>   <xf:label>problematic group</xf:label>
>   <svg:circle cx="30" cy="30" r="30">
>   <xf:input ref="//count">
>     <xf:label>current count: </xf:label>
>   </xf:input>
>   <xf:hint ref="./hinttext"/>
> </xf:group>
> 
> So if I understand what you are trying to do, then if '//count' changes, we'll
> go into group's Bind(), it'll see that it changed the bound node, so will call
> Bind() on its first child (as pulled from mFormControls).  This resolves to
> label's Bind().  Label's bind will see that its context node changed, so will
> call Bind() on its first child (none in this case) and next sibling.  Next
> sibling is an SVG element which won't have a Bind().
> 
> So I guess what you are thinking is that nextSibling and firstChild as stored
> in  mFormControl list is the nextSibling and firstChild that implement the
> nsIXFormsControlBase interface, and not strictly the nextSibling and firstChild
> like I thought.  Ok, now we are on the same page :)

Yes, that's what I meant :) I will not store more nodes in mFormControls than we do now, I will just restructure it to be a tree, instead of a plain list. So yes, firstXFormsChild and nextXFormsSibling if you will. "The specific model's view of the DOM".

Comment 14

[Allan Beaufour]

I'm working on this one... XPCOM/XTF have been playing some tricks on me, but I think I've gotten around them. A simple tree is 10% slower than the nsVoidArray, but if I optimize the AddFormControl() calls to only happen when actually needed, we're back to equal performance. I need some more performance tests though. I'll write some.

I'm also actually doubting the tree data structure. The operation we need most is "traversal of all controls", which does not call for a tree. We need the hierachy though... thinking...

Comment 15

[Allan Beaufour]

(In reply to comment #7)
> Created an attachment (id=206128) [edit]
> testcase dynamically causing rebind
> 
> another testcase.  This one will add and remove @bind on a group which will
> force the group to rebind and should cause contents to rebind.

Hmmm, I would rather do a follow-up bug for this testcase, because scripting support is an "added bonus" to me. So I'd rather fix "pure XForms" behaviour first.

Comment 16

[Allan Beaufour]

Attachment: Patch

mFormControls in the model is now a tree structure, and a control that changes its bound node during refresh triggers a rebind on all child nodes.

AddFormControl() now takes a parent argument, so nsXFormsUtils functions changed to return that.

I've restructured MaybeRemoveFromModel() and its sister... they are now Before/AfterSetAttribute(), and they have been generalized so Repeat also can use them now.

XXXs:
* The comparison between models (in controls) and between controls (in models) are a bit more expensive than I would like them to be though
* the nsXFormsModelFormControl name stinks... suggestions?

Comment 17

[aaronr]

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

>diff -X patch-excludes -uprN -U8 xforms/nsXFormsControlStub.h xforms.uidep/nsXFormsControlStub.h
>--- xforms/nsXFormsControlStub.h	2006-02-23 12:19:46.000000000 +0100
>+++ xforms.uidep/nsXFormsControlStub.h	2006-03-21 13:11:24.000000000 +0100
>@@ -206,34 +206,63 @@ protected:

>+
>+  /**
>+   * Binds the control to the model. Does _not_ set mBoundnode, etc. Just sets
>+   * mModel, and handle attaching to the model.
>+   */
>+  nsresult BindToModel();

nit: mBound_N_ode
nit: maybe add to comment that in addition to attaching the control to the model, it detaches
the control from any old model.

>diff -X patch-excludes -uprN -U8 xforms/nsXFormsModelElement.cpp xforms.uidep/nsXFormsModelElement.cpp
>--- xforms/nsXFormsModelElement.cpp	2006-03-21 10:15:25.000000000 +0100
>+++ xforms.uidep/nsXFormsModelElement.cpp	2006-03-21 15:35:37.000000000 +0100
>@@ -77,17 +77,17 @@
> #include "nsIDOMDocumentXBL.h"
> #include "nsIProgrammingLanguage.h"
> #include "nsDOMError.h"
> 
> #define XFORMS_LAZY_INSTANCE_BINDING \
>   "chrome://xforms/content/xforms.xml#xforms-lazy-instance"
> 
> #ifdef DEBUG
>-//#define DEBUG_MODEL
>+#define DEBUG_MODEL
> #endif

Is this a just a temporary thing?  I'd rather not have all of those printf's going to console
as a normal course of business.  Could introduce timing issues when trying to debug,
esp. large forms.


>+nsresult
>+nsXFormsModelFormControl::AddControl(nsIXFormsControl *aControl,
>+                                     nsIXFormsControl *aParent)
>+{
>+  // Three insertion posibilities:
>+
>+  // 1) control with no parent
>+  if (!aParent) {
>+    nsXFormsModelFormControl* newNode = new nsXFormsModelFormControl(aControl);
>+    NS_ENSURE_STATE(newNode);
>+
>+    // We are the root node
>+    if (!mNode) {
>+      if (mFirstChild) {
>+        return mFirstChild->AddControl(aControl, nsnull);
>+      }
>+      mFirstChild = newNode;
>+      return NS_OK;
>+    }
>+

Unless I am reading this wrong, it looks like any time that we add a top-layer
control, you'll be allocating and testing a newNode that we'll never use and never free.


>+nsresult
>+nsXFormsModelFormControl::RemoveControl(nsIXFormsControl *aControl,
>+                                        PRBool &aRemoved)
>+{
>+  nsXFormsModelFormControl* deleteMe = nsnull;
>+  aRemoved = PR_FALSE;

nit: align the params


>-NS_IMETHODIMP
>-nsXFormsModelElement::Refresh()
>+nsresult
>+nsXFormsModelElement::RefreshSubTree(nsXFormsModelFormControl *aCurrent,
>+                                     PRBool aForceRebind)
> {

nit: align params

>-#ifdef DEBUG
>-  printf("nsXFormsModelElement::Refresh()\n");
>-#endif
>-  nsPostRefresh postRefresh = nsPostRefresh();
>+  nsresult rv;
> 
>-  // Iterate over all form controls if not during initialization phase (then
>-  // this is handled in InitializeControls())
>-  if (mDocumentLoaded) {
>-    PRInt32 controlCount = mFormControls.Count();
>-    for (PRInt32 i = 0; i < controlCount; ++i) {
>-      nsIXFormsControl* control = NS_STATIC_CAST(nsIXFormsControl*, mFormControls[i]);
>-      /// @todo If a control is removed because of previous control has been
>-      /// refreshed, we do, obviously, not need to refresh it. So mFormControls
>-      /// should have weak bindings to the controls I guess? (XXX)
>-      ///
>-      /// This could happen for \<repeatitem\>s for example.
>-      if (!control) {
>-        continue;
>-      }
>+  while (aCurrent) {
>+    nsCOMPtr<nsIXFormsControl> control(aCurrent->Control());
>+    NS_ASSERTION(control, "A tree node without a control?!");
>+    
>+    // Get bound node
>+    nsCOMPtr<nsIDOMNode> boundNode;
>+    control->GetBoundNode(getter_AddRefs(boundNode));
> 
>-      // Get bound node
>-      nsCOMPtr<nsIDOMNode> boundNode;
>-      control->GetBoundNode(getter_AddRefs(boundNode));
>+    PRBool rebind = aForceRebind;
>+    PRBool refresh = PR_FALSE;
>+    PRBool rebindChildren = PR_FALSE;
>+
>+#ifdef DEBUG_MODEL
>+      nsCOMPtr<nsIDOMElement> controlElement;
>+      control->GetElement(getter_AddRefs(controlElement));
>+      printf("rebind: %d, mNeedsRefresh: %d, rebindChildren: %d\n",
>+             rebind, mNeedsRefresh, rebindChildren);
>+      if (controlElement) {
>+        printf("Checking control: ");
>+        //DBG_TAGINFO(controlElement);
>+      }
>+#endif
> 
>-      PRBool rebind = PR_FALSE;
>-      PRBool refresh = PR_FALSE;
>+    if (mNeedsRefresh || rebind) {
>+      refresh = PR_TRUE;
>+    } else {
>+      PRBool usesModelBinding = PR_FALSE;
>+      control->GetUsesModelBinding(&usesModelBinding);
> 
>-      if (mNeedsRefresh) {
>-        refresh = PR_TRUE;
>-      } else {
>-        PRBool usesModelBinding = PR_FALSE;
>-        control->GetUsesModelBinding(&usesModelBinding);
>+#ifdef DEBUG_MODEL
>+      printf("usesModelBinding: %d\n", usesModelBinding);
>+#endif
> 
>-        nsCOMArray<nsIDOMNode> *deps = nsnull;
>-        if (usesModelBinding) {
>-          if (!boundNode)
>+      nsCOMArray<nsIDOMNode> *deps = nsnull;
>+      if (usesModelBinding) {
>+        if (!boundNode) {
>           // If a control uses a model binding, but has no bound node a
>           // rebuild is the only thing that'll (eventually) change it
>+          aCurrent = aCurrent->NextSibling();
>           continue;
>-        } else {
>-          // Get dependencies
>-          control->GetDependencies(&deps);    
>         }
>-        PRUint32 depCount = deps ? deps->Count() : 0;
>+      } else {
>+        // Get dependencies
>+        control->GetDependencies(&deps);
>+      }
>+      PRUint32 depCount = deps ? deps->Count() : 0;
>         
> #ifdef DEBUG_MODEL
>-        nsCOMPtr<nsIDOMElement> controlElement;
>-        control->GetElement(getter_AddRefs(controlElement));
>-        if (controlElement) {
>-          printf("Checking control: ");      
>-          //DBG_TAGINFO(controlElement);
>-          nsAutoString boundName;
>-          if (boundNode)
>-            boundNode->GetNodeName(boundName);
>-          printf("\tDependencies: %d, Bound to: '%s' [%p]\n",
>-                 depCount,
>-                 NS_ConvertUTF16toUTF8(boundName).get(),
>-                 (void*) boundNode);
>-
>-          nsAutoString depNodeName;
>-          for (PRUint32 t = 0; t < depCount; ++t) {
>-            nsCOMPtr<nsIDOMNode> tmpdep = deps->ObjectAt(t);
>-            if (tmpdep) {
>-              tmpdep->GetNodeName(depNodeName);
>-              printf("\t\t%s [%p]\n",
>-                     NS_ConvertUTF16toUTF8(depNodeName).get(),
>-                     (void*) tmpdep);
>-            }
>-          }
>+      nsAutoString boundName;
>+      if (boundNode)
>+        boundNode->GetNodeName(boundName);
>+      printf("\tDependencies: %d, Bound to: '%s' [%p]\n",
>+             depCount,
>+             NS_ConvertUTF16toUTF8(boundName).get(),
>+             (void*) boundNode);
>+
>+      nsAutoString depNodeName;
>+      for (PRUint32 t = 0; t < depCount; ++t) {
>+        nsCOMPtr<nsIDOMNode> tmpdep = deps->ObjectAt(t);
>+        if (tmpdep) {
>+          tmpdep->GetNodeName(depNodeName);
>+          printf("\t\t%s [%p]\n",
>+                 NS_ConvertUTF16toUTF8(depNodeName).get(),
>+                 (void*) tmpdep);
>         }
>+      }
> #endif
> 
>-        nsCOMPtr<nsIDOM3Node> curChanged;
>+      nsCOMPtr<nsIDOM3Node> curChanged;
> 
>-        for (PRInt32 j = 0; j < mChangedNodes.Count(); ++j) {
>-          curChanged = do_QueryInterface(mChangedNodes[j]);
>+      for (PRInt32 j = 0; j < mChangedNodes.Count(); ++j) {
>+        curChanged = do_QueryInterface(mChangedNodes[j]);
> 
>-          // Check whether the bound node is dirty. If so, we need to refresh the
>-          // control (get updated node value from the bound node)
>-          if (!refresh && boundNode) {
>-            curChanged->IsSameNode(boundNode, &refresh);
>-
>-            // Two ways to go here. Keep in mind that controls using model
>-            // binding expressions never needs to have dependencies checked as
>-            // they only rebind on xforms-rebuild
>-            if (refresh && usesModelBinding) {
>-              // 1) If the control needs a refresh, and uses model bindings,
>-              // we can stop checking here
>-              break;
>-            }
>-            if (refresh || usesModelBinding) {
>-              // 2) If either the control needs a refresh or it uses a model
>-              // binding we can continue to next changed node
>-              continue;
>-            }
>+        // Check whether the bound node is dirty. If so, we need to refresh the
>+        // control (get updated node value from the bound node)
>+        if (!refresh && boundNode) {
>+          curChanged->IsSameNode(boundNode, &refresh);
>+
>+          // Two ways to go here. Keep in mind that controls using model
>+          // binding expressions never needs to have dependencies checked as
>+          // they only rebind on xforms-rebuild
>+          if (refresh && usesModelBinding) {
>+            // 1) If the control needs a refresh, and uses model bindings,
>+            // we can stop checking here
>+            break;
>           }
>-
>-          // Check whether any dependencies are dirty. If so, we need to rebind
>-          // the control (re-evaluate it's binding expression)
>-          for (PRUint32 k = 0; k < depCount; ++k) {
>-            /// @note beaufour: I'm not to happy about this ...
>-            /// O(mChangedNodes.Count() * deps->Count()), but using the pointers
>-            /// for sorting and comparing does not work...
>-            curChanged->IsSameNode(deps->ObjectAt(k), &rebind);
>-            if (rebind)
>-              // We need to rebind the control, no need to check any more
>-              break;
>+          if (refresh || usesModelBinding) {
>+            // 2) If either the control needs a refresh or it uses a model
>+            // binding we can continue to next changed node
>+            continue;
>           }
>         }
>+
>+        // Check whether any dependencies are dirty. If so, we need to rebind
>+        // the control (re-evaluate it's binding expression)
>+        for (PRUint32 k = 0; k < depCount; ++k) {
>+          /// @note beaufour: I'm not to happy about this ...
>+          /// O(mChangedNodes.Count() * deps->Count()), but using the pointers
>+          /// for sorting and comparing does not work...
>+          curChanged->IsSameNode(deps->ObjectAt(k), &rebind);
>+          if (rebind)
>+            // We need to rebind the control, no need to check any more
>+            break;
>+        }
>+      }


Remember, per the fix for bug 310276, this should break out of both 'for' loops if
rebind == PR_TRUE.


> 
> NS_IMETHODIMP
>@@ -975,28 +1300,30 @@ nsXFormsModelElement::HandleEvent(nsIDOM
>   }
> 
>   return NS_OK;
> }
> 
> // nsIModelElementPrivate
> 
> NS_IMETHODIMP
>-nsXFormsModelElement::AddFormControl(nsIXFormsControl *aControl)
>+nsXFormsModelElement::AddFormControl(nsIXFormsControl *aControl,
>+                                     nsIXFormsControl *aParent)
> {
>-  if (mFormControls.IndexOf(aControl) == -1)
>-    mFormControls.AppendElement(aControl);
>-  return NS_OK;
>+  return mFormControls.AddControl(aControl, aParent);
> }
> 
> NS_IMETHODIMP
> nsXFormsModelElement::RemoveFormControl(nsIXFormsControl *aControl)
> {
>-  mFormControls.RemoveElement(aControl);
>-  return NS_OK;
>+  PRBool removed;
>+  nsresult rv = mFormControls.RemoveControl(aControl, removed);
>+  NS_WARN_IF_FALSE(removed,
>+                   "Tried to remove control that was not in the model");
>+  return rv;
> }


General comment about AddFormControl and RemoveFormControl: you are doing a lot of
work in mFormControls.XxxxxControl...is it worth checking to make sure that
aControl (and maybe aParentControl) are non-null before starting into them?


>diff -X patch-excludes -uprN -U8 xforms/nsXFormsModelElement.h xforms.uidep/nsXFormsModelElement.h
>--- xforms/nsXFormsModelElement.h	2006-03-15 10:14:47.000000000 +0100
>+++ xforms.uidep/nsXFormsModelElement.h	2006-03-21 13:41:26.000000000 +0100
>@@ -112,16 +112,112 @@ public:
>   void DropReferences();
> 

>+  /** Return the first child of the node*/
>+  nsXFormsModelFormControl* FirstChild() { return mFirstChild; };
>+
>+  /** Return the next sibling of the node*/
>+  nsXFormsModelFormControl* NextSibling() { return mNextSibling; };
>+

nit: for the comments for FirstChild() and NextSibling(), please have add a 'space'
char before '*/'


>@@ -242,20 +338,23 @@ private:
>    */
>   NS_HIDDEN_(nsresult) HandleLoad(nsIDOMEvent *aEvent);
> 
>   /**
>    * Called by HandleEvent.  Event handler for the 'unload' event.
>    */
>   NS_HIDDEN_(nsresult) HandleUnload(nsIDOMEvent *aEvent);
> 
>+  NS_HIDDEN_(nsresult) RefreshSubTree(nsXFormsModelFormControl *aCurrent,
>+                                      PRBool aForceRebind);
>+

nit: align params

Patch looks good and makes sense to me.  As for the name of the class nsXFormsModelFormControl...maybe
nsXFormsControlListItem?

With those fixed, r=me

Comment 18

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

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

With Aaron's comment, r=me.
But I'm wondering whether something bad may happen if the control tree is mutated while iterating through it. I think not, but please ensure.

Comment 19

[Allan Beaufour]

(In reply to comment #17)
> > #ifdef DEBUG
> >-//#define DEBUG_MODEL
> >+#define DEBUG_MODEL
> > #endif
> 
> Is this a just a temporary thing?  I'd rather not have all of those printf's
> going to console as a normal course of business.  Could introduce timing
> issues when trying to debug, esp. large forms.

Argh, that was just me forgetting to disable it again.

> 
> >+nsresult
> >+nsXFormsModelFormControl::AddControl(nsIXFormsControl *aControl,
> >+                                     nsIXFormsControl *aParent)
> >+{
> >+  // Three insertion posibilities:
> >+
> >+  // 1) control with no parent
> >+  if (!aParent) {
> >+    nsXFormsModelFormControl* newNode = new nsXFormsModelFormControl(aControl);
> >+    NS_ENSURE_STATE(newNode);
> >+
> >+    // We are the root node
> >+    if (!mNode) {
> >+      if (mFirstChild) {
> >+        return mFirstChild->AddControl(aControl, nsnull);
> >+      }
> >+      mFirstChild = newNode;
> >+      return NS_OK;
> >+    }
> >+
> 
> Unless I am reading this wrong, it looks like any time that we add a top-layer
> control, you'll be allocating and testing a newNode that we'll never use and
> never free.

You are 100% correct. Ouch, how did I end up doing that...

> Patch looks good and makes sense to me.  As for the name of the class
> nsXFormsModelFormControl...maybe
> nsXFormsControlListItem?

Much better!

Comment 20

[Allan Beaufour]

(In reply to comment #18)
> (From update of attachment 215777 [details] [diff] [review] [edit])
> With Aaron's comment, r=me.
> But I'm wondering whether something bad may happen if the control tree is
> mutated while iterating through it. I think not, but please ensure.

Unfair question on a Monday! I'm pretty sure that it should behave nicely, but being a Monday, I'll wait and give you a definitive answer tomorrow.

Comment 21

[Allan Beaufour]

Attachment: w/Aaron's comments and updated to tip

Comment 22

[Allan Beaufour]

(In reply to comment #20)
> (In reply to comment #18)
> > (From update of attachment 215777 [details] [diff] [review] [edit] [edit])
> > With Aaron's comment, r=me.
> > But I'm wondering whether something bad may happen if the control tree is
> > mutated while iterating through it. I think not, but please ensure.
> 
> Unfair question on a Monday! I'm pretty sure that it should behave nicely, but
> being a Monday, I'll wait and give you a definitive answer tomorrow.

Waited until Wednesday, just to be sure ;) Yes, it should be sane.

Checked into trunk.