368583 - Support XForms 1.1 insert action

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

We've had numerous requests for support of the 1.1 insert action to allow for easier copying of instance data between instance documents, especially when the documents live in different models.

Comment 1

[alexander :surkov (:asurkov)]

(In reply to comment #0)
> We've had numerous requests for support of the 1.1 insert action to allow for
> easier copying of instance data between instance documents, especially when the
> documents live in different models.
> 

Do you suppose this for trunk only or for branch too?

Comment 2

[aaronr]

Attachment: patch1

Here is an old patch written by Denis Scherbakov and attached to the newsgroup.  We need to resync it to trunk and finish up the work.  Thanks Denis for doing all of this work!

When we are done with it we'll have to put it under the same level of 'protection' as we do with our SOAP patch (need a pref to activate it, trunk only, etc).  Until the 1.1 spec is finished, of course.

Comment 3

[aaronr]

(In reply to comment #1)
> (In reply to comment #0)
> > We've had numerous requests for support of the 1.1 insert action to allow for
> > easier copying of instance data between instance documents, especially when the
> > documents live in different models.
> > 
> 
> Do you suppose this for trunk only or for branch too?
> 

Like the SOAP stuff, I think we should keep it trunk only until the 1.1 spec reaches some level of finality.  At least 'last call'.  We've still got a lot of 1.0 errata stuff to implement.  We don't have time to track changes to 1.1, too.

Comment 4

[aaronr]

Attachment: testcase1

Comment 5

[aaronr]

Attachment: patch1 updated to the trunk

Updated the original patch from the NG (patch1) to compile and work on the trunk.  Cleaned up some styling issues, some extra whitespaces, extraneous line insertions/deletions.  The patch essentially works against the testcase, though there are a few changes to insert since this patch was written.

We still need to add:
1) @at and @position no longer required in 1.1, so need to handle those possibilities.
2) missing some context node determination rules (like if the binding nodeset is empty)
3) new @model handling rules, nodeset being document root handling, etc.
4) checking of node types so that an attribute node can't be inserted as a child of an element node, for example.

I'll try to get to those tonight/tomorrow.

Comment 6

[aaronr]

Merle's going to finish this up for me.

Comment 7

[Merle Sterling]

Attachment: patch

This patch implements all of the rules for insert AND delete from the 1.1 spec. The order of processing all of the optional attributes is in the exact order specified in the spec.

The testcase works properly but we need to write quite a few more testcases to cover more of the allowable combinations of attributes (because they are optional). Specifically we need testcases that use @bind and @context because both of those override @nodeset.

Comment 8

[Doron Rosenberg (IBM)]

Merle, do you have someone to review the patch?

Comment 9

[Merle Sterling]

Attachment: patch2 (works for majority of testsuite)

This patch implements all of the rules for 1.1 insert/delete and works for nearly all of the 1.1 testsuite insert testcases. 

Testcases 9.3.5.a-g and 9.3.5i work 100%, testcase 9.3.5.h doesn't appear to work even without pressing the trigger to perform the insert (need to verify the testcase is correct), and testcase 9.3.5.j works for some of the inserts. The 'j' testcase tests all of the unusual error conditions like mismatched node types and that part of the code still needs some work. I think that testcase is lower priority if we want to accelerate checkin for the patch after review because they are fringe cases that can be fixed in a follow-on bug.

The two 1.0 testcases I tried appear to work as well.

Comment 10

[Merle Sterling]

(In reply to comment #8)
> Merle, do you have someone to review the patch?

I usually ask Aaron first but I understand he is not full time on XForms now. Feel free to take a look at the patch if you'd like. 

Comment 11

[aaronr]

Comment on attachment 266819 [details] [diff] [review]
patch2 (works for majority of testsuite)


One issue that we need to ensure that we handle correctly is the idea of nested binds.  You should probably include that in a testcase to see if that works correctly with insert and if it doesn't, we'll need to fix that.  It is one of the main differences between how the evaluation context stuff works in 1.0 vs. 1.1.  We have some kind of support for it, but I doubt that it is 1.1 level.  Also, please test that we correctly handle the case where @bind comes from a different model than is specified in @model.

>Index: nsXFormsInsertDeleteElement.cpp
>===================================================================

>+
>+  // Get the context node using @bind or @model.
>+  if (!modelId.IsEmpty() || !bindExpr.IsEmpty()) {
>+    nsCOMPtr<nsIDOMElement> bindElement;
>+    nsCOMPtr<nsIXFormsControl> parentControl;
>+    PRBool outerBind;
>+    rv = nsXFormsUtils::GetNodeContext(mElement,
>+                                       nsXFormsUtils::ELEMENT_WITH_MODEL_ATTR,
>+                                       getter_AddRefs(model),
>+                                       getter_AddRefs(bindElement),
>+                                       &outerBind,
>+                                       getter_AddRefs(parentControl),
>+                                       getter_AddRefs(contextNode));
>+

By running GetNodeContext here and then possibly EvaluateNodeBinding below if @bind, in that case you'll end up doing GetNodeContext twice.  What about testing for @context, if it is there (and no @model or @bind) run EvaluateNodeBinding on it to get the context node.  If not, test for @bind.  If found, run EvaluateNodeBinding on THAT to get the context node.  If neither of these, then run GetNodeContext to get the context node.

>+  } else {
>+    if (!nodesetExpr.IsEmpty()) {
>+      if (contextNode) {
>+        // Evaluate the nodeset attribute within the context.
>+        rv = nsXFormsUtils::EvaluateXPath(nodesetExpr, contextNode, contextNode,
>+                                          nsIDOMXPathResult::ORDERED_NODE_SNAPSHOT_TYPE,
>+                                          getter_AddRefs(nodeset));
>+        NS_ENSURE_SUCCESS(rv, rv);

contextNode shouldn't be used as the resolver node since it comes from a whole different document whose namespace prefixes may be different or resolve to different namespaces than exist in the XForms document from which we are grabbing the xpath expression.

>+
>+      } else {
>+        rv = nsXFormsUtils::EvaluateNodeBinding(mElement,
>+                                                nsXFormsUtils::ELEMENT_WITH_MODEL_ATTR,
>+                                                NS_LITERAL_STRING("nodeset"),
>+                                                EmptyString(),
>+                                                nsIDOMXPathResult::ORDERED_NODE_SNAPSHOT_TYPE,
>+                                                getter_AddRefs(model),
>+                                                getter_AddRefs(nodeset),
>+                                                &usesModelBinding);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+        if (!model)
>+          return NS_OK;
>+
>+      }
>+
>+      rv = nodeset->GetSnapshotLength(&nodesetSize);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+  }

Using my approach mentioned above, you'd have an insertContext by this point, so wouldn't need to call EvaluateNodeBinding here, just the EvaluateXPath above.

> 
>+  // The insert action is terminated with no effect if any of the
>+  // following conditions is true:
>+  // The context attribute is not given and the Node Set Binding node-set is
>+  // the empty node-set.

I don't see where you are testing for this.

>+  // The context attribute is given, the insert context does not evaluate to an
>+  // element node, and the Node Set Binding node-set is the empty node-set.
>+  //
>+  // The delete action is terminated with no effect if the Node Set Binding
>+  // node-set is the empty node-set.
>+  if (!nodeset || nodesetSize < 1) {
>+    if (mIsInsert) {
>+      if (!contextExpr.IsEmpty()) {
>+        PRUint16 nodeType;
>+        contextNode->GetNodeType(&nodeType);
>+        if (nodeType != nsIDOMNode::ELEMENT_NODE)
>+          return NS_OK;

The spec says we should return without any effect if the insert context is not an element.  I don't think that you should test for !contextExpr.IsEmpty() before making the test.  Also, you could move it up to test right after we figure out what the insert context should be.  So in the end you should have a standalone test for the insert context being an element and this test to make sure that if there is no @context that we then have a non-empty nodeset.

>+  //
>+  // Determine the origin node-set (if Insert).
>+  //
>+  nsCOMPtr<nsIDOMXPathResult> originNodeset;
>+  PRUint32 originNodesetSize = 0;
>+
>+  if ( mIsInsert ) {
>+    nsAutoString origin;
>+    mElement->GetAttribute(NS_LITERAL_STRING("origin"), origin);
>+
>+    // If the origin attribute is given, the origin node-set is the result of
>+    // the evaluation of the origin attribute in the insert context.
>+    if (!origin.IsEmpty()) {
>+      if (contextNode) {
>+        rv = nsXFormsUtils::EvaluateXPath(origin, contextNode, contextNode,
>+                                          nsIDOMXPathResult::ORDERED_NODE_SNAPSHOT_TYPE,
>+                                          getter_AddRefs(originNodeset));

again, contextNode shouldn't be passed as the resolver node.

>+        NS_ENSURE_SUCCESS(rv, rv);
>+      } else {
>+        nsCOMPtr<nsIModelElementPrivate> originModel;
>+        PRBool originUsesModelBinding;
>+        rv = nsXFormsUtils::EvaluateNodeBinding(mElement,
>+                                                nsXFormsUtils::ELEMENT_WITH_MODEL_ATTR,
>+                                                NS_LITERAL_STRING("origin"),
>+                                                EmptyString(),
>+                                                nsIDOMXPathResult::ORDERED_NODE_SNAPSHOT_TYPE,
>+                                                getter_AddRefs(originModel),
>+                                                getter_AddRefs(originNodeset),
>+                                                &originUsesModelBinding);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+        if (!originModel)
>+          return NS_OK;
>+      }
>+
>+      if (!originNodeset)
>+        return NS_OK;
>+
>+      rv = originNodeset->GetSnapshotLength(&originNodesetSize);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      // The insert action is terminated with no effect if the origin node-set
>+      // is the empty node-set.
>+      if (originNodesetSize < 1)
>+        return NS_OK;
>+    }
>+  }
>+

I'd suggest that here if you don't have an originNodeset that you get the last node in the Node Set Binding nodeset so that at this point originNodeset has a value.  You really don't save anything by waiting until later.

>+  //
>+  // Determine the insert/delete location node.
>+  //
>+  nsCOMPtr<nsIDOMNode> locationNode;
>   PRUint32 atInt = 0;
>-  double atDoub;
>-  rv = at->GetNumberValue(&atDoub);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  double atDoub = 0;
> 
>-  /// XXX we need to check for NaN, and select last row. but isnan() is not
>-  /// portable :(
>-  if (atDoub < 1) {
>-    atInt = 1;
>+  nsAutoString atExpr;
>+  mElement->GetAttribute(NS_LITERAL_STRING("at"), atExpr);
>+
>+  if (atExpr.IsEmpty()) {
>+    if (mIsInsert) {
>+      // The insert location node is the last node of the Node Set Binding
>+      // node-set.
>+      nodeset->SnapshotItem(nodesetSize - 1, getter_AddRefs(locationNode));
>+      NS_ENSURE_STATE(locationNode);
>+    }
>+    // else there is no delete location and each node in the Node Set Binding
>+    // node-set will be deleted unless the node is the root document element
>+    // of an instance.
>   } else {

Here you seem to assume that if there is no @at then there has to be a nodeset.  This isn't necessarily true.  If there is no @bind or @nodeset but there is a @context, then you won't have a Node Set Binding nodeset.  In this case, your locationNode should be the insert context.

>+  //
>+  // Do the insert or delete.
>+  //
>+  // XXX: Insert - The clones are deep copies of the original nodes except the
>+  // contents of nodes of type xsd:ID are modified to remain as unique values
>+  // in the instance data after the clones are inserted.

Which part is the XXX for?  If it is for the 'unique values' part of the comment, I would suggest that you add something like, "and we still need to do the uniqueness piece" or something like that.  Your comment is a statement of functionality, which is good, but the part that is XXX'd needs to be clearly stated as a todo.  And you should open a bug on it and mention the bug number here.

>+    // If the cloned node is an attribute, then the target location is before
>+    // the first attribute of the insert location node. If the cloned node is
>+    // not an attribute, then the target location is before the first child
>+    // of the insert location node.
>+    if (!nodeset ||
>+       (nodeset && nodesetSize > 1 && newNodeType != locationNodeType)) {
>+      InsertNode(locationNode, newNode, PR_FALSE, getter_AddRefs(resNode));
>+    } else {
>+      // If insert location node is the root element of an instance, then that
>+      // instance root element location is the target location and the
>+      // cloned node replaces the instance element. [Spec section 9.3.5,
>+      // steps 6c and 7.]
>+      //
>+      // If there is more than one cloned node to insert, only the
>+      // first node that does not cause a conflict is considered.
>+      nsCOMPtr<nsIDOMElement> locationDocElement;
>+      locationDoc->GetDocumentElement(getter_AddRefs(locationDocElement));
>+
>+      if (SameCOMIdentity(locationNode, locationDocElement)) {
>+        // Replace the instance element with the first element node of
>+        // the cloned node(s).
>+        nsCOMPtr<nsIDOMNode> insertNode;
>+        GetFirstNodeOfType(newNode, nsIDOMNode::ELEMENT_NODE,
>+                           getter_AddRefs(insertNode));
>+        if (insertNode) {
>+          locationNode->GetParentNode(getter_AddRefs(parentNode));
>+          NS_ENSURE_STATE(parentNode);
>+          parentNode->ReplaceChild(insertNode, locationNode,
>+                                   getter_AddRefs(resNode));

Did you verify that a document root will have a parent node?  This doesn't make sense in my mind.

>+
>+    // Delete the node(s) unless the delete location is the root document
>+    // element of an instance.
>+    if (!SameCOMIdentity(locationNode, locationDocElement)) {
>+      locationNode->GetParentNode(getter_AddRefs(parentNode));
>+      NS_ENSURE_STATE(parentNode);
>+
>+      rv = parentNode->RemoveChild(locationNode, getter_AddRefs(resNode));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    } else {
>+      didDelete = PR_FALSE;
>+    }

ditto

>+
>+    // The delete action is terminated with no effect if no node is deleted.
>+    if (!didDelete)
>+      return NS_OK;
>   }
> 
>   // Dispatch xforms-insert/delete event to the instance node we have modified
>   // data for
>   nsCOMPtr<nsIDOMNode> instNode;
>   rv = nsXFormsUtils::GetInstanceNodeForData(resNode, getter_AddRefs(instNode));
>   NS_ENSURE_SUCCESS(rv, rv);
> 

The spec mentions how repeat index values should behave upon deletion.  Please verify that these things do happen since they are slightly different in 1.1 than they were in 1.0.  If they don't, you don't need to fix them in this patch but open bugs for the issues.


> nsresult
>+nsXFormsInsertDeleteElement::GetFirstNodeOfType(nsIDOMNode *aNode,
>+                                                PRUint16 aNodeType,
>+                                                nsIDOMNode **aResult)
>+{
>+  nsCOMPtr<nsIDOMNode> currentNode, node;
>+
>+  currentNode = aNode;
>+  PRUint16 nodeType;
>+
>+  while (currentNode) {
>+    currentNode->GetNodeType(&nodeType);
>+    if (nodeType == aNodeType) {
>+      *aResult = currentNode;
>+      break;
>+    }
>+
>+    currentNode->GetNextSibling(getter_AddRefs(node));
>+    currentNode.swap(node);
>+  }
>+

I don't think that this function works like it is supposed to.  From what I read in the spec this function should walk through the cloned nodes looking for the first one that doesn't cause a conflict.  However, this function isn't walking through the cloned nodeset, it is walking through a DOM structure.  And since the node hasn't been inserted into the document yet, there won't be any sibling nodes.

>+nsresult
>+nsXFormsInsertDeleteElement::InsertNode(nsIDOMNode *aLocationNode,
>+                                        nsIDOMNode *aNewNode,
>+                                        PRBool aInsertAfter,
>+                                        nsIDOMNode **aResNode)
>+{
>+  NS_ENSURE_ARG(aLocationNode);
>+  NS_ENSURE_ARG(aNewNode);
>+  NS_ENSURE_ARG_POINTER(aResNode);
>+
>+  nsCOMPtr<nsIDOMNode> resNode;
>+
>+  PRUint16 locationNodeType, newNodeType;
>+  aLocationNode->GetNodeType(&locationNodeType);
>+  aNewNode->GetNodeType(&newNodeType);
>+
>+  if (locationNodeType == nsIDOMNode::ELEMENT_NODE) {
>+    // Can insert element nodes or attribute nodes.
>+    if (newNodeType == nsIDOMNode::ELEMENT_NODE) {
>+      // Inserting an element node to an element node.
>+      nsCOMPtr<nsIDOMNode> parentNode;
>+      aLocationNode->GetParentNode(getter_AddRefs(parentNode));
>+      NS_ENSURE_STATE(parentNode);
>+
>+      if (aInsertAfter) {
>+        nsCOMPtr<nsIDOMNode> temp;
>+        // If we're at the end of the nodeset, this returns nsnull, which is
>+        // fine, because InsertBefore then inserts at the end of the nodeset.
>+        aLocationNode->GetNextSibling(getter_AddRefs(temp));
>+        temp.swap(aLocationNode);
>+      }
>+      parentNode->InsertBefore(aNewNode, aLocationNode,
>+                               getter_AddRefs(resNode));
>+
>+    } else if (newNodeType == nsIDOMNode::ATTRIBUTE_NODE) {
>+      // Get the first attribute of the element node.
>+      nsCOMPtr<nsIDOMNamedNodeMap> attrs;
>+      aLocationNode->GetAttributes(getter_AddRefs(attrs));
>+
>+      // XXX: Can we insert before/after a given attribute or insert an
>+      // attribute to an element that doesn't yet have any attributes?

You need to figure out the answer to this XXX before checkin and remove the comment.  This isn't a theoretical question that we have to ponder, the answers can be found.

>+      if (attrs) {
>+         attrs->SetNamedItem(aNewNode, getter_AddRefs(resNode));
>+      }
>+    }
>+
>+  } else if (locationNodeType == nsIDOMNode::ATTRIBUTE_NODE) {
>+    // aLocationNode is an attribute node so the only type of node that
>+    // does not conflict is another attribute node.
>+    nsCOMPtr<nsIDOMNode> insertNode;
>+    if (newNodeType == nsIDOMNode::ATTRIBUTE_NODE) {
>+      insertNode = aNewNode;
>+    } else {
>+      // Find the first attribute node.
>+      GetFirstNodeOfType(aNewNode, nsIDOMNode::ATTRIBUTE_NODE,
>+                         getter_AddRefs(insertNode));

Ummm, if the cloned node type disagrees with the location node, then you shouldn't be getting the first cloned node that agrees with the location node type, you need to alter the target location, right?

>+    }
>+
>+    if (insertNode) {
>+      nsCOMPtr<nsIDOMNamedNodeMap> attrs;
>+      aLocationNode->GetAttributes(getter_AddRefs(attrs));
>+
>+      // XXX: Can we insert before/after a given attribute or insert an
>+      // attribute to an element that doesn't yet have any attributes?

You need to figure out the answer to this XXX before checkin and remove the comment.  This isn't a theoretical question that we have to ponder, the answers can be found.

>+      if (attrs) {
>+         attrs->SetNamedItem(aNewNode, getter_AddRefs(resNode));
>+      }
>+    }
>+  }
>+
>+  *aResNode = resNode;
>+
>+  return NS_OK;
>+}

I also don't see in your code where you are cloning more than one node from the origin nodeset.  I see where you get the prototype node, but then what?

Comment 12

[Merle Sterling]

(In reply to comment #11)
> >+  //
> >+  // Determine the origin node-set (if Insert).
> >+  //
> >+  nsCOMPtr<nsIDOMXPathResult> originNodeset;
> >+  PRUint32 originNodesetSize = 0;
> >+
> >+  if ( mIsInsert ) {
> >+    nsAutoString origin;
> >+    mElement->GetAttribute(NS_LITERAL_STRING("origin"), origin);
> >+
> >+    // If the origin attribute is given, the origin node-set is the result of
> >+    // the evaluation of the origin attribute in the insert context.
> >+    if (!origin.IsEmpty()) {
> >+      if (contextNode) {
> >+        rv = nsXFormsUtils::EvaluateXPath(origin, contextNode, contextNode,
> >+                                          nsIDOMXPathResult::ORDERED_NODE_SNAPSHOT_TYPE,
> >+                                          getter_AddRefs(originNodeset));
> again, contextNode shouldn't be passed as the resolver node.
> >+        NS_ENSURE_SUCCESS(rv, rv);
> >+      } else {
> >+        nsCOMPtr<nsIModelElementPrivate> originModel;
> >+        PRBool originUsesModelBinding;
> >+        rv = nsXFormsUtils::EvaluateNodeBinding(mElement,
> >+                                                nsXFormsUtils::ELEMENT_WITH_MODEL_ATTR,
> >+                                                NS_LITERAL_STRING("origin"),
> >+                                                EmptyString(),
> >+                                                nsIDOMXPathResult::ORDERED_NODE_SNAPSHOT_TYPE,
> >+                                                getter_AddRefs(originModel),
> >+                                                getter_AddRefs(originNodeset),
> >+                                                &originUsesModelBinding);
> >+        NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+        if (!originModel)
> >+          return NS_OK;
> >+      }
> >+
> >+      if (!originNodeset)
> >+        return NS_OK;
> >+
> >+      rv = originNodeset->GetSnapshotLength(&originNodesetSize);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+      // The insert action is terminated with no effect if the origin node-set
> >+      // is the empty node-set.
> >+      if (originNodesetSize < 1)
> >+        return NS_OK;
> >+    }
> >+  }
> >+
> I'd suggest that here if you don't have an originNodeset that you get the last
> node in the Node Set Binding nodeset so that at this point originNodeset has a
> value.  You really don't save anything by waiting until later.
If @origin is present but resolves to an empty node-set the insert is terminated with no effect. The xxxNodeset variable are DOMXPathResult whereas the last node of nodeset (from @nodeset) is a single node. I wait until Step 5 which describes cloning the nodes to decide whether to use the last node of nodeset or the nodes in originNodeset.

> >+    // If the cloned node is an attribute, then the target location is before
> >+    // the first attribute of the insert location node. If the cloned node is
> >+    // not an attribute, then the target location is before the first child
> >+    // of the insert location node.
> >+    if (!nodeset ||
> >+       (nodeset && nodesetSize > 1 && newNodeType != locationNodeType)) {
> >+      InsertNode(locationNode, newNode, PR_FALSE, getter_AddRefs(resNode));
> >+    } else {
> >+      // If insert location node is the root element of an instance, then that
> >+      // instance root element location is the target location and the
> >+      // cloned node replaces the instance element. [Spec section 9.3.5,
> >+      // steps 6c and 7.]
> >+      //
> >+      // If there is more than one cloned node to insert, only the
> >+      // first node that does not cause a conflict is considered.
> >+      nsCOMPtr<nsIDOMElement> locationDocElement;
> >+      locationDoc->GetDocumentElement(getter_AddRefs(locationDocElement));
> >+
> >+      if (SameCOMIdentity(locationNode, locationDocElement)) {
> >+        // Replace the instance element with the first element node of
> >+        // the cloned node(s).
> >+        nsCOMPtr<nsIDOMNode> insertNode;
> >+        GetFirstNodeOfType(newNode, nsIDOMNode::ELEMENT_NODE,
> >+                           getter_AddRefs(insertNode));
> >+        if (insertNode) {
> >+          locationNode->GetParentNode(getter_AddRefs(parentNode));
> >+          NS_ENSURE_STATE(parentNode);
> >+          parentNode->ReplaceChild(insertNode, locationNode,
> >+                                   getter_AddRefs(resNode));
> Did you verify that a document root will have a parent node?  This doesn't make sense in my mind.
Wouldn't the document root be the first child of xf:Instance? If the node to be inserted is the same as the document root then the spec says to replace the document root with that node. Is that not what this code is doing?

If it is possible that the document root doesn't have a parent (I don't see how) then what would you do - just ignore the insertion?

> > nsresult
> >+nsXFormsInsertDeleteElement::GetFirstNodeOfType(nsIDOMNode *aNode,
> >+                                                PRUint16 aNodeType,
> >+                                                nsIDOMNode **aResult)
> >+{
> >+  nsCOMPtr<nsIDOMNode> currentNode, node;
> >+
> >+  currentNode = aNode;
> >+  PRUint16 nodeType;
> >+
> >+  while (currentNode) {
> >+    currentNode->GetNodeType(&nodeType);
> >+    if (nodeType == aNodeType) {
> >+      *aResult = currentNode;
> >+      break;
> >+    }
> >+
> >+    currentNode->GetNextSibling(getter_AddRefs(node));
> >+    currentNode.swap(node);
> >+  }
> >+
> I don't think that this function works like it is supposed to.  From what I
> read in the spec this function should walk through the cloned nodes looking for
> the first one that doesn't cause a conflict.  However, this function isn't
> walking through the cloned nodeset, it is walking through a DOM structure.  And
> since the node hasn't been inserted into the document yet, there won't be any
> sibling nodes.
It walks a tree rooted by whatever node I pass in (one of the nodes to be cloned) but probably needs to walk the entire tree because there won't be any siblings of the single node from which I start. I had intended to revisit this function but forgot because it worked - mainly because the root node already matches in the testsuite testcases.

I agree that we should iterate over the list of cloned nodes but that begs the question, should we look only at the node itself to determine if it conflicts or not or should we also walk all of its children? 


> >+nsresult
> >+nsXFormsInsertDeleteElement::InsertNode(nsIDOMNode *aLocationNode,
> >+                                        nsIDOMNode *aNewNode,
> >+                                        PRBool aInsertAfter,
> >+                                        nsIDOMNode **aResNode)
> >+{
> >+  NS_ENSURE_ARG(aLocationNode);
> >+  NS_ENSURE_ARG(aNewNode);
> >+  NS_ENSURE_ARG_POINTER(aResNode);
> >+
> >+  nsCOMPtr<nsIDOMNode> resNode;
> >+
> >+  PRUint16 locationNodeType, newNodeType;
> >+  aLocationNode->GetNodeType(&locationNodeType);
> >+  aNewNode->GetNodeType(&newNodeType);
> >+
> >+  if (locationNodeType == nsIDOMNode::ELEMENT_NODE) {
> >+    // Can insert element nodes or attribute nodes.
> >+    if (newNodeType == nsIDOMNode::ELEMENT_NODE) {
> >+      // Inserting an element node to an element node.
> >+      nsCOMPtr<nsIDOMNode> parentNode;
> >+      aLocationNode->GetParentNode(getter_AddRefs(parentNode));
> >+      NS_ENSURE_STATE(parentNode);
> >+
> >+      if (aInsertAfter) {
> >+        nsCOMPtr<nsIDOMNode> temp;
> >+        // If we're at the end of the nodeset, this returns nsnull, which is
> >+        // fine, because InsertBefore then inserts at the end of the nodeset.
> >+        aLocationNode->GetNextSibling(getter_AddRefs(temp));
> >+        temp.swap(aLocationNode);
> >+      }
> >+      parentNode->InsertBefore(aNewNode, aLocationNode,
> >+                               getter_AddRefs(resNode));
> >+
> >+    } else if (newNodeType == nsIDOMNode::ATTRIBUTE_NODE) {
> >+      // Get the first attribute of the element node.
> >+      nsCOMPtr<nsIDOMNamedNodeMap> attrs;
> >+      aLocationNode->GetAttributes(getter_AddRefs(attrs));
> >+
> >+      // XXX: Can we insert before/after a given attribute or insert an
> >+      // attribute to an element that doesn't yet have any attributes?
> You need to figure out the answer to this XXX before checkin and remove the
> comment.  This isn't a theoretical question that we have to ponder, the answers can be found.
It was more of a 'note to self' because I wasn't sure how to add an attribute to an element node if GetAttributes returns a null NamedNodeMap. Also I see no way to place an attribute at a specific position. A Map is inherently unordered.

> >+      if (attrs) {
> >+         attrs->SetNamedItem(aNewNode, getter_AddRefs(resNode));
> >+      }
> >+    }
> >+
> >+  } else if (locationNodeType == nsIDOMNode::ATTRIBUTE_NODE) {
> >+    // aLocationNode is an attribute node so the only type of node that
> >+    // does not conflict is another attribute node.
> >+    nsCOMPtr<nsIDOMNode> insertNode;
> >+    if (newNodeType == nsIDOMNode::ATTRIBUTE_NODE) {
> >+      insertNode = aNewNode;
> >+    } else {
> >+      // Find the first attribute node.
> >+      GetFirstNodeOfType(aNewNode, nsIDOMNode::ATTRIBUTE_NODE,
> >+                         getter_AddRefs(insertNode));
> Ummm, if the cloned node type disagrees with the location node, then you
> shouldn't be getting the first cloned node that agrees with the location node
> type, you need to alter the target location, right?
Not according to my interpretation of the spec. In Step 6c (if cloned node is the root of an instance) it says 'only the first node that doesn't cause a conflict is considred' and then in Step 7 it says 'if a node would cause a conflict, the insertion of that particular node is ignored'. So we do not alter the target location, we look for the first node in the set of cloned nodes that does not cause a conflict.

I will rearrange the code for determining the context per your suggestions and add code to clone every node in the orgin nodeset. Then we can step through the array of cloned nodes to deal with the case of conflicting node types.

Comment 13

[aaronr]

(In reply to comment #12)
> (In reply to comment #11)

> > Did you verify that a document root will have a parent node?  This doesn't make sense in my mind.
> Wouldn't the document root be the first child of xf:Instance? If the node to be
> inserted is the same as the document root then the spec says to replace the
> document root with that node. Is that not what this code is doing?
> 
> If it is possible that the document root doesn't have a parent (I don't see
> how) then what would you do - just ignore the insertion?

I don't think that the root of a document has a parent, though I could be wrong.  Remember that the instance document is a stand alone document initialized from the contents of the xf:instance element, but that is where the association ends.  Modifying the instance document, for example, doesn't change the contents of the xf:instance element in the xforms document and changing the contents of the xf:instance element won't affect the instance document at all.

The spec says, "If the target location was the root element of an instance, then the cloned node replaces the instance root element."

> > I don't think that this function works like it is supposed to.  From what I
> > read in the spec this function should walk through the cloned nodes looking for
> > the first one that doesn't cause a conflict.  However, this function isn't
> > walking through the cloned nodeset, it is walking through a DOM structure.  And
> > since the node hasn't been inserted into the document yet, there won't be any
> > sibling nodes.
> It walks a tree rooted by whatever node I pass in (one of the nodes to be
> cloned) but probably needs to walk the entire tree because there won't be any
> siblings of the single node from which I start. I had intended to revisit this
> function but forgot because it worked - mainly because the root node already
> matches in the testsuite testcases.
> 
> I agree that we should iterate over the list of cloned nodes but that begs the
> question, should we look only at the node itself to determine if it conflicts
> or not or should we also walk all of its children? 
> 

The spec only talks about conflicts when dealing with the cloned node, so I'd say only worry about each cloned node in the nodeset and don't worry about any of their children.

> It was more of a 'note to self' because I wasn't sure how to add an attribute
> to an element node if GetAttributes returns a null NamedNodeMap. Also I see no
> way to place an attribute at a specific position. A Map is inherently
> unordered.

Yeah, I don't know how the spec can say that about ordering the attributes since I don't think that attributes are guaranteed to be ordered in document order in the first place.  I'd just say make sure that you get them in the list and that it handles duplicates correctly.

> > Ummm, if the cloned node type disagrees with the location node, then you
> > shouldn't be getting the first cloned node that agrees with the location node
> > type, you need to alter the target location, right?
> Not according to my interpretation of the spec. In Step 6c (if cloned node is
> the root of an instance) it says 'only the first node that doesn't cause a
> conflict is considred' and then in Step 7 it says 'if a node would cause a
> conflict, the insertion of that particular node is ignored'. So we do not alter
> the target location, we look for the first node in the set of cloned nodes that
> does not cause a conflict.
> 

You are right that in those two cases then nodes from the cloned nodeset can be skipped over.  But only if 6a and 6b don't apply since they will both alter the target location if the insert location node and the clone node have conflicting types.

Comment 14

[Merle Sterling]

Attachment: patch 3

Refactoring according to review comments. Works for all testcases in the testsuite except testcase 'h' which tests if repeats are updated correctly. That testcase needs to be verified as correct and we need additional testcases to test if repeats are updated properly for delete as well. Because the rules for updating repeats have changed they will be handled in a different bug.

Comment 15

[aaronr]

Comment on attachment 268980 [details] [diff] [review]
patch 3

>+  /** Insert a node.
>+   *
>+   *  @param aTargetNode     target location node
>+   *  @param aNewNode          node to insert
>+   *  @param aInsertAfter      insert before or after target?
>+   *
>+   *  @return aResult          result node
>+   */
>+  nsresult InsertNode(nsIDOMNode *aTargetNode, nsIDOMNode *aNewNode,
>+                      PRBool aInsertAfter, nsIDOMNode **aResNode);


nit: align comments, please.

> NS_IMETHODIMP
> nsXFormsInsertDeleteElement::HandleAction(nsIDOMEvent            *aEvent,
>                                           nsIXFormsActionElement *aParentAction)
> {

>+  //
>+  // Step 1 (Insert or Delete): Determine the insert/delete context.
>+  //
>+  // If the bind attribute is present, it is evaluated to determine the
>+  // in-scope evaluation context and the context attribute is ignored;
>+  // otherwise, the context attribute is evaluated and overrides the
>+  // in-scope evaluation context.
>+  //
>+  // The nodeset binding is required unless the context attribute is present.

nit: I'd qualify this comment since it isn't in the context of the spec so easy to confuse what you mean by 'the nodeset binding'.  Maybe say, "A NodeSet binding attribute (@bind or @nodeset) is required unless the context attribute is present."

>+  nsAutoString nodesetExpr;
>+  mElement->GetAttribute(NS_LITERAL_STRING("nodeset"), nodesetExpr);
>+
>+  if (bindExpr.IsEmpty()) {
>+    if (!nodesetExpr.IsEmpty()) {

nit: you don't use nodesetExpr outside of the bindExpr.IsEmpty() section, so no reason to declare an auto string or to call GetAttribute outside of the 'if' section.

>+
>+  //
>+  // Step 4 (Insert), Step 3 (Delete):
>+  // Determine the insert/delete location node.

>+     if (!nodeset || nodesetSize < 1) {
>+        // The insert location node is the insert context node.
>+        locationNode = contextNode;

nit: indented 3 space, not 2.  Craziness, I tells ya! :-)

>+    for (PRUint32 i = cloneIndex; i < cloneNodesetSize; ++i) {
>+      cloneNodeset->SnapshotItem(cloneIndex, getter_AddRefs(prototypeNode));
>+      NS_ENSURE_STATE(prototypeNode);
>+
>+      // The prototypeNode (node to be cloned) and the locationNode (node to
>+      // which the clone will be inserted) may belong to different instances.
>+      nsCOMPtr<nsIDOMDocument> originDoc, locationDoc;
>+      prototypeNode->GetOwnerDocument(getter_AddRefs(originDoc));
>+      NS_ENSURE_STATE(originDoc);
>+      locationNode->GetOwnerDocument(getter_AddRefs(locationDoc));
>+      NS_ENSURE_STATE(locationDoc);
>+
>+      if (originDoc != locationDoc) {

I don't know if this comparison is guaranteed to work.  You might need to use SameCOMIdentity here, too.  Also, the originDoc and locationDoc can't change, can they?  You shouldn't need to figure them out for every node in the cloneNodeset.

>+    // Delete
>+    // If there is no delete location, each node in the Node Set Binding
>+    // node-set is deleted, unless the node is the root document element of an
>+    // instance.
>+    //
>+    // If there is a delete location, the node at the delete location in the
>+    // Node Set Binding node-set is deleted, unless the node is the root
>+    // document element of an instance.
>+    PRBool didDelete = PR_TRUE;
>+
>+    PRUint32 deleteIndex, deleteCount;
>+
>+    if (!locationNode) {
>+      // Delete all the nodes in the node-set.
>+      deleteIndex = 0;
>+      deleteCount = nodesetSize;
>+    } else {
>+      // Delete the node at the delete location.
>+      deleteIndex = atInt - 1;
>+      deleteCount = 1;
>+    }
>+
>+    for (PRUint32 i = deleteIndex; i < deleteCount; i++) {

Hmmm, if you look above for the condition where there is a location node, you are setting deleteCount to 1.  But what if atInt is 7?  With the test you are doing in the for loop, you aren't going to delete anything since 6 isn't less than 1.  deleteCount should be atInt, shouldn't it?

>+      nodeset->SnapshotItem(i, getter_AddRefs(locationNode));
>+      NS_ENSURE_STATE(locationNode);
>+
>+      locationNode->GetOwnerDocument(getter_AddRefs(locationDoc));
>+      NS_ENSURE_STATE(locationDoc);
>+
>+      locationDoc->GetDocumentElement(getter_AddRefs(locationDocElement));

locationDoc and locationDocElement shouldn't ever change in this loop, should they?  No reason to figure them out each time through the loop, is there?

> 
>   // Dispatch xforms-insert/delete event to the instance node we have modified
>   // data for
>   nsCOMPtr<nsIDOMNode> instNode;
>-  rv = nsXFormsUtils::GetInstanceNodeForData(resNode, getter_AddRefs(instNode));
>+  rv = nsXFormsUtils::GetInstanceNodeForData(locationNode, getter_AddRefs(instNode));
>   NS_ENSURE_SUCCESS(rv, rv);
>-
>+  

nit: looks like you are inserting a space on this empty line.  Please remove it.

>+nsXFormsInsertDeleteElement::GetFirstNodeOfType(nsCOMArray<nsIDOMNode> *aNodes,
>+                                                PRUint16 aNodeType,
>+                                                nsIDOMNode **aResult)
>+{
>+  nsCOMPtr<nsIDOMNode> currentNode;
>+
>+  for (PRInt32 i = 0; i < aNodes->Count(); ++i) {
>+    currentNode = aNodes->ObjectAt(i);
>+    PRUint16 nodeType;
>+    currentNode->GetNodeType(&nodeType);
>+    if (nodeType == aNodeType) {
>+      *aResult = currentNode;
>+      break;
>+    }
>+  }

do you need to addref aResult -> NS_IF_ADDREF(*aResult = currentNode) ?  Or you could use swap.

>+nsresult
>+nsXFormsInsertDeleteElement::InsertNode(nsIDOMNode *aTargetNode,
>+                                        nsIDOMNode *aNewNode,
>+                                        PRBool aInsertAfter,
>+                                        nsIDOMNode **aResNode)
>+{
>+  NS_ENSURE_ARG(aTargetNode);
>+  NS_ENSURE_ARG(aNewNode);
>+  NS_ENSURE_ARG_POINTER(aResNode);

You should null out aResNode so that you don't end up returning junk if you hit one of the conditions that you don't handle (like when things are attributes or element nodes).

>+
>+  // Step 7 - The new node is inserted at the target location depending on its
>+  // node type. If the cloned node is a duplicate of another attribute in its
>+  // parent element, then the duplicate attribute is first removed. If a cloned
>+  // node cannot be placed at the target location due to a node type conflict,
>+  // then the insertion for that particular clone node is ignored.
>+  nsCOMPtr<nsIDOMNode> resNode;
>+  
>+  PRUint16 targetNodeType, newNodeType;
>+  aTargetNode->GetNodeType(&targetNodeType);
>+  aNewNode->GetNodeType(&newNodeType);
>+  
>+  if (newNodeType == nsIDOMNode::ATTRIBUTE_NODE) {
>+    // Can add an attribute to an element node or the owning element
>+    // of an attribute node.
>+    nsCOMPtr<nsIDOMElement> ownerElement;
>+    nsCOMPtr<nsIDOMAttr> attrNode = do_QueryInterface(aNewNode);

nit: per mozilla, please use the format -> ...attrNode(do_QueryInterface(aNewNode))

>+  } else {
>+    // New node is not an attribute so it can only be inserted to an
>+    // element node.
>+    if (targetNodeType == nsIDOMNode::ELEMENT_NODE) {
>+        nsCOMPtr<nsIDOMNode> parentNode;
>+        aTargetNode->GetParentNode(getter_AddRefs(parentNode));
>+        NS_ENSURE_STATE(parentNode);
>+
>+        if (aInsertAfter) {
>+          nsCOMPtr<nsIDOMNode> temp;
>+          // If we're at the end of the nodeset, this returns nsnull, which is
>+          // fine, because InsertBefore then inserts at the end of the nodeset.
>+          aTargetNode->GetNextSibling(getter_AddRefs(temp));
>+          temp.swap(aTargetNode);

Ummm, I'm not a big fan of swapping with an argument unless it is a return buffer.  Might cause problems as people change this code over time and potentially using the parameter later, unaware it has changed from its original value.  Please move the declaration of temp out of the if test, rename it to be have a meaningful name and initialize it with aTargetNode.  Thanks.


Nothing super major in here that would need me to review it again, I don't think.  Just fix my comments and let me know if you have any questions with any of them.  With those changes, r=me

Comment 16

[aaronr]

PS, super commenting!!!!  Made things incredibly easy to read and understand!

Comment 17

[Merle Sterling]

(In reply to comment #15)
> >+    for (PRUint32 i = cloneIndex; i < cloneNodesetSize; ++i) {
> >+      cloneNodeset->SnapshotItem(cloneIndex, getter_AddRefs(prototypeNode));
> >+      NS_ENSURE_STATE(prototypeNode);
> >+
> >+      // The prototypeNode (node to be cloned) and the locationNode (node to
> >+      // which the clone will be inserted) may belong to different instances.
> >+      nsCOMPtr<nsIDOMDocument> originDoc, locationDoc;
> >+      prototypeNode->GetOwnerDocument(getter_AddRefs(originDoc));
> >+      NS_ENSURE_STATE(originDoc);
> >+      locationNode->GetOwnerDocument(getter_AddRefs(locationDoc));
> >+      NS_ENSURE_STATE(locationDoc);
> >+
> >+      if (originDoc != locationDoc) {
> I don't know if this comparison is guaranteed to work.  You might need to use
> SameCOMIdentity here, too.  Also, the originDoc and locationDoc can't change,
> can they?  You shouldn't need to figure them out for every node in the
> cloneNodeset.
Ok for SameCOMIdentity but yes I think originDoc and locationDoc need to be checked for every node in the nodeset and in fact testsuite testcase 'c' no longer works if I do not because it inserts nodes from two different instances.

I'm leaving the same kind of logic for the delete case but will revisit it if necessary when working on the bug for updating repeats.

> >+    // Delete
> >+    // If there is no delete location, each node in the Node Set Binding
> >+    // node-set is deleted, unless the node is the root document element of an
> >+    // instance.
> >+    //
> >+    // If there is a delete location, the node at the delete location in the
> >+    // Node Set Binding node-set is deleted, unless the node is the root
> >+    // document element of an instance.
> >+    PRBool didDelete = PR_TRUE;
> >+
> >+    PRUint32 deleteIndex, deleteCount;
> >+
> >+    if (!locationNode) {
> >+      // Delete all the nodes in the node-set.
> >+      deleteIndex = 0;
> >+      deleteCount = nodesetSize;
> >+    } else {
> >+      // Delete the node at the delete location.
> >+      deleteIndex = atInt - 1;
> >+      deleteCount = 1;
> >+    }
> >+
> >+    for (PRUint32 i = deleteIndex; i < deleteCount; i++) {
> Hmmm, if you look above for the condition where there is a location node, you
> are setting deleteCount to 1.  But what if atInt is 7?  With the test you are
> doing in the for loop, you aren't going to delete anything since 6 isn't less
> than 1.  deleteCount should be atInt, shouldn't it?

The intention of deleteCount was to be the number of nodes to delete but yes in that case it would be wrong. Now the issue is whether or not 'delete the node at the delete location' means just that ONE node (and its children) or it could be interpreted to mean starting at that location; ie if the nodeset size is 5 and atInt is 3 do you just delete the node at index 2 in the nodeset or nodes at index 2 - 4? I'm using a literal interpretation of the spec and only deleting the one node at the delete location so deleteCount needs to be set to atInt.

Comment 18

[Merle Sterling]

Attachment: patch 4

Comment 19

[aaronr]

> Ok for SameCOMIdentity but yes I think originDoc and locationDoc need to be
> checked for every node in the nodeset and in fact testsuite testcase 'c' no
> longer works if I do not because it inserts nodes from two different instances.

testcase 'c'?  Could you please attach the testcase to this bug or a link to it or at least copy the insert element here so that we can see what you mean?  I personally couldn't think of a way to write an XPath expression that can reach across multiple documents.


> 
> The intention of deleteCount was to be the number of nodes to delete but yes in
> that case it would be wrong. Now the issue is whether or not 'delete the node
> at the delete location' means just that ONE node (and its children) or it could
> be interpreted to mean starting at that location; ie if the nodeset size is 5
> and atInt is 3 do you just delete the node at index 2 in the nodeset or nodes
> at index 2 - 4? I'm using a literal interpretation of the spec and only
> deleting the one node at the delete location so deleteCount needs to be set to
> atInt.
> 

I would also think that only one node (and its children) would be deleted.  If you aren't sure, it is worth asking the WG.

Comment 20

[Merle Sterling]

Attachment: testsuite 1.1: testcase 9.3.5.c

Comment 21

[Merle Sterling]

Attachment: Revised patch4

Figured out what I did wrong. Now only getting the owner document once instead of inside the loop and testcase 9.3.5.c works.

Comment 22

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

Comment on attachment 269899 [details] [diff] [review]
Revised patch4

>+
>+  if ( mIsInsert ) {

Remove space before and after mIsInsert

>+            if (!position.IsEmpty()) {
>+              if (position.EqualsLiteral("before")) {
>+                insertAfter = PR_FALSE;
>+              } else if (!position.EqualsLiteral("after")) {
>+                // This is not a valid document...
>+                return NS_ERROR_ABORT;

Maybe NS_ERROR_FAILURE, does't matter much though.

>+      parentNode->InsertBefore(aNewNode, targetNode,
>+                               getter_AddRefs(resNode));
>+      *aResNode = resNode;

Shouldn't you addref aResNode here? Or maybe use swap.

Those fixed, r=me

Comment 23

[Merle Sterling]

Attachment: final patch

Comment 24

[aaronr]

checked into trunk and 1.8 branch for msterlin