279063 - Implement copy element

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

We need to implement the copy element as per spec ->
http://www.w3.org/TR/xforms/index-all.html#ui-adv-copy.

Comment 1

[aaronr]

Attachment: testcase that uses copy

testcase shows problem.  But you will need to update the testcase to point to
your own server to echo the submitted instance data.

Comment 2

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

oops, I wasn't going to take this bug. Reassigning to default owner.

Comment 3

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

Hmm, or why not. Assigning back to me :)

Comment 4

[aaronr]

I started to give this a little thought today until I realized that there is no
sense starting this until the XBL patch is in and see how much that changes how
select and select1 will work.

Thoughts that I had: probably as the 'value' of the copy, could just store the
contextnode and the xpath expression that needs to be executed when this
particular item in the select box is selected.  Then simply evalate it and do a
deep copy as a child of the instance data node pointed at by select/select1
single node binding.  Not too tough.  The tough part as I see it will be the
default selection.  If that instance node already contains some pre-seeded
information on load, it causes the proper default selection to be made in the
select/select1 list...selecting the item who has the matching nodeset as a
value.  So I guess that a compare will be need to be made between that
pre-seeded content and the content resolved by the copy element.  Either
serailizing both into a string and comparing the string (inefficient) or going
node by node to make sure that all of the namespaces, localnames, text nodes,
etc. match.  Fun, fun, fun.

Comment 5

[Doron Rosenberg (IBM)]

Thoughts on copy:

Copy needs to provide 3 apis probably:
  - Do()
  - Undo()
  - nsIDOMElement GetValue()

Select/select1 needs to make copy do its magic and undo in case the item is
unselected.  I believe that default values apply to copy (I think, not exactly
sure), so GetValue() would help with that.

The question is, who should do the adding/removing from the instance document -
copy or the select/select1 widget?  The issue here is undo - someone needs to
remember what domelement copy added.

Comment 6

[aaronr]

I'm going to start working on a patch today.

Comment 7

[aaronr]

Attachment: testcase that works better on other processors

Comment 8

[aaronr]

Attachment: first stab at a patch

Please try out this patch.  I'd appreciate comments of course about the code
itself, but just as important, I'd like feedback about the behavior on the
attached testcase.  With this patch, we behave like Novell's plugin.  But
you'll notice that if you click on 'strawberry', that the other items will stay
selected which probably isn't what the user would expect.  Yet we are honoring
the spec, at least using the logic as documented under xf:setvalue.

Please let me know what you think.

Comment 9

[Allan Beaufour]

(In reply to comment #8)
> Created an attachment (id=200188) [edit]

+nsXFormsUtils::AreEntitiesEqual(nsIDOMNamedNodeMap *aEntities1,
+                                nsIDOMNamedNodeMap *aEntities2)
+{
...
+  for (int i=0; i<entLength1; i++) {

PRUint32, and insert spaces...

same goes for the other Are...Equal().

(compile with warnings, or just watch them :), and you would have seen this)

Comment 10

[Allan Beaufour]

The first time I click on "Pick here for strawberry" in attachment.cgi 200186,
it selects the bottom three again...

Comment 11

[aaronr]

(In reply to comment #10)
> The first time I click on "Pick here for strawberry" in attachment.cgi 200186,
> it selects the bottom three again...

Yep, I pointed that out in comment #8.  Pretty crazy, right?  Not what I would
expect as a user.  But working just like Novell's plugin and according to the
details in SetValue.  We could change how select and select1 work so that ONLY
the items in the select(1) are ever under the bound node.  But then we aren't
being consistent with the other controls.

Comment 12

[Allan Beaufour]

(In reply to comment #11)
> (In reply to comment #10)
> > The first time I click on "Pick here for strawberry" in attachment.cgi 200186,
> > it selects the bottom three again...
> 
> Yep, I pointed that out in comment #8.  Pretty crazy, right?  Not what I would
> expect as a user.  But working just like Novell's plugin and according to the
> details in SetValue.  We could change how select and select1 work so that ONLY
> the items in the select(1) are ever under the bound node.  But then we aren't
> being consistent with the other controls.

Argh. sorry!

Testcase 9.3.4.a behaves weird though... is it invalid or is it the patch?

Comment 13

[Allan Beaufour]

In attachment 200186 [details]:
1. Click [e-mail]
2. CTRL-click [e-mail] (to unselect it)
3. Click "Pick here for strawberry"
This results in:
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE)
[nsIXFormsItemElement.value]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)" 
location: "JS frame :: chrome://xforms/content/select.xml :: _updateSelection ::
line 614"  data: no]

Comment 14

[Allan Beaufour]

(In reply to comment #11)
> (In reply to comment #10)
> > The first time I click on "Pick here for strawberry" in attachment.cgi 200186,
> > it selects the bottom three again...
> 
> Yep, I pointed that out in comment #8.

But still... the second time I click it, it visually deselects the other items
but if I sumbit, they are still selected...

Comment 15

[aaronr]

Attachment: second stab

Ok, this patch behaves more intuitively for the user.  If you click on an item that contains a copy and then select an item that contains a value, then the element node is removed from under the bound node in favor of the textnode that will be created and inserted that holds the value from the xf:value.  This way the items that are selected are the items that the user chose ONLY.

Also fixed the visual behavior when trying to use xf:copy when bound to a node that isn't an element node.

Comment 16

[aaronr]

Attachment: third stab

made a few function names common between select and select1.  Fixed a bug where select1 wouldn't clean out bound node's content when switching from copyItem to valueItem.

Comment 17

[aaronr]

Attachment: updated to trunk

Comment 18

[aaronr]

Attachment: update to use accessors

Ugh.  This patch is getting too hard to maintain.  I've updated it to the trunk.  I've gotten all but a couple of bugs out of it, which I've marked with XXX.  I will fix those after this goes in.  Right now I'm not rebuilding the model after selecting/deselecting a copyItem, and the code in select.xml might be improved if I cache the selected copyNode and comparing future selections against that instead of building the select fully every time there is a selection when a element already exists under the boundNode.  But comparisons of nodes aren't cheap either.

But this patch will move copyNodes under the elements when copyItem is selected.  It has select and select1 properly dispatching xforms-select/deselect events, dispatches the xforms-binding-exceptions, etc.

Comment 19

[aaronr]

Attachment: binding exception with copy testcase

Comment 20

[aaronr]

Attachment: xforms-select/deselect with copy testcase

Comment 21

[aaronr]

Attachment: select/deselect with copy + select1 testcase

Comment 22

[aaronr]

Attachment: rebuild caused by copy + select1 testcase

Comment 23

[Doron Rosenberg (IBM)]

Comment on attachment 202437 [details] [diff] [review]
update to use accessors

>Index: nsIModelElementPrivate.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIModelElementPrivate.idl,v
>retrieving revision 1.13
>diff -u -8 -p -r1.13 nsIModelElementPrivate.idl
>--- nsIModelElementPrivate.idl	25 Oct 2005 17:29:56 -0000	1.13
>+++ nsIModelElementPrivate.idl	9 Nov 2005 23:50:48 -0000
>@@ -42,17 +42,17 @@ interface nsIXFormsControl;
> interface nsISchemaType;
> interface nsIInstanceElementPrivate;
> interface nsIDOMNode;
> 
> /**
>  * Private interface implemented by the model element for other
>  * elements to use.
>  */
>-[uuid(8c5e2b7d-043e-4278-bc3e-1519bd9c62ec)]
>+[uuid(ccba3717-ef05-41cd-b831-f7f5ab3b921c)]
> interface nsIModelElementPrivate : nsIXFormsModelElement
> {
>   /**
>    * Called by form control elements when they are bound to or unbound from
>    * this model.  These form controls will be refreshed when refresh() is
>    * called on the model.
>    */
>   void addFormControl(in nsIXFormsControl control);
>@@ -95,16 +95,23 @@ interface nsIModelElementPrivate : nsIXF
> 
>   /**
>    * Get the value of an instance node.
>    */
>   void getNodeValue(in nsIDOMNode contextNode,
>                     out AString  nodeValue);
> 
>   /**
>+   * Insert a node under an instance node
>+   */
>+  void setNodeContent(in nsIDOMNode contextNode,
>+                      in nsIDOMNode nodeContent,
>+                      out boolean nodeChanged);
>+
>+  /**

Do we follow some sort of convention for when to use out and when to just return it?


>Index: nsIXFormsItemElement.idl

>+  /**
>+   * Indicates whether the item element contains a value child or a copy
>+   * child.  We'll assume that if the item is NOT a copy item, then it must
>+   * be a value item.  Which means that it must contain a XForms value element
>+   * child.
>+   */
>+  attribute boolean copyItem;
>+

Perhaps call it hasCopyItem?

>Index: nsXFormsAccessors.cpp
>+NS_IMETHODIMP
>+nsXFormsAccessors::GetContent(nsIDOMNode **aContentEnvelope)
>+{
>+  NS_ENSURE_ARG_POINTER(aContentEnvelope);
>+
>+  // NOTE: returning LIVE data.  Seems pretty inefficient to do a deep
>+  //       clone node of the mBoundNode.  If this doesn't get used too much
>+  //       that would probably be a safer approach, though.
>+
>+  return GetBoundNode(aContentEnvelope);
>+}
Perhaps make thix an XXX: comment?

>+
>+NS_IMETHODIMP
>+nsXFormsAccessors::SetContent(nsIDOMNode *aNode)
>+{
>+  nsCOMPtr<nsIDOMNode> boundNode;
>+  nsCOMPtr<nsIModelElementPrivate> modelPriv;
>+
>+  nsresult rv = GetBoundNode(getter_AddRefs(boundNode));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (mElement) {
>+    modelPriv = nsXFormsUtils::GetModel(mElement);
>+  }
>+
>+  if (!boundNode || !modelPriv)
>+    return NS_OK;

If mElement doesn't exist, then is null and the above if check will bail out.

Better to add a if check for mElement at the top of the method, and then remove the mElement check and then defining the modelPriv nsCOMPtr when you call ::GetModel.

>+NS_IMETHODIMP
>+nsXFormsAccessors::GetElement(nsIDOMElement **aElement)
>+{
>+  NS_IF_ADDREF(*aElement = mElement);
>+  return NS_OK;
>+}
>+
>+
>+

too many newlines there :)

> 
>+

why this extra newline?

> NS_IMETHODIMP
>+nsXFormsItemElement::SelectItemByNode(nsIDOMNode *aNode, nsIDOMNode **aSelected)
>+{
>+  NS_ENSURE_ARG_POINTER(aSelected);
>+  NS_ENSURE_STATE(mElement);
>+  PRBool isCopyItem;
>+  GetCopyItem(&isCopyItem);
>+  if (!isCopyItem) {
>+    // If this item doesn't contain a copy element but instead has a value
>+    // element, then there is no sense continuing.
>+    *aSelected = nsnull;
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIDOMNode> copyNode;
>+  nsresult rv = GetCopyNode(getter_AddRefs(copyNode));
>+  if (NS_SUCCEEDED(rv)) {
>+    if(copyNode) {

space between if and the "("

>Index: nsXFormsMDGEngine.cpp
>+  if(nodesEqual) {
>+    return NS_OK;
>+  }

space between the if and the "(" :)

>+  nsCOMPtr<nsIDOMNode> resultNode;
>+  if(hasChildNodes) {
>+    nsCOMPtr<nsIDOMNodeList> childList;
>+    rv = aContextNode->GetChildNodes(getter_AddRefs(childList));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    if(childList) {
>+      PRUint32 length;
>+      rv = childList->GetLength(&length);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      for(int i=length; i>0; i--) {
>+        nsCOMPtr<nsIDOMNode> childNode;
>+        rv = childList->Item(i-1, getter_AddRefs(childNode));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+        rv = aContextNode->RemoveChild(childNode, getter_AddRefs(resultNode));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+      }
>+    }
>+  }
>+
>+  // add contents of the envelope under aContextNode
>+  rv = aContentEnvelope->HasChildNodes(&hasChildNodes);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if(hasChildNodes) {
>+    nsCOMPtr<nsIDOMNode> childNode;
>+    rv = aContentEnvelope->GetFirstChild(getter_AddRefs(childNode));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    while (childNode) {
>+      rv = aContextNode->AppendChild(childNode, getter_AddRefs(resultNode));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      rv = aContentEnvelope->GetFirstChild(getter_AddRefs(childNode));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+  }

space between the if/for and the "(" :)

>+
>+  // NB: Never reached for Readonly nodes.  
>+  if (aNodeChanged) {
>+    *aNodeChanged = PR_TRUE;
>+  }
>+  if (aMarkNode) {
>+      MarkNodeAsChanged(aContextNode);
>+  }

indentation is more that 2 characters here.

>Index: nsXFormsUtils.cpp

>+/* static */ PRBool
>+nsXFormsUtils::AreEntitiesEqual(nsIDOMNamedNodeMap *aEntities1,
>+                                nsIDOMNamedNodeMap *aEntities2)
>+{
>+  if(!aEntities1 && !aEntities2) {
>+    return PR_TRUE;
>+  }
>+
>+  if(!aEntities1 || !aEntities2) {
>+    return PR_FALSE;
>+  }

space between the if and the "(" :)

>+#if 0
>+    rv1 = ent1->GetInputEncoding(buffer1);
>+    rv2 = ent2->GetInputEncoding(buffer2);
>+    if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
>+      return PR_FALSE;
>+    }
>+
>+    rv1 = ent1->GetXmlEncoding(buffer1);
>+    rv2 = ent2->GetXmlEncoding(buffer2);
>+    if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
>+      return PR_FALSE;
>+    }
>+    rv1 = ent1->GetXmlVersion(buffer1);
>+    rv2 = ent2->GetXmlVersion(buffer2);
>+    if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
>+      return PR_FALSE;
>+    }
>+#endif

Probably should be removed?

>+
>+    // the order of the attributes on the two nodes doesn't matter.  But
>+    // every attribute on node1 must exist on node2 (and no more)
>+    for (PRUint32 i = 0; i < attrLength1; ++i) {
>+      nsCOMPtr<nsIDOMNode> attr1, attr2;
>+      rv1 = attrs1->Item(i, getter_AddRefs(attr1));
>+      if (!attr1) {
>+        return PR_FALSE;
>+      }
>+
>+      attr1->GetLocalName(buffer1);
>+      attr1->GetNamespaceURI(buffer2);
>+      attrs2->GetNamedItemNS(buffer2, buffer1, getter_AddRefs(attr2));
>+      if (!attr2) {
>+        return PR_FALSE;
>+      }
>+
>+      rv1 = attr1->GetNodeValue(buffer1);
>+      rv2 = attr2->GetNodeValue(buffer2);
>+      if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
>+        return PR_FALSE;
>+      }
>+    }
>+  }

Problem with using buffer1/buffer2 as variable names is that it makes the code less readable.


This pass was mainly code style comments, probably do another one for functionality.

>Index: resources/content/select.xml

>+          } else {
>+            var boundNode = this.accessors.getBoundNode();
>+            var child = boundNode ? boundNode.firstChild : null;
>+            this._defaultHash = null;
>+            while (child) {
>+              var type = child.nodeType;
>+              if (type == Node.TEXT_NODE || 
>+                  type == Node.CDATA_SECTION_NODE) {
>+                // if child is a text node completely filled with
>+                // whitespace let's ignore it and get the next node
>+                var string = child.nodeValue;
>+                var nonWhitespace = false;
>+                if (string) {
>+                  for (var i=0; i<string.length; i++) {
>+                    nonWhitespace = !(/^\s/.test(string.charAt(i)));
>+                    if (nonWhitespace) {
>+                      break;
>+                    }
>+                  }
>+                }

could you add a comment exactly what the regexp does?

>+          // if this item contains a copy element AND if the bound node contains
>+          // non-text elements, then see if any of these non-text elements match
>+          // this copyItem's node.
>+          if (copyItem && this._selectedElementSize > 0) {
>+            var item = aItemElement.QueryInterface(Components.interfaces.nsIXFormsSelectChild); 
>+            for (var j = 0; j < this._selectedElementSize; j++ ) {
>+              var selectedItem =
>+                item.selectItemByNode(this._selectedElementArray[j].element);
>+              if (selectedItem) {
>+                this._selectedElementArray[j].hits++;
>+                option.selected = true;
>+                // XXX It is possible that two identical elements are under the
>+                // bound node.  I guess we shouldn't mark one and not the other
>+                // if there is an item in the select that matches it.  So we'll
>+                // go through the whole list.  But this is quite an edge case
>+                // and will cause more inefficiency just to prevent an errant
>+                // xforms-out-of-range.
>+              }
>+            }
>+          }
Yeah, I don't think we should worry about identical elements too much.


> 
>       <method name="_getSelectedValues">
>         <body>
>         <![CDATA[
>           var selectedValues = "";
>           var newSelectedControls = new Array();
> 
>           // select if found, unselect if not
>           var options = this._controlArray;
> 
>+          var boundNode = this.accessors.getBoundNode();
>+          if (!boundNode) {
>+            return;
>+          }
>+
>+          var contentEnvelope = null;
>+          contentEnvelope = boundNode.cloneNode(false);
>+          if (!contentEnvelope) {
>+            return;
>+          }

this if check is kinda useless, cloneNode should't fail if boundNode exists.

Also, could you comment on what contentEnvelope is used for?  It is not apparent by the variable name.

Skipping select1 code :)

Comment 24

[Doron Rosenberg (IBM)]

Comment on attachment 202437 [details] [diff] [review]
update to use accessors


>+/* static */ PRBool
>+nsXFormsUtils::AreEntitiesEqual(nsIDOMNamedNodeMap *aEntities1,
>+                                nsIDOMNamedNodeMap *aEntities2)
>+{
>+  if(!aEntities1 && !aEntities2) {
>+    return PR_TRUE;
>+  }
>+
>+  if(!aEntities1 || !aEntities2) {
>+    return PR_FALSE;
>+  }

Entities have to appear in the same order to be equal, or does nsIDOMNamedNodeMap do some sort of sorting?

>+
>+/* static */ PRBool
>+nsXFormsUtils::AreNodesEqual(nsIDOMNode *aFirstNode, nsIDOMNode *aSecondNode,
>+                             PRBool aAlreadyNormalized)
>+{
...
>+
>+  rv1 = aFirstNode->GetNodeName(buffer1);
>+  rv2 = aSecondNode->GetNodeName(buffer2);
>+  if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
>+    return PR_FALSE;
>+  }
>+
>+  rv1 = aFirstNode->GetLocalName(buffer1);
>+  rv2 = aSecondNode->GetLocalName(buffer2);
>+  if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
>+    return PR_FALSE;
>+  }
>+
>+  rv1 = aFirstNode->GetNamespaceURI(buffer1);
>+  rv2 = aSecondNode->GetNamespaceURI(buffer2);
>+  if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
>+    return PR_FALSE;
>+  }
>+
>+  rv1 = aFirstNode->GetPrefix(buffer1);
>+  rv2 = aSecondNode->GetPrefix(buffer2);
>+  if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
>+    return PR_FALSE;
>+  }

Do prefixes have to be same?  isn't ns1:foo and ns2:foo the same if ns1 and ns2 are the same namespace?

>+
>+  // now looking at the child nodes.  They have to be 'equal' and at the same
>+  // index inside each of the parent nodes.
>+  PRBool hasChildren1, hasChildren2;
>+  rv1 = aFirstNode->HasChildNodes(&hasChildren1);
>+  rv2 = aSecondNode->HasChildNodes(&hasChildren2);
>+  if (NS_FAILED(rv1) || NS_FAILED(rv2) || hasChildren1 != hasChildren2) {
>+    return PR_FALSE;
>+  }
>+
>+  if (hasChildren1) {
>+    nsCOMPtr<nsIDOMNodeList> children1, children2;
>+    PRUint32 childrenLength1, childrenLength2;
>+
>+    rv1 = aFirstNode->GetChildNodes(getter_AddRefs(children1));
>+    rv2 = aSecondNode->GetChildNodes(getter_AddRefs(children2));
>+    if (NS_FAILED(rv1) || NS_FAILED(rv2) || !children1 || !children2) {
>+      return PR_FALSE;
>+    }

Comment 25

[aaronr]

(In reply to comment #24)
> (From update of attachment 202437 [details] [diff] [review] [edit])
> 
> >+/* static */ PRBool
> >+nsXFormsUtils::AreEntitiesEqual(nsIDOMNamedNodeMap *aEntities1,
> >+                                nsIDOMNamedNodeMap *aEntities2)
> >+{
> >+  if(!aEntities1 && !aEntities2) {
> >+    return PR_TRUE;
> >+  }
> >+
> >+  if(!aEntities1 || !aEntities2) {
> >+    return PR_FALSE;
> >+  }
> 
> Entities have to appear in the same order to be equal, or does
> nsIDOMNamedNodeMap do some sort of sorting?

Yep, entities have to be in the same order to be equal.  Isn't that what I'm doing?

> 
> >+
> >+/* static */ PRBool
> >+nsXFormsUtils::AreNodesEqual(nsIDOMNode *aFirstNode, nsIDOMNode *aSecondNode,
> >+                             PRBool aAlreadyNormalized)
> >+{
> ...
> >+
> >+  rv1 = aFirstNode->GetNodeName(buffer1);
> >+  rv2 = aSecondNode->GetNodeName(buffer2);
> >+  if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
> >+    return PR_FALSE;
> >+  }
> >+
> >+  rv1 = aFirstNode->GetLocalName(buffer1);
> >+  rv2 = aSecondNode->GetLocalName(buffer2);
> >+  if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
> >+    return PR_FALSE;
> >+  }
> >+
> >+  rv1 = aFirstNode->GetNamespaceURI(buffer1);
> >+  rv2 = aSecondNode->GetNamespaceURI(buffer2);
> >+  if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
> >+    return PR_FALSE;
> >+  }
> >+
> >+  rv1 = aFirstNode->GetPrefix(buffer1);
> >+  rv2 = aSecondNode->GetPrefix(buffer2);
> >+  if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
> >+    return PR_FALSE;
> >+  }
> 
> Do prefixes have to be same?  isn't ns1:foo and ns2:foo the same if ns1 and ns2
> are the same namespace?
> 

I also thought that it would be enough to just test the namespaceURI, but prefixes are spelled out specifically in DOM3's isEqualNode, so I am testing for them specifically, too.

Comment 26

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

Comment on attachment 202437 [details] [diff] [review]
update to use accessors


>+  /**
>+   * nsIDOMElement that the control is representing.
>+   */
>+  nsIDOMElement getElement();

Do you need this for anything?

>+
>+  /**
>+   * The node bound to the XForms control (i.e. via copy element)
>+   */
>+  nsIDOMNode getContent();
>+
>+  /**
>+   * Set the elements contained under the node bound (i.e. via copy element)
>+   *
>+   * @param aNode         aNode should be a copy of the bound node.  setContent
>+   *                      will take the contents of aNode and move them under
>+   *                      bound node.
>+   */
>+  void setContent(in nsIDOMNode aNode);

Perhaps better names for these methods? Or at least better comment for getContent()


> NS_IMETHODIMP
>+nsXFormsItemElement::SelectItemByNode(nsIDOMNode *aNode, nsIDOMNode **aSelected)
>+{
>+  NS_ENSURE_ARG_POINTER(aSelected);
>+  NS_ENSURE_STATE(mElement);
>+  PRBool isCopyItem;
>+  GetCopyItem(&isCopyItem);
>+  if (!isCopyItem) {
>+    // If this item doesn't contain a copy element but instead has a value
>+    // element, then there is no sense continuing.
>+    *aSelected = nsnull;
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIDOMNode> copyNode;
>+  nsresult rv = GetCopyNode(getter_AddRefs(copyNode));
>+  if (NS_SUCCEEDED(rv)) {
>+    if(copyNode) {
>+      PRUint16 nodeType;
>+      copyNode->GetNodeType(&nodeType);
>+      if(nodeType == nsIDOMNode::ELEMENT_NODE)  {
>+        // to maintain compatibility with formsPlayer and Novell IE plugins,
>+        // can't select items containing copy elements that don't bind to
>+        // element nodes.
>+        if (nsXFormsUtils::AreNodesEqual(copyNode, aNode)) {
>+          NS_ADDREF(*aSelected = mElement);
>+        } else {
>+          *aSelected = nsnull;
>+        }
>+      } else {
>+        *aSelected = nsnull;
>+      }
>+    }
>+  } else {
>+    *aSelected = nsnull;
>+  }
>+
>+  return NS_OK;
>+}

Set *aSelected = nsnull only once, in the beginning of the method.

>Index: nsXFormsMDGEngine.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsMDGEngine.cpp,v
>retrieving revision 1.15
>diff -u -8 -p -r1.15 nsXFormsMDGEngine.cpp
>--- nsXFormsMDGEngine.cpp	25 Jul 2005 14:29:47 -0000	1.15
>+++ nsXFormsMDGEngine.cpp	9 Nov 2005 23:50:48 -0000

Allan might want to say something about these changes.


>+/* static */ PRBool
>+nsXFormsUtils::AreEntitiesEqual(nsIDOMNamedNodeMap *aEntities1,
>+                                nsIDOMNamedNodeMap *aEntities2)

>+/* static */ PRBool
>+nsXFormsUtils::AreNotationsEqual(nsIDOMNamedNodeMap *aNotations1,
>+                                 nsIDOMNamedNodeMap *aNotations2)

Do we actually have to test Entities and Notations?
I'd like to see an XForms use case which is using Entities or Notations in such
way that these methods are actually needed (doesn't need to work in Mozilla right now, 
but if full DOM 2 & 3 Core are implemented eventually...).


>+copyError            = XForms Error (25): Error a copy element was found whose parent is not an itemset element


Should this be:
XForms Error (25): A copy element was found whose parent is not an itemset element

Comment 27

[aaronr]

> >@@ -95,16 +95,23 @@ interface nsIModelElementPrivate : nsIXF
> > 
> >   /**
> >    * Get the value of an instance node.
> >    */
> >   void getNodeValue(in nsIDOMNode contextNode,
> >                     out AString  nodeValue);
> > 
> >   /**
> >+   * Insert a node under an instance node
> >+   */
> >+  void setNodeContent(in nsIDOMNode contextNode,
> >+                      in nsIDOMNode nodeContent,
> >+                      out boolean nodeChanged);
> >+
> >+  /**
> 
> Do we follow some sort of convention for when to use out and when to just
> return it?
> 

I just followed the convention in the file.  I don't know what the Mozilla standard is.

> 
> >Index: nsIXFormsItemElement.idl
> 
> >+  /**
> >+   * Indicates whether the item element contains a value child or a copy
> >+   * child.  We'll assume that if the item is NOT a copy item, then it must
> >+   * be a value item.  Which means that it must contain a XForms value element
> >+   * child.
> >+   */
> >+  attribute boolean copyItem;
> >+
> 
> Perhaps call it hasCopyItem?

Changed to isCopyItem.  isParentNode is the only other non-readonly boolean attribute we have in out .idl files, so consistent with that, at least.

> >+#if 0
> >+    rv1 = ent1->GetInputEncoding(buffer1);
> >+    rv2 = ent2->GetInputEncoding(buffer2);
> >+    if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
> >+      return PR_FALSE;
> >+    }
> >+
> >+    rv1 = ent1->GetXmlEncoding(buffer1);
> >+    rv2 = ent2->GetXmlEncoding(buffer2);
> >+    if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
> >+      return PR_FALSE;
> >+    }
> >+    rv1 = ent1->GetXmlVersion(buffer1);
> >+    rv2 = ent2->GetXmlVersion(buffer2);
> >+    if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
> >+      return PR_FALSE;
> >+    }
> >+#endif
> 
> Probably should be removed?

I'm afraid if we remove them, that we'll never think to put them back in or even know when Mozilla supports them for that matter.  I added an XXX so that if someone comes across it in the code they'll probably take the time to check.

> 
> >+
> >+    // the order of the attributes on the two nodes doesn't matter.  But
> >+    // every attribute on node1 must exist on node2 (and no more)
> >+    for (PRUint32 i = 0; i < attrLength1; ++i) {
> >+      nsCOMPtr<nsIDOMNode> attr1, attr2;
> >+      rv1 = attrs1->Item(i, getter_AddRefs(attr1));
> >+      if (!attr1) {
> >+        return PR_FALSE;
> >+      }
> >+
> >+      attr1->GetLocalName(buffer1);
> >+      attr1->GetNamespaceURI(buffer2);
> >+      attrs2->GetNamedItemNS(buffer2, buffer1, getter_AddRefs(attr2));
> >+      if (!attr2) {
> >+        return PR_FALSE;
> >+      }
> >+
> >+      rv1 = attr1->GetNodeValue(buffer1);
> >+      rv2 = attr2->GetNodeValue(buffer2);
> >+      if (NS_FAILED(rv1) || NS_FAILED(rv2) || !buffer1.Equals(buffer2)) {
> >+        return PR_FALSE;
> >+      }
> >+    }
> >+  }
> 
> Problem with using buffer1/buffer2 as variable names is that it makes the code
> less readable.
> 

Yeah, but nsAutoStrings take up quite a bit of local stack space and these strings are used all over the place, so that would make the local stack pretty huge.  Just trying to make cuts where I can.  I can certainly make this more readable if you guys think that would be better.

> >+          // if this item contains a copy element AND if the bound node contains
> >+          // non-text elements, then see if any of these non-text elements match
> >+          // this copyItem's node.
> >+          if (copyItem && this._selectedElementSize > 0) {
> >+            var item = aItemElement.QueryInterface(Components.interfaces.nsIXFormsSelectChild); 
> >+            for (var j = 0; j < this._selectedElementSize; j++ ) {
> >+              var selectedItem =
> >+                item.selectItemByNode(this._selectedElementArray[j].element);
> >+              if (selectedItem) {
> >+                this._selectedElementArray[j].hits++;
> >+                option.selected = true;
> >+                // XXX It is possible that two identical elements are under the
> >+                // bound node.  I guess we shouldn't mark one and not the other
> >+                // if there is an item in the select that matches it.  So we'll
> >+                // go through the whole list.  But this is quite an edge case
> >+                // and will cause more inefficiency just to prevent an errant
> >+                // xforms-out-of-range.
> >+              }
> >+            }
> >+          }
> Yeah, I don't think we should worry about identical elements too much.
> 
> 

What does everyone else think?  Is it better to cut down some processing or to make sure we don't inaccurately generate xforms-out-of-range in an edge case?

> > 
> >       <method name="_getSelectedValues">
> >         <body>
> >         <![CDATA[
> >           var selectedValues = "";
> >           var newSelectedControls = new Array();
> > 
> >           // select if found, unselect if not
> >           var options = this._controlArray;
> > 
> >+          var boundNode = this.accessors.getBoundNode();
> >+          if (!boundNode) {
> >+            return;
> >+          }
> >+
> >+          var contentEnvelope = null;
> >+          contentEnvelope = boundNode.cloneNode(false);
> >+          if (!contentEnvelope) {
> >+            return;
> >+          }
> 
> this if check is kinda useless, cloneNode should't fail if boundNode exists.
> 

we check to make sure clonenode worked all over our code.  Leaving this test in to stay consistent.

Comment 28

[aaronr]

Attachment: fixes smaug and doron comments

fixed smaug and doron comments except the ones that I noted in bug replies.  Allan, if you could look at least at the MDG changes and accessor changes, that would be great, too.  A full review would also be welcome of course :=)

Comment 29

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

Comment on attachment 202597 [details] [diff] [review]
fixes smaug and doron comments

r=me, but I'd like to get Allan's comment about MDG and nsIXFormsAccessors changes.

Comment 30

[Allan Beaufour]

Comment on attachment 202597 [details] [diff] [review]
fixes smaug and doron comments

> Index: nsIXFormsAccessors.idl
> ===================================================================
>    /**
>     * true, if XForms control is bound to a node in a data model.
>     */
>    boolean hasBoundNode();
> +
> +  /**
> +   * Node that the control is bound to in its data model.
> +   */
> +  nsIDOMNode getBoundNode();
> +
> +  /**
> +   * Like getBoundNode, this returns the node that this control is bound to.

So what's the difference then?

> +   * This function is meant to be used like getValue() except that it can
> +   * be used to get the complete contents of the bound node instead of just the
> +   * contents of the first textnode under the bound node.
> +   * XXX: In the future it might be found more desireable/efficient to return
> +   *      a copy (deep clone) of the bound node instead of the bound node
> +   *      itself.
> +   */
> +  nsIDOMNode getContent();

> Index: nsXFormsItemElement.cpp
> ===================================================================

>  NS_IMETHODIMP
>  nsXFormsItemElement::SelectItemByValue(const nsAString &aValue, nsIDOMNode **aSelected)
>  {
>    NS_ENSURE_ARG_POINTER(aSelected);
>    NS_ENSURE_STATE(mElement);
>    nsAutoString value;
> -  GetValue(value);
> +  nsresult rv = GetValue(value);
> +  if (NS_FAILED(rv)) {
> +    // If this item contains a copy element instead of a value element, then
> +    // it is perfectly ok for GetValue to return nothing.  But there is no
> +    // sense continuing.

So GetValue() returns an error, when there is in fact no error? Something's wrong here. At least that you do not check mIsCopyItem, so any error from GetValue() will be "converted" to NS_OK. Badness.

>  NS_IMETHODIMP
> +nsXFormsItemElement::SelectItemByNode(nsIDOMNode *aNode, nsIDOMNode **aSelected)
> +{
> +  NS_ENSURE_ARG_POINTER(aSelected);
> +  NS_ENSURE_STATE(mElement);
> +  PRBool isCopyItem;
> +  *aSelected = nsnull;
> +
> +  GetIsCopyItem(&isCopyItem);
> +  if (!isCopyItem) {
> +    // If this item doesn't contain a copy element but instead has a value
> +    // element, then there is no sense continuing.
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsIDOMNode> copyNode;
> +  nsresult rv = GetCopyNode(getter_AddRefs(copyNode));
> +  if (NS_SUCCEEDED(rv)) {
> +    if (copyNode) {

Again, you convert an error to NS_OK, are you sure you want to do that? At least merge the if statements, and do early return.

> +      PRUint16 nodeType;
> +      copyNode->GetNodeType(&nodeType);
> +      if (nodeType == nsIDOMNode::ELEMENT_NODE)  {
> +        // to maintain compatibility with formsPlayer and Novell IE plugins,
> +        // can't select items containing copy elements that don't bind to
> +        // element nodes.

The comment should be before the if, and the spec states this pretty clearly:
"The target node, selected by the binding attributes on the list form control, must be an element node, otherwise an exception results"
(http://www.w3.org/TR/2005/PER-xforms-20051006/slice9.html#ui-adv-copy)
nsIXFormsCopyElement should never return a node, if it is not an element. This class should not care about that.

> +NS_IMETHODIMP
> +nsXFormsItemElement::GetCopyNode(nsIDOMNode **aNode)
> +{
> +  NS_ENSURE_ARG_POINTER(aNode);
> +
> +  PRBool isCopyItem;
> +  GetIsCopyItem(&isCopyItem);
> +  if (!isCopyItem) {
> +    // If this item doesn't contain a copy element but instead has a value
> +    // element, then there is no sense continuing.
> +    *aNode = nsnull;
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<nsIDOMNode> firstChild, container;
> +  mElement->GetFirstChild(getter_AddRefs(firstChild));
> +
> +  // If this element is generated inside an <itemset>, there is a
> +  // <contextcontainer> element between this element and the actual childnodes.
> +  // If this item really contains a copy element, then firstChild MUST be
> +  // a contextcontainer since copy elements can only exist as a child of an
> +  // itemset.
> +  if (!nsXFormsUtils::IsXFormsElement(firstChild,
> +                                      NS_LITERAL_STRING("contextcontainer"))) {
> +    // should never have reached here with GetIsCopyItem returning true.
> +    *aNode = nsnull;
> +    SetIsCopyItem(PR_FALSE);
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  container = firstChild;

? kill one of them

> +  
> +  // Find our copy child and get its node.

Not exactly what you do, please fix this comment.

> +  nsCOMPtr<nsIDOMNodeList> children;
> +  nsresult rv = container->GetChildNodes(getter_AddRefs(children));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  PRUint32 childCount;
> +  children->GetLength(&childCount);
> +
> +  nsCOMPtr<nsIDOMNode> child;
> +  nsAutoString value;
> +
> +  for (PRUint32 i = 0; i < childCount; ++i) {
> +    children->Item(i, getter_AddRefs(child));
> +    nsCOMPtr<nsIXFormsCopyElement> copyElement = do_QueryInterface(child);
> +    if (copyElement) {
> +      return copyElement->GetCopyNode(aNode);
> +    }
> +  }     
> +
> +  // No copy element as a child.  Set return node to null and I guess set the
> +  // copyitem boolean to false.

you guess?

> Index: nsXFormsMDGEngine.cpp
> ===================================================================

> @@ -738,16 +739,137 @@ nsXFormsMDGEngine::SetNodeValueInternal(
>    }
>    if (aMarkNode) {
>        MarkNodeAsChanged(aContextNode);
>    }
   
>    return NS_OK;
>  }
 
> +nsresult
> +nsXFormsMDGEngine::SetNodeContent(nsIDOMNode       *aContextNode,
> +                                  nsIDOMNode       *aNodeContent,
> +                                  PRBool           *aNodeChanged)
> +{
> +  return SetNodeContentInternal(aContextNode,
> +                                aNodeContent,
> +                                PR_TRUE,
> +                                PR_FALSE,
> +                                aNodeChanged);
> +}

? You are not actually using SetNodeContentInternal() anywhere (which is the reason for the split of SetNodeValue()).

> +
> +nsresult
> +nsXFormsMDGEngine::SetNodeContentInternal(nsIDOMNode       *aContextNode,
> +                                          nsIDOMNode       *aContentEnvelope,
> +                                          PRBool            aMarkNode,
> +                                          PRBool            aIsCalculate,
> +                                          PRBool           *aNodeChanged)
> +{
> +  NS_ENSURE_ARG(aContextNode);
> +  NS_ENSURE_ARG(aContentEnvelope);
> +
> +  // ok, this is tricky.  This function will REPLACE the contents of
> +  // aContextNode with the CONTENTS of aContentEnvelope.  No, not a clone of 
> +  // the contents, but the contents themselves.  If aContentEnvelope has no
> +  // contents, then any contents that aContextNode has will still be removed.
> +  // In order to determine whether the incoming node content is the same as what
> +  // is already contained in aContextNode, aContentEnvelope MUST be a clone (not
> +  // deep) of aContextNode, otherwise aNodeChanged will always be returned as
> +  // being PR_TRUE.  I took this approach because I think it is much more
> +  // efficient for the caller to build a complete list of what goes in the
> +  // contents in one go rather than allowing any number of appends to existing
> +  // content one node at a time.  There are quite a few links in the call chain
> +  // to go from nsXFormsDelegateStub to here.

Not the best structured comment, but I'll probably live. (or not, *gasp*, air!, *argh* ;-) )

> +  // remove any child nodes that aContextNode already contains
> +  PRBool hasChildNodes;
> +  rv = aContextNode->HasChildNodes(&hasChildNodes);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIDOMNode> resultNode;
> +  if (hasChildNodes) {

You do not need the HasChildNodes() call, the iteration will handle that.

> +    nsCOMPtr<nsIDOMNodeList> childList;
> +    rv = aContextNode->GetChildNodes(getter_AddRefs(childList));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    if (childList) {
> +      PRUint32 length;
> +      rv = childList->GetLength(&length);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      for (int i=length; i>0; i--) {

PRUint32, and spaces bewteen operators

> +        nsCOMPtr<nsIDOMNode> childNode;
> +        rv = childList->Item(i-1, getter_AddRefs(childNode));

Is it more efficient to remove in the reverse direction, or? And please use (i = length - 1; i >= 0, so you have the correct "i" instead of this odd "-1".

> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        rv = aContextNode->RemoveChild(childNode, getter_AddRefs(resultNode));
> +        NS_ENSURE_SUCCESS(rv, rv);
> +      }
> +    }
> +  }
> +
> +  // add contents of the envelope under aContextNode
> +  rv = aContentEnvelope->HasChildNodes(&hasChildNodes);
> +  NS_ENSURE_SUCCESS(rv, rv);

Again, you do not need that.

> +  if (hasChildNodes) {
> +    nsCOMPtr<nsIDOMNode> childNode;
> +    rv = aContentEnvelope->GetFirstChild(getter_AddRefs(childNode));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    while (childNode) {
> +      rv = aContextNode->AppendChild(childNode, getter_AddRefs(resultNode));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      rv = aContentEnvelope->GetFirstChild(getter_AddRefs(childNode));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +    }
> +  }
> +
> +  // NB: Never reached for Readonly nodes.  
> +  if (aNodeChanged) {
> +    *aNodeChanged = PR_TRUE;
> +  }
> +  if (aMarkNode) {
> +    MarkNodeAsChanged(aContextNode);
> +  }

Does not make much sense to mark it changed. After all this messing with the instance data, the MDG is out of sync. That's why copy needs to do a "full computational dependency rebuild", as states in the spec. You are triggering a rebuild, etc. somewhere right?

> Index: nsXFormsUtils.cpp
> ===================================================================

Oh boy, the Are...Equal() functions are ugly and costly. But I guess that is unavoidable at this stage.

> Index: nsXFormsUtils.h
> ===================================================================
> +  /**
> +   * Returns whether the given nodes are equal as described in the isEqualNode
> +   * function defined in the DOM Level 3 Core spec.
> +   * http://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/DOM3-Core.html#core-Node3-isEqualNode
> +   *

Please include an XXX, and a link to the isEqualNode bug about why this is only here temporaryli -- which I hope it is?

> +   * @param aFirstNode          The first node to compare
> +   * @param aSecondNode         The second node to compare
> +   * @param aAlreadyNormalized  Whether the two nodes and their children, etc.
> +   *                            have already been normalized to allow for
> +   *                            more accurate child node comparisons, as
> +   *                            recommended in the DOM Level 3 Core spec.
> +   */
> +  static NS_HIDDEN_(PRBool) AreNodesEqual(nsIDOMNode *aFirstNode,
> +                                          nsIDOMNode *aSecondNode,
> +                                          PRBool      aAlreadyNormalized = PR_FALSE);

> Index: resources/content/select.xml
> ===================================================================
 
>        <method name="_buildSelect">
> +        <parameter name="aContainsNonText"/>
>          <body>
>          <![CDATA[
>            // select builds its own UI by parsing it's children.
 
> -          // replace new line (\n), tabs (\t) and carriage returns (\r) with "".
> -          var value = "";
> -
> -          if (this.accessors.getValue())
> -            value = this.accessors.getValue().replace(/\n|\t|\r/g, " ");
> +          // if delegate.value has something, then only text node(s) should
> +          // exist under the bound node
 
> -          // get an array of values selected in the bound node
> -          var selectedArray = value.split(" ");
> +          // holds an array of DOMElements that exist under bound node,
> +          this._selectedElementSize = 0;
> +          this._selectedElementArray = new Array();
> +
> +          if (!aContainsNonText) {
> +            // replace new line (\n), tabs (\t) and carriage returns (\r) with
> +            // "".
> +            var value = "";
> +            var accessValue = this.accessors.getValue();
> +  
> +            if (accessValue)
> +              value = accessValue.replace(/\n|\t|\r/g, " ");
> +  
> +            // get an array of values selected in the bound node
> +            var selectedArray = value.split(" ");
> +  
> +            // create a hash from the default values so we can store how often
> +            // we encountered them.  This allows us to figure out later if any
> +            // were not hit, which requires us to send an event.
> +            this._defaultHash = new Object();
> +            for (var run = 0; run < selectedArray.length; run++) {
> +              this._defaultHash[selectedArray[run]] = {hits: 0}
> +            }
> +          } else {
> +            var boundNode = this.accessors.getBoundNode();
> +            var child = boundNode ? boundNode.firstChild : null;
> +            this._defaultHash = null;
> +            while (child) {
> +              var type = child.nodeType;
> +              if (type == Node.TEXT_NODE || 
> +                  type == Node.CDATA_SECTION_NODE) {
> +                // if child is a text node completely filled with
> +                // whitespace let's ignore it and get the next node
> +                var string = child.nodeValue;
> +                var nonWhitespace = false;
> +                if (string) {
> +                  // this regexp tests whether only whitespace is contained
> +                  // between the beginning and ending of the string.
> +                  nonWhitespace = !(/^\s*$/.test(string));
> +                  if (nonWhitespace) {
> +                    break;
> +                  }
> +                }
> +                if (nonWhitespace) {

When will this ever be reached? If it evals to true you break out.

> +                  // replace new line (\n), tabs (\t) and carriage returns (\r)
> +                  // with " ".
> +                  var value = string.replace(/\n|\t|\r/g, " ");
> +                 
> +                  // get an array of values selected in the bound node
> +                  var selectedArray = value.split(" ");
> +      
> +                  // create a hash from the default values so we can store how
> +                  // often we encountered them.  This allows us to figure out
> +                  // later if any were not hit, which requires us to send an
> +                  // event.
> +                  if (!this._defaultHash) {
> +                    this._defaultHash = new Object();
> +                  }
> +                  for (var run = 0; run < selectedArray.length; run++) {
> +                    this._defaultHash[selectedArray[run]] = {hits: 0}
> +                  }
> +                }
> +              } else {
> +                // if its not a text node, we'll assume that we are looking at

it's

> +                // a node worth comparing, I guess, and as such look for an

again, you are guessing?

> +                // item with a copy element that might match this node.
> +                this._selectedElementArray[this._selectedElementSize] = 
> +                  {element: child, hits: 0}
> +                this._selectedElementSize++;
> +              }
 
> Index: resources/content/select1.xml
> ===================================================================

>        <method name="refresh">
>          <body>
>            <![CDATA[
>            try {
> -            var newValue = this.stringValue;
> +            var nodeValue = null, newValue = null;
> +            var boundNode = this.accessors.getBoundNode();
> +            var outOfRange = false;
> +            if (boundNode && boundNode.hasChildNodes()) {
> +              // Since this is a select1, there should normally be just one
> +              // child node here.  But no guarantee that a select1 generated
> +              // the value coming in.  So we'll look for text node with 
> +              // non-whitespace characters to compare with an item's xf:value.
> +              // Any other node that we encounter we look to match with an
> +              // item's xf:copy.  If more than one of either of these exists
> +              // in the instance data, we need to generate a xforms-out-of-range
> +              // event and style the select1 as out-of-range since by
> +              // definition a select1 can not select more than one item.
> +              var child = boundNode.firstChild;
> +              while (child) {
> +                var type = child.nodeType;
> +                if (type == Node.TEXT_NODE) {
> +                  // if child is a text node completely filled with
> +                  // whitespace let's ignore it and get the next node
> +                  var string = child.nodeValue;
> +                  var nonWhitespace = false;
> +                  if (string) {
> +                    for (var i=0; i<string.length; i++) {
> +                      nonWhitespace = !(/^\s/.test(string.charAt(i)));
> +                      if (nonWhitespace) {
> +                        break;
> +                      }
> +                    }
> +                  }
> +                  if (nonWhitespace) {

Same comment as above.


> --- nsXFormsCopyElement.cpp	1969-12-31 18:00:00.000000000 -0600

> +NS_IMETHODIMP
> +nsXFormsCopyElement::GetCopyNode(nsIDOMNode **aNode)
> +{
> +  if (!aNode) {
> +    return NS_ERROR_FAILURE;
> +  }

Use NS_ENSURE..

> +  *aNode = nsnull;
> +
> +  nsCOMPtr<nsIModelElementPrivate> model;
> +  nsCOMPtr<nsIDOMXPathResult> result;
> +  nsresult rv = nsXFormsUtils::EvaluateNodeBinding(mElement,
> +    nsXFormsUtils::ELEMENT_WITH_MODEL_ATTR, NS_LITERAL_STRING("ref"), 
> +    EmptyString(), nsIDOMXPathResult::FIRST_ORDERED_NODE_TYPE,
> +    getter_AddRefs(model), getter_AddRefs(result));

The indentation is bad here, push the function call down on the next line too.

> +
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (result) {
> +    nsCOMPtr<nsIDOMNode> singleNode;
> +    result->GetSingleNodeValue(getter_AddRefs(singleNode));
> +
> +    if (singleNode) {
> +      NS_IF_ADDREF(*aNode = singleNode);

Either check it or IF it, no reason to do both.

> --- nsIXFormsCopyElement.idl	1969-12-31 18:00:00.000000000 -0600
> +/**
> + * This interface is implemented by XForms copy elements.
> + */

nit: \<copy\>

I've only looked very quickly on select.xml and select1.xml, I'm guessing that Olli and Doron has gotten that.

Comment 31

[aaronr]

Attachment: fixes Allan's comments other than noted

Comment 32

[aaronr]

(In reply to comment #30)
> (From update of attachment 202597 [details] [diff] [review] [edit])
> > Index: nsIXFormsAccessors.idl
> > ===================================================================
> >    /**
> >     * true, if XForms control is bound to a node in a data model.
> >     */
> >    boolean hasBoundNode();
> > +
> > +  /**
> > +   * Node that the control is bound to in its data model.
> > +   */
> > +  nsIDOMNode getBoundNode();
> > +
> > +  /**
> > +   * Like getBoundNode, this returns the node that this control is bound to.
> 
> So what's the difference then?

No difference right now.  I figured that if there is a setContent, then there should be a getContent.  And as noted in the XXX, in the future it could end up being better to use a clone of bound node rather than a live bound node for a contentEnvelope and then getContent would change to accommodate this.

> > +    nsCOMPtr<nsIDOMNodeList> childList;
> > +    rv = aContextNode->GetChildNodes(getter_AddRefs(childList));
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +    if (childList) {
> > +      PRUint32 length;
> > +      rv = childList->GetLength(&length);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +      for (int i=length; i>0; i--) {
> 
> PRUint32, and spaces bewteen operators
> 
> > +        nsCOMPtr<nsIDOMNode> childNode;
> > +        rv = childList->Item(i-1, getter_AddRefs(childNode));
> 
> Is it more efficient to remove in the reverse direction, or? And please use (i
> = length - 1; i >= 0, so you have the correct "i" instead of this odd "-1".

I changed it to use PRInt32 instead of PRUint32 since PRUint32 won't work with
this suggestion.  After i=0, i will equal 0xffffffff which will cause this to keep running if i is a PRUint32.

> Does not make much sense to mark it changed. After all this messing with the
> instance data, the MDG is out of sync. That's why copy needs to do a "full
> computational dependency rebuild", as states in the spec. You are triggering a
> rebuild, etc. somewhere right?

I'm not currently triggering a rebuild, but I know that I need to (already noted in this bug, in my code with XXX's and tested by a testcase that I attached here).  I want to get this already-too-huge patch in and then I'll fix the fact that it isn't rebuilding.  I'll probably have to add a flag to setContent as to whether it should cause a rebuild on change and I also have to figure out a clean way to cause a rebuild if a copyItem is deselected in the case where it was the only copyItem selected to begin with (and thus won't go through setContent).

> > +                if (string) {
> > +                  // this regexp tests whether only whitespace is contained
> > +                  // between the beginning and ending of the string.
> > +                  nonWhitespace = !(/^\s*$/.test(string));
> > +                  if (nonWhitespace) {
> > +                    break;
> > +                  }
> > +                }
> > +                if (nonWhitespace) {
> 
> When will this ever be reached? If it evals to true you break out.

Doh!  Nice catch.  Thanks!  I had this in an interator going character by character and tor suggested doing the whole string at once (which I didn't know as possible) and I missed removing the if test.  I fixed this in select1.xml, too (added the test for the whole string at once and removed the if nonwhitespace test).

Comment 33

[Allan Beaufour]

(In reply to comment #32)
> (In reply to comment #30)
> > (From update of attachment 202597 [details] [diff] [review] [edit] [edit])
> > > Index: nsIXFormsAccessors.idl
> > > ===================================================================
> > >    /**
> > >     * true, if XForms control is bound to a node in a data model.
> > >     */
> > >    boolean hasBoundNode();
> > > +
> > > +  /**
> > > +   * Node that the control is bound to in its data model.
> > > +   */
> > > +  nsIDOMNode getBoundNode();
> > > +
> > > +  /**
> > > +   * Like getBoundNode, this returns the node that this control is bound to.
> > 
> > So what's the difference then?
> 
> No difference right now.  I figured that if there is a setContent, then there
> should be a getContent.  And as noted in the XXX, in the future it could end up
> being better to use a clone of bound node rather than a live bound node for a
> contentEnvelope and then getContent would change to accommodate this.

I do not like having this NO-OP around. Especially when it only "might be found more desirable..." to implement it differently. We might never do that, and then we need to have this function around for compatibility reasons forever. I vote for killing this function, and putting in when we actually do something intelligent with it.

Comment 34

[Allan Beaufour]

Comment on attachment 203078 [details] [diff] [review]
fixes Allan's comments other than noted

> Index: nsXFormsItemElement.cpp
> ===================================================================
>  NS_IMETHODIMP
>  nsXFormsItemElement::SelectItemByValue(const nsAString &aValue, nsIDOMNode **aSelected)
>  {
>    NS_ENSURE_ARG_POINTER(aSelected);
>    NS_ENSURE_STATE(mElement);
>    nsAutoString value;
> -  GetValue(value);
> +  nsresult rv = GetValue(value);
> +  if (NS_FAILED(rv)) {
> +    PRBool isCopyItem;
> +    GetIsCopyItem(&isCopyItem);
> +    if (isCopyItem) {
> +      // If this item contains a copy element instead of a value element, then
> +      // it is perfectly ok for GetValue to return nothing.  But there is no
> +      // sense continuing.
> +      *aSelected = nsnull;
> +      return NS_OK;
> +    } else {
> +      return rv;
> +    }
> +  }
> +
>    if (aValue.Equals(value)) {
>      NS_ADDREF(*aSelected = mElement);
>    } else {
>      *aSelected = nsnull;
>    }
 
>    return NS_OK;
>  }

... GetValue() sets mIsCopyItem, and always return an error when it does.

*aSelected = nsnull;
nsresult rv = GetValue(value);
if (mIsCopyItem) {
  // XXX ... A comment like: Copy elements are selected by node 
  rv = NS_OK;
} else if (NS_SUCEEDED(rv) && aValue.Equals(value)) {
  NS_ADDREF(*aSelected = mElement);  
}

return rv;


 
>  NS_IMETHODIMP
> +nsXFormsItemElement::SelectItemByNode(nsIDOMNode *aNode, nsIDOMNode **aSelected)
> +{
> +  NS_ENSURE_ARG_POINTER(aSelected);
> +  NS_ENSURE_STATE(mElement);
> +  PRBool isCopyItem;
> +  *aSelected = nsnull;
> +
> +  // If this item doesn't contain a copy element but instead has a value
> +  // element, then there is no sense testing further.
> +  GetIsCopyItem(&isCopyItem);
> +  if (!isCopyItem) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<nsIDOMNode> copyNode;
> +  nsresult rv = GetCopyNode(getter_AddRefs(copyNode));
> +  NS_ENSURE_TRUE(copyNode, rv);

NS_ENSURE_STATE(copyNode);

> +
> +  PRUint16 nodeType;
> +  copyNode->GetNodeType(&nodeType);
> +
> +  // copy elements are only allowed to bind to ELEMENT_NODEs per spec.  But
> +  // test first before doing all of this work.
> +  if (nodeType == nsIDOMNode::ELEMENT_NODE)  {
> +    if (nsXFormsUtils::AreNodesEqual(copyNode, aNode)) {

Merge the ifs!

> +      NS_ADDREF(*aSelected = mElement);
> +    }
> +  }
> +
> +  return NS_OK;
> +}
 
> +NS_IMETHODIMP
> +nsXFormsItemElement::GetCopyNode(nsIDOMNode **aNode)
> +{
> +  NS_ENSURE_ARG_POINTER(aNode);
> +
> +  PRBool isCopyItem;
> +  GetIsCopyItem(&isCopyItem);
> +  if (!isCopyItem) {
> +    // If this item doesn't contain a copy element but instead has a value
> +    // element, then there is no sense continuing.
> +    *aNode = nsnull;
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<nsIDOMNode> firstChild, container;
> +  mElement->GetFirstChild(getter_AddRefs(firstChild));
> +
> +  // Since this item really contains a copy element, then firstChild MUST be
> +  // a contextcontainer since copy elements can only exist as a child of an
> +  // itemset.
> +  container = firstChild;

You still have one unnecessary variable here. Kill one of them.

With these nits fixed, and getContent() killed r=me.

Comment 35

[aaronr]

Attachment: ready for checkin

fixed Allan's nits, removed GetContent().

Comment 36

[Doron Rosenberg (IBM)]

checked into trunk

Comment 37

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

Removing P1 priority so that 
https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=&component=XForms&long_desc_type=substring&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&resolution=DUPLICATE&resolution=---&priority=P1&emailassigned_to1=1&emailtype1=exact&email1=&emailassigned_to2=1&emailreporter2=1&emailqa_contact2=1&emailtype2=exact&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=
somehow usable.

Comment 38

[David Landwehr]

It seems to me that the setContent of the nsIXFormsAccessors interface is there to handle the copy element ? Would it be possible to make a specific accessor interface for select and select1 instead? 
The workings of setContent is a bit off to me, since the user of the interface will have to use a cloned node because of the _move_, it also requires that the user ensures that the node he passes into setContent is from the same DOM (otherwise he will get a DOMException). Should there be some additional logic in here to handle that case? 

Comment 39

[aaronr]

(In reply to comment #38)
> It seems to me that the setContent of the nsIXFormsAccessors interface is there
> to handle the copy element ? Would it be possible to make a specific accessor
> interface for select and select1 instead? 
> The workings of setContent is a bit off to me, since the user of the interface
> will have to use a cloned node because of the _move_, 

Yeah, I struggled with this for a long time.  I figured that if we are going to have an interface into accessors just for copy, I should make it something that  a form author might actually use.  So I thought up setContent and the idea of an envelope.  I agree that it certainly has some rough spots :=).  But if I used seperate interfaces for select and select1, then it seemed to me that it would be even more complicated for the form author to know which to use.  Can you give me an example of what you think that the interfaces should look like?

> it also requires that the
> user ensures that the node he passes into setContent is from the same DOM
> (otherwise he will get a DOMException). Should there be some additional logic
> in here to handle that case? 
> 

I don't get what you mean.  Do you mean that if the 'contentEnvelope' isn't from the same document as the node that you are setting the content of?  In the comments for setContent I say that aNode should be a copy of the bound node, but I guess that I don't enforce that anywhere, do I?  Is that what you mean?

I could check to make sure that the envelope and bound node have the same owning document a lot earlier in the process.  But do you want just a better error, an error reported to the console, or should I make a new envelope and move the contents over and pass the new envelope up the chain? 

Comment 40

[aaronr]

David and I came to one agreement.  I'll move the responsibility for cloning from the script author to the processor.  So no matter what the script author sends in setContent, we'll clone its contents and insert them under the bound node.  I will leave our logic that just clears the contents of the bound node and sets it to the contents of the content envelope.  But we'll leave further optimization for a time in the future.  David wants us to do a lot of testing so that if the node structure coming in in the same as what is there (the most likely scenario), we'd just have to modify values in the current structure without having to do all the clearing and cloning involved in building the new structure to replace the old structure.  I'm thinking that the level of detailed testing that would be required (on the level of isEqualNode, but perhaps not quite that much detail) is so much that in most cases the benefit of the testing over the clear 'n set method would be minimal if at all.  When we get some real world testcases with large data strutures, we can really test the performance to see.

I will make the setContent change in a future bug, probably the rebuild bug (bug  316895)

Comment 41

[aaronr]

checked into MOZILLA_1_8_BRANCH via bug 323691.  Leaving open for now until it gets into 1.8.0

Comment 42

[aaronr]

verfied fixed on MOZILLA_1_8_BRANCH