need to write hexBinary schema validation code

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
a year ago

People

(Reporter: jhp (no longer active), Assigned: Doron Rosenberg (IBM))

Tracking

({fixed1.8.0.2, fixed1.8.1})

Trunk
fixed1.8.0.2, fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
Offshoot of bug 275453.
(Assignee)

Comment 1

12 years ago
Created attachment 201567 [details] [diff] [review]
le patch
Attachment #201567 - Flags: review?(jhpedemonte)
(Reporter)

Comment 2

12 years ago
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+
(Assignee)

Comment 3

12 years ago
Created attachment 201660 [details] [diff] [review]
fixes pedemonte's comments
Attachment #201567 - Attachment is obsolete: true
Attachment #201660 - Flags: review?(aaronr)

Comment 4

12 years ago
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-
(Assignee)

Comment 5

12 years ago
Created attachment 201854 [details] [diff] [review]
better patch

aResult will always be present, since nsSchemaValidator is the only one calling those methods.
Attachment #201854 - Flags: review?(aaronr)

Comment 6

12 years ago
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+
(Assignee)

Comment 7

12 years ago
checked into trunk
Status: NEW → ASSIGNED
Whiteboard: xf-to-branch

Comment 8

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

Updated

12 years ago
Whiteboard: xf-to-branch

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8.0.2
Resolution: --- → FIXED

Comment 9

11 years ago
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.