278449 - Implement XML Schema Validation for Complex Types

Status: RESOLVED FIXED

(Core Graveyard :: Web Services, defect)

Description

[Doron Rosenberg (IBM)]

Comment 1

[Doron Rosenberg (IBM)]

Attachment: work in progress, just in case something goes bad with my new box

Comment 2

[Doron Rosenberg (IBM)]

Attachment: current progress

Current progress, includes the inherited simpletype patch and the attribute
qualification patch in 2 other bugs.

testsuite also added, currently the 100 complex type tests results are 100%
identical in this code and xerces-J

Comment 3

[Doron Rosenberg (IBM)]

Attachment: current progress - trunk

Latest progress, requires trunk due to a change to nsISchema that isn't on the branches.

Comment 4

[Doron Rosenberg (IBM)]

Attachment: latest progress

Pretty much reviewable

Comment 5

[Doron Rosenberg (IBM)]

Attachment: first reviewable patch

I didn't diff my testsuite stuff here, just c++ changes.  I also fixed some minor datetime parsing bugs (wasn't testing length requirements).

Comment 6

[aaronr]

Comment on attachment 216052 [details] [diff] [review]
first reviewable patch


>Index: extensions/schema-validation/src/nsSchemaValidator.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/schema-validation/src/nsSchemaValidator.cpp,v
>retrieving revision 1.22
>diff -u -r1.22 nsSchemaValidator.cpp
>--- extensions/schema-validation/src/nsSchemaValidator.cpp	21 Mar 2006 16:39:35 -0000	1.22
>+++ extensions/schema-validation/src/nsSchemaValidator.cpp	23 Mar 2006 21:24:00 -0000


>+  /* 
>+   * We allow the schema validator to continue validating a structure
>+   * even if the nodevalue is invalid per its simpletype binding.  We remember
>+   * this by using mForceInvalid as an override for the final return.
>+   */
>+

Ummm, why do all this work if you are just going to make aResult PR_FALSE anyhow?  Just
for the rv?  Please comment why you do this.

>+  PRBool isValid = PR_FALSE;
>+  rv = ValidateAgainstType(aElement, type, &isValid);
>+
>+  *aResult = mForceInvalid ? PR_FALSE : isValid;
>+  return rv;
> }
> 
> NS_IMETHODIMP
>@@ -253,9 +308,38 @@
>         NS_ConvertUTF16toUTF8(nodeValue).get()));
> 
>       rv = ValidateSimpletype(nodeValue, simpleType, &isValid);


please use -p when you cvs diff.  Nice to know what function you are changing w/o
having to open the file and go to the line number.

Also, no sense assigning into rv if you aren't going to use it.

>+
>+      nsAutoString typeName, nodeName;
>+      rv = aType->GetName(typeName);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      rv = aElement->GetLocalName(nodeName);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      // go on validating, but remember we failed
>+      if (!isValid) {
>+        mForceInvalid = PR_TRUE;
>+        isValid = PR_TRUE;
>+      }
>+
>+      // set the property so that callers can check validity of nodes
>+      nsCOMPtr<nsIContent> content = do_QueryInterface(aElement);
>+      NS_ENSURE_STATE(content);
>+
>+      nsCOMPtr<nsIAtom> myAtom = do_GetAtom("xsdtype");
>+
>+      // We need to make aType live one cycle longer, so that getProperty
>+      // can access it.  ReleaseObject will take care of the releasing.
>+      NS_ADDREF(aType);
>+      rv = content->SetProperty(myAtom, aType, ReleaseObject);
>+      NS_ENSURE_SUCCESS(rv, rv);
>     }
>   } else if (typevalue == nsISchemaType::SCHEMA_TYPE_COMPLEX) {
>-    rv = NS_ERROR_NOT_IMPLEMENTED;
>+    nsCOMPtr<nsISchemaComplexType> complexType = do_QueryInterface(aType);
>+    if (complexType) {
>+      LOG(("  Type is a complex type!"));
>+      rv = ValidateComplextype(aElement, complexType, &isValid);
>+    }
>   } else {
>     rv = NS_ERROR_UNEXPECTED;
>   }
>@@ -278,7 +362,7 @@
>     // if we are a built-in type, we can get a nsISchemaType for it from
>     // nsISchemaCollection->GetType.
>     nsCOMPtr<nsISchemaLoader> schemaLoader =
>-      do_GetService("@mozilla.org/xmlextras/schemas/schemaloader;1", &rv);
>+      do_CreateInstance("@mozilla.org/xmlextras/schemas/schemaloader;1", &rv);
>     NS_ENSURE_SUCCESS(rv, rv);
>     mSchema = do_QueryInterface(schemaLoader);
>     NS_ENSURE_STATE(mSchema);
>@@ -1504,7 +1588,7 @@
>   // GDay looks like this: ---DD(Z|(+|-)hh:mm)
> 
>   PRUint32 strLength = aNodeValue.Length();
>-  //   ---DD               ---DDZ              ---DD(+|-)hh:mm
>+  //   ---DD            ---DDZ            ---DD(+|-)hh:mm
>   if (strLength != 5 && strLength != 6 && strLength != 11)
>     return PR_FALSE;
> 
>@@ -1628,7 +1712,7 @@
>   // GMonth looks like this: --MM(Z|(+|-)hh:mm)
> 
>   PRUint32 strLength = aNodeValue.Length();
>-  //   --MM              --MMZ             --MM(+|-)hh:mm
>+  //   --MM                --MMZ               --MM(+|-)hh:mm
>   if ((strLength != 4) && (strLength != 5) && (strLength != 10))
>     return PR_FALSE;
> 
>@@ -1642,7 +1726,7 @@
>   aNodeValue.EndReading(end);
>   nsAutoString nodeValue(aNodeValue);
> 
>-  // validate the --MM-- part
>+  // validate the --MM part
>   PRBool isValid = Substring(start.get(), start.get()+2).EqualsLiteral("--") &&
>                    IsValidSchemaGType(Substring(start.get()+2, start.get()+4),
>                                       1, 12, &monthInt);
>@@ -1931,7 +2015,8 @@
>       // back one up since we have the separator included
>       year = Substring(buffStart, --start);
> 
>-      isValid = IsValidSchemaGYear(year, aYearMonth ? &aYearMonth->gYear : nsnull);
>+      isValid = IsValidSchemaGYear(year,
>+                                   aYearMonth ? &aYearMonth->gYear : nsnull);
> 
>       if (isValid) {
>         nsAutoString month;
>@@ -1939,7 +2024,8 @@
>         start++;
>         month.Append(Substring(start, end));
> 
>-        isValid = IsValidSchemaGMonth(month, aYearMonth ? &aYearMonth->gMonth : nsnull);
>+        isValid = IsValidSchemaGMonth(month,
>+                                      aYearMonth ? &aYearMonth->gMonth : nsnull);
>       }
>       done = PR_TRUE;
>     } else {
>@@ -2054,7 +2140,8 @@
>       nsAutoString month;
>       month.AppendLiteral("--");
>       month.Append(Substring(buffStart, start.advance(2)));
>-      isValid = IsValidSchemaGMonth(month, aMonthDay ? &aMonthDay->gMonth : nsnull);
>+      isValid = IsValidSchemaGMonth(month,
>+                                    aMonthDay ? &aMonthDay->gMonth : nsnull);
> 
>       if (isValid) {
>         buffStart = start;
>@@ -2067,7 +2154,8 @@
>       nsAutoString day;
>       day.AppendLiteral("---");
>       day.Append(Substring(++buffStart, end));
>-      isValid = IsValidSchemaGDay(day, aMonthDay ? &aMonthDay->gDay : nsnull);
>+      isValid = IsValidSchemaGDay(day,
>+                                  aMonthDay ? &aMonthDay->gDay : nsnull);
>       done = PR_TRUE;
>     }
>   }
>@@ -2361,7 +2449,6 @@
>     char fulldate[100] = "";
>     nsCAutoString monthShorthand;
>     nsSchemaValidatorUtils::GetMonthShorthand(dateTime.date.month, monthShorthand);
>-
>     // 22-AUG-1993 10:59:12.82
>     sprintf(fulldate, "%d-%s-%u %d:%d:%d.%u",
>       dateTime.date.day,
>@@ -2398,9 +2485,8 @@
> 
>   nsAutoString dateString(aNodeValue); 
>   if (dateString.First() == '-') {
>-    /* nspr can't handle negative years it seems */
>     aResult->isNegative = PR_TRUE;
>- }
>+  }
> 
>  /*
>     http://www.w3.org/TR/xmlschema-2/#date
>@@ -2507,7 +2593,8 @@
>   PRBool isValid = PR_FALSE;
>   nsCOMPtr<nsISchemaDuration> duration;
> 
>-  isValid = nsSchemaValidatorUtils::ParseSchemaDuration(aNodeValue, getter_AddRefs(duration));
>+  isValid = nsSchemaValidatorUtils::ParseSchemaDuration(aNodeValue,
>+                                                        getter_AddRefs(duration));
> 
>   duration.swap(*aResult);
>   return isValid;
>@@ -2656,7 +2743,8 @@
>   }
> 
>   if (isValid && aEnumerationList && (aEnumerationList->Count() > 0)) {
>-    isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue, *aEnumerationList);
>+    isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue,
>+                                                        *aEnumerationList);
>   }
> 
>  *aResult = isValid;
>@@ -2738,7 +2826,8 @@
>   }
> 
>   if (isValid && aEnumerationList && (aEnumerationList->Count() > 0)) {
>-    isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue, *aEnumerationList);
>+    isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue,
>+                                                        *aEnumerationList);
>   }
> 
> #ifdef PR_LOGGING
>@@ -2878,7 +2967,8 @@
>   }
> 
>   if (isValid && aEnumerationList && (aEnumerationList->Count() > 0)) {
>-    isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue, *aEnumerationList);
>+    isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue,
>+                                                        *aEnumerationList);
>   }
> 
> #ifdef PR_LOGGING
>@@ -3009,7 +3099,8 @@
>             (!aMaxLength || length <= aMaxLength);
> 
>   if (isValid && aEnumerationList && (aEnumerationList->Count() > 0)) {
>-    isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue, *aEnumerationList);
>+    isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue,
>+                                                        *aEnumerationList);
>   }
> 
>   *aResult = isValid;
>@@ -3051,7 +3142,7 @@
>   if (isValid) {
>     length = strlen(decodedString);
> 
>-    if (aLengthDefined && (length != aLength)) {  
>+    if (aLengthDefined && (length != aLength)) {
>       isValid = PR_FALSE;
>       LOG(("  Not valid: Not the right length (%d)", length));
>     }
>@@ -3068,7 +3159,8 @@
>   }
> 
>   if (isValid && aEnumerationList && (aEnumerationList->Count() > 0)) {
>-    isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue, *aEnumerationList);
>+    isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue,
>+                                                        *aEnumerationList);
>   }
> 
> #ifdef PR_LOGGING
>@@ -3126,7 +3218,8 @@
>   }
> 
>   if (isValid && aEnumerationList && (aEnumerationList->Count() > 0)) {
>-    isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue, *aEnumerationList);
>+    isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue,
>+                                                        *aEnumerationList);
>   }
> 
> #ifdef PR_LOGGING
>@@ -3195,7 +3288,8 @@
>     }
> 
>     if (isValid && aEnumerationList && (aEnumerationList->Count() > 0)) {
>-      isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue, *aEnumerationList);
>+      isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue,
>+                                                          *aEnumerationList);
>     }
>   }
> 
>@@ -3245,3 +3339,1053 @@
>   LOG(("  Base Type is %s (%d)", NS_ConvertUTF16toUTF8(typeName).get(),foo));
> }
> #endif
>+
>+
>+
>+/****************************************************
>+ * Complex Type Validation                          *
>+ ****************************************************/
>+
>+// http://w3c.org/TR/xmlschema-1/#Complex_Type_Definitions
>+nsresult
>+nsSchemaValidator::ValidateComplextype(nsIDOMNode* aNode,
>+                                       nsISchemaComplexType *aSchemaComplexType,
>+                                       PRBool *aResult)
>+{
>+  PRBool isValid = PR_FALSE;

nit: initialize *aResult to false here?  Otherwise in some error conditions
you are setting it to false, other times you don't do anything.

>+
>+  PRUint16 contentModel;
>+  nsresult rv = aSchemaComplexType->GetContentModel(&contentModel);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  switch(contentModel) {
>+    case nsISchemaComplexType::CONTENT_MODEL_EMPTY: {
>+      // element has no children
>+      rv = NS_ERROR_NOT_IMPLEMENTED;
>+      break;
>+    }
>+
>+    case nsISchemaComplexType::CONTENT_MODEL_SIMPLE: {
>+      LOG(("    complex type, simple content"));
>+      rv = ValidateComplexModelSimple(aNode, aSchemaComplexType, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaComplexType::CONTENT_MODEL_ELEMENT_ONLY: {
>+      LOG(("    xsd:element only"));
>+      rv = ValidateComplexModelElement(aNode, aSchemaComplexType, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaComplexType::CONTENT_MODEL_MIXED: {
>+      rv = NS_ERROR_NOT_IMPLEMENTED;
>+      break;
>+    }
>+  }

nit: if isValid is false, can't you just exit out right here?

>+
>+  // if we are valid, validate the attributes
>+  if (isValid) {
>+    nsCOMPtr<nsIDOMNamedNodeMap> attrMap;
>+    rv = aNode->GetAttributes(getter_AddRefs(attrMap));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    PRUint32 attrCount = 0, nsAttrCount = 0, totalAttr = 0, foundAttrCount = 0;
>+

maybe name nsAttrCount to omitAttrCount or ignoreAttrCount or something similar.
When I see nsAttrCount, I'm not thinking namespace, but rather netscape.  So 
makes the code a little less understandable at a quick glance.

>+nsresult
>+nsSchemaValidator::ValidateComplexModelSimple(nsIDOMNode* aNode,
>+                                              nsISchemaComplexType *aSchemaComplexType,
>+                                              PRBool *aResult)
>+{
>+  PRBool isValid = PR_FALSE;
>+
>+  PRUint16 derivation;;
>+  nsresult rv = aSchemaComplexType->GetDerivation(&derivation);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // two choices for complex model with simple content: derivation through
>+  // extension or through restriction
>+  switch(derivation) {
>+    case nsISchemaComplexType::DERIVATION_EXTENSION_SIMPLE: {
>+      LOG(("      -- deriviation by extension of a simple type"));
>+
>+      nsCOMPtr<nsISchemaSimpleType> simpleBaseType;
>+      rv = aSchemaComplexType->GetSimpleBaseType(getter_AddRefs(simpleBaseType));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      nsAutoString nodeValue;
>+      nsCOMPtr<nsIDOM3Node> domNode3 = do_QueryInterface(aNode);
>+      domNode3->GetTextContent(nodeValue);
>+
>+      rv = ValidateSimpletype(nodeValue, simpleBaseType, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaComplexType::DERIVATION_RESTRICTION_SIMPLE: {
>+      LOG(("      -- deriviation by restriction of a simple type"));
>+
>+      nsCOMPtr<nsISchemaSimpleType> simpleBaseType;
>+      rv = aSchemaComplexType->GetSimpleBaseType(getter_AddRefs(simpleBaseType));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      nsAutoString nodeValue;
>+      nsCOMPtr<nsIDOM3Node> domNode3 = do_QueryInterface(aNode);
>+      rv = domNode3->GetTextContent(nodeValue);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      rv = ValidateSimpletype(nodeValue, simpleBaseType, &isValid);
>+      break;
>+    }
>+  }
>+

Ummm...aren't you doing the exact same thing in both cases?  Well, except for the LOG
statement.

>+
>+nsresult
>+nsSchemaValidator::ValidateComplexSequence(nsIDOMNode* aStartNode,
>+                                           nsISchemaModelGroup *aSchemaModelGroup,
>+                                           nsIDOMNode **aLeftOvers,
>+                                           PRBool *aNotFound, PRBool *aResult)
>+{
>+  if (!aStartNode || !aSchemaModelGroup)
>+    return NS_ERROR_UNEXPECTED;
>+
>+  PRBool isValid = PR_FALSE;
>+  PRBool notFound = PR_FALSE;
>+
>+  // get the model group details
>+  PRUint32 minOccurs;
>+  nsresult rv = aSchemaModelGroup->GetMinOccurs(&minOccurs);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRUint32 maxOccurs;
>+  rv = aSchemaModelGroup->GetMaxOccurs(&maxOccurs);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRUint32 particleCount;
>+  rv = aSchemaModelGroup->GetParticleCount(&particleCount);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // xsd:sequence means that the order of the particles matters
>+  PRUint32 validatedNodes = 0;
>+  PRUint32 particleCounter = 0;
>+  PRUint16 nodeType;
>+  PRBool done = PR_FALSE;
>+  nsCOMPtr<nsISchemaParticle> particle;
>+  nsCOMPtr<nsIDOMNode> currentNode(aStartNode), leftOvers, tmpNode;
>+
>+  LOG(("====================== New Sequence ==========================="));
>+
>+  // while valid and not done
>+  // we are done when we hit a node that doesn't fit our schema
>+  while (!done && currentNode && (particleCounter < particleCount)) {
>+    // get node type
>+    currentNode->GetNodeType(&nodeType);
>+
>+    // if not an element node, skip
>+    if (nodeType != nsIDOMNode::ELEMENT_NODE) {
>+      currentNode->GetNextSibling(getter_AddRefs(tmpNode));
>+      currentNode = tmpNode;
>+      continue;
>+    }
>+
>+    // get the particle
>+    rv = aSchemaModelGroup->GetParticle(particleCounter,
>+                                        getter_AddRefs(particle));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    rv = ValidateComplexParticle(currentNode, particle,
>+                                 getter_AddRefs(leftOvers), &notFound, &isValid);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // if we found the node, increment the amount of validated nodes.
>+    if (!notFound)
>+      validatedNodes++;
>+
>+    if (isValid) {
>+      particleCounter++;
>+    } else {
>+      done = PR_TRUE;
>+    }
>+
>+    // valid and not found means it was optional, so ok
>+    if (isValid && notFound) {
>+      isValid = PR_TRUE;
>+      notFound = PR_FALSE;
>+    }

isValid is already true.  Why set it again?

I'm about 1/2 of the way through the review.  More tomorrow.

Comment 7

[Doron Rosenberg (IBM)]

(In reply to comment #6)
> (From update of attachment 216052 [details] [diff] [review] [edit])
> 
> >Index: extensions/schema-validation/src/nsSchemaValidator.cpp
> >===================================================================
> >RCS file: /cvsroot/mozilla/extensions/schema-validation/src/nsSchemaValidator.cpp,v
> >retrieving revision 1.22
> >diff -u -r1.22 nsSchemaValidator.cpp
> >--- extensions/schema-validation/src/nsSchemaValidator.cpp	21 Mar 2006 16:39:35 -0000	1.22
> >+++ extensions/schema-validation/src/nsSchemaValidator.cpp	23 Mar 2006 21:24:00 -0000
> 
> 
> >+  /* 
> >+   * We allow the schema validator to continue validating a structure
> >+   * even if the nodevalue is invalid per its simpletype binding.  We remember
> >+   * this by using mForceInvalid as an override for the final return.
> >+   */
> >+
> 
> Ummm, why do all this work if you are just going to make aResult PR_FALSE
> anyhow?  Just
> for the rv?  Please comment why you do this.
> 

Reason is, so that we continue marking nodes' schema types even if we encounter a node value that is invalid.  But yeah, needs to be commented better.

thanks for reviewing!

Comment 8

[aaronr]

Comment on attachment 216052 [details] [diff] [review]
first reviewable patch


>+nsresult
>+nsSchemaValidator::ValidateComplexElement(nsIDOMNode* aNode,
>+                                          nsISchemaParticle *aSchemaParticle,
>+                                          PRBool *aResult)
>+{
>+  PRBool isValid = PR_FALSE;
>+
>+  nsCOMPtr<nsISchemaElement> schemaElement = do_QueryInterface(aSchemaParticle);
>+  if (!schemaElement)
>+    return NS_ERROR_UNEXPECTED;
>+

nit: according to nsCOMPtr docs, should use 'direct initialisation' rather than
'copy initialisation' :-)  All over your code.  I won't mention it at each place.


>+nsresult
>+nsSchemaValidator::ValidateComplexChoice(nsIDOMNode* aStartNode,
>+                                         nsISchemaModelGroup *aSchemaModelGroup,
>+                                         nsIDOMNode **aLeftOvers,
>+                                         PRBool *aNotFound, PRBool *aResult)
>+{
>+  // get the model group details
>+  PRUint32 minOccurs;
>+  nsresult rv = aSchemaModelGroup->GetMinOccurs(&minOccurs);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRUint32 maxOccurs;
>+  rv = aSchemaModelGroup->GetMaxOccurs(&maxOccurs);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRUint32 particleCount;
>+  rv = aSchemaModelGroup->GetParticleCount(&particleCount);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIDOMNodeList> nodeList;
>+  aStartNode->GetChildNodes(getter_AddRefs(nodeList));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!nodeList)
>+    return NS_ERROR_UNEXPECTED;
>+
>+  PRUint32 childNodesLength;
>+  rv = nodeList->GetLength(&childNodesLength);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  /*
>+    xsd:choice means one of the particles must validate.
>+  */
>+  PRBool isValid = PR_FALSE;
>+  PRBool notFound = PR_FALSE;
>+  nsCOMPtr<nsISchemaParticle> particle;
>+  nsCOMPtr<nsIDOMNode> currentNode(aStartNode), leftOvers, tmpNode;
>+  nsAutoString localName, particleName;
>+  PRUint32 particleCounter = 0;
>+
>+  LOG(("======================== New Choice ==============================="));
>+
>+  while (!isValid && currentNode && (particleCounter < particleCount)) {
>+    // get node type
>+    PRUint16 nodeType;
>+    currentNode->GetNodeType(&nodeType);
>+
>+    // if not an element node, skip
>+    if (nodeType != nsIDOMNode::ELEMENT_NODE) {
>+      currentNode->GetNextSibling(getter_AddRefs(tmpNode));
>+      currentNode = tmpNode;
>+      continue;
>+    }
>+
>+    // get the particle
>+    rv = aSchemaModelGroup->GetParticle(particleCounter, getter_AddRefs(particle));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    particle->GetName(particleName);
>+    currentNode->GetLocalName(localName);
>+
>+    // if the particle has no name (so not an xsd:element) or the name patches
>+    // the node's localname, then we should try to validate.
>+    if (particleName.IsEmpty() || localName.Equals(particleName)) {
>+      rv = ValidateComplexParticle(currentNode, particle,
>+                                   getter_AddRefs(leftOvers), &notFound,
>+                                   &isValid);

'patches'?  Or do you mean 'matches'?

>+
>+      // if not valid and the names matched, we can bail early
>+      if (!isValid && localName.Equals(particleName)) {
>+        // The schema spec says that you can't have 2 particles with the same name,
>+        // so we know we don't have to continue looking for matching particles.
>+        LOG(("  Invalid: We found a matching particle, but it did not validate"));
>+        break;
>+      }
>+
>+      currentNode = leftOvers;
>+    }
>+
>+    particleCounter++;
>+  }
>+
>+  if (!isValid) {
>+    // None of the particles managed to validate.
>+    notFound = PR_TRUE;
>+  }
>+
>+  // make sure aLeftOvers points to null or an element node
>+  nsSchemaValidatorUtils::SetToNullOrElement(currentNode, aLeftOvers);
>+
>+  *aNotFound = notFound;
>+  *aResult = isValid;
>+
>+  return rv;
>+}
>+
>+nsresult
>+nsSchemaValidator::ValidateComplexAll(nsIDOMNode* aStartNode,
>+                                      nsISchemaModelGroup *aSchemaModelGroup,
>+                                      nsIDOMNode **aLeftOvers, PRBool *aNotFound,
>+                                      PRBool *aResult)
>+{
>+  if (!aStartNode || !aSchemaModelGroup)
>+    return NS_ERROR_UNEXPECTED;
>+
>+  // get the model group details
>+  PRUint32 minOccurs;
>+  nsresult rv = aSchemaModelGroup->GetMinOccurs(&minOccurs);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRUint32 maxOccurs;
>+  rv = aSchemaModelGroup->GetMaxOccurs(&maxOccurs);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRUint32 particleCount;
>+  rv = aSchemaModelGroup->GetParticleCount(&particleCount);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // xsd:all means that the order of the particles does not matter, and
>+  // that particles can only occur 0 or 1 time
>+  PRBool isValid = PR_FALSE;
>+  PRBool notFound = PR_FALSE;
>+  PRBool done = PR_FALSE;
>+  nsCOMPtr<nsISchemaParticle> particle;
>+  nsCOMPtr<nsIDOMNode> currentNode(aStartNode), leftOvers, tmpNode;
>+  nsAutoString localName, particleName;
>+
>+  // since xsd:all does not care about order, we need to record how often each
>+  // particle was hit
>+  nsDataHashtable<nsISupportsHashKey,PRUint32> particleHits;
>+  if (!particleHits.Init())
>+    return NS_ERROR_OUT_OF_MEMORY;

You should move the isValid, etc. stuff above to below here.  No sense doing
that work if you are just going to jump out.

>+
>+  for (PRUint32 i = 0; i < particleCount; ++i) {
>+    rv = aSchemaModelGroup->GetParticle(i, getter_AddRefs(particle));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    particleHits.Put(particle, 0);
>+  }
>+
>+  LOG(("====================== New All ==========================="));
>+
>+  PRUint32 validatedNodes = 0;
>+
>+  // we are done when we hit a node that doesn't fit our schema
>+  while (!done && currentNode) {
>+    // get node type
>+    PRUint16 nodeType;
>+    currentNode->GetNodeType(&nodeType);
>+    currentNode->GetLocalName(localName);
>+
>+    // if not an element node, skip
>+    if (nodeType != nsIDOMNode::ELEMENT_NODE) {
>+      currentNode->GetNextSibling(getter_AddRefs(tmpNode));
>+      currentNode = tmpNode;
>+      continue;
>+    }
>+
>+    LOG(("      - Validating element (%s)",
>+         NS_ConvertUTF16toUTF8(localName).get()));
>+
>+    // walk all the particles until we find one that validates
>+    PRUint32 particleNum = 0;
>+    PRBool foundParticle = PR_FALSE;
>+
>+    while (!foundParticle && particleNum < particleCount) {
>+      rv = aSchemaModelGroup->GetParticle(particleNum, getter_AddRefs(particle));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      particle->GetName(particleName);
>+
>+      if (particleName.Equals(localName)) {
>+        // try to validate
>+        rv = ValidateComplexParticle(currentNode, particle,
>+                                     getter_AddRefs(leftOvers), &notFound,
>+                                     &foundParticle);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+        nsAutoString tmps;
>+        leftOvers->GetLocalName(tmps);
>+      }

Ummmm, why get local name here if you never use it?  tmps will go out of scope right after
you call GetLocalName.


>+
>+nsresult
>+nsSchemaValidator::ValidateSchemaAttribute(nsIDOMNode* aNode,
>+                                           nsISchemaAttribute *aAttr,
>+                                           const nsAString & aAttrName,
>+                                           PRUint32 *aFoundAttrCount,
>+                                           PRBool *aResult)
>+{
>+  PRUint16 use;
>+  nsresult rv = aAttr->GetUse(&use);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsAutoString fixedValue, attrValue;
>+  rv = aAttr->GetFixedValue(fixedValue);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsISchemaSimpleType> simpleType;
>+  rv = aAttr->GetType(getter_AddRefs(simpleType));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsISchemaComponent> schema(do_QueryInterface(simpleType, &rv));
>+  NS_ENSURE_SUCCESS(rv, rv);

You don't reference 'use' or 'simpleType' until much later. Need to do the test this
early?  Also, I don't see where you use 'schema' at all.


Most of these comments were just nits or simple things.  With them, r=me

Comment 9

[Doron Rosenberg (IBM)]

fixed trunk/1.8 branch

Comment 10

[Doron Rosenberg (IBM)]

checked into the 1.8.0.3 branch