265216 - Tracking bug for XForms Group Module

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

This is the tracking bug for for the implementation of the XForms Group module.

Comment 1

[Allan Beaufour]

Attachment: Group Element patch #1

Here's the first implementation of <group>.

It is implemented using nsIXFormsContextControl. This interface allows children
to ask parents that implement this for their context (nsXFormsControl is
extended to do this). Basically this means, that it should work automatically
for all elements inheriting from nsXFormsControl. There are still some todo's,
but it works.

The patch also includes:
* A DEBUG_XF_* define in many classes, that allows one to toggle debugging on
and off for each class.
* Support for multinode bindings in nsXFormsControl, which is needed for
repeat.
* Change int to PRInt32 in nsXFormsMDGEngine.h
* Some JavaDoc comments for classes, and misc. comment corrections

I'd rather not seperate these last issues in separate patches... I hope it's
ok?

Comment 2

[Brian Ryner (not reading)]

Comment on attachment 162711 [details] [diff] [review]
Group Element patch #1

I think this is probably ok, but can you merge this to the current trunk? I
think my recent reorg will affect this patch.

Comment 3

[Allan Beaufour]

Yep. I was working on it yesterday, will probably finish it today.

Comment 4

[Allan Beaufour]

Attachment: Update to current CVS

Updated to current CVS, and removed all <group> unrelated things.

Comment 5

[Allan Beaufour]

Attachment: Saved a couple of QI's

Just realised that I could save a couple of QI's in FindParentContext, so I did
:)

Comment 6

[Allan Beaufour]

Attachment: Updated to current CVS

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 164166 [details] [diff] [review]
Updated to current CVS

>--- ../xforms.orig/nsXFormsGroupElement.cpp	1970-01-01 01:00:00.000000000 +0100
>+++ ./nsXFormsGroupElement.cpp	2004-11-01 14:06:41.644262024 +0100
>+static const nsIID sScriptingIIDs[] = {
>+  NS_IDOMELEMENT_IID,
>+  NS_IDOM3NODE_IID
>+};

Don't you want NS_IDOMEVENTTARGET_IID ?

>+class nsXFormsGroupElement : public nsIXFormsControl,
>+                             public nsIXTFXMLVisual,
>+                             public nsIXFormsContextControl {

If you don't mind, please put the opening brace on a new line, just so we're
consistent.

>+protected:
>+  /** The DOM element for the node */
>+  nsIDOMElement *mElement;
>+
>+  /** The context node for the children of this element */
>+  nsCOMPtr<nsIDOMElement> mContextNode;
>+
>+  /** The UI HTML element used to represent the tag */
>+  nsCOMPtr<nsIDOMHTMLDivElement> mHTMLElement;
>+
>+  /** Process element */
>+  NS_IMETHODIMP Process();

Just:

NS_HIDDEN_(nsresult) Process();   is fine here.  NS_IMETHODIMP should only be
used for implementations declared with NS_IMETHOD (which should only be used
for virtual methods).

>--- ../xforms.orig/nsXFormsUtils.cpp	2004-11-01 14:00:44.001628224 +0100
>+++ ./nsXFormsUtils.cpp	2004-11-01 14:01:07.599622368 +0100
>+/* static */ nsresult
>+nsXFormsUtils::FindParentContext(nsIDOMElement* aElement,
>+                                 nsIDOMElement* * aContextNode)
>+{
...
>+    // Next ancestor
>+    rv = contextNode->GetParentNode(getter_AddRefs(contextNode));

This will crash.  The reason is that getter_AddRefs() will release the current
value in |contextNode| and null it out, and that happens before the method
call.  You'll need to use a temporary, see GetParentModel for an example.

>--- ../xforms.orig/nsXFormsUtils.h	2004-11-01 14:00:44.158761360 +0100
>+++ ./nsXFormsUtils.h	2004-11-01 14:01:07.638655440 +0100
>@@ -135,11 +135,22 @@ public:
>   /**
>    * Clone the set of IIDs in |aIIDList| into |aOutArray|.
>    * |aOutCount| is set to |aIIDCount|.
>    */
>   static NS_HIDDEN_(nsresult) CloneScriptingInterfaces(const nsIID *aIIDList,
>                                                        unsigned int aIIDCount,
>                                                        PRUint32 *aOutCount,
>                                                        nsIID ***aOutArray);
>+
>+  /**
>+   * Returns the context node for the element, it set by a parent node.

"if set"?

Please address those comments and then this should be fine.

Comment 8

[Allan Beaufour]

Attachment: Fixed review comments

I agree with all the comments, so they are all fixed.

Comment 9

[Allan Beaufour]

Comment on attachment 164371 [details] [diff] [review]
Fixed review comments 

Found a problem with finding the correct model... new patch on the way

Comment 10

[Allan Beaufour]

Attachment: Fixed comments and context problems

Ok, here's a new patch. It fixes the review comments and moreover also works :)


- It adds nsIXFormsContextControl, that is used by node children to retrieve
context from parents

- Rewrote nsXFormsUtils::GetModelAndBind() to nsXFormsUtils::GetNodeContext(), 
and made a nsXFormsUtils::GetModel() shortcut for old function instead (no
callers used bind value).

GetNodeContext() should be specified in the spec. section 7.4, but it is not
exactly clear. I will attach my interpretation of how to find the correct
model.

- Changed nsXFormsUtils::GetParentModel() to be a proper Getter-function.

There is a known bug, as written in the comment for nsXFormsGroupElement:
"If a group only has a model attribute, the group fails to set this for
children, as it is impossible to distinguish between a failure and absence
of binding attributes when calling EvaluateNodeBinding()."

The parameter list for EvaluateNodeBinding() is getting quite large... so I did
not want to mess with it, we might need to redesign it? This problem also
influences nsXFormsInputElement, that fails to clear its content on failed node
bindings.

BTW: There are a lot of NS_ENSURE_TRUE(x, y) that fails, even though nothing is
wrong. I guess I should use "if (x) return y" instead?

Comment 11

[Allan Beaufour]

Attachment: Finding the correct model

Here's my view of how to find the correct model.

Comment 12

[Allan Beaufour]

... Aaron pointed out that my note about Action Module is not totally correct,
<action> has <setvalue> that needs to find its context. But my point was that no
elements in Action Module needs to implement nsIXFormsContextControl. And since
<action> has no node bindings, this is still true. So the wording is slightly
wrong, but the meaning is good enough. Until proved otherwise of course :)

Comment 13

[Allan Beaufour]

Attachment: Updated to current CVS

Aaah, nsXFormsXMLVisualStub. I was about to do that myself.

BTW: It would be nice if XTF included something like that. It makes XTF quite a
lot easier to understand.

Comment 14

[aaronr]

We need to settle on a way to mark something "todo".  I've seen a couple of
different ways, including 'XXX'.  Just make sure that your todo's match todo's
in other code so that if someone wants to grep for todo's, it is consistent.

Is there some reason why you changed nsXFormsUtils::GetParentModel to return
model as an output parameter, but the function nsXFormsUtils::GetModel doesn't?
 It returns the model as the return value.

I don't know if this is an error or not, just something I
observed...GetContextNode can return without returning a contextNode should
FindParentContext not return a context node.  Shouldn't it return the root node
from the first instance document in its model in this case?  I would think that
with a name GetContextNode, it should return a context node if there is a way to
figure out the context node.  You kinda handle this exact situation in
nsXFormsUtils::EvaluateNodeBinding.  I just think that it should be moved into
GetContextNode if that is a logical place to put it.

Any way to pass a flag into GetContextNode so that if all I want is the model,
it doesn't try to find the context node when it isn't necessary?  For example,
when GetModel calls it and there is a bind or model attr on my element.

Looks good!

Comment 15

[aaronr]

Also doesn't look like you handle this item mentioned in the spec, "Setting the
input focus on a group results in the focus being set to the first form control
in the navigation order within that group."

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 165110 [details] [diff] [review]
Updated to current CVS

>--- xforms/nsIXFormsContextControl.idl	1970-01-01 01:00:00.000000000 +0100
>+++ xforms.group/nsIXFormsContextControl.idl	2004-11-05 09:26:23.000000000 +0100
+interface nsIXFormsContextControl : nsISupports

Would it make sense for nsIXFormsContextControl to inherit from
nsIXFormsControl?

>+  void getContext(out DOMString aModelID, out nsIDOMElement aContextNode, out long aContextPosition, out long aContextSize);

The model id should probably be 'AString' not 'DOMString'.  I'm not actually
sure what the distinction is internally, but I've never seen DOMString used
outside of the DOM interfaces.

>--- xforms/nsXFormsGroupElement.cpp	1970-01-01 01:00:00.000000000 +0100
>+++ xforms.group/nsXFormsGroupElement.cpp	2004-11-08 10:56:34.651752424 +0100
>+static const nsIID sScriptingIIDs[] = {
>+  NS_IDOMELEMENT_IID,
>+  NS_IDOM3NODE_IID,
>+  NS_IDOMEVENTTARGET_IID
>+};

This is not needed since you are using the stub.  It implements
GetScriptingInterfaces() for you.

>+class nsXFormsGroupElement : public nsIXFormsControl,
>+                             public nsXFormsXMLVisualStub,
>+                             public nsIXFormsContextControl
>+{
...
>+  /** The current ID of the model node is bound to */
>+  nsAutoString mModelID;

Typically we say not to use nsAutoString as a class member because it increases
object size (it has a 64-byte internal buffer).  It's not a huge deal since
group elements won't be _too_ numerous, but if you think you'd like to cut the
object size, you can change this to an nsString at the cost of an extra malloc
when assigning into the model id.

>+// nsIXTFXMLVisual
>+NS_IMETHODIMP
>+nsXFormsGroupElement::OnCreated(nsIXTFXMLVisualWrapper *aWrapper)
>+{
...
>+  domDoc->CreateElementNS(NS_LITERAL_STRING("http://www.w3.org/1999/xhtml"),

NS_NAMESPACE_XHTML (nsXFormsUtils.h)

>+NS_IMETHODIMP
>+nsXFormsGroupElement::GetElementType(PRUint32 *aElementType)
>+{
>+  *aElementType = ELEMENT_TYPE_XML_VISUAL;
>+  return NS_OK;
>+}

Not needed, the stub implements it.


>--- xforms/nsXFormsUtils.cpp	2004-11-05 09:26:01.000000000 +0100
>+++ xforms.group/nsXFormsUtils.cpp	2004-11-05 13:55:49.000000000 +0100
>@@ -197,78 +198,101 @@ nsXFormsUtils::GetParentModel(nsIDOMElem
>       if (namespaceURI.EqualsLiteral(NS_NAMESPACE_XFORMS))
>         break;
>     }
> 
>     temp.swap(modelWrapper);
>     temp->GetParentNode(getter_AddRefs(modelWrapper));
>   }
> 
>-  // We're releasing this reference, but the node is owned by the DOM.
>-  return modelWrapper;
>+  NS_IF_ADDREF(*aModel = modelWrapper);

Ok with me if you want to change this to return the addref'd model as an
out-param, but you can do it without adding an extra addref:

*aModel = nsnull;
modelWrapper.swap(*aModel);

This swaps the values of *aModel and modelWrapper without doing any
refcounting.  And since *aModel will be null, when the nsCOMPtr goes away it
won't bother releasing anything.

>+/* static */ nsresult
>+nsXFormsUtils::GetNodeContext(nsIDOMElement  *aElement,
>+                              PRUint32        aElementFlags,
>+                              nsIDOMNode    **aModel,
>+                              nsIDOMElement **aBindElement,
>+                              nsIDOMElement **aContextNode)
>+{
..
>+    // CASE 2: Use @model
>+    // If bind did not set model, and the element has a model attribute we use this
>     nsAutoString modelId;
>     aElement->GetAttribute(NS_LITERAL_STRING("model"), modelId);
>+    
>+    if (!modelId.IsEmpty()) {
>+      nsCOMPtr<nsIDOMElement> modelElement;
>+      domDoc->GetElementById(modelId, getter_AddRefs(modelElement));
>+      nsCOMPtr<nsIDOMNode> modelNode = do_QueryInterface(modelElement);

You don't need to QI a nsIDOMElement to a nsIDOMNode; it inherits from
nsIDOMNode.

>+/* static */ nsIDOMNode*
>+nsXFormsUtils::GetModel(nsIDOMElement  *aElement,
>+                        PRUint32        aElementFlags)
...
>+  // @todo beaufour: I think not addref'ing it is bad style...
>+  // We're releasing this reference, but the node is owned by the DOM.
>+  return model;
> }

One thing you could do is to return |already_AddRefed<nsIDOMNode>|.  That makes
it such that the caller can assign the result into an nsCOMPtr without an
additional addref (like getter_AddRefs() except for the return value).	There's
a negligable code size increase from doing this since each caller now may have
to call Release().

>@@ -384,63 +408,73 @@ nsXFormsUtils::EvaluateNodeset(nsIDOMEle
> nsXFormsUtils::EvaluateNodeBinding(nsIDOMElement  *aElement,
>                                    PRUint32        aElementFlags,
>                                    const nsString &aBindingAttr,
>                                    const nsString &aDefaultRef,
>                                    PRUint16        aResultType,
>                                    nsIDOMNode    **aModel,
>                                    nsIDOMElement **aBind)
> {
...
>+  if (!contextNode) {
>+    // XXXfixme when xpath extensions are implemented (instance())
>+    // beaufour: instance() does not change the evaluation context

Hm, let's not have an ongoing discussion via code comments.  If you don't think
there will be any changes needed here to support instance(), just remove my
original comment.

Comment 17

[Allan Beaufour]

(In reply to comment #14)
> We need to settle on a way to mark something "todo".  I've seen a couple of
> different ways, including 'XXX'.  Just make sure that your todo's match todo's
> in other code so that if someone wants to grep for todo's, it is consistent.

Yes, you've commented on that before, but did we take a decision? I like to have
@todo and @bug in the code, then it shows in Doxygen, but I can append XXX to
the comments if you want that?
> Is there some reason why you changed nsXFormsUtils::GetParentModel to return
> model as an output parameter, but the function nsXFormsUtils::GetModel doesn't?
>  It returns the model as the return value.

Lazyness? :) I'll change that.

> I don't know if this is an error or not, just something I
> observed...GetContextNode can return without returning a contextNode should
> FindParentContext not return a context node.  Shouldn't it return the root node
> from the first instance document in its model in this case?  I would think that
> with a name GetContextNode, it should return a context node if there is a way to
> figure out the context node.  You kinda handle this exact situation in
> nsXFormsUtils::EvaluateNodeBinding.  I just think that it should be moved into
> GetContextNode if that is a logical place to put it.
> 
> Any way to pass a flag into GetContextNode so that if all I want is the model,
> it doesn't try to find the context node when it isn't necessary?  For example,
> when GetModel calls it and there is a bind or model attr on my element.

Well, I left it there for two reasons: 1) That part of the code does not need to
be run in EvaluateNodeBinding() if there's no binding attribute or there's a
@bind and 2) The function does what it needs to do for the two calls currently
in the code. So I would rather leave it as it is, until there's another need for it.

Comment 18

[Allan Beaufour]

(In reply to comment #15)
> Also doesn't look like you handle this item mentioned in the spec, "Setting the
> input focus on a group results in the focus being set to the first form control
> in the navigation order within that group."

Correct, I've written a todo for it -- "implement-me-later"(tm) :)

Comment 19

[Allan Beaufour]

(In reply to comment #16)
> (From update of attachment 165110 [details] [diff] [review])
> >--- xforms/nsIXFormsContextControl.idl	1970-01-01 01:00:00.000000000 +0100
> >+++ xforms.group/nsIXFormsContextControl.idl	2004-11-05 09:26:23.000000000 +0100
> +interface nsIXFormsContextControl : nsISupports
> 
> Would it make sense for nsIXFormsContextControl to inherit from
> nsIXFormsControl?

No, I do not think so. Because nsXFormsModelElement should also one day set
context, for any action children, and thus inherit from nsIXFormsContextControl.
Maybe we should drop the "Control" part? nsIXFormsContext?

> >+  void getContext(out DOMString aModelID, out nsIDOMElement aContextNode,
out long aContextPosition, out long aContextSize);
> 
> The model id should probably be 'AString' not 'DOMString'.  I'm not actually
> sure what the distinction is internally, but I've never seen DOMString used
> outside of the DOM interfaces.

Ok, I just copy'n'paste'd from a DOM interface :) AString it is.

> >+class nsXFormsGroupElement : public nsIXFormsControl,
> >+                             public nsXFormsXMLVisualStub,
> >+                             public nsIXFormsContextControl
> >+{
> ...
> >+  /** The current ID of the model node is bound to */
> >+  nsAutoString mModelID;
> 
> Typically we say not to use nsAutoString as a class member because it increases
> object size (it has a 64-byte internal buffer).  It's not a huge deal since
> group elements won't be _too_ numerous, but if you think you'd like to cut the
> object size, you can change this to an nsString at the cost of an extra malloc
> when assigning into the model id.

Ok, I'll change that to nsString instead.

> >@@ -384,63 +408,73 @@ nsXFormsUtils::EvaluateNodeset(nsIDOMEle
> > nsXFormsUtils::EvaluateNodeBinding(nsIDOMElement  *aElement,
> >                                    PRUint32        aElementFlags,
> >                                    const nsString &aBindingAttr,
> >                                    const nsString &aDefaultRef,
> >                                    PRUint16        aResultType,
> >                                    nsIDOMNode    **aModel,
> >                                    nsIDOMElement **aBind)
> > {
> ...
> >+  if (!contextNode) {
> >+    // XXXfixme when xpath extensions are implemented (instance())
> >+    // beaufour: instance() does not change the evaluation context
> 
> Hm, let's not have an ongoing discussion via code comments.  If you don't think
> there will be any changes needed here to support instance(), just remove my
> original comment.
> 

Yeah, sorry about that. I should just have mentioned it in the BugZilla
comments. But correct me if I'm wrong, but instance() only influences the
evaluation context for the expression inside the instance call. The global
evaluation context is still the same.

All in all: I agree with all your comments, I'll submit a new patch later.

Comment 20

[Allan Beaufour]

Attachment: Fixed comments, etc.

I've fixed all comments, and have also added context size and position
arguments on the necessary functions (see bug 269132).

Maybe I could  take advantage of the new BeginAddingChildren(), but I'll create
a new bug for that later then.

Comment 21

[Darin Fisher]

fixed-on-trunk