307421 - Make binds static

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect, P2)

Description

[Allan Beaufour]

Right now, controls that use @bind just copy the XPath expression found there
and evaluate it. That's not correct behaviour. Binds are static, and should only
be reevaluated when rebuild is done. So we need to store the xpathresult and get
that from the bind, when a controls uses @bind
(it might be worthwhile to look at bug 307420 at the same time)

Comment 1

[Allan Beaufour]

Attachment: Testcase

Comment 2

[Allan Beaufour]

Ok, I'll head down this path. I'll take the approach I described in comment 0, and fix bug 307420 on the way.

The "resolution" of inner binds is, afaik, that it should in fact be possible to reference them, and you will get the first node of the nodeset that the bind is bound to. That means that for that reason we should store a reference to that node on the bind.

It might also make sense to save the XPathExpression on the bind, instead of re-creating that on each rebuild... that might end up in a follow bug.

Comment 3

[Allan Beaufour]

Hmm, how this should actually should work is a bit questionable. I might have to delay this. I've written to the WG.

http://lists.w3.org/Archives/Member/w3c-forms/2006JanMar/0149.html
(for those who have access)

Comment 4

[Allan Beaufour]

Attachment: in-progress patch

As bug 307420 has "undefined behaviour" I'm skipping that for the moment. Enough questions arises with static binds in the first place regarding lazy instances.

Nevertheless here's the patch so far. I'm not overly satisfied with the handling of the different types of XPath result types, but I'm not sure I can avoid it.

The patch makes the binds static alright, but for some reason the controls need two rebuilds to get the idea that they have changed into their heads.

Comment 5

[Allan Beaufour]

(In reply to comment #1)
> Created an attachment (id=211857) [edit]
> Testcase

Hmm, I think I have taken the concept of "static binds" a bit too strict. The staticness should only regard the nodeset that the bind is bound to, the values should be updated in the controls. Would be weird otherwise.

Comment 6

[Allan Beaufour]

Attachment: Better testcase

Comment 7

[Allan Beaufour]

Attachment: Testcase for repeat

Comment 8

[Allan Beaufour]

Attachment: Patch

Comment 9

[aaronr]

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


>diff -X patch-excludes -uprN -U8 xforms/nsXFormsModelElement.cpp xforms.staticbind/nsXFormsModelElement.cpp
>--- xforms/nsXFormsModelElement.cpp	2006-02-17 11:11:30.000000000 +0100
>+++ xforms.staticbind/nsXFormsModelElement.cpp	2006-02-20 17:12:45.000000000 +0100

>@@ -860,20 +872,27 @@ nsXFormsModelElement::Refresh()
>         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);
>         
>-            if (refresh)
>-              // We need to refresh the control. We cannot break out of the loop
>-              // as we need to check dependencies
>+            if (refresh ^ usesModelBinding)
>+              // We need to refresh, but have to keep checking for
>+              // dependencies, or the control uses a model binding, which
>+              // means that no dependencies should be checked. Either way, no
>+              // more to do for this changed node.
>               continue;
>+            if (refresh && usesModelBinding)
>+              // a node that uses model bindings do not need to have its
>+              // dependencies ehcked, so as soon as we have found out that it
>+              // needs a refresh, break out.
>+              break;
>           }

nit: spelling, ehcked -> checked.  Also, maybe add a comment explaining why a control bound via  @bind doesn't care whether a dependency is dirty or not but @ref does.  Might even be able to do away with a comment on every 'if' test if there is a kind of 'this is what we are trying to accomplish' type of comment above the beginning of the for loop.

>diff -X patch-excludes -uprN -U8 xforms/nsXFormsOutputElement.cpp xforms.staticbind/nsXFormsOutputElement.cpp
>--- xforms/nsXFormsOutputElement.cpp	2005-08-03 20:43:44.000000000 +0200
>+++ xforms.staticbind/nsXFormsOutputElement.cpp	2006-02-20 13:17:57.000000000 +0100
>@@ -119,17 +119,22 @@ nsXFormsOutputElement::Bind()
>   }
>   
>   if (NS_FAILED(rv)) {
>     nsXFormsUtils::ReportError(NS_LITERAL_STRING("controlBindError"), mElement);
>     return rv;
>   }
> 
>   if (result) {
>-    result->GetSingleNodeValue(getter_AddRefs(mBoundNode));
>+    if (mUsesModelBinding) {
>+      // When bound via @bind, we'll get a snapshot back
>+      result->SnapshotItem(0, getter_AddRefs(mBoundNode));
>+    } else {
>+      result->GetSingleNodeValue(getter_AddRefs(mBoundNode));
>+    }
>   }

would it be worth setting mHasBinding = mUsesModelBinding and testing it before calling HasAttribute for both 'ref' and 'bind' earlier in this function?  Just got to make sure that we still check for @bind even if mUsesModelBinding is false, otherwise we'll end up using @value when we shouldn't (i.e. when there is a @bind that points to an inner bind).


This is as far as I got.  I'll finish it up tomorrow.  Looks really good so far.

Comment 10

[aaronr]

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

>diff -X patch-excludes -uprN -U8 xforms/nsXFormsUtils.cpp xforms.staticbind/nsXFormsUtils.cpp
>--- xforms/nsXFormsUtils.cpp	2006-02-17 11:11:30.000000000 +0100
>+++ xforms.staticbind/nsXFormsUtils.cpp	2006-02-20 13:19:46.000000000 +0100
>@@ -465,25 +465,27 @@ nsXFormsUtils::EvaluateXPath(const nsASt
> /* static */ nsresult
> nsXFormsUtils::EvaluateNodeBinding(nsIDOMElement           *aElement,
>                                    PRUint32                 aElementFlags,
>                                    const nsString          &aBindingAttr,
>                                    const nsString          &aDefaultRef,
>                                    PRUint16                 aResultType,
>                                    nsIModelElementPrivate **aModel,
>                                    nsIDOMXPathResult      **aResult,
>+                                   PRBool                  *aUsesModelBind,
>                                    nsCOMArray<nsIDOMNode>  *aDeps,
>                                    nsStringArray           *aIndexesUsed)
> {
>-  if (!aElement || !aModel || !aResult) {
>-    return NS_OK;
>+  if (!aElement || !aModel || !aResult || !aUsesModelBind) {
>+    return NS_ERROR_FAILURE;
>   }
> 
>   *aModel = nsnull;
>   *aResult = nsnull;
>+  *aUsesModelBind = PR_FALSE;
> 
>   nsCOMPtr<nsIDOMNode>    contextNode;
>   nsCOMPtr<nsIDOMElement> bindElement;
>   PRBool outerBind;
>   PRInt32 contextPosition;
>   PRInt32 contextSize;
>   nsresult rv = GetNodeContext(aElement,
>                                aElementFlags,
>@@ -495,52 +497,67 @@ nsXFormsUtils::EvaluateNodeBinding(nsIDO
>                                &contextSize);
> 
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   if (!contextNode) {
>     return NS_OK;   // this will happen if the doc is still loading
>   }
> 
>-  nsAutoString expr;
>+  ////////////////////
>+  // STEP 1: Handle @bind'ings
>   if (bindElement) {
>-    if (!outerBind) {
>-      // "When you refer to @id on a nested bind it returns an emtpy nodeset
>-      // because it has no meaning. The XForms WG will assign meaning in the
>-      // future."
>-      // @see http://www.w3.org/MarkUp/Group/2004/11/f2f/2004Nov11#resolution6
>+    if (outerBind) {
>+      // If there is an outer bind element, we retrieve its nodeset.
>+      nsCOMPtr<nsIContent> content(do_QueryInterface(bindElement));
>+      NS_ASSERTION(content, "nsIDOMElement not implementing nsIContent?!");
>+
>+      NS_IF_ADDREF(*aResult =
>+                   NS_STATIC_CAST(nsIDOMXPathResult*,
>+                                  content->GetProperty(nsXFormsAtoms::bind)));
>+      *aUsesModelBind = PR_TRUE;
>+    }
>+
>+    // References to inner binds are not defined.
>+    // "When you refer to @id on a nested bind it returns an emtpy nodeset
>+    // because it has no meaning. The XForms WG will assign meaning in the
>+    // future."
>+    // @see http://www.w3.org/MarkUp/Group/2004/11/f2f/2004Nov11#resolution6
> 

I think that we should report a warning to the JS Console or something.  Because if someone is using a form that works in another processor and it doesn't work here because they handle inner binds differently than we do, the poor form author is going to have a tough time tracking down why the form doesn't work.  Especially if they didn't even author it but copied it from somewhere else and modified it for their own usage.

with this and my review comments from yesterday, r=me

Comment 11

[Allan Beaufour]

> >@@ -119,17 +119,22 @@ nsXFormsOutputElement::Bind()
> >   }
> >   
> >   if (NS_FAILED(rv)) {
> >     nsXFormsUtils::ReportError(NS_LITERAL_STRING("controlBindError"), mElement);
> >     return rv;
> >   }
> > 
> >   if (result) {
> >-    result->GetSingleNodeValue(getter_AddRefs(mBoundNode));
> >+    if (mUsesModelBinding) {
> >+      // When bound via @bind, we'll get a snapshot back
> >+      result->SnapshotItem(0, getter_AddRefs(mBoundNode));
> >+    } else {
> >+      result->GetSingleNodeValue(getter_AddRefs(mBoundNode));
> >+    }
> >   }
> 
> would it be worth setting mHasBinding = mUsesModelBinding and testing it before
> calling HasAttribute for both 'ref' and 'bind' earlier in this function?

Problem is that mUsesModelBinding is maintained by ProcessNodeBinding(), called after the checks.

Comment 12

[Allan Beaufour]

(In reply to comment #10)
> >+    // References to inner binds are not defined.
> >+    // "When you refer to @id on a nested bind it returns an emtpy nodeset
> >+    // because it has no meaning. The XForms WG will assign meaning in the
> >+    // future."
> >+    // @see http://www.w3.org/MarkUp/Group/2004/11/f2f/2004Nov11#resolution6
> > 
> 
> I think that we should report a warning to the JS Console or something. 
> Because if someone is using a form that works in another processor and it
> doesn't work here because they handle inner binds differently than we do, the
> poor form author is going to have a tough time tracking down why the form
> doesn't work.  Especially if they didn't even author it but copied it from
> somewhere else and modified it for their own usage.

Good point. Noted.

Comment 13

[Allan Beaufour]

Attachment: With Aaron's comments

Fixed Aaron's comments.

I also added an extra check in nsXFormsModelElement::Refresh(), dunno if you want to look it through too Aaron?

Comment 14

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

Comment on attachment 212716 [details] [diff] [review]
With Aaron's comments

>diff -X patch-excludes -uprN -U8 xforms/nsIXFormsControl.idl xforms.staticbind/nsIXFormsControl.idl
>--- xforms/nsIXFormsControl.idl	2005-06-21 18:47:29.000000000 +0200
>+++ xforms.staticbind/nsIXFormsControl.idl	2006-02-20 16:52:48.000000000 +0100
>@@ -81,9 +81,15 @@ interface nsIXFormsControl : nsIXFormsCo

Because you're modifying an interface, change its uuid


>@@ -163,16 +165,21 @@ protected:
>   PRBool                              mHasParent;
> 
>   /** State to prevent infinite loop when generating and handling xforms-next
>    *  and xforms-previous events
>    */
>   PRBool                              mPreventLoop;
> 
>   /**
>+   * Does the control use a model bind? That is, does it have a @bind.
>+   */
>+  PRBool                              mUsesModelBinding;

While you're here, could you please change PRBool members to use PRPackedBool.



>@@ -859,26 +878,33 @@ nsXFormsModelElement::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 either the control needs a refresh or it uses a model
>+              // binding we can continue to next changed node
>               continue;
>+            if (refresh && usesModelBinding)
>+              // 2) If the control needs a refresh, and uses model bindings,
>+              // we can stop checking here
>+              break;
>           }

Could you write this in a bit different way...
Something like:
    if (refresh && usesModelBinding) {
      // ...
      // ...
      break
    } else if (refresh || usesModelBinding) {
      // ...
      // ...
      continue;  
    }

If nothing else, use at least {} -brackets after |if|.

Comment 15

[Allan Beaufour]

Attachment: Checkin ready patch

w/smaug's comments fixed

Comment 16

[Allan Beaufour]

Checked in on trunk