Also needs to retrieve stored schema types that the schema validator places, which the work in bug 278449 introduces.

Comment on attachment 216846 [details] [diff] [review] patch Please add a comment near ValidateNode to point out that it validates a node AND all child nodes. It isn't immediately apparent to us novices :-) >? extensions/xforms/attachment.cgi?id=189100 >? extensions/xforms/attachment.cgi?id=205748 >Index: extensions/xforms/nsXFormsModelElement.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.cpp,v >retrieving revision 1.101 >diff -u -r1.101 nsXFormsModelElement.cpp >--- extensions/xforms/nsXFormsModelElement.cpp 29 Mar 2006 10:49:27 -0000 1.101 >+++ extensions/xforms/nsXFormsModelElement.cpp 31 Mar 2006 17:51:45 -0000 >@@ -1509,6 +1509,43 @@ > return NS_OK; > } > >+NS_IMETHODIMP >+nsXFormsModelElement::ValidateDocument(nsIDOMDocument *aInstanceDocument, >+ PRBool *aResult) >+{ nit: align params >+ NS_ENSURE_ARG_POINTER(aResult); >+ NS_ENSURE_STATE(aInstanceDocument); >+ nit: how about NS_ENSURE_ARG for aInstanceDocument? Better error code returned. >+ } else { >+ // XXX: should we not return NS_OK here? >+ const PRUnichar *strings[] = { nsuri.get() }; >+ nsXFormsUtils::ReportError(NS_LITERAL_STRING("noSchemaForInstance"), >+ strings, 1, element, nsnull); >+ } please serialize the xf:instance element, not the document root element in the error. In my humble opinion, I think that we should return an error if we can't validate. Someone some day might want to be able to draw the distinction between an invalid instance document and one that couldn't even be validated. >@@ -1579,6 +1616,22 @@ > // If there was no type information on the node itself, check for a type > // bound to the node via \<xforms:bind\> > if (!typeVal && !mNodeToType.Get(aInstanceData, &typeVal)) { >+ // check if schema validation left us a nsISchemaType* >+ nsCOMPtr<nsIContent> content = do_QueryInterface(aInstanceData); >+ >+ if (content) { >+ nsISchemaType *type; >+ nsCOMPtr<nsIAtom> myAtom = do_GetAtom("xsdtype"); >+ >+ type = NS_STATIC_CAST(nsISchemaType *, content->GetProperty(myAtom)); >+ if (type) { >+ type->GetName(aType); >+ type->GetTargetNamespace(aNSUri); >+ type = nsnull; >+ return NS_OK; >+ } >+ } >+ why set type to null? > // No type information found > return NS_ERROR_NOT_AVAILABLE; > } >@@ -1923,6 +1976,27 @@ > nsXFormsUtils::DispatchEvent(model->mElement, eEvent_ModelConstructDone); > } > I believe that this should probably be done in FinishConstruction() before we do the xforms-revalidate. Allan, could you verify if I speaketh the truth? But might not be a big deal if we aren't going to mark the controls invalid on page load. >+ // validate the instance documents >+ if (mInstanceDocuments) { >+ PRUint32 instCount; >+ mInstanceDocuments->GetLength(&instCount); >+ if (instCount) { >+ nsCOMPtr<nsIDOMDocument> document; >+ >+ for (PRUint32 i = 0; i < instCount; ++i) { >+ nsIInstanceElementPrivate* instEle = mInstanceDocuments->GetInstanceAt(i); >+ nsCOMPtr<nsIXFormsNSInstanceElement> NSInstEle(instEle); >+ NSInstEle->GetInstanceDocument(getter_AddRefs(document)); >+ >+ if (document) { >+ // we don't care about the result >+ PRBool isValid = PR_FALSE; >+ ValidateDocument(document, &isValid); >+ } >+ } Just an idea...maybe call it ValidateInstance and pass in the InstanceElementPrivate rather than the document. Then you'll have the instance element already in your hands if you want to serialize the instance element upon error. >+ } >+ } >+ > nsXFormsModelElement::ProcessDeferredBinds(domDoc); > > nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc); >@@ -1945,6 +2019,7 @@ > model->mReadyHandled = PR_TRUE; > nsXFormsUtils::DispatchEvent(model->mElement, eEvent_Ready); > } >+ > } > > nsresult >Index: extensions/xforms/nsXFormsModelElement.h >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.h,v >retrieving revision 1.42 >diff -u -r1.42 nsXFormsModelElement.h >--- extensions/xforms/nsXFormsModelElement.h 31 Mar 2006 08:36:52 -0000 1.42 >+++ extensions/xforms/nsXFormsModelElement.h 31 Mar 2006 17:51:45 -0000 >@@ -343,7 +343,10 @@ > NS_HIDDEN_(nsresult) HandleUnload(nsIDOMEvent *aEvent); > > NS_HIDDEN_(nsresult) RefreshSubTree(nsXFormsControlListItem *aCurrent, >- PRBool aForceRebind); >+ PRBool aForceRebind); >+ >+ NS_HIDDEN_(nsresult) ValidateDocument(nsIDOMDocument *aInstanceDocument, >+ PRBool *aResult); > > nsIDOMElement *mElement; > nsCOMPtr<nsISchemaLoader> mSchemas; >Index: extensions/xforms/nsXFormsSchemaValidator.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSchemaValidator.cpp,v >retrieving revision 1.9 >diff -u -r1.9 nsXFormsSchemaValidator.cpp >--- extensions/xforms/nsXFormsSchemaValidator.cpp 22 Mar 2006 19:36:27 -0000 1.9 >+++ extensions/xforms/nsXFormsSchemaValidator.cpp 31 Mar 2006 17:51:45 -0000 >@@ -83,6 +83,17 @@ > } > > PRBool >+nsXFormsSchemaValidator::ValidateNode(nsIDOMNode* aElement) >+{ >+ PRBool isValid = PR_FALSE; >+ NS_ENSURE_TRUE(mSchemaValidator, isValid); >+ >+ mSchemaValidator->Validate(aElement, &isValid); >+ >+ return isValid; >+} >+ >+PRBool > nsXFormsSchemaValidator::ValidateNode(nsIDOMNode* aElement, > const nsAString & aType, > const nsAString & aNamespace) >Index: extensions/xforms/nsXFormsSchemaValidator.h >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSchemaValidator.h,v >retrieving revision 1.6 >diff -u -r1.6 nsXFormsSchemaValidator.h >--- extensions/xforms/nsXFormsSchemaValidator.h 29 Jun 2005 20:42:24 -0000 1.6 >+++ extensions/xforms/nsXFormsSchemaValidator.h 31 Mar 2006 17:51:45 -0000 >@@ -64,6 +64,14 @@ > const nsAString & aNamespace); > > /** >+ * Validates a node against the loaded schema(s) >+ * >+ * @param aElement Node to validate >+ * @return Whether the string is valid or not >+ */ >+ PRBool ValidateNode(nsIDOMNode* aElement); >+ nit: s/Whether the string/Whether the node. Should probably mention here that this validates the node and all sub-nodes/children. If you do add the comment here, probably don't need it where I pointed it out before. >+ /** > * Validates a node against a namespace and schema type > * > * @param aElement Node to validate >Index: extensions/xforms/resources/locale/en-US/xforms.properties >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/resources/locale/en-US/xforms.properties,v >retrieving revision 1.21 >diff -u -r1.21 xforms.properties >--- extensions/xforms/resources/locale/en-US/xforms.properties 22 Mar 2006 18:05:49 -0000 1.21 >+++ extensions/xforms/resources/locale/en-US/xforms.properties 31 Mar 2006 17:51:45 -0000 >@@ -68,6 +68,7 @@ > externalLink1Error = XForms Error (30): External file (%S) for %S element not found > externalLink2Error = XForms Error (31): Failed to load %S element from external file: %S > externalLinkLoadOrigin = XForms Error (32): Security check failed! Trying to load %S data from a different domain than document >+noSchemaForInstance = XForms Error (33): Could not find a schema for validating the instance document (namespace=%S) > > # Warning Messages: > warnSOAP = XForms Warning (1): You are using the SOAP post feature, which is an experimental feature! Beware that the functionality might change, and forms may stop working at any time. r-'ing for now until Allan either agrees/disagrees with where ValidateDocument should get called. Otherwise most of the comments are just nits.

Comment on attachment 216846 [details] [diff] [review] patch (In reply to comment #4) > (From update of attachment 216846 [details] [diff] [review] [edit]) > > Please add a comment near ValidateNode to point out that it validates a node > AND all child nodes. It isn't immediately apparent to us novices :-) Ouch, yes, if that is what is happening, that's bad. But that's because the function name itself is confusing. Why is the function not called ValidateTree(), ValidateSubtree(), or ValidateNodeAndChildren()? It's slightly confusing. You also seriously need some documentation in nsISchemaValidator.idl. > >@@ -1923,6 +1976,27 @@ > > nsXFormsUtils::DispatchEvent(model->mElement, eEvent_ModelConstructDone); > > } > > > > I believe that this should probably be done in FinishConstruction() before we > do the xforms-revalidate. Allan, could you verify if I speaketh the truth? I assume that you refer to the ValidateDocument() call. > But might not be a big deal if we aren't going to mark the controls invalid on > page load. That was just my misinterpretation, we should mark controls invalid on page load, we should just not send events on load (as there is no refresh() call). Hmmm, my understanding has always been that "complex type schema validation" only happens on submission, so we should not do that on form load, because that is an xforms-revalidate. I cannot seem to find that in the spec... but I'm hopefully just Monday-blind.

I added some more comments. Note that we don't ever do anything with the validation result, we just use it to add type properties (see the testcase!).

Comment on attachment 217154 [details] [diff] [review] commented patch Please use diff -p ... >+ PRBool isValid = PR_FALSE; >+ >+ // get namespace from node >+ nsAutoString nsuri; >+ element->GetNamespaceURI(nsuri); >+ >+ nsXFormsSchemaValidator validator; move this further down. no need to instanciate it if you are not going to use it :) >+ nsCOMPtr<nsISchemaCollection> schemaColl = do_QueryInterface(mSchemas); >+ if (schemaColl) { early return, por favor. >+ nsCOMPtr<nsISchema> schema; >+ schemaColl->GetSchema(nsuri, getter_AddRefs(schema)); >+ if (schema) { >+ validator.LoadSchema(schema); >+ // ValidateNode will validate the node and its subtree, as per the schema >+ // specification. >+ isValid = validator.ValidateNode(element); >+ } else { >+ // no schema found for the instance document's namespace. >+ nsCOMPtr<nsIDOMNode> instanceElement; >+ nsXFormsUtils::GetInstanceNodeForData(element, >+ getter_AddRefs(instanceElement)); >+ const PRUnichar *strings[] = { nsuri.get() }; >+ nsXFormsUtils::ReportError(NS_LITERAL_STRING("noSchemaForInstance"), >+ strings, 1, instanceElement, nsnull); >+ rv = NS_ERROR_UNEXPECTED; >+ } >+ } >+ >+ *aResult = isValid; >+ return NS_OK; return rv, or the above rv = ... does not do much > nsXFormsUtils::DispatchEvent(model->mElement, eEvent_ModelConstructDone); > } > >+ // validate the instance documents do the below comment here. ie "validate because blah blah". >+ if (mInstanceDocuments) { >+ PRUint32 instCount; >+ mInstanceDocuments->GetLength(&instCount); >+ if (instCount) { >+ nsCOMPtr<nsIDOMDocument> document; >+ >+ for (PRUint32 i = 0; i < instCount; ++i) { >+ nsIInstanceElementPrivate* instEle = mInstanceDocuments->GetInstanceAt(i); >+ nsCOMPtr<nsIXFormsNSInstanceElement> NSInstEle(instEle); >+ NSInstEle->GetInstanceDocument(getter_AddRefs(document)); >+ >+ if (document) { Ok, this should not fail should it? Rather fatal if it does I guess. I would go for a NS_ASSERTION instead then, or at least a NS_ENSURE_STATE() >+ // we don't care about the validation result here. We want >+ // schemaValidation to add schema type properties from the schema file >+ // unto our instance document elements. >+ PRBool isValid = PR_FALSE; >+ ValidateDocument(document, &isValid); I think we should ReportError() or debugging why types are not set because of complex type validation is failing will be hard. > nsresult >Index: extensions/xforms/nsXFormsModelElement.h >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.h,v >retrieving revision 1.42 >diff -u -r1.42 nsXFormsModelElement.h >--- extensions/xforms/nsXFormsModelElement.h 31 Mar 2006 08:36:52 -0000 1.42 >+++ extensions/xforms/nsXFormsModelElement.h 4 Apr 2006 15:21:42 -0000 >@@ -343,7 +343,10 @@ > NS_HIDDEN_(nsresult) HandleUnload(nsIDOMEvent *aEvent); > > NS_HIDDEN_(nsresult) RefreshSubTree(nsXFormsControlListItem *aCurrent, >- PRBool aForceRebind); >+ PRBool aForceRebind); >+ >+ NS_HIDDEN_(nsresult) ValidateDocument(nsIDOMDocument *aInstanceDocument, >+ PRBool *aResult); Heh, you fix the above indentation but do not bother to indent your new function? > nsIDOMElement *mElement; > nsCOMPtr<nsISchemaLoader> mSchemas; >Index: extensions/xforms/nsXFormsSchemaValidator.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSchemaValidator.cpp,v >retrieving revision 1.9 >diff -u -r1.9 nsXFormsSchemaValidator.cpp >--- extensions/xforms/nsXFormsSchemaValidator.cpp 22 Mar 2006 19:36:27 -0000 1.9 >+++ extensions/xforms/nsXFormsSchemaValidator.cpp 4 Apr 2006 15:21:43 -0000 >@@ -83,6 +83,17 @@ > } > > PRBool >+nsXFormsSchemaValidator::ValidateNode(nsIDOMNode* aElement) >+{ (I still think the name is misleading, but ignore that for now -- just so you are warned: I might get back to you on that :) ) >Index: extensions/xforms/nsXFormsSchemaValidator.h >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSchemaValidator.h,v >retrieving revision 1.6 >diff -u -r1.6 nsXFormsSchemaValidator.h >--- extensions/xforms/nsXFormsSchemaValidator.h 29 Jun 2005 20:42:24 -0000 1.6 >+++ extensions/xforms/nsXFormsSchemaValidator.h 4 Apr 2006 15:21:43 -0000 >@@ -64,12 +64,20 @@ > const nsAString & aNamespace); > > /** >- * Validates a node against a namespace and schema type >+ * Validates a node and it's subtree against the loaded schema(s) "it's" -> "its" >+ * >+ * @param aElement Node to validate >+ * @return Whether the string is valid or not >+ */ >+ PRBool ValidateNode(nsIDOMNode* aElement); >+ >+ /** >+ * Validates a node and it's subtree against a namespace and schema type Huh, does this also check the subtree? > * > * @param aElement Node to validate > * @param aType Type > * @param aNamespace Namespace >- * @return Whether the string is valid or not >+ * @return Whether the node is valid or not > */ > PRBool ValidateNode(nsIDOMNode* aElement, const nsAString & aType, > const nsAString & aNamespace); With these changes I'm the happy camper, so r=me if you do not totally rewrite everything :)

Comment on attachment 217154 [details] [diff] [review] commented patch with Allan's comments fixed, r=me

checked into trunk