Closed Bug 421468 Opened 16 years ago Closed 16 years ago

[1.1] Support @resource on instance element

Categories

(Core Graveyard :: XForms, defect)

x86
Windows XP
defect
Not set
normal

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
Attached patch patch (obsolete) — Splinter Review
Attachment #307919 - Flags: review?(aaronr)
Assignee: nobody → msterlin
Status: UNCONFIRMED → NEW
Ever confirmed: true
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-
(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. :)




Attached patch patch2 (obsolete) — Splinter Review
Attachment #307919 - Attachment is obsolete: true
Attachment #310064 - Flags: review?(aaronr)
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+
Attached patch patch3 (obsolete) — Splinter Review
Adding comment to CloneInlineInstance.
Attachment #310064 - Attachment is obsolete: true
Attachment #310077 - Flags: review?(Olli.Pettay)
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+
Attached patch patch for check-in (obsolete) — Splinter Review
Attachment #310077 - Attachment is obsolete: true
Attachment #307913 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
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
Attached patch patchSplinter Review
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)
Attachment #312808 - Flags: review?(Olli.Pettay) → review+
checked into trunk for msterlin
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch-11
patch for 1.8 branch...same code as other patch, just updated to 1.8 branch current level of code
checked into 1.8 branch for msterlin
Keywords: fixed1.8.1.17
Whiteboard: xf-to-branch-11
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: