Last Comment Bug 310962 - Multiple schema types for a node not being processed correctly
: Multiple schema types for a node not being processed correctly
Status: RESOLVED FIXED
: fixed1.8.1.12
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Peter Nunn
: Stephen Pride
Mentors:
http://www.w3.org/TR/2006/REC-xforms-...
: 331473 (view as bug list)
Depends on:
Blocks: 326372 326373 410239
  Show dependency treegraph
 
Reported: 2005-10-03 12:14 PDT by Peter Nunn
Modified: 2016-07-15 14:46 PDT (History)
8 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Sample form showing problems (2.77 KB, application/xhtml+xml)
2005-10-03 20:14 PDT, Peter Nunn
no flags Details
updated test case, appears to still be issues (3.41 KB, application/xhtml+xml)
2006-10-13 07:33 PDT, Steve Speicher
no flags Details
Test case for types including attributes (3.72 KB, application/xhtml+xml)
2007-01-22 13:42 PST, Peter Nunn
no flags Details
Patch to add full attribute support (19.27 KB, patch)
2007-01-25 01:32 PST, Peter Nunn
no flags Details | Diff | Splinter Review
Path to add attribute support (19.79 KB, patch)
2007-01-25 11:10 PST, Peter Nunn
no flags Details | Diff | Splinter Review
Patch to add attribute type support (20.71 KB, patch)
2007-01-29 00:49 PST, Peter Nunn
doronr: review+
aaronr: review-
Details | Diff | Splinter Review
Patch to support schema types for attributes (21.73 KB, patch)
2007-03-07 15:41 PST, Peter Nunn
aaronr: review-
Details | Diff | Splinter Review
Only cosmetic fixes as requested by aaronr (43.59 KB, patch)
2007-10-10 18:10 PDT, Philipp Wagner [:imphil]
aaronr: review+
Details | Diff | Splinter Review
patch for trunk (41.56 KB, patch)
2007-11-08 00:30 PST, aaronr
doronr: review+
Details | Diff | Splinter Review

Description Peter Nunn 2005-10-03 12:14:29 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051003 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051003 Firefox/1.6a1

6.2.1 of spec states that the order for schema types is:
1. xml schema associated with instance (don't think it is supported yet)
2. xsi:type on the instance element
3. xforms type on a bind
4. default to an xsd:string

At present the processor will apply rule 4. if rule 2. is not met and when a
type is assigned on a bind rule 3. results in a duplicate MIP property error in
the javascript error log.

Working on a patch, will upload when complete.

Reproducible: Always
Comment 1 aaronr 2005-10-03 12:22:55 PDT
Reassigning to Peter.  He has a patch and testcase coming for this bug.
Comment 2 Peter Nunn 2005-10-03 20:14:51 PDT
Created attachment 198399 [details]
Sample form showing problems

This is a form that in the current build should allow schema types on nodes. 
Should not work when mozilla supports schemas on instance nodes.
Comment 3 Peter Nunn 2005-10-04 03:03:13 PDT
Comment on attachment 198399 [details]
Sample form showing problems

Messed up the test case, am re-writing
Comment 4 Allan Beaufour 2006-04-03 01:56:02 PDT
*** Bug 331473 has been marked as a duplicate of this bug. ***
Comment 5 Allan Beaufour 2006-04-03 01:56:58 PDT
We might not behave like comment 0, but we still do it wrong.
Comment 6 Allan Beaufour 2006-04-05 07:43:32 PDT
Started a thread on www-forms about this.
http://lists.w3.org/Archives/Public/www-forms/2006Apr/0043.html
An erratum is needed for 6.1.1 ...
Comment 7 Steve Speicher 2006-10-13 07:33:32 PDT
Created attachment 242184 [details]
updated test case, appears to still be issues

Am running into similar issues with an XML instance doc that has an associated schema.  Within the instance, there are also complex types with xsi:type.
Comment 8 Peter Nunn 2007-01-22 13:42:00 PST
Created attachment 252371 [details]
Test case for types including attributes

I have a fix for this in my version in MozzIE also implemented schema types as attributes.  Will only work on 1.9 of gecko as need DOM3 types.
This attachment is the test case where the types are stored as attributes as well as elements.
Comment 9 Peter Nunn 2007-01-25 01:32:46 PST
Created attachment 252755 [details] [diff] [review]
Patch to add full attribute support

This patch needs to be run in the extensions directory as we patch schema validation.
Adds support for returning the type of the node in a DOM3 node user data which is available for both attributes and elements.
Also improves the handling of types more in line with the spec.
Once applied the test case should run and xsd:types can be determined on attributes as well as on elements.
Will only work on the trunk builds as need IDOM3Node
Comment 10 Olli Pettay [:smaug] 2007-01-25 03:14:09 PST
1.8 branch does have nsIDOM3Node.
And please, do not use tabs, but spaces :)
Comment 11 Peter Nunn 2007-01-25 11:10:21 PST
Created attachment 252797 [details] [diff] [review]
Path to add attribute support

Un-tabified the patch.  Must have edited with the wrong setting on my editor.
If there is a nsIDOM3Node in gecko 1.8 then this should work OK.  I had trouble compiling against the 1.8 tree.  Will try again.
Comment 12 alexander :surkov 2007-01-28 21:11:19 PST
(In reply to comment #11)
> Created an attachment (id=252797) [details]
> Path to add attribute support
> 
> Un-tabified the patch. 

Please use two spaces for indentation.
Comment 13 Peter Nunn 2007-01-29 00:49:40 PST
Created attachment 253149 [details] [diff] [review]
Patch to add attribute type support

Checked the tabs and looked ok at 2 spaces.  Have redone just to be sure
Comment 14 alexander :surkov 2007-02-01 06:51:26 PST
Is the patch ready for review?
Comment 15 Peter Nunn 2007-02-02 11:47:08 PST
Yes it is.  This patch is for the 1.9 build.  I have a patch for the 1.8 build as well, or is that closed?
Comment 16 alexander :surkov 2007-02-02 19:28:32 PST
Comment on attachment 253149 [details] [diff] [review]
Patch to add attribute type support

Then it would be fine if Doron will look at this.
Comment 17 Doron Rosenberg (IBM) 2007-02-05 13:21:46 PST
Comment on attachment 253149 [details] [diff] [review]
Patch to add attribute type support

Looks good to me.
Comment 18 Steve Speicher 2007-02-07 16:37:30 PST
I did look at attachment 253149 [details] [diff] [review]

>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIModelElementPrivate.idl,v
>retrieving revision 1.24
>diff -u -8 -p -r1.24 nsIModelElementPrivate.idl
>--- xforms/nsIModelElementPrivate.idl	1 Nov 2006 23:02:13 -0000	1.24
>+++ xforms/nsIModelElementPrivate.idl	22 Jan 2007 23:25:25 -0000
>@@ -61,16 +61,22 @@ interface nsIModelElementPrivate : nsIXF
> 
>   /**
>    * Determine the type for a form control based on the schema included by
>    * this model.
>    */
>   nsISchemaType getTypeForControl(in nsIXFormsControl control);
> 
>   /** 
>+   * Determine the type for a form control based on the schema included by
>+   * this model.
>+   */
>+  nsISchemaType getTypeForNode(in nsIDOMNode node);
>+
>+  /** 

Don't interface updates require uuid to be updated?  Aaron or others?

>    * This function takes an instance data node, finds the type bound to it, and
>    * returns the separated out type and namespace URI.  If no type is set for
>    * the node, then it returns the defaults: "http://www.w3.org/2001/XMLSchema"
>    * and "string"
>    */
>   void getTypeAndNSFromNode(in nsIDOMNode instancenode, out AString type, 
>                             out AString namespaceURI);
> 
>Index: xforms/nsXFormsModelElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.cpp,v
>retrieving revision 1.139
>diff -u -8 -p -r1.139 nsXFormsModelElement.cpp
>--- xforms/nsXFormsModelElement.cpp	4 Jan 2007 20:07:03 -0000	1.139
>+++ xforms/nsXFormsModelElement.cpp	29 Jan 2007 08:41:23 -0000
>@@ -40,16 +40,18 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsXFormsModelElement.h"
> #include "nsIXTFElementWrapper.h"
> #include "nsMemory.h"
> #include "nsIDOMElement.h"
> #include "nsIDOM3Node.h"
> #include "nsIDOMNodeList.h"
>+#include "nsIDOM3Attr.h"
>+#include "nsIVariant.h"
> #include "nsString.h"
> #include "nsIDocument.h"
> #include "nsXFormsAtoms.h"
> #include "nsINameSpaceManager.h"
> #include "nsIServiceManager.h"
> #include "nsIDOMEvent.h"
> #include "nsIDOMDOMImplementation.h"
> #include "nsIDOMXMLDocument.h"
>@@ -177,18 +179,19 @@ GetModelList(nsIDOMDocument *domDoc)
>   return NS_STATIC_CAST(nsVoidArray *,
>                         doc->GetProperty(nsXFormsAtoms::modelListProperty));
> }
> 
> static void
> SupportsDtorFunc(void *aObject, nsIAtom *aPropertyName,
>                  void *aPropertyValue, void *aData)
> {
>-  nsISupports *propertyValue = NS_STATIC_CAST(nsISupports*, aPropertyValue);
>-  NS_IF_RELEASE(propertyValue);
>+  nsISupports *propertyValue = NS_REINTERPRET_CAST(nsISupports*, aPropertyValue);
>+  if(propertyValue != nsnull)
>+    propertyValue->Release();
> }
> 
> 
> //------------------------------------------------------------------------------
> // --- nsXFormsControlListItem  ---
> 
> 
> nsXFormsControlListItem::iterator::iterator()
>@@ -315,18 +318,21 @@ nsXFormsControlListItem::AddControl(nsIX
> {
>   // Four insertion posibilities:
> 
>   // 1) Delegate to first child from root node
>   if (!mNode && mFirstChild) {
>     return mFirstChild->AddControl(aControl, aParent);
>   }
> 
>+  // Locate parent
>+  nsXFormsControlListItem* parentControl = FindControl(aParent);
>+
>   // 2) control with no parent
>-  if (!aParent) {
>+  if (!parentControl) {
>     nsXFormsControlListItem* newNode =
>       new nsXFormsControlListItem(aControl, mControlListHash);
> 
>     NS_ENSURE_STATE(newNode);
> 
>     // Empty tree (we have already checked mFirstChild)
>     if (!mNode) {
>       mFirstChild = newNode;
>@@ -350,20 +356,16 @@ nsXFormsControlListItem::AddControl(nsIX
>                    "Node already in tree!!");
>       next = next->mNextSibling;
>     }
> #endif
> 
>     return NS_OK;
>   }
> 
>-  // Locate parent
>-  nsXFormsControlListItem* parentControl = FindControl(aParent);
>-  NS_ASSERTION(parentControl, "Parent not found?!");
>-
>   // 3) parentControl has a first child, insert as sibling to that
>   if (parentControl->mFirstChild) {
>     return parentControl->mFirstChild->AddControl(aControl, nsnull);
>   }
> 
>   // 4) first child for parentControl
>   nsXFormsControlListItem* newNode =
>     new nsXFormsControlListItem(aControl, mControlListHash);
>@@ -506,16 +508,19 @@ nsXFormsControlListItem::end()
> {
>   return nsnull;
> }
> 
> 
> //------------------------------------------------------------------------------
> 
> static const nsIID sScriptingIIDs[] = {
>+  NS_IDOMELEMENT_IID,
>+  NS_IDOMEVENTTARGET_IID,
>+  NS_IDOM3NODE_IID,
>   NS_IXFORMSMODELELEMENT_IID,
>   NS_IXFORMSNSMODELELEMENT_IID
> };
> 
> static nsIAtom* sModelPropsList[eModel__count];
> 
> // This can be nsVoidArray because elements will remove
> // themselves from the list if they are deleted during refresh.
>@@ -563,27 +568,29 @@ nsPostRefresh::~nsPostRefresh()
>   }
> 
>   --sRefreshing;
> 
>   // process sContainerPostRefreshList after we've decremented sRefreshing.
>   // container->refresh below could ask for ContainerNeedsPostRefresh which
>   // will add an item to the sContainerPostRefreshList if sRefreshing > 0.
>   // So keeping this under sRefreshing-- will avoid an infinite loop.
>+  if (sContainerPostRefreshList) {

nit: update this block by 2 spaces

> (sContainerPostRefreshList && sContainerPostRefreshList->Count()) {
>     PRInt32 last = sContainerPostRefreshList->Count() - 1;
>     nsIXFormsControl* container =
>       NS_STATIC_CAST(nsIXFormsControl*, sContainerPostRefreshList->ElementAt(last));
>     sContainerPostRefreshList->RemoveElementAt(last);
>     if (container) {
>       container->Refresh();
>     }
>   }
>   delete sContainerPostRefreshList;
>   sContainerPostRefreshList = nsnull;
>+  }
> }
> 
> const nsVoidArray* 
> nsPostRefresh::PostRefreshList()
> {
>   return sPostRefreshList;
> }
> 
>@@ -1462,50 +1469,84 @@ nsXFormsModelElement::GetTypeForControl(
>   aControl->GetBoundNode(getter_AddRefs(boundNode));
>   if (!boundNode) {
>     // if the control isn't bound to instance data, it doesn't make sense to 
>     // return a type.  It is perfectly valid for there to be no bound node,
>     // so no need to use an NS_ENSURE_xxx macro, either.
>     return NS_ERROR_FAILURE;
>   }
> 
>+  return GetTypeForNode(boundNode, aType);
>+}
>+
>+NS_IMETHODIMP nsXFormsModelElement::GetTypeForNode(nsIDOMNode  *boundNode,
>+                                                   nsISchemaType    **aType)
>+{
>   nsAutoString schemaTypeName, schemaTypeNamespace;
>-  nsresult rv = GetTypeAndNSFromNode(boundNode, schemaTypeName,
>-                                     schemaTypeNamespace);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  nsresult rv = GetTypeFromNode(boundNode, schemaTypeName, schemaTypeNamespace);
> 
>+  if(NS_SUCCEEDED(rv))
>+  {

nit: update this block by 2 spaces

>   nsXFormsSchemaValidator validator;
> 
>   nsCOMPtr<nsISchemaCollection> schemaColl = do_QueryInterface(mSchemas);
>   if (schemaColl) {
>     nsCOMPtr<nsISchema> schema;
>     schemaColl->GetSchema(schemaTypeNamespace, getter_AddRefs(schema));
>     // if no schema found, then we will only handle built-in types.
>     if (schema)
>       validator.LoadSchema(schema);
>   }
> 
>-  PRBool foundType = validator.GetType(schemaTypeName, schemaTypeNamespace,
>-                                       aType);
>-  return foundType ? NS_OK : NS_ERROR_FAILURE;
>+    rv = validator.GetType(schemaTypeName, schemaTypeNamespace, aType) ? NS_OK : NS_ERROR_FAILURE;
>+  }
>+
>+  return rv;
> }
> 
> /* static */ nsresult
> nsXFormsModelElement::GetTypeAndNSFromNode(nsIDOMNode *aInstanceData,
>                                            nsAString &aType, nsAString &aNSUri)
> {
>-  nsresult rv = GetTypeFromNode(aInstanceData, aType, aNSUri);
>+  // 6.2.1 1. see if the instance data has a schema type.
>+  // if the control has a schema type then we will then
>+  // have to set a MIP node.
>+  nsCOMPtr<nsISchemaType> schemaType;
>+  nsresult rv = GetTypeForNode(aInstanceData, getter_AddRefs(schemaType));
>+
>+  if(rv == NS_OK)
>+{
>+    schemaType->GetTargetNamespace(aNSUri);
>+    schemaType->GetName(aType);
> 
>-  if (rv == NS_ERROR_NOT_AVAILABLE) {
>+    return NS_OK;
>+  }
>+
>+  // 6.2.1 2 & 3
>+  // see if the type is assigned as an xsi:type, or XForms:type
>+  nsAutoString schemaTypePrefix;
>+  rv = nsXFormsUtils::ParseTypeFromNode(aInstanceData, aType, schemaTypePrefix);
>+
>+  // 6.2.1 4. Otherwise it is a string
>+  if(rv == NS_ERROR_NOT_AVAILABLE) {
>     // if there is no type assigned, then assume that the type is 'string'
>     aNSUri.Assign(NS_LITERAL_STRING(NS_NAMESPACE_XML_SCHEMA));
>     aType.Assign(NS_LITERAL_STRING("string"));
>     rv = NS_OK;
>-  }
>+  } else {
>+    if (schemaTypePrefix.IsEmpty()) {
>+      aNSUri.AssignLiteral("");
>+    } else {
>+      // get the namespace url from the prefix
>+      nsCOMPtr<nsIDOM3Node> domNode3 = do_QueryInterface(mElement, &rv);
>+      NS_ENSURE_SUCCESS(rv, rv);
> 
>+      rv = domNode3->LookupNamespaceURI(schemaTypePrefix, aNSUri);
>+    }
>+  }
>   return rv;
> }
> 
> NS_IMETHODIMP
> nsXFormsModelElement::InstanceLoadStarted()
> {
>   ++mPendingInstanceCount;
>   return NS_OK;
>@@ -1623,31 +1664,34 @@ nsXFormsModelElement::SetNodeContent(nsI
> NS_IMETHODIMP
> nsXFormsModelElement::ValidateNode(nsIDOMNode *aInstanceNode, PRBool *aResult)
> {
>   NS_ENSURE_ARG_POINTER(aResult);
> 
>   nsAutoString schemaTypeName, schemaTypeNamespace;
>   nsresult rv = GetTypeAndNSFromNode(aInstanceNode, schemaTypeName, 
>                                      schemaTypeNamespace);
>-  NS_ENSURE_SUCCESS(rv, rv);
> 
>+  PRBool isValid = PR_FALSE;
>+  if(NS_SUCCEEDED(rv))
>+  {

nit: indent this block by 2 spaces

>   nsXFormsSchemaValidator validator;
>   nsCOMPtr<nsISchemaCollection> schemaColl = do_QueryInterface(mSchemas);
>   if (schemaColl) {
>     nsCOMPtr<nsISchema> schema;
>     schemaColl->GetSchema(schemaTypeNamespace, getter_AddRefs(schema));
>     // if no schema found, then we will only handle built-in types.
>     if (schema)
>       validator.LoadSchema(schema);
>   }
> 
>   nsAutoString value;
>   nsXFormsUtils::GetNodeValue(aInstanceNode, value);
>-  PRBool isValid = validator.ValidateString(value, schemaTypeName, schemaTypeNamespace);
>+    isValid = validator.ValidateString(value, schemaTypeName, schemaTypeNamespace);
>+  }
> 
>   *aResult = isValid;
>   return NS_OK;
> }
> 
> nsresult
> nsXFormsModelElement::ValidateDocument(nsIDOMDocument *aInstanceDocument,
>                                        PRBool         *aResult)
>@@ -1764,24 +1808,31 @@ nsXFormsModelElement::GetTypeFromNode(ns
>       typeVal = &typeAttribute;
>     }
>   }
> 
>   // 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");
>+    // this is stored on the DOM3Node as a property called xsdtype
>+    // which is userdata
>+    nsCOMPtr<nsIDOM3Node> dom3Node(do_QueryInterface(aInstanceData));
>+
>+    nsCOMPtr<nsIVariant> xsdType;
>+    if(NS_SUCCEEDED(dom3Node->GetUserData(NS_LITERAL_STRING("xsdtype"), getter_AddRefs(xsdType))) &&
>+        xsdType)
>+    {
>+      nsCOMPtr<nsISchemaType> type;
>+      nsIID *containedInterface;
> 
>-      type = NS_STATIC_CAST(nsISchemaType *, content->GetProperty(myAtom));
>-      if (type) {
>+      if(NS_SUCCEEDED(xsdType->GetAsInterface(&containedInterface, getter_AddRefs(type))) &&
>+        type)
>+      {
>         type->GetName(aType);
>         type->GetTargetNamespace(aNSUri);
>         return NS_OK;
>       }
>     }
> 
>     // No type information found
>     return NS_ERROR_NOT_AVAILABLE;
>@@ -1802,22 +1853,22 @@ nsXFormsModelElement::GetTypeFromNode(ns
>     // http://www.w3.org/TR/2004/REC-xmlschema-1-20041028/#src-qname and pick
>     // up the default namespace.  Which will happen by passing an empty string
>     // as first parameter to LookupNamespaceURI.
>     prefix = EmptyString();
>     aType.Assign(*typeVal);
>   } else {
>     prefix.Assign(Substring(*typeVal, 0, separator));
>     aType.Assign(Substring(*typeVal, ++separator, typeVal->Length()));
>+  }
> 

nit: the following if block needs to be outdented 2 spaces

>     if (prefix.IsEmpty()) {
>       aNSUri = EmptyString();
>       return NS_OK;
>     }
>-  }
> 
>   // get the namespace url from the prefix using instance data node
>   nsresult rv;
>   nsCOMPtr<nsIDOM3Node> domNode3 = do_QueryInterface(aInstanceData, &rv);
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = domNode3->LookupNamespaceURI(prefix, aNSUri);
> 
>   if (DOMStringIsNull(aNSUri)) {
>@@ -2351,22 +2402,24 @@ nsXFormsModelElement::ProcessBind(nsIDOM
> 
>   NS_ENSURE_STATE(result);
>   
>   // If this is an outer bind, store the nodeset, as controls binding to this
>   // bind will need this.
>   if (aIsOuter) {
>     nsCOMPtr<nsIContent> content(do_QueryInterface(aBindElement));
>     NS_ASSERTION(content, "nsIDOMElement not implementing nsIContent?!");
>-    rv = content->SetProperty(nsXFormsAtoms::bind, result,
>-                              SupportsDtorFunc);
>-    NS_ENSURE_SUCCESS(rv, rv);
> 
>-    // addref, circumventing nsDerivedSave
>-    NS_ADDREF(NS_STATIC_CAST(nsIDOMXPathResult*, result));
>+    // addref, as we are placing this into a map
>+    nsISupports *pSupports;
>+    result->QueryInterface(::nsISupports::COMTypeInfo<int>::kIID, (void **) &pSupports);
>+    pSupports->AddRef();
>+
>+    rv = content->SetProperty(nsXFormsAtoms::bind, result, SupportsDtorFunc);
>+    NS_ENSURE_SUCCESS(rv, rv);
>   }
> 
>   PRUint32 snapLen;
>   rv = result->GetSnapshotLength(&snapLen);
>   NS_ENSURE_SUCCESS(rv, rv);
>   
> 
>   // Iterate over resultset
>

Looks good to me.  Only have uuid and some general indentation issues.
Comment 19 aaronr 2007-02-07 18:01:29 PST
Comment on attachment 253149 [details] [diff] [review]
Patch to add attribute type support

You have extraneous whitespace at the end of some lines and other lines run past 80 characters.  Over 80 characters sometimes isn't avoidable (esp. in JS) or really necessary if they only run a couple of characters over and the source code reader can easily guess what have overrun.  But running over 80 characters should be avoided if at all possible.  Please run patches through http://beaufour.dk/jst-review/ to find some of the easier coding/styling issues that you may hit.  It is a useful tool that most of us use.


>Index: schema-validation/src/nsSchemaValidator.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/schema-validation/src/nsSchemaValidator.cpp,v
>retrieving revision 1.33
>diff -u -8 -p -r1.33 nsSchemaValidator.cpp
>--- schema-validation/src/nsSchemaValidator.cpp	19 Sep 2006 17:29:41 -0000	1.33
>+++ schema-validation/src/nsSchemaValidator.cpp	29 Jan 2007 08:40:55 -0000

>@@ -321,26 +329,26 @@ nsSchemaValidator::ValidateAgainstType(n
>       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);
>+      nsCOMPtr<nsIWritableVariant> holder = do_CreateInstance("@mozilla.org/variant;1");
>+      NS_ENSURE_STATE(holder);
>+
>+      holder->SetAsInterface(nsISchemaType::GetIID(), aType);
>+
>+      // and save on the node
>+      nsCOMPtr<nsIVariant> oldVariant;
>+      rv = domNode3->SetUserData(NS_LITERAL_STRING("xsdtype"), holder, nsnull, 
>+        getter_AddRefs(oldVariant));
>+

nit: please line up the getter_AddRefs with the first parameter.

Also you got rid of some comments.  If they don't apply, please replace them with more appropriate comments.  It isn't immediately apparent why you are doing what you are doing.

>@@ -4590,17 +4598,21 @@ nsSchemaValidator::ValidateAttributeComp
>       rv = ValidateSchemaAttributeGroup(aNode, attrGroup, name, aFoundAttrCount,
>                                         &isValid);
>       NS_ENSURE_SUCCESS(rv, rv);
> 
>       break;
>     }
> 
>     case nsISchemaAttributeComponent::COMPONENT_TYPE_ANY: {
>-      rv = NS_ERROR_NOT_IMPLEMENTED;
>+      // for now we just accept this one as being valid.
>+      // should look at the attribute namespace and validate the
>+      // attribute against it
>+      // TODO: implement this
>+      isValid = PR_TRUE;


Instead of using TODO, we use XXX.  Also, please open a bug on implementing this and note the bug number in the comment here.  Thanks.


>Index: xforms/nsIModelElementPrivate.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIModelElementPrivate.idl,v
>retrieving revision 1.24
>diff -u -8 -p -r1.24 nsIModelElementPrivate.idl
>--- xforms/nsIModelElementPrivate.idl	1 Nov 2006 23:02:13 -0000	1.24
>+++ xforms/nsIModelElementPrivate.idl	22 Jan 2007 23:25:25 -0000
>@@ -61,16 +61,22 @@ interface nsIModelElementPrivate : nsIXF
> 
>   /**
>    * Determine the type for a form control based on the schema included by
>    * this model.
>    */
>   nsISchemaType getTypeForControl(in nsIXFormsControl control);
> 
>   /** 
>+   * Determine the type for a form control based on the schema included by
>+   * this model.
>+   */
>+  nsISchemaType getTypeForNode(in nsIDOMNode node);
>+

so now we'll have getTypeFromNode and getTypeForNode?  Too confusing.  The function names and comments should be clear enough that if I want to use them I know which to use in what circumstance.  And then you say "Determine the type for a form control...", so does that mean that the 'node' parameter is the form control converted to a dom node or is 'node' the bound node for the control?  Might be worth using the @param style of commenting like is used in other parts of the .idl file so that we can easily see what 'node' is.

Plus, you need to generate a new uuid to use since you are adding an interface.  Though if this is never going to get called from outside nsxformsmodelelement.cpp, you could just make it a private worker function and skip the .idl altogether.

>Index: xforms/nsXFormsModelElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.cpp,v
>retrieving revision 1.139
>diff -u -8 -p -r1.139 nsXFormsModelElement.cpp
>--- xforms/nsXFormsModelElement.cpp	4 Jan 2007 20:07:03 -0000	1.139
>+++ xforms/nsXFormsModelElement.cpp	29 Jan 2007 08:41:23 -0000

>@@ -177,18 +179,19 @@ GetModelList(nsIDOMDocument *domDoc)
>   return NS_STATIC_CAST(nsVoidArray *,
>                         doc->GetProperty(nsXFormsAtoms::modelListProperty));
> }
> 
> static void
> SupportsDtorFunc(void *aObject, nsIAtom *aPropertyName,
>                  void *aPropertyValue, void *aData)
> {
>-  nsISupports *propertyValue = NS_STATIC_CAST(nsISupports*, aPropertyValue);
>-  NS_IF_RELEASE(propertyValue);
>+  nsISupports *propertyValue = NS_REINTERPRET_CAST(nsISupports*, aPropertyValue);
>+  if(propertyValue != nsnull)
>+    propertyValue->Release();
> }

Why the change away from using NS_IF_RELEASE(propertyValue)?

> 
> 
> //------------------------------------------------------------------------------
> // --- nsXFormsControlListItem  ---
> 
> 
> nsXFormsControlListItem::iterator::iterator()
>@@ -315,18 +318,21 @@ nsXFormsControlListItem::AddControl(nsIX
> {
>   // Four insertion posibilities:
> 
>   // 1) Delegate to first child from root node
>   if (!mNode && mFirstChild) {
>     return mFirstChild->AddControl(aControl, aParent);
>   }
> 
>+  // Locate parent
>+  nsXFormsControlListItem* parentControl = FindControl(aParent);
>+
>   // 2) control with no parent
>-  if (!aParent) {
>+  if (!parentControl) {

Why this code change?  If !parentControl happens but aParent exists, that is an error condition.  We shouldn't create a new node and try to insert it into the tree.  'Course, we'll end up crashing anyhow.  I guess I don't care if you leave this change in since it does save us an 'if' test for each xforms element that is below another xforms element in the DOM, but at least please comment above it that you realize that we know that we are doing this check this way on purpose otherwise someone might think, "hey, that's an error condition" and set it back.  And please put the assertion back in as an extra heads up for us developers.

>@@ -506,16 +508,19 @@ nsXFormsControlListItem::end()
> {
>   return nsnull;
> }
> 
> 
> //------------------------------------------------------------------------------
> 
> static const nsIID sScriptingIIDs[] = {
>+  NS_IDOMELEMENT_IID,
>+  NS_IDOMEVENTTARGET_IID,
>+  NS_IDOM3NODE_IID,
>   NS_IXFORMSMODELELEMENT_IID,
>   NS_IXFORMSNSMODELELEMENT_IID
> };
> 
> static nsIAtom* sModelPropsList[eModel__count];
> 
> // This can be nsVoidArray because elements will remove
> // themselves from the list if they are deleted during refresh.
>@@ -563,27 +568,29 @@ nsPostRefresh::~nsPostRefresh()
>   }
> 
>   --sRefreshing;
> 
>   // process sContainerPostRefreshList after we've decremented sRefreshing.
>   // container->refresh below could ask for ContainerNeedsPostRefresh which
>   // will add an item to the sContainerPostRefreshList if sRefreshing > 0.
>   // So keeping this under sRefreshing-- will avoid an infinite loop.
>+  if (sContainerPostRefreshList) {
>   while (sContainerPostRefreshList && sContainerPostRefreshList->Count()) {

why this change?  That's already taken care of in the while test.  Also a change like this you'd need to indent the 'while' and the rest of it.

>@@ -1462,50 +1469,84 @@ nsXFormsModelElement::GetTypeForControl(
>   aControl->GetBoundNode(getter_AddRefs(boundNode));
>   if (!boundNode) {
>     // if the control isn't bound to instance data, it doesn't make sense to 
>     // return a type.  It is perfectly valid for there to be no bound node,
>     // so no need to use an NS_ENSURE_xxx macro, either.
>     return NS_ERROR_FAILURE;
>   }
> 
>+  return GetTypeForNode(boundNode, aType);
>+}
>+

changing this to use GetTypeForNode instead of GetTypeAndNSFromNode will change the way that this function works, right?  If GetTypeForNode doesn't find the type, it'll return an error.  If GetTypeAndNSFromNode doesn't get the type from GetTypeFromNode, then it will make the type the default xforms type, xsd:string.  We still need that behavior, right?

>+NS_IMETHODIMP nsXFormsModelElement::GetTypeForNode(nsIDOMNode  *boundNode,
>+                                                   nsISchemaType    **aType)
>+{

nit: Parameters into a function in Mozilla always begin with an 'a', like aType.  So please change boundNode to aBoundNode.  The style of this file seems to be to line up the parameters, too (like in GetTypeForControl right above it).  Please do the same.

like: GetTypeForNode(nsIDOMNode     *aBoundNode,
                     nsISchemaType **aType)

>   nsAutoString schemaTypeName, schemaTypeNamespace;
>-  nsresult rv = GetTypeAndNSFromNode(boundNode, schemaTypeName,
>-                                     schemaTypeNamespace);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  nsresult rv = GetTypeFromNode(boundNode, schemaTypeName, schemaTypeNamespace);
> 
>+  if(NS_SUCCEEDED(rv))
>+  {

nit: You are wrapping code in an 'if', you need to indent the contents and correct any 'code beyond 80 chars' this may cause.

>   nsXFormsSchemaValidator validator;
> 
>   nsCOMPtr<nsISchemaCollection> schemaColl = do_QueryInterface(mSchemas);
>   if (schemaColl) {
>     nsCOMPtr<nsISchema> schema;
>     schemaColl->GetSchema(schemaTypeNamespace, getter_AddRefs(schema));
>     // if no schema found, then we will only handle built-in types.
>     if (schema)
>       validator.LoadSchema(schema);
>   }
> 
>-  PRBool foundType = validator.GetType(schemaTypeName, schemaTypeNamespace,
>-                                       aType);
>-  return foundType ? NS_OK : NS_ERROR_FAILURE;
>+    rv = validator.GetType(schemaTypeName, schemaTypeNamespace, aType) ? NS_OK : NS_ERROR_FAILURE;
>+  }
>+
>+  return rv;
> }
> 
> /* static */ nsresult
> nsXFormsModelElement::GetTypeAndNSFromNode(nsIDOMNode *aInstanceData,
>                                            nsAString &aType, nsAString &aNSUri)
> {
>-  nsresult rv = GetTypeFromNode(aInstanceData, aType, aNSUri);
>+  // 6.2.1 1. see if the instance data has a schema type.
>+  // if the control has a schema type then we will then
>+  // have to set a MIP node.
>+  nsCOMPtr<nsISchemaType> schemaType;
>+  nsresult rv = GetTypeForNode(aInstanceData, getter_AddRefs(schemaType));
>+
>+  if(rv == NS_OK)

nits: need space between if and '('.  Also, '{' needs to be on the same line as the 'if'.  For consistency, please use "if (NS_SUCCEEDED(rv)) {"

>+{
>+    schemaType->GetTargetNamespace(aNSUri);
>+    schemaType->GetName(aType);
> 
>-  if (rv == NS_ERROR_NOT_AVAILABLE) {
>+    return NS_OK;
>+  }
>+
>+  // 6.2.1 2 & 3
>+  // see if the type is assigned as an xsi:type, or XForms:type

What is a XForms:type?  I assume you mean from the type MIP from a bound MIP?  If so, please make a clearer comment.

>+  nsAutoString schemaTypePrefix;
>+  rv = nsXFormsUtils::ParseTypeFromNode(aInstanceData, aType, schemaTypePrefix);
>+

Why call nsXFormsUtils::ParseTypeFromNode?  This is just the really long way around to calling nsXFormsModelElement::GetTypeFromNode when you don't have a model at your fingertips.  That shouldn't be the case here.

>+  // 6.2.1 4. Otherwise it is a string
>+  if(rv == NS_ERROR_NOT_AVAILABLE) {
>     // if there is no type assigned, then assume that the type is 'string'
>     aNSUri.Assign(NS_LITERAL_STRING(NS_NAMESPACE_XML_SCHEMA));
>     aType.Assign(NS_LITERAL_STRING("string"));
>     rv = NS_OK;
>-  }
>+  } else {
>+    if (schemaTypePrefix.IsEmpty()) {
>+      aNSUri.AssignLiteral("");
>+    } else {
>+      // get the namespace url from the prefix
>+      nsCOMPtr<nsIDOM3Node> domNode3 = do_QueryInterface(mElement, &rv);

nit: Mozilla prefers developers use domNode3(do_QueryInterface(mElement, &rv)) to initialize a declared variable, rather than '='.

>+      NS_ENSURE_SUCCESS(rv, rv);
> 
>+      rv = domNode3->LookupNamespaceURI(schemaTypePrefix, aNSUri);
>+    }
>+  }

neither ParseTypeFromNode nor GetTypeFromNode return a prefix.  You should be able to pass in aNSUri directly into the call and not worry about any of this stuff.

>   return rv;
> }
> 
> NS_IMETHODIMP
> nsXFormsModelElement::InstanceLoadStarted()
> {
>   ++mPendingInstanceCount;
>   return NS_OK;
>@@ -1623,31 +1664,34 @@ nsXFormsModelElement::SetNodeContent(nsI
> NS_IMETHODIMP
> nsXFormsModelElement::ValidateNode(nsIDOMNode *aInstanceNode, PRBool *aResult)
> {
>   NS_ENSURE_ARG_POINTER(aResult);
> 
>   nsAutoString schemaTypeName, schemaTypeNamespace;
>   nsresult rv = GetTypeAndNSFromNode(aInstanceNode, schemaTypeName, 
>                                      schemaTypeNamespace);
>-  NS_ENSURE_SUCCESS(rv, rv);
> 
>+  PRBool isValid = PR_FALSE;
>+  if(NS_SUCCEEDED(rv))
>+  {

nit: same as earlier, need space after if, need { on same line as the if.  Need to indent inside the if block.

nit: we tend to return out of a function as soon as we know we can to save on indentation.  I'd suggest changing your test to if (NS_FAILED(rv)) and set *aResult to PR_FALSE and return right there.  Then you can continue on without the extra indentation.  Plus, as I noted earlier, I think that GetTypeAndNSFromNode is expected to return a default of xsd:string, so it should never fail and if it does fail it should be worthy of a NS_ENSURE_SUCCESS, not a silent failure.

> nsresult
> nsXFormsModelElement::ValidateDocument(nsIDOMDocument *aInstanceDocument,
>                                        PRBool         *aResult)
>@@ -1764,24 +1808,31 @@ nsXFormsModelElement::GetTypeFromNode(ns
>       typeVal = &typeAttribute;
>     }
>   }
> 
>   // 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");
>+    // this is stored on the DOM3Node as a property called xsdtype
>+    // which is userdata
>+    nsCOMPtr<nsIDOM3Node> dom3Node(do_QueryInterface(aInstanceData));
>+
>+    nsCOMPtr<nsIVariant> xsdType;
>+    if(NS_SUCCEEDED(dom3Node->GetUserData(NS_LITERAL_STRING("xsdtype"), getter_AddRefs(xsdType))) &&
>+        xsdType)
>+    {
>+      nsCOMPtr<nsISchemaType> type;
>+      nsIID *containedInterface;
> 
>-      type = NS_STATIC_CAST(nsISchemaType *, content->GetProperty(myAtom));
>-      if (type) {
>+      if(NS_SUCCEEDED(xsdType->GetAsInterface(&containedInterface, getter_AddRefs(type))) &&
>+        type)

nit: line up 'type' with NS_SUCCEEDED.  Need space afer the 'if', before the test.


>@@ -1802,22 +1853,22 @@ nsXFormsModelElement::GetTypeFromNode(ns
>     // http://www.w3.org/TR/2004/REC-xmlschema-1-20041028/#src-qname and pick
>     // up the default namespace.  Which will happen by passing an empty string
>     // as first parameter to LookupNamespaceURI.
>     prefix = EmptyString();
>     aType.Assign(*typeVal);
>   } else {
>     prefix.Assign(Substring(*typeVal, 0, separator));
>     aType.Assign(Substring(*typeVal, ++separator, typeVal->Length()));
>+  }
> 
>     if (prefix.IsEmpty()) {
>       aNSUri = EmptyString();
>       return NS_OK;
>     }
>-  }

since you eliminated a wrapping 'if', please fix the indentation for the "if (prefix...)" block by moving it all back two columns.
Comment 20 alexander :surkov 2007-02-12 02:34:59 PST
Btw, setUserData/getUserData are not implemented on 1.8 brach (for example, see http://lxr.mozilla.org/mozilla1.8/source/content/base/src/nsGenericElement.cpp#433)
Comment 21 Peter Nunn 2007-02-12 02:43:38 PST
Yes I know.  For the 1.8 branch I have had to patch the setUserData/getUserData with an implementation for it.  99% of what was needed was in the 1.8 build just about 10 lines of code needed to be added.
Can provide a trunk patch, but isn't 1.8 closed?
Comment 22 alexander :surkov 2007-02-12 18:29:37 PST
(In reply to comment #21)
> Yes I know.  For the 1.8 branch I have had to patch the setUserData/getUserData
> with an implementation for it.  99% of what was needed was in the 1.8 build
> just about 10 lines of code needed to be added.
> Can provide a trunk patch, but isn't 1.8 closed?
> 

AFAIK 1.8 is closed if your changing is not major bug fix. But it doesn't concern if you patch extensions/xforms.
Comment 23 Peter Nunn 2007-02-12 18:48:28 PST
Unfortunately the fix is in the main trunk of mozilla as the routines are stubbed out in the routines, so I guess this patch is not suitable for 1.8/0.7 of firefox. 
Comment 24 Olli Pettay [:smaug] 2007-02-14 01:01:44 PST
Is there a reason to use userData? Why not nsIContent/nsIAttribute's 
Get/SetProperty?
Comment 25 Peter Nunn 2007-02-19 00:08:33 PST
(In reply to comment #24)
> Is there a reason to use userData? Why not nsIContent/nsIAttribute's 
> Get/SetProperty?
> 

Thanks for the tip, works like a charm, so the next patch will be suitable for 1.8/0.7
Will have posted in the morning, still a few tests to do before I think it works 100%.
Comment 26 Peter Nunn 2007-03-07 15:41:15 PST
Created attachment 257768 [details] [diff] [review]
Patch to support schema types for attributes

Changed code to pass the type using atoms instead of DOM3Node properties, as suggested
Comment 27 alexander :surkov 2007-03-15 21:18:25 PDT
Comment on attachment 257768 [details] [diff] [review]
Patch to support schema types for attributes

reviewed yourself? :)

Aaron, can you look?
Comment 28 aaronr 2007-03-23 13:36:01 PDT
Comment on attachment 257768 [details] [diff] [review]
Patch to support schema types for attributes

Peter, please run your patches through http://beaufour.dk/jst-review/ to catch simple coding things that mozilla tries to avoid.  For example, in this patch on the lines you added there is whitespace at the end of the lines.  It might also complain about lines that go past 80 chars.  You can probably ignore those if you are only going over by a character or two.  But please fix the whitespace lines.  And please, no tabs in patches.  Mozilla lines up things using spaces.

>Index: schema-validation/src/nsSchemaValidator.cpp
>===================================================================

>@@ -320,26 +336,34 @@ nsSchemaValidator::ValidateAgainstType(n
>       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);
>+			nsCOMPtr<nsIWritableVariant> holder = do_CreateInstance("@mozilla.org/variant;1");
>+			NS_ENSURE_STATE(holder);
>+
>+			holder->SetAsInterface(nsISchemaType::GetIID(), aType);
>+
>+			// and save on the node
>+      nsCOMPtr<nsIContent> pContent = do_QueryInterface(domNode3);

? We already have the variable content that holds this.  No need to remove that and add pContent.

>+
>+      // we have to be really carefull to set the destructor function correctly.
>+      // this also has to be a pointer to a variant
>+      nsCOMPtr<nsIAtom> key = do_GetAtom("xsdtype");
>+      if (!key) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }

nit: I'd use NS_ENSURE_TRUE(key, NS_ERROR_OUT_OF_MEMORY) so that we get debug messages in the console if we are running a debug build.

>+
>+      nsIVariant *pVariant = holder;
>+      NS_IF_ADDREF(pVariant);
>+      rv = pContent->SetProperty(key, pVariant, &VariantDTor);

no need for this reassignment and addref, is there?  Can't you just set the property with holder?  It shouldn't go away until a deleteProperty is called, I don't think.

>@@ -3689,17 +3713,17 @@ nsSchemaValidator::ValidateComplextype(n
> 
>   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;
>+			isValid = PR_TRUE;

I think that your tree is out of date.  Steve checked in a patch for this piece already.  He's added a function called: ValidateComplexModelEmpty

>@@ -4589,17 +4613,21 @@ nsSchemaValidator::ValidateAttributeComp
>       rv = ValidateSchemaAttributeGroup(aNode, attrGroup, name, aFoundAttrCount,
>                                         &isValid);
>       NS_ENSURE_SUCCESS(rv, rv);
> 
>       break;
>     }
> 
>     case nsISchemaAttributeComponent::COMPONENT_TYPE_ANY: {
>-      rv = NS_ERROR_NOT_IMPLEMENTED;
>+			// for now we just accept this one as being valid.
>+			// should look at the attribute namespace and validate the
>+			// attribute against it
>+			// TODO: implement this
>+      isValid = PR_TRUE;

Please use 'XXX:', not 'TODO:'

>@@ -4621,43 +4649,42 @@ nsSchemaValidator::ValidateSchemaAttribu
>   nsCOMPtr<nsISchemaSimpleType> simpleType;
>   rv = aAttr->GetType(getter_AddRefs(simpleType));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   nsCOMPtr<nsIDOMElement> elm(do_QueryInterface(aNode));
> 
>   PRBool hasAttr = PR_FALSE;
>   PRBool isValid = PR_FALSE;
>+  nsAutoString qualifiedNamespace;
> 
>-  // XXX: We don't know if the attribute needs to be qualified or not, since
>-  // nsISchema on the 1.8.0/1.8 branches doesn't support it.  So try both
>-  // options.
>-
>-  nsAutoString targetNamespace;
>-  aAttr->GetTargetNamespace(targetNamespace);
>-
>-  if (!targetNamespace.IsEmpty()) {
>-    rv = elm->HasAttributeNS(targetNamespace, aAttrName, &hasAttr);
>+  // check if the attribute is to be qualified or not
>+  /*
>+  aAttr->GetQualifiedNamespace(qualifiedNamespace);
>+
>+  if (!qualifiedNamespace.IsEmpty()) {
>+  */
>+    rv = elm->HasAttributeNS(qualifiedNamespace, aAttrName, &hasAttr);

Doesn't look like you do anything with qualifiedNamespace.  If you just wanted an empty string, you could use EmptyString().  And if you don't need the stuff that is commented out, please remove it from the patch.

>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     if (hasAttr) {
>-      rv = elm->GetAttributeNS(targetNamespace, aAttrName, attrValue);
>+      rv = elm->GetAttributeNS(qualifiedNamespace, aAttrName, attrValue);
>       NS_ENSURE_SUCCESS(rv, rv);
>     }
>-  }
>-
>-  if (!hasAttr) {
>+  /*
>+  } else {
>     rv = elm->HasAttribute(aAttrName, &hasAttr);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     if (hasAttr) {
>       rv = elm->GetAttribute(aAttrName, attrValue);
>       NS_ENSURE_SUCCESS(rv, rv);
>     }
>   }
>+    */
> 

same here, if not needed, please remove commented out code from patch so that we don't check it in that way.

>@@ -4681,16 +4708,44 @@ nsSchemaValidator::ValidateSchemaAttribu
>   } else {
>     (*aFoundAttrCount)++;
>     if (use == nsISchemaAttribute::USE_PROHIBITED) {
>       // If it is is prohibited and it exists, we don't have to do anything,
>       // since we default to invalid.
>       LOG(("        -- attribute prohibited!"));
>     } else {
>       if (simpleType) {
>+				// save the type on the attribute
>+				nsCOMPtr<nsIDOMAttr> attrNode;
>+
>+				if(NS_SUCCEEDED(elm->GetAttributeNode(aAttrName, getter_AddRefs(attrNode))))
>+				{
>+					nsCOMPtr<nsIWritableVariant> holder = do_CreateInstance("@mozilla.org/variant;1");
>+					NS_ENSURE_STATE(holder);
>+
>+					holder->SetAsInterface(nsISchemaType::GetIID(), simpleType);
>+
>+					// and save on the node
>+          nsCOMPtr<nsIAttribute> pAttribute = do_QueryInterface(attrNode);
>+

Mozilla prefers that if we are using do_QueryInterface to initialize a nsCOMPtr pointer that we do nsCOMPtr<nsIAttribute> pAttribute(do_QueryInterface(attrNode));

>+          if(pAttribute) {
>+            // we have to be really carefull to set the destructor function correctly.
>+            // this also has to be a pointer to a variant
>+            nsCOMPtr<nsIAtom> key = do_GetAtom("xsdtype");
>+            if (!key) {
>+              return NS_ERROR_OUT_OF_MEMORY;
>+            }

same as above

>+
>+            nsIVariant *pVariant = holder;
>+            NS_IF_ADDREF(pVariant);
>+            rv = pAttribute->SetProperty(key, pVariant, &VariantDTor);

same as above

>Index: xforms/nsIModelElementPrivate.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIModelElementPrivate.idl,v
>retrieving revision 1.12.4.5
>diff -u -8 -p -r1.12.4.5 nsIModelElementPrivate.idl
>--- xforms/nsIModelElementPrivate.idl	22 Nov 2006 08:49:32 -0000	1.12.4.5
>+++ xforms/nsIModelElementPrivate.idl	7 Mar 2007 23:30:17 -0000
>@@ -61,18 +61,24 @@ interface nsIModelElementPrivate : nsIXF
> 
>   /**
>    * Determine the type for a form control based on the schema included by
>    * this model.
>    */
>   nsISchemaType getTypeForControl(in nsIXFormsControl control);
> 
>   /** 
>+   * Determine the type for a form control based on the schema included by
>+   * this model.
>+   */
>+  nsISchemaType getTypeForNode(in nsIDOMNode node);
>+

If you add things to an interface, you need to generate a new uuid for the interface.

>Index: xforms/nsXFormsModelElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.cpp,v
>retrieving revision 1.67.2.8
>diff -u -8 -p -r1.67.2.8 nsXFormsModelElement.cpp
>--- xforms/nsXFormsModelElement.cpp	22 Nov 2006 08:49:33 -0000	1.67.2.8
>+++ xforms/nsXFormsModelElement.cpp	7 Mar 2007 23:30:25 -0000
>@@ -40,16 +40,17 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsXFormsModelElement.h"
> #include "nsIXTFGenericElementWrapper.h"
> #include "nsMemory.h"
> #include "nsIDOMElement.h"
> #include "nsIDOM3Node.h"
> #include "nsIDOMNodeList.h"
>+#include "nsIVariant.h"
> #include "nsString.h"
> #include "nsIDocument.h"
> #include "nsXFormsAtoms.h"
> #include "nsINameSpaceManager.h"
> #include "nsIServiceManager.h"
> #include "nsIDOMEvent.h"
> #include "nsIDOMDOMImplementation.h"
> #include "nsIDOMXMLDocument.h"
>@@ -314,18 +315,21 @@ nsXFormsControlListItem::AddControl(nsIX
> {
>   // Four insertion posibilities:
> 
>   // 1) Delegate to first child from root node
>   if (!mNode && mFirstChild) {
>     return mFirstChild->AddControl(aControl, aParent);
>   }
> 
>+  // Locate parent
>+  nsXFormsControlListItem* parentControl = FindControl(aParent);
>+
>   // 2) control with no parent
>-  if (!aParent) {
>+  if (!parentControl) {

? Why is this change here?  There is a big difference between no aParent passed in to AddControl and the fact that aParent doesn't already exist on the list.  We shouldn't treat them both the same.

>@@ -565,27 +565,29 @@ nsPostRefresh::~nsPostRefresh()
>   }
> 
>   --sRefreshing;
> 
>   // process sContainerPostRefreshList after we've decremented sRefreshing.
>   // container->refresh below could ask for ContainerNeedsPostRefresh which
>   // will add an item to the sContainerPostRefreshList if sRefreshing > 0.
>   // So keeping this under sRefreshing-- will avoid an infinite loop.
>+  if (sContainerPostRefreshList) {
>   while (sContainerPostRefreshList && sContainerPostRefreshList->Count()) {
>     PRInt32 last = sContainerPostRefreshList->Count() - 1;

Why this change?  The test for sContainerPostRefreshList will be the first thing that the while will test for, so a preemptive test won't save anything, will it?

>@@ -1465,50 +1467,84 @@ nsXFormsModelElement::GetTypeForControl(
>   aControl->GetBoundNode(getter_AddRefs(boundNode));
>   if (!boundNode) {
>     // if the control isn't bound to instance data, it doesn't make sense to 
>     // return a type.  It is perfectly valid for there to be no bound node,
>     // so no need to use an NS_ENSURE_xxx macro, either.
>     return NS_ERROR_FAILURE;
>   }
> 
>+	return GetTypeForNode(boundNode, aType);
>+}
>+
>+NS_IMETHODIMP nsXFormsModelElement::GetTypeForNode(nsIDOMNode  *boundNode,
>+																								 nsISchemaType    **aType)
>+{

nit: please change *boundNode to *aBoundNode.  In mozilla, they try to prefix all parameters with 'a' so that it is easy to see in the function what is a parameter and what isn't.

>   nsAutoString schemaTypeName, schemaTypeNamespace;
>-  nsresult rv = GetTypeAndNSFromNode(boundNode, schemaTypeName,
>-                                     schemaTypeNamespace);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+	nsresult rv = GetTypeFromNode(boundNode, schemaTypeName, schemaTypeNamespace);
> 
>+	if(NS_SUCCEEDED(rv))
>+	{


Could you use NS_ENSURE_SUCCESS(rv, rv), please?  I like to see the debug messages when something fails that shouldn't.

> /* static */ nsresult
> nsXFormsModelElement::GetTypeAndNSFromNode(nsIDOMNode *aInstanceData,
>                                            nsAString &aType, nsAString &aNSUri)
> {
>-  nsresult rv = GetTypeFromNode(aInstanceData, aType, aNSUri);
>-
>-  if (rv == NS_ERROR_NOT_AVAILABLE) {
>+  // 6.2.1 1. see if the instance data has a schema type.
>+  // if the control has a schema type then we will then
>+  // have to set a MIP node.
>+  nsCOMPtr<nsISchemaType> schemaType;
>+  nsresult rv = GetTypeForNode(aInstanceData, getter_AddRefs(schemaType));
>+
>+  if(rv == NS_OK)
>+{
>+    schemaType->GetTargetNamespace(aNSUri);
>+    schemaType->GetName(aType);
>+
>+    return NS_OK;
>+  }
>+
>+  // 6.2.1 2 & 3
>+  // see if the type is assigned as an xsi:type, or XForms:type
>+  nsAutoString schemaTypePrefix;
>+  rv = nsXFormsUtils::ParseTypeFromNode(aInstanceData, aType, schemaTypePrefix);
>+
>+  // 6.2.1 4. Otherwise it is a string
>+  if(rv == NS_ERROR_NOT_AVAILABLE) {
>     // if there is no type assigned, then assume that the type is 'string'
>     aNSUri.Assign(NS_LITERAL_STRING(NS_NAMESPACE_XML_SCHEMA));
>     aType.Assign(NS_LITERAL_STRING("string"));
>     rv = NS_OK;
>+  } else {
>+    if (schemaTypePrefix.IsEmpty()) {
>+      aNSUri.AssignLiteral("");

nit: using aNSUri.Assign(EmptyString()) probably faster.

>+    } else {
>+      // get the namespace url from the prefix
>+      nsCOMPtr<nsIDOM3Node> domNode3 = do_QueryInterface(mElement, &rv);

nit: same as above -> domNode3(do_QueryInterface(mElement, &rv))

>@@ -1767,24 +1806,46 @@ nsXFormsModelElement::GetTypeFromNode(ns
>       typeVal = &typeAttribute;
>     }
>   }
> 
>   // 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");
>+    nsCOMPtr<nsIAtom> key = do_GetAtom("xsdtype");
>+    if (!key) {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
> 

nit: NS_ENSURE_TRUE(key, NS_ERROR_OUT_OF_MEMORY), please

>-      type = NS_STATIC_CAST(nsISchemaType *, content->GetProperty(myAtom));
>-      if (type) {
>+    nsresult rv = NS_ERROR_OUT_OF_MEMORY;

I'd suggest using NS_ERROR_FAILURE.  Since you are just using this as an initializer anyhow, no need to use something so specific that might cause an error to get misreported later if someone sticks in a NS_ENSURE_xxx in a couple of months without bothering to change the initialization of rv.

>+    nsCOMPtr<nsIVariant> xsdType;
>+		// this is stored on the DOM3Node as a property called xsdtype
>+    nsCOMPtr<nsIContent> pContent(do_QueryInterface(aInstanceData));
>+
>+    if(pContent) {
>+       xsdType = NS_STATIC_CAST(nsIVariant*, pContent->GetProperty(key, &rv));
>+    } else {
>+      // see if this is stored on an attribute node
>+      nsCOMPtr<nsIAttribute> pAttribute(do_QueryInterface(aInstanceData));
>+
>+      if(pAttribute) {
>+        xsdType = NS_STATIC_CAST(nsIVariant*, pAttribute->GetProperty(key, &rv));
>+      }
>+    }
>+
>+		if(NS_SUCCEEDED(rv) && xsdType)
>+		{
>+	    nsCOMPtr<nsISchemaType> type;
>+			nsIID *containedInterface;
>+
>+			if(NS_SUCCEEDED(xsdType->GetAsInterface(&containedInterface, getter_AddRefs(type))) &&
>+				type)
>+			{

nit: we use a space between 'if' and the '('.  Please change the 4 places above where you have no space.  Also, we have the '{' on the same line as the if test.  Please change the two places above where you put it on the next line.  Thanks.

>@@ -1999,17 +2063,16 @@ nsXFormsModelElement::ProcessBindElement
>     return NS_OK;
> 
>   nsCOMPtr<nsIDOMElement> firstInstanceRoot;
>   firstInstanceDoc->GetDocumentElement(getter_AddRefs(firstInstanceRoot));
> 
>   nsresult rv;
>   nsCOMPtr<nsIXFormsXPathEvaluator> xpath = 
>            do_CreateInstance("@mozilla.org/dom/xforms-xpath-evaluator;1", &rv);
>-  NS_ENSURE_TRUE(xpath, rv);

why are you removing this test?  I'd rather we return the error than to crash later on in ::ProcessBind

Most of my comments were cleanup type of stuff and not really related to the fix.  But r-'ing because I'd like to see the next patch with these changes to make sure I don't still have questions.
Comment 29 Philipp Wagner [:imphil] 2007-10-10 18:10:57 PDT
Created attachment 284397 [details] [diff] [review]
Only cosmetic fixes as requested by aaronr

I am experiencing the same bug and I needed a quick fix, so I tried to clean up the provided patch to meet the requirements. I am no C-coder, I just tried my best. 
Some suggested changes were not possible.
Changing

      nsIVariant *pVariant = holder;
      NS_IF_ADDREF(pVariant);
      rv = pContent->SetProperty(key, pVariant, &VariantDTor);

to
      rv = pContent->SetProperty(key, holder, &VariantDTor);

(I hope that is what was meant) leads to an error and firefox crashes:

###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file ../../dist/include/xpcom/nsCOMPtr.h, line 594
Break: at file ../../dist/include/xpcom/nsCOMPtr.h, line 594
pure virtual method called

The following suggestion also causes problems:

>>+    if (schemaTypePrefix.IsEmpty()) {
>>+      aNSUri.AssignLiteral("");
>
>nit: using aNSUri.Assign(EmptyString()) probably faster.

Replacing "" with EmptyString() causes:

/home/philipp/xforms-build/mozilla/extensions/xforms/nsXFormsModelElement.cpp:1536: error: no matching function for call to ‘nsAString_internal::AssignLiteral(const nsString&)’

The last question which I'm not exactly sure about is the generation of the new UUID. I created a new UUID in the file Makefile.in, but I'm not sure if that is the right one.

This patch applies cleanly to the current MOZILLA_1_8_BRANCH.

I used the testcase to make sure this updated patch has the same functionality as the previous patch. I don't know if that is intended or another bug, but the <xf:output> sections are not updated when I enter a new value into any of the text fields (attributes and values alike). Recalculate(), Revalidate() and Refresh() are called according to the debug messages.
Comment 30 alexander :surkov 2007-10-10 20:32:40 PDT
Comment on attachment 284397 [details] [diff] [review]
Only cosmetic fixes as requested by aaronr

when you put new patch marked r- previouslu please  be sure to reask review.
Comment 31 aaronr 2007-11-08 00:27:17 PST
Comment on attachment 284397 [details] [diff] [review]
Only cosmetic fixes as requested by aaronr

Just a few issues I saw still in the patch.

1) I don't understand why the INSTALL_EXTENSION_ID was changed.
2) uuid still needs updating for nsIModelElementPrivate.idl
3) The change to GetTyupeForControl didn't really need to be made.  My fault.  Bad review comment on my part.

Also the patch wouldn't apply cleanly to the trunk.  I'll attach a version that fixes these 3 things and works on the trunk.
Comment 32 aaronr 2007-11-08 00:30:30 PST
Created attachment 287809 [details] [diff] [review]
patch for trunk

Here's the patch that will apply to trunk.  Doron, could you please review it again to make sure that it still makes sense from a schema-validation perspective?
Comment 33 aaronr 2007-11-12 16:54:40 PST
checked into the trunk for peter and philipp
Comment 34 aaronr 2008-01-08 19:25:09 PST
checked into 1.8 branch via bug 410239.
Comment 35 Al Billings [:abillings] 2008-01-18 16:48:44 PST
How can QA test this? Both of the attached test cases do not seem runnable "as is" as they look the same pre and post path on 1.8. They simply render:

Bound schema types.

String Value:
Date Value:
Boolean Value:
Input Attrib:
Date Attrib:
Boolean Attrib:
String Value:
Date Value:
Boolean Value:
Input Attrib:
Date Attrib:
Boolean Attrib:
Comment 36 aaronr 2008-01-18 17:17:23 PST
(In reply to comment #35)
> How can QA test this? Both of the attached test cases do not seem runnable "as
> is" as they look the same pre and post path on 1.8. They simply render:
> 

You'd have to build with XForms enabled, which you probably aren't doing.
Comment 37 Al Billings [:abillings] 2008-01-18 17:21:51 PST
We test on nightlies and release builds so, no, we don't have XForms enabled. QA doesn't commonly generate builds.

Dveditz, do you have a build handy with XForms enabled?
Comment 38 aaronr 2008-01-18 17:25:24 PST
xforms is still generated on nightly builds for the 1.8 branch.  For example, you can get the latest xforms.xpi for windows here: ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla1.8/windows-xpi

Note You need to log in before you can comment on or make changes to this bug.