Closed Bug 308500 Opened 15 years ago Closed 15 years ago

need to write hexBinary schema validation code

Categories

(Core Graveyard :: XForms, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jhpedemonte, Assigned: doronr)

Details

(Keywords: fixed1.8.0.2, fixed1.8.1)

Attachments

(2 files, 1 obsolete file)

Offshoot of bug 275453.
Attached patch le patch (obsolete) — Splinter Review
Attachment #201567 - Flags: review?(jhpedemonte)
Comment on attachment 201567 [details] [diff] [review]
le patch

+    // For hexBinary, length is measured in octets (8 bits) of binary data.

To this, add another line about "one byte of binary data is represented by two hex digits, so divide by 2 to get binary data length."  Or something like that.  You may even want to rename "length" to "binaryDataLength" to make it more obvious.

+  if (length % 2 == 0) {

Rather than encapsulating the main code of the funtion in this if statement, just do an early return for "!= 0".  Then you won't need to have two PRBools ('isValid' and 'foundError').

+        validate("0FB7", "hexbinary-test-1", true);
+        validate("023-", "hexbinary-test-1", false);
+        validate("0HB7", "hexbinary-test-1", false);

Add another test for an odd length string.

With that, r+.
Attachment #201567 - Flags: review?(jhpedemonte) → review+
Attachment #201567 - Attachment is obsolete: true
Attachment #201660 - Flags: review?(aaronr)
Comment on attachment 201660 [details] [diff] [review]
fixes pedemonte's comments

>? extensions/schema-validation/f
>Index: extensions/schema-validation/src/nsSchemaValidator.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/schema-validation/src/nsSchemaValidator.cpp,v
>retrieving revision 1.15
>diff -w -u -r1.15 nsSchemaValidator.cpp
>--- extensions/schema-validation/src/nsSchemaValidator.cpp	14 Oct 2005 16:49:47 -0000	1.15
>+++ extensions/schema-validation/src/nsSchemaValidator.cpp	2 Nov 2005 20:13:28 -0000

>@@ -2777,6 +2791,84 @@
>   return isValid;
> }
> 
>+// http://www.w3.org/TR/xmlschema-2/#hexBinary
>+nsresult
>+nsSchemaValidator::ValidateBuiltinTypeHexBinary(const nsAString & aNodeValue,
>+                                                PRUint32 aLength,
>+                                                PRBool aLengthDefined,
>+                                                PRUint32 aMinLength,
>+                                                PRBool aMinLengthDefined,
>+                                                PRUint32 aMaxLength,
>+                                                PRBool aMaxLengthDefined,
>+                                                nsStringArray *aEnumerationList,
>+                                                PRBool *aResult)
>+{
>+  PRBool isValid = PR_FALSE;

Do you want to test if aResult even exists before going through the pain of figuring out if the value is valid?

>+
>+  isValid = IsValidSchemaHexBinary(aNodeValue);
>+
>+  if (isValid) {
>+    // For hexBinary, length is measured in octets (8 bits) of binary data.  So
>+    // one byte of binary data is represented by two hex digits, so we
>+    // divide by 2 to get the binary data length.
>+
>+    PRUint32 binaryDataLength = aNodeValue.Length() / 2;
>+
>+    if (aLengthDefined && (binaryDataLength != aLength)) {
>+      isValid = PR_FALSE;
>+      LOG(("  Not valid: Not the right length (%d)", binaryDataLength));
>+    }
>+
>+    if (aMinLengthDefined && (binaryDataLength < aMinLength)) {
>+      isValid = PR_FALSE;
>+      LOG(("  Not valid: Length (%d) is too small", binaryDataLength));
>+    }
>+
>+    if (aMaxLengthDefined && (binaryDataLength > aMaxLength)) {
>+      isValid = PR_FALSE;
>+      LOG(("  Not valid: Length (%d) is too large", binaryDataLength));
>+    }
>+  }

Shouldn't you return if isValid = PR_FALSE?  Why keep testing?  It can't go back to true and the continued logging doesn't seem to make sense in some cases.  Like how can the length be too small AND too large?

>+
>+  if (isValid && aEnumerationList && (aEnumerationList->Count() > 0)) {
>+    isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue, *aEnumerationList);
>+  }
>+
>+#ifdef PR_LOGGING
>+  LOG((isValid ? ("  Value is valid!") : ("  Value is not valid!")));
>+#endif
>+

Why is this log ifdef'd and the others aren't?

>+  *aResult = isValid;
>+  return NS_OK;
>+}
>+
>+PRBool
>+nsSchemaValidator::IsValidSchemaHexBinary(const nsAString & aString)
>+{
>+  // hex binary length has to be even
>+  PRUint32 length = aString.Length();
>+
>+  if (length % 2 != 0)
>+    return PR_FALSE;
>+
>+  nsAString::const_iterator start, end;
>+  aString.BeginReading(start);
>+  aString.EndReading(end);
>+
>+  PRBool isValid = PR_TRUE;
>+
>+  // each character has to be in [0-9a-fA-F]
>+  while ((start != end) && isValid) {
>+    PRUnichar temp = *start++;
>+
>+    if (!isxdigit(temp)) {
>+      isValid = PR_FALSE;
>+    }
>+  }
>+
>+  return isValid;
>+}
>+

Wouldn't it be more efficient to break out of the while loop as soon as you set isValid is false instead of checking for isValid on every iteration?
Attachment #201660 - Flags: review?(aaronr) → review-
Attached patch better patchSplinter Review
aResult will always be present, since nsSchemaValidator is the only one calling those methods.
Attachment #201854 - Flags: review?(aaronr)
Comment on attachment 201854 [details] [diff] [review]
better patch


>+// http://www.w3.org/TR/xmlschema-2/#hexBinary
>+nsresult
>+nsSchemaValidator::ValidateBuiltinTypeHexBinary(const nsAString & aNodeValue,
>+                                                PRUint32 aLength,
>+                                                PRBool aLengthDefined,
>+                                                PRUint32 aMinLength,
>+                                                PRBool aMinLengthDefined,
>+                                                PRUint32 aMaxLength,
>+                                                PRBool aMaxLengthDefined,
>+                                                nsStringArray *aEnumerationList,
>+                                                PRBool *aResult)
>+{
>+  PRBool isValid = PR_FALSE;
>+
>+  isValid = IsValidSchemaHexBinary(aNodeValue);
>+

should move the IsValidSchemaHexBinary up to where isValid is initialized.

with that r=me.
Attachment #201854 - Flags: review?(aaronr) → review+
checked into trunk
Status: NEW → ASSIGNED
Whiteboard: xf-to-branch
checked into MOZILLA_1_8_BRANCH via bug 323691.  Leaving open for now until it gets into 1.8.0
Whiteboard: xf-to-branch
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: fixed1.8.0.2
Resolution: --- → FIXED
verified fixed in MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.