Closed
Bug 421468
Opened 16 years ago
Closed 16 years ago
[1.1] Support @resource on instance element
Categories
(Core Graveyard :: XForms, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: msterlin, Assigned: msterlin)
Details
(Keywords: fixed1.8.1.17)
Attachments
(7 files, 6 obsolete files)
User-Agent: Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322; .NET CLR 2.0.50727) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022816 Minefield/3.0b4pre Need to support @resource on instance element per the rules of section 3.3.2 of the spec. Reproducible: Always
Assignee | ||
Comment 1•16 years ago
|
||
Assignee | ||
Comment 2•16 years ago
|
||
Assignee | ||
Comment 3•16 years ago
|
||
Assignee | ||
Comment 4•16 years ago
|
||
Assignee | ||
Comment 5•16 years ago
|
||
Assignee | ||
Comment 6•16 years ago
|
||
Attachment #307919 -
Flags: review?(aaronr)
Assignee: nobody → msterlin
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Comment on attachment 307919 [details] [diff] [review] patch >Index: nsIModelElementPrivate.idl >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsIModelElementPrivate.idl,v >retrieving revision 1.27 >diff -u -8 -p -r1.27 nsIModelElementPrivate.idl >--- nsIModelElementPrivate.idl 12 Feb 2008 14:36:39 -0000 1.27 >+++ nsIModelElementPrivate.idl 7 Mar 2008 09:38:05 -0000 >@@ -60,17 +60,17 @@ interface nsIModelElementPrivate : nsIXF > void removeFormControl(in nsIXFormsControl control); > > /** > * Notification that an instance element has started or finished >loading > * its instance data. Model construction cannot complete until all of > * the instances have loaded their data. > */ > void instanceLoadStarted(); >- void instanceLoadFinished(in boolean success); >+ void instanceLoadFinished(in boolean success, in AString uri); You are changing the .idl but didn't change the uuid. Also, please add a comment about what the 'uri' parameter is for. > nsresult > nsXFormsInstanceElement::ReplacePrincipal(nsIDocument *aDocument) >@@ -470,77 +503,126 @@ nsXFormsInstanceElement::Initialize() ... > >- if (src.IsEmpty()) { >- return CloneInlineInstance(); >+ if (!src.IsEmpty()) { >+ LoadExternalInstance(src); >+ } else { >+ // If we have a first child element then we have inline content. >+ nsCOMPtr<nsIDOMNode> child, temp; >+ mElement->GetFirstChild(getter_AddRefs(child)); >+ while (child) { >+ PRUint16 nodeType; >+ child->GetNodeType(&nodeType); >+ >+ if (nodeType == nsIDOMNode::ELEMENT_NODE) >+ break; >+ >+ temp.swap(child); >+ temp->GetNextSibling(getter_AddRefs(child)); >+ } >+ >+ if (child) { >+ rv = CloneInlineInstance(); >+ } This is inconsistent with what you have previously. In AttributeRemoved you used GetFirstChild to do this logic. Here you are doing it all inline. Why aren't you using GetFirstChild here? > // private methods > > nsresult > nsXFormsInstanceElement::CloneInlineInstance() > { > // Clear out our existing instance data > nsresult rv = CreateInstanceDocument(EmptyString()); > if (NS_FAILED(rv)) > return rv; // don't warn, we might just not be in the document yet > >- // look for our first child element (skip over text nodes, etc.) >- nsCOMPtr<nsIDOMNode> child, temp; >- mElement->GetFirstChild(getter_AddRefs(child)); >- while (child) { >- PRUint16 nodeType; >- child->GetNodeType(&nodeType); >- >- if (nodeType == nsIDOMNode::ELEMENT_NODE) >- break; >- >- temp.swap(child); >- temp->GetNextSibling(getter_AddRefs(child)); >- } You are ending up calling GetFirstChild twice each time CloneInlineInstance is called. Perhaps you should make it so that you call CloneInlineInstance and rather than reporting its error to the screen, have it return a unique error for not having a child element. Then the caller sees this error and if there is a @resource things just go on and if no @resource then show the error to the screen. You could also pass the first child element directly into CloneInlineInstance and if it sees it it knows not to call GetFirstChild. But someone the duplication should be avoided.
Attachment #307919 -
Flags: review?(aaronr) → review-
Assignee | ||
Comment 8•16 years ago
|
||
(In reply to comment #7) > > > > nsresult > > nsXFormsInstanceElement::CloneInlineInstance() > > { > > // Clear out our existing instance data > > nsresult rv = CreateInstanceDocument(EmptyString()); > > if (NS_FAILED(rv)) > > return rv; // don't warn, we might just not be in the document yet > > > >- // look for our first child element (skip over text nodes, etc.) > >- nsCOMPtr<nsIDOMNode> child, temp; > >- mElement->GetFirstChild(getter_AddRefs(child)); > >- while (child) { > >- PRUint16 nodeType; > >- child->GetNodeType(&nodeType); > >- > >- if (nodeType == nsIDOMNode::ELEMENT_NODE) > >- break; > >- > >- temp.swap(child); > >- temp->GetNextSibling(getter_AddRefs(child)); > >- } > You are ending up calling GetFirstChild twice each time CloneInlineInstance is > called. Perhaps you should make it so that you call CloneInlineInstance and > rather than reporting its error to the screen, have it return a unique error > for not having a child element. Then the caller sees this error and if there > is a @resource things just go on and if no @resource then show the error to the > screen. You could also pass the first child element directly into > CloneInlineInstance and if it sees it it knows not to call GetFirstChild. But > someone the duplication should be avoided. Yes, that is the whole problem. To implement the precedence scheme I have to know if inline content exists. It used to be an error if there was no @src and no inline content but now it is not if there is an @resource. I didn't want to return a different error because the only time it is an error if there is no first child or the first child has siblings is when the precdence rules say we must use inline content. I wanted to keep that logic in CloneInlineInstance where it belongs rather than pull out all the error code into a new function and then call it from multiple places. I like the idea of passing the child, if any, to CloneInlineInstance. That way we won't call GetFirstChild twice and waste 10 microseconds. :)
Assignee | ||
Comment 9•16 years ago
|
||
Attachment #307919 -
Attachment is obsolete: true
Attachment #310064 -
Flags: review?(aaronr)
Comment 10•16 years ago
|
||
Comment on attachment 310064 [details] [diff] [review] patch2 I personally would have just made the parameter to CloneInlineInstance take an optional parameter and if it wasn't there, call GetFirstChild. But your way works, too. As a nit, you need to add a comment in CloneInlineInstance that says that aChild must be the first child element underneath the xf:instance. That isn't apparent, especially since the signature just has it as a nsIDOMNode. with that, r=me
Attachment #310064 -
Flags: review?(aaronr) → review+
Assignee | ||
Comment 11•16 years ago
|
||
Adding comment to CloneInlineInstance.
Attachment #310064 -
Attachment is obsolete: true
Attachment #310077 -
Flags: review?(Olli.Pettay)
Comment 12•16 years ago
|
||
Comment on attachment 310077 [details] [diff] [review] patch3 > nsXFormsInstanceElement::AttributeSet(nsIAtom *aName, ... >+ // @src has precdence over inline content which has precedence >+ // over @resource. fix 'precdence' >+nsXFormsInstanceElement::GetFirstChild(nsIDOMNode **aChild) I would call this GetFirstChildElement. The out parameter could be then nsIDOMElement** or nsIDOMNode**. With those, r=me
Attachment #310077 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 13•16 years ago
|
||
Attachment #310077 -
Attachment is obsolete: true
Comment 14•16 years ago
|
||
Attachment #307913 -
Attachment is obsolete: true
Assignee | ||
Comment 15•16 years ago
|
||
This patch fixes the issue where no xforms-link-exception is generated for testcase 'instance with invalid resource link' when run from the server.
Attachment #311667 -
Attachment is obsolete: true
Assignee | ||
Comment 16•16 years ago
|
||
Simplifying previous patch. We can get the uri of the failed link from the request object in OnStopRequest rather than storing the uri in a member variable.
Attachment #312789 -
Attachment is obsolete: true
Attachment #312808 -
Flags: review+
Attachment #312808 -
Flags: review?(Olli.Pettay)
Updated•16 years ago
|
Attachment #312808 -
Flags: review?(Olli.Pettay) → review+
Comment 17•16 years ago
|
||
checked into trunk for msterlin
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch-11
Comment 18•16 years ago
|
||
patch for 1.8 branch...same code as other patch, just updated to 1.8 branch current level of code
Comment 19•16 years ago
|
||
checked into 1.8 branch for msterlin
Keywords: fixed1.8.1.17
Whiteboard: xf-to-branch-11
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•