Closed
Bug 307421
Opened 19 years ago
Closed 19 years ago
Make binds static
Categories
(Core Graveyard :: XForms, defect, P2)
Core Graveyard
XForms
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: allan, Assigned: allan)
References
()
Details
(Keywords: fixed1.8.0.4, fixed1.8.1)
Attachments
(3 files, 4 obsolete files)
2.79 KB,
application/xhtml+xml
|
Details | |
2.35 KB,
application/xhtml+xml
|
Details | |
37.15 KB,
patch
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Updated•19 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•19 years ago
|
||
Assignee | ||
Comment 2•19 years ago
|
||
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.
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•19 years ago
|
||
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)
Assignee | ||
Comment 4•19 years ago
|
||
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.
Assignee | ||
Comment 5•19 years ago
|
||
(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.
Assignee | ||
Comment 6•19 years ago
|
||
Attachment #211857 -
Attachment is obsolete: true
Assignee | ||
Comment 7•19 years ago
|
||
Assignee | ||
Comment 8•19 years ago
|
||
Attachment #212207 -
Attachment is obsolete: true
Attachment #212485 -
Flags: review?(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•19 years ago
|
||
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
Attachment #212485 -
Flags: review?(aaronr) → review+
Assignee | ||
Comment 11•19 years ago
|
||
> >@@ -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.
Assignee | ||
Comment 12•19 years ago
|
||
(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.
Assignee | ||
Comment 13•19 years ago
|
||
Fixed Aaron's comments.
I also added an extra check in nsXFormsModelElement::Refresh(), dunno if you want to look it through too Aaron?
Attachment #212485 -
Attachment is obsolete: true
Attachment #212716 -
Flags: review?(smaug)
Comment 14•19 years ago
|
||
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|.
Attachment #212716 -
Flags: review?(smaug) → review+
Assignee | ||
Comment 15•19 years ago
|
||
w/smaug's comments fixed
Attachment #212716 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•19 years ago
|
Keywords: fixed1.8.0.3,
fixed1.8.1
Assignee | ||
Updated•19 years ago
|
Whiteboard: xf-to-branch
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•