User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4 When 'bind' element has type then rebuild() method of model throws an exception: Error: XForms Error (7): Multiple defined model item property on element Reproducible: Always

(In reply to comment #0) > When 'bind' element has type then rebuild() method of model throws an exception: > Error: XForms Error (7): Multiple defined model item property on element You are absolutely right. We need to clear the nsXFormsAtoms::type on all instance nodes on rebuild. Ouch, that's expensive :(

(In reply to comment #2) > > We need to clear the nsXFormsAtoms::type on all instance nodes on rebuild. Ouch, > that's expensive :( What means expensive?

(In reply to comment #3) > (In reply to comment #2) > > > > We need to clear the nsXFormsAtoms::type on all instance nodes on rebuild. Ouch, > > that's expensive :( > > What means expensive? We need to run through all instance nodes, that takes time. So expensive == time consuming.

Confirmed on 2005-12-05-06-trunk. In addition, @type is required for file uploads, so I'm stuck! Code fragments: <xf:bind id="file-bind" nodeset="instance('file')/file" type="xsd:anyURI" constraint="string-length(instance('context')/slotId) &gt; 0 and string-length(instance('context')/jobDescription) &gt; 0"/> ... <xf:upload bind="file-bind" mediatype="*"> <xf:label>File</xf:label> <xf:filename ref="instance('jobsFile')/jobs:Jobs/jobs:Job[jobs:Description=instance('context')/jobDescription and jobs:ParentSlot/@id=instance('context')/slotId]/jobs:Files/jobs:File[last()]"/> </xf:upload> Error message: Error: XForms Error (7): Multiple defined model item property on element Source File: file:///C:/LIMO-dev/install/Mozilla/jobs.xhtml Line: 0 Source Code: <xf:bind id="file-bind" nodeset="instance('file')/file" type="xsd:anyURI"/>

Oh yeah, when removing the bind and using @ref and @type on the upload element directly, I get the following: Error: XForms Error (24): Upload element not bound to valid datatype. Must be bound to datatype 'xsd:anyURI', 'xsd:base64Binary', or 'xsd:hexBinary'. Source File: file:///C:/LIMO-dev/install/Mozilla/jobs.xhtml Line: 0 Source Code: <xf:upload ref="instance('file')/file" mediatype="*" type="xsd:anyURI" type="http://www.w3.org/2001/XMLSchema#string"/> Seems like @type is completely ignored on this element; i.e., I only specified the first one, but the second one is always appended, while the first is ignored.

(In reply to comment #6) > Oh yeah, when removing the bind and using @ref and @type on the upload element > directly, I get the following: > Error: XForms Error (24): Upload element not bound to valid datatype. Must be > bound to datatype 'xsd:anyURI', 'xsd:base64Binary', or 'xsd:hexBinary'. > Source File: file:///C:/LIMO-dev/install/Mozilla/jobs.xhtml > Line: 0 > Source Code: > <xf:upload ref="instance('file')/file" mediatype="*" type="xsd:anyURI" > type="http://www.w3.org/2001/XMLSchema#string"/> > > Seems like @type is completely ignored on this element; i.e., I only specified > the first one, but the second one is always appended, while the first is > ignored. > @type on a control isn't proper XForms. XForms uses @type on xf:bind to describe the instance data that the control is bound to. You can also put @type on the instance element itself (though in that case you would need to use @xsi:type). So in the example that you cited, the two places that we look for typing (on the instance data and on the bind) turned up nothing so we are explaining that we didn't find the proper type for the control.

(In reply to comment #7) > @type on a control isn't proper XForms. XForms uses @type on xf:bind to > describe the instance data that the control is bound to. You can also put > @type on the instance element itself (though in that case you would need to use > @xsi:type). Then the error message makes even less sense, since it indicates that the XForms engine inserts @type there, and expects the user to override it. The "natural" place to correct would be where the XForms engine points out the error...

(In reply to comment #8) > (In reply to comment #7) > > @type on a control isn't proper XForms. XForms uses @type on xf:bind to > > describe the instance data that the control is bound to. You can also put > > @type on the instance element itself (though in that case you would need to use > > @xsi:type). > > Then the error message makes even less sense, since it indicates that the > XForms engine inserts @type there, and expects the user to override it. The > "natural" place to correct would be where the XForms engine points out the > error... > Hmmm, well it's telling you that this control is not _bound_ to the correct type. That's correct, and makes sense. We could help further by writing out the node that it is bound to, and the type. I'll give you that, but that's a different bug.

(In reply to comment #5) > Confirmed on 2005-12-05-06-trunk. In addition, @type is required for file > uploads, so I'm stuck! Well, don't rebuild then :) No, seriously, I'm sorry for that. It's my doing, I really need to fix this. When I implemented it, I thought it was smart to store the type on the instance data node -- I just forgot about rebuild. Either we need to version/timestamp the type information, or we need to store it somewhere else where it's easy to clear. Right now we have to run through all nodes and kill the type info on a rebuild. Not good at all.

(In reply to comment #10) > (In reply to comment #5) > > Confirmed on 2005-12-05-06-trunk. In addition, @type is required for file > > uploads, so I'm stuck! > > Well, don't rebuild then :) No, seriously, I'm sorry for that. It's my doing, I > really need to fix this. > > When I implemented it, I thought it was smart to store the type on the instance > data node -- I just forgot about rebuild. > > Either we need to version/timestamp the type information, or we need to store > it somewhere else where it's easy to clear. Right now we have to run through > all nodes and kill the type info on a rebuild. Not good at all. > I started looking at fixing this and then got sidetracked with Minimo. The problem with a side list is that while it is easy to clear, but you'll have to repopulate it on every rebuild to make sure that you have an accurate list since instance data could be changed by JS prior to the rebuild. And the list would have to be keyed off of the pointer value or something to keep it as small a list as possible since we will have a list living in memory for every instance document that we have. That's as far as I got in my thinking :=)

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #5) > > > Confirmed on 2005-12-05-06-trunk. In addition, @type is required for file > > > uploads, so I'm stuck! > > > > Well, don't rebuild then :) No, seriously, I'm sorry for that. It's my doing, I > > really need to fix this. > > > > When I implemented it, I thought it was smart to store the type on the instance > > data node -- I just forgot about rebuild. > > > > Either we need to version/timestamp the type information, or we need to store > > it somewhere else where it's easy to clear. Right now we have to run through > > all nodes and kill the type info on a rebuild. Not good at all. > > > > > I started looking at fixing this and then got sidetracked with Minimo. The > problem with a side list is that while it is easy to clear, but you'll have to > repopulate it on every rebuild to make sure that you have an accurate list > since instance data could be changed by JS prior to the rebuild. And the list > would have to be keyed off of the pointer value or something to keep it as > small a list as possible since we will have a list living in memory for every > instance document that we have. That's as far as I got in my thinking :=) Well, whether we save it in a list or store it as properties we use memory. The same goes for what we have to do on a rebuild, except that clearing a list is much quicker than traversing all instance nodes.

<thinking out loud> To fix this with code as it is today, on detection of rebuild: for (each instance doc in model) { for (each node in instance doc) { if (node has type property) { if (node has @type) { set type property } else { delete type property } } else if (node has @type) { set type property } if (node has p3ptype property) { delete property } } } model->ProcessBindElements(); model->ApplyTypesFromSchemas(); /*yet to be implemented*/ Or if we use a side list: model->ClearTypeList(); for (each instance doc in model) { for (each node in instance doc) { if (node has @type) { model->AddTypeToList(node, type); } } } model->ProcessBindElements(); model->ApplyTypesFromSchemas(); /*yet to be implemented*/ So either way we'll have to walk every node in the instance document. Either way we'll have to set the node type, whether it is on the node iteself or in the list. What we are saving is a getProperty on every node for both type and p3ptype and a remove property on all p3ptype nodes as well as nodes that don't have @type set on them. Also safeguards us from a JS author setting the properties themselves on the instance nodes and potentially screwing us up. </thinking out loud>

(In reply to comment #13) > Or if we use a side list: > > model->ClearTypeList(); > > for (each instance doc in model) { > for (each node in instance doc) { > if (node has @type) { > model->AddTypeToList(node, type); > } > } > } No need for that. When we need to get the schema type or a node, we just first check the node directly, and if there's no xsi:type there we check the "type list". It's the same as we do today, just using a list instead of properties on each node.

As it can bee seen, I'm for the list approach as I believe it will be the most efficient. I see two options: 1) Replace the property-handling with a list (hashmap) in each model 2) Use the existing "instance data node -> meta data" mapping that the MDG already contains. Option 2 is where we should go, but each entry in the MDG is "heavy", and looking at it with fresh eyes I should redo the MDG datastructures a bit. There is some redundancy. Some optimizations would help quite a lot on memory consumption I think. But, messing around with the MDG is always fun for me, so unless somebody objects loudly I'll do the "quick fix" option 1, and then follow up with a option 2 when the MDG is refactored.

Here's a patch taking the list (hashtables) approach. I've moved ParseTypeFromNode() functionality from nsXFormsUtils to the model, as the data lives there. The old function is still there for when the model is not known by the caller.

Comment on attachment 211721 [details] [diff] [review] Patch using hashtables Few comments: >+ // Initialize hash tables >+ NS_ENSURE_TRUE(mNodeToType.Init(), NS_ERROR_FAILURE); >+ NS_ENSURE_TRUE(mNodeToP3PType.Init(), NS_ERROR_FAILURE); I *think* this should return NS_ERROR_OUT_OF_MEMORY if not true. >+ >+ if (separator == kNotFound) { >+ // no namespace prefix, which is valid; >+ prefix.AssignLiteral(""); I'd prefer prefix = EmptyString() or prefix.Assign(EmptyString()); (also elsewhere) > >+ // Insert value >+ NS_ENSURE_TRUE(table->Put(node, new nsString(propStrings[j])), >+ NS_ERROR_FAILURE); >+ IIRC, also this should be NS_ERROR_OUT_OF_MEMORY, not NS_ERROR_FAILURE. And please check whether new nsString(propStrings[j]) succeeds.

Comment on attachment 211721 [details] [diff] [review] Patch using hashtables >@@ -635,16 +639,22 @@ nsXFormsModelElement::Rebuild() > printf("nsXFormsModelElement::Rebuild()\n"); > #endif > > // 1 . Clear graph > nsresult rv; > rv = mMDG.Clear(); > NS_ENSURE_SUCCESS(rv, rv); > >+ // Clear any type information >+ NS_ENSURE_TRUE(mNodeToType.IsInitialized() && mNodeToType.IsInitialized(), >+ NS_ERROR_FAILURE); >+ mNodeToType.Clear(); >+ mNodeToP3PType.Clear(); >+ checking IsInitialized() twice on the same hashtable. I assume one of these should be mNodeToP3PType. Gotta run. More feedback tomorrow.

Comment on attachment 211838 [details] [diff] [review] Patch v2 >diff -X patch-excludes -uprN -U8 xforms/nsXFormsModelElement.cpp xforms.typefix/nsXFormsModelElement.cpp >--- xforms/nsXFormsModelElement.cpp 2006-02-09 09:53:29.000000000 +0100 >+++ xforms.typefix/nsXFormsModelElement.cpp 2006-02-14 11:20:41.000000000 +0100 >@@ -1180,16 +1190,89 @@ nsXFormsModelElement::GetLazyAuthored(PR > > NS_IMETHODIMP > nsXFormsModelElement::GetIsReady(PRBool *aIsReady) > { > *aIsReady = mReadyHandled; > return NS_OK; > } > >+NS_IMETHODIMP >+nsXFormsModelElement::GetTypeFromNode(nsIDOMNode *aInstanceData, >+ nsAString &aType, >+ nsAString &aNSUri) >+{ >+ // aInstanceData could be an instance data node or it could be an attribute >+ // on an instance data node (basically the node that a control is bound to). >+ >+ nsString *typeVal = nsnull; >+ >+ // Get type stored directly on instance node >+ nsString typeAttribute; >+ nsCOMPtr<nsIDOMElement> nodeElem(do_QueryInterface(aInstanceData)); >+ if (nodeElem) { >+ nodeElem->GetAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_XML_SCHEMA_INSTANCE), >+ NS_LITERAL_STRING("type"), typeAttribute); >+ if (!typeAttribute.IsEmpty()) { >+ typeVal = &typeAttribute; >+ } >+ } >+ doesn't typeAttribute need to be an nsAutoString here? Also, shouldn't we throw a binding exception (and report multiMIP to the console) if we have an instance node with a xsi:type on it and then we have a xf:bind pointing at the node and setting type="xxx" on it? I don't see where that would happen in this code. Though, if it is any consolation, wouldn't have happened in the previous code, either. Otherwise code looks good to me.

(In reply to comment #20) > (From update of attachment 211838 [details] [diff] [review] [edit]) > >+ // Get type stored directly on instance node > >+ nsString typeAttribute; > >+ nsCOMPtr<nsIDOMElement> nodeElem(do_QueryInterface(aInstanceData)); > >+ if (nodeElem) { > >+ nodeElem->GetAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_XML_SCHEMA_INSTANCE), > >+ NS_LITERAL_STRING("type"), typeAttribute); > >+ if (!typeAttribute.IsEmpty()) { > >+ typeVal = &typeAttribute; > >+ } > >+ } > >+ > > doesn't typeAttribute need to be an nsAutoString here? Well, it does not need to be nsAutoString, but there was real reason for me to use nsString there. Back to original. > Also, shouldn't we throw a binding exception (and report multiMIP to the > console) if we have an instance node with a xsi:type on it and then we have a > xf:bind pointing at the node and setting type="xxx" on it? I don't see where > that would happen in this code. Though, if it is any consolation, wouldn't > have happened in the previous code, either. This patch does not, as you also state, change behaviour. If you want changed behaviour: Create a bug for it :)

nsString -> nsAutoString

Comment on attachment 211960 [details] [diff] [review] Patch v3 I can't figure out if it should be a binding exception or not. fP throws a link exception in the testcase that I created (and I'm not linking to anything) so I assume that this is why, but I'm not sure. I'll open another bug if I can get an answer back from the WG.

Checked in on trunk

Checked in on 1_8_0