280423 - We need to be able to set context information for XForms events

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect, P5)

Description

[Allan Beaufour]

We need to be able to set context info for:
* xforms-insert
* xforms-delete
* xforms-submit-error
* xforms-link-exception
* xforms-link-error
* xforms-compute-exception

Strings all of them, either xsd:string or xsd:anyURI.

Comment 1

[aaronr]

When I asked David at Novell if we should use the UIEvent interface, he said
that "There will be an interface in XForms 1.1 for getting to the context
information."

This is it: http://www.w3.org/TR/xforms11/#event-info

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I discussed about this with Allan and I think this is not a high priority.
Let's try to implement 1.0 first ;)
And IMO UIEvent is wrong in this case, it doesn't help us here.

Comment 3

[alexander :surkov (:asurkov)]

context information is used by event() function http://www.w3.org/TR/xforms11/#fn-event. (it's good to see an example of it). Also I guess we can provide own event interface to expose context info because it don't see DOM events spec address this issue.

Comment 4

[Merle Sterling]

Attachment: testcase: submission

Submission element with resource element. Use for testing context info on submit events.

Comment 5

[Merle Sterling]

Attachment: Initial Patch

My initial take on context info:

- new interface (nsIXFormsContextInfo) and class (nsXFormsContextInfo)
The context info can be of many types: string, number, node, nodeset. nsXFormsContextInfo contains member variables for each type and they are accessed via the nsIXFormsContextInfo interface.

- new interface (nsIXFormsDOMEvent) and class (nsXFormsDOMEvent)
nsXFormsDOMEvent maintains a hashtable of nsIXFormsContextInfo keyed by the string name of the context info (eg. 'resource-uri'). nsXFormsDOMEvent implements nsIDOMNSEvent and nsIPrivateDOMEvent and forwards all calls to its contained nsIDOMEvent.

SubmissionElement is setting the context info on the submit-done event as an example of how we would do it for each of the other elements.

Comment 6

[alexander :surkov (:asurkov)]

Probably you could use variant instead of string/number and so on. You could use redirection macros to implement nsIDOMEvent and so on methods. Also do we fire nsIDOMEvent (document.createEvent("Events")) only in XForms that requires context information?

Comment 7

[Merle Sterling]

(In reply to comment #6)
> Probably you could use variant instead of string/number and so on.
So nsIVariant and nsDiscriminatedUnion is the preferred way to implement a union? Just took a look at it and it could probably work but it will be way more work than simply maintaining 4 different pointers in the nsXFormsContextInfo class and will use far more memory.

> You could use redirection macros to implement nsIDOMEvent and so on methods. 
I do use NS_FORWARD_NSIDOMEVENT(mInner->) to forward nsIDOMEvent methods. 

The nsIXFormsDOMEvent extends nsIDOMEvent so I can pass that pointer to the dispatch methods. The catch is that the dom event dispatcher also uses the nsIDOMNSEvent and nsIPrivateDOMEvent interfaces and when it QI's to either of those interfaces it fails. I have to 'implement' those methods and I do so by having each method of those interfaces QI to the right interface and then forward the call to the contained nsIDOMEvent (after all the nsIDOMEvent ptr is really a concrete nsDOMEvent class). See nsXFormsDOMEvent.cpp.

> Also do we fire nsIDOMEvent (document.createEvent("Events")) only in XForms > that requires context information?

Yes. The nsIXFormsDOMEvent subclasses nsIDOMEvent and is implemented by nsXFormsDOMEvent. So we create the normal dom event and then an xforms dom event and pass it the real dom event in the constructor. The methods of nsIDOMEvent, nsIDOMNSEvent, and nsIPrivateDOMEvent get delegated to the inner nsIDOMEvent.

Comment 8

[alexander :surkov (:asurkov)]

Look at the bug 368835. I'm going to add common event to store data. It might be suitable to store context info for xforms events.

> > Also do we fire nsIDOMEvent (document.createEvent("Events")) only in XForms > that requires context information?
> 
> Yes. The nsIXFormsDOMEvent subclasses nsIDOMEvent and is implemented by
> nsXFormsDOMEvent. So we create the normal dom event and then an xforms dom
> event and pass it the real dom event in the constructor. The methods of
> nsIDOMEvent, nsIDOMNSEvent, and nsIPrivateDOMEvent get delegated to the inner
> nsIDOMEvent.
> 

I meant do we need store context info for UI/key/mouse and so on event?

Comment 9

[Merle Sterling]

> I meant do we need store context info for UI/key/mouse and so on event?

No, this is context information for XForms events - like xforms-submit-done, xforms-insert, xforms-delete, etc.

Comment 10

[Merle Sterling]

Olli,
Can you take a look at this and comment on it?

Comment 11

[Merle Sterling]

Olli,
Have you had a chance to review the intial patch? I want to submit a new patch that uses the new classes and interfaces and it will be harder to see how it works with changes to a dozen files.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Argh, sorry. Looking at the patch right now!

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 279606 [details] [diff] [review]
Initial Patch


>+/**
>+ * Interface exposing the event context info of an XForms control.
>+ *
>+ */
>+
>+[scriptable, uuid(b012a3e5-7dbc-42b4-99da-5e94c37dc44e)]
>+interface nsIXFormsContextInfo : nsISupports
>+{
>+    readonly attribute DOMString name;
>+    readonly attribute long type;

Please document what there attributes mean.

>+
>+NS_IMPL_ISUPPORTS1(nsXFormsContextInfo, nsIXFormsContextInfo)
>+
>+nsXFormsContextInfo::nsXFormsContextInfo()
>+ : mType(-1), mNode(nsnull), mNodeset(nsnull), mNumber(0)
>+{
>+ mName.AssignLiteral("");
>+ mString.AssignLiteral("");

Strings are empty by default. And EmptyString() is a better way to
assign "". You may want to set strings to null (SetIsVoid or something).

Should the getter methods thrown some error if type isn't right?

>+  // Set resource-uri context info.
>+  nsCOMPtr<nsXFormsContextInfo> contextInfo = new nsXFormsContextInfo();

Check for OOM

This wasn't a real review, but yes, creating the event that way should be ok.
What is wonder is that httpheader thingie.

Comment 14

[Merle Sterling]

(In reply to comment #13)
> What is wonder is that httpheader thingie.

One of many complications with implementing context info...:)

One context property of submission element is 'response-headers' and the type is supposed to be a nodeset. The only way to get a nodeset is to evaluate an xpath expression, so I have to create a document on the fly and then evaluate an xpath to give me a nodeset of <header> nodes. You can't just get an array of the headers. You have to implement nsIHttpHeaderVisitor so I add to the document each time the VisitHeader method is called.

I'm trying to implement the xpath Event function and I think I am going to end up changing the nsXFormsContextInfo class to have only one member var of type  nsIDOMXPathResult. That means I'll have to do the same kind of thing that was done for response-headers (create a doc and evaluate an xpath for it) for every property, including numbers and strings. Nothing is ever simple and straightforward with the mozilla codebase.

Comment 15

[Merle Sterling]

Attachment: testcase: insert

Testcase for insert. Displays contextinfo for 'inserted-nodes' and 'position'.

Comment 16

[Merle Sterling]

Attachment: patch

Patch implements context info for xforms-insert, xforms-delete, xforms-link-exception, xforms-link-error, xforms-compute-exception, xforms-submit-done.

There are a few issues with xforms-submit-serialize and we need clarification from the WG so those changes are not included. xforms-submit-error is done with the exception of the property 'response-body'. We should do the remaining context info for submission in a separate bug once it is clear exactly how sumbit-serialize is supposed to work.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #16)
> There are a few issues with xforms-submit-serialize and we need clarification
> from the WG so those changes are not included. xforms-submit-error is done with
> the exception of the property 'response-body'. We should do the remaining
> context info for submission in a separate bug once it is clear exactly how
> sumbit-serialize is supposed to work.
> 

Could you open a new bug for these issues

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 283776 [details] [diff] [review]
patch

the patch is huge, partial review...

> # Temporary files that will be deleted on f.x. make clean
>-GARBAGE		+= install.js install.template
>+

Is this really for this bug?

>+interface nsIXFormsContextInfo : nsISupports

Just wondering, could you combine nsIXFormsContextInfo to nsIXFormsDOMEvent?

And are there any better ways to implement contextinfo? Perhaps nsXFormsContenxtInfo could
have just 4 members, mElement, mName, mType and mValue. mValue would be then
perhaps an union of nsIDOMDocument*, nsIDOMNode*, nsIDOMXPathResult*, nsString*.
The first 3 types in union should be refcounted. This way the size of the object wouldn't be so big.


>+  NS_ADDREF(mCurrentEvent = aEvent);
>+
So you're leaking events here; AddReffing but not Releasing.
Could mCurrentEvent be just nsCOMPtr<nsIDOMEvent>?

> NS_IMETHODIMP
>+nsXFormsActionElement::GetCurrentEvent(nsIDOMEvent **aEvent)
>+{
>+  *aEvent = mCurrentEvent;
Out parameters must be AddReffed.

>+NS_IMETHODIMP
>+nsXFormsActionModuleBase::GetCurrentEvent(nsIDOMEvent **aEvent)
>+{
>+  *aEvent = mCurrentEvent;
Same here.

>+
>+  // The element to which the context info refers.
>+  nsIDOMElement *mElement;
>+  // Document to create nodes for String and Number types.
>+  nsCOMPtr<nsIDOMDocument> mContextDoc;

Could you use mElement->GetOwnerDocument here?

r- because of memory leaks.

Comment 19

[Merle Sterling]

(In reply to comment #18)
> (From update of attachment 283776 [details] [diff] [review])
> > # Temporary files that will be deleted on f.x. make clean
> >-GARBAGE		+= install.js install.template
> >+
> Is this really for this bug?
Aaron mentioned that we no longer build install.js or install.template so they don't need to be deleted. Just removing that line as part of this bug instead of opening a new one because this patch requires makefile changes.

> >+interface nsIXFormsContextInfo : nsISupports
> Just wondering, could you combine nsIXFormsContextInfo to nsIXFormsDOMEvent?
> And are there any better ways to implement contextinfo? Perhaps
> nsXFormsContenxtInfo could
> have just 4 members, mElement, mName, mType and mValue. mValue would be then
> perhaps an union of nsIDOMDocument*, nsIDOMNode*, nsIDOMXPathResult*,
> nsString*.
> The first 3 types in union should be refcounted. This way the size of the
> object wouldn't be so big.
I suppose we could combine nsIXFormsContextInfo and nsIXFormsDOMEvent but I'd rather not. Right now you get the nsIXFormsContextInfo from the event and then set or get context properties using it. If they were combined I'd have to have methods like setContextInfoStringValue and the like. I think its better to keep them separate.

As for the union idea, it will not save any space. Name and Type are required. nsIDOMNode and nsIDOMXPathResult types would occupy 8 bytes. If I use an nsDiscriminatedUnion they would also occupy 8 bytes because the union uses a struct with two members. The downside is that I then need to implement the nsIVariant interface and add dozens of lines of code to deal with the union. That adds complexity to the code with no benefit because it will not save memory. I don't like that approach.

> >+  NS_ADDREF(mCurrentEvent = aEvent);
> >+
> So you're leaking events here; AddReffing but not Releasing.
> Could mCurrentEvent be just nsCOMPtr<nsIDOMEvent>?
Hmm...not so sure about that. Without the NS_ADDREF the event was released one too many times and the code traps in the event subsystem.

> > NS_IMETHODIMP
> >+nsXFormsActionElement::GetCurrentEvent(nsIDOMEvent **aEvent)
> >+{
> >+  *aEvent = mCurrentEvent;
> Out parameters must be AddReffed.
> >+NS_IMETHODIMP
> >+nsXFormsActionModuleBase::GetCurrentEvent(nsIDOMEvent **aEvent)
> >+{
> >+  *aEvent = mCurrentEvent;
> Same here.
Have to investigate that. Maybe if they were addref'd here I wouldn't have to addref when I save the current event in mCurrentEvent.

> >+
> >+  // The element to which the context info refers.
> >+  nsIDOMElement *mElement;
> >+  // Document to create nodes for String and Number types.
> >+  nsCOMPtr<nsIDOMDocument> mContextDoc;
> Could you use mElement->GetOwnerDocument here?

As opposed to creating my own stand-alone doc as is done now? Probably, and it would save a few lines of code and a bit of memory, but that 'doc' is used solely to create elements to encapsulate string and number types (because they have to be nsIDOMNodes to set via txINodeset. I don't really need or want them to be related to the owner document.

I know this patch is large but it is difficult to separate all the different context info into separate bugs. There are a lot of files affected because of things like having to modify a dozen files to save the current event - but they are all the same.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #19)
> As for the union idea, it will not save any space. Name and Type are required.
> nsIDOMNode and nsIDOMXPathResult types would occupy 8 bytes. If I use an
> nsDiscriminatedUnion they would also occupy 8 bytes because the union uses a
> struct with two members. The downside is that I then need to implement the
> nsIVariant interface and add dozens of lines of code to deal with the union.
> That adds complexity to the code with no benefit because it will not save
> memory. I don't like that approach.

I meant just normal C union. The size would be either 4 or 8 bytes depending 
on system. And the union could have nsIDOMDocument*, nsIDOMNode*, 
nsIDOMXPathResult*, nsString* as member types.

> 
> > >+  NS_ADDREF(mCurrentEvent = aEvent);
> > >+
> > So you're leaking events here; AddReffing but not Releasing.
> > Could mCurrentEvent be just nsCOMPtr<nsIDOMEvent>?
> Hmm...not so sure about that. Without the NS_ADDREF the event was released one
> too many times and the code traps in the event subsystem.
> 

But you're not releasing it. Either you need to use nsCOMPtr as a member or
NS_ADDREF and NS_RELEASE. nsCOMPtr makes it easier to handle refcounting.

> > >+nsXFormsActionModuleBase::GetCurrentEvent(nsIDOMEvent **aEvent)
> > >+{
> > >+  *aEvent = mCurrentEvent;
> > Same here.
> Have to investigate that. Maybe if they were addref'd here I wouldn't have to
> addref when I save the current event in mCurrentEvent.

Out param must be addrefed here if mCurrentEvent is handled correctly elsewhere.
> As opposed to creating my own stand-alone doc as is done now? Probably, and it
> would save a few lines of code and a bit of memory, but that 'doc' is used
> solely to create elements to encapsulate string and number types (because they
> have to be nsIDOMNodes to set via txINodeset. I don't really need or want them
> to be related to the owner document.

But adding that extra member makes the object larger.

> 
> I know this patch is large but it is difficult to separate all the different
> context info into separate bugs. There are a lot of files affected because of
> things like having to modify a dozen files to save the current event - but they
> are all the same.
> 
Yes, it is ok to keep this all here, but just allow me to do few reviews, not
all at once, please.

Comment 21

[Merle Sterling]

(In reply to comment #18)
A few more comments based on trying to address the comments below:

> Perhaps nsXFormsContenxtInfo could
> have just 4 members, mElement, mName, mType and mValue. mValue would be then
> perhaps an union of nsIDOMDocument*, nsIDOMNode*, nsIDOMXPathResult*,
> nsString*.
> The first 3 types in union should be refcounted. This way the size of the
> object wouldn't be so big.
The union would only contain nsIDOMNode* and nsIDOMXPathResult* for a whopping savings of 4 bytes. Then you have to addref them whenever they are set; otherwise we trap.

> >+  NS_ADDREF(mCurrentEvent = aEvent);
> >+
> So you're leaking events here; AddReffing but not Releasing.
> Could mCurrentEvent be just nsCOMPtr<nsIDOMEvent>?
Tried it without ns_addref and changing the member to nsCOMPtr<nsIDOMEvent>. It traps.

> > NS_IMETHODIMP
> >+nsXFormsActionElement::GetCurrentEvent(nsIDOMEvent **aEvent)
> >+{
> >+  *aEvent = mCurrentEvent;
> Out parameters must be AddReffed.
> >+NS_IMETHODIMP
> >+nsXFormsActionModuleBase::GetCurrentEvent(nsIDOMEvent **aEvent)
> >+{
> >+  *aEvent = mCurrentEvent;
> Same here.
The caller does getter_AddRefs. It is not necessary to addref it here. If you do, it traps.

> >+
> >+  // The element to which the context info refers.
> >+  nsIDOMElement *mElement;
> >+  // Document to create nodes for String and Number types.
> >+  nsCOMPtr<nsIDOMDocument> mContextDoc;
> Could you use mElement->GetOwnerDocument here?
I create the document to use to create elements once in the constructor. The method CreateContextDoc does use mElement->GetOwnerDocument so it can get the DOMDOMImplementation to create elements. If I don't save the pointer to the doc, I'd have to create it in the methods that set string and number values. IMO, it's better to do it once and save the pointer rather than doing extra work on every call to SetStringValue and SetNumberValue.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #21)
> The caller does getter_AddRefs. It is not necessary to addref it here. If you
> do, it traps.

It is needed to AddRef. If there are some problems, they are there because of
something else.
getter_AddRefs means that the Getter method AddRefs.

Comment 23

[Merle Sterling]

Attachment: patch2

- AddRef the saved DOMEvent (mCurrentEvent).
- Using a union for Node and Nodeset context info types.
- Added xforms-submit-serialize

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 285531 [details] [diff] [review]
patch2


>+[scriptable, uuid(b012a3e5-7dbc-42b4-99da-5e94c37dc44e)]
>+interface nsIXFormsContextInfo : nsISupports
>+{
>+    // Name of the event property; eg. 'resource-uri'
>+    readonly attribute DOMString name;
>+    // Type of the property: String, Number, Node, Nodeset
>+    readonly attribute long type;

Where are the types defined? Why not add some consts to .idl

>+nsresult
>+nsXFormsContextInfo::SetStringValue(nsAString &aString)
>+{
Should you assert here that type isn't set yet.
Also in other SetXXXValue methods.

>+  nsresult rv;
>+
>+  if (!mContextDoc)
>+    return NS_ERROR_FAILURE;
>+
>+  // Text node containing the string type property value.
>+  nsCOMPtr<nsIDOMText> stringTextNode;
>+  rv = mContextDoc->CreateTextNode(aString,
>+                                   getter_AddRefs(stringTextNode));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIDOMNode> newChild;
>+  mContextDoc->AppendChild(stringTextNode, getter_AddRefs(newChild));
I don't understand this AppendChild. Same also elsewhere.

>+NS_IMETHODIMP
>+nsXFormsContextInfo::NodeValue(nsIDOMNode **aResult)
>+{
>+  *aResult = mValue.mNode;

You must AddRef |out| parameters!

>+  enum ContextType {
>+    STRING_TYPE  = 0,
>+    NUMBER_TYPE,
>+    NODE_TYPE,
>+    NODESET_TYPE
>+  };
So perhaps better to move this to .idl.

>+  union {
>+    // DOMNode if the type is Node, String, or Number. Numbers
>+    // and strings are encapsulated in a text node.
>+    nsIDOMNode *mNode;
>+    // nsIDOMXPathResult if the type is Nodeset.
>+    nsIDOMXPathResult *mNodeset;
>+  } mValue;

Why not

union {
  nsString* mString;
  nsIDOMNode *mNode;
  nsIDOMXPathResult *mNodeset;
  PRInt32 mNumber; // or should this be PRInt32* mNumber;
};
Of course that would lead to some changes in methods, but no need to keep string/number value
in mNode. Then depending on the type either delete or release the value when deleting the object.
And note, only one need to be set to null in constructor, for example mString = nsnull; then
also mNode etc will have value nsnull. 

>+nsXFormsDOMEvent::nsXFormsDOMEvent(nsIDOMEvent *aInner,
>+                                   nsCOMArray<nsIXFormsContextInfo> *aContextInfo)
>+{
>+  mInner = aInner;
>+  mContextInfo.Init(16);
Is 16 a bit too high value? How many context infos are usually kept in mContextInfo?
Or could you use the ->Count() from aContextInfo to Init()?

>+nsresult
>+nsXFormsInsertDeleteElement::SetContextInfo()
>+{
>+  nsAutoString contextName;
>+  nsCOMPtr<nsXFormsContextInfo> contextInfo = new nsXFormsContextInfo(mElement);
So you create contextInfo here, but don't use it at all?

>+    NS_ENSURE_TRUE(contextInfo, NS_ERROR_OUT_OF_MEMORY);
>+    contextName.AssignLiteral("inserted-nodes");
>+    contextInfo->SetName(contextName);
>+    contextInfo->SetNodesetValue(mInsertedNodes);

Hmm, should SetXXXValue methods have const char* aName as the first
parameter, then you wouldn't need contextName string and SetName calls here.
So something like contextInfo->SetNodesetValue("inserted-nodes", mInsertedNodes);
Then in ::SetNodesetValue(const char* aName, nsIDOMNodeSet* aValue) you handle
converting aName to nsString or whatever.

Comment 25

[Merle Sterling]

> Why not
> union {
>   nsString* mString;
>   nsIDOMNode *mNode;
>   nsIDOMXPathResult *mNodeset;
>   PRInt32 mNumber; // or should this be PRInt32* mNumber;
> };
> Of course that would lead to some changes in methods, but no need to keep
> string/number value
> in mNode. Then depending on the type either delete or release the value when
> deleting the object.
The union could very well look like that but the reason it wasn't done that way originally is because I DO need string/number values in a node. The only type I can return from the XPath Event function is txINodeset and the only way to add to it is via Add(nsIDOMNode *). So I either store the numbers and strings as-is and then later create a text node and add it to the txINodeset to return or I create the text node and add the number/string when the number/string value is first set (which is how I did it). I'm not against changing the union and the affected methods and will do so.

If we go that way then I could probably do all of the text node creation stuff in the Event function and eliminate the mContextDoc in the context info class that you didn't like.

> Hmm, should SetXXXValue methods have const char* aName as the first
> parameter, then you wouldn't need contextName string and SetName calls here.
> So something like contextInfo->SetNodesetValue("inserted-nodes",
> mInsertedNodes);
> Then in ::SetNodesetValue(const char* aName, nsIDOMNodeSet* aValue) you > handle converting aName to nsString or whatever.

That is a perfectly good idea and one I considered. I try to avoid lots of conversions between types but we would avoid extra method calls to SetName and eliminate a few local variables so I'll do it that way.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #25)
> The union could very well look like that but the reason it wasn't done that way
> originally is because I DO need string/number values in a node.

Ah, sorry, my mistake.

Comment 27

[Merle Sterling]

Attachment: patch3

Summary of current changes and comments regarding prior review comments.

- Passing const char *aName to Set methods so we avoid extra local variables and calls to SetName.
- Fixed missing AddRefs and removed extraneous code that created an object and then never used the variable.
- The union of types can ONLY contain nsIDOMNode* and nsIDOMXPathResult. We cannot have nsString* and PRInt32 (for number types) in the union because they must be nodes to add to the txINodeset returned by the XPath Event function. However, I can eliminate mString and mNumber member vars since they are encapsulated in a text node.
- Moved the code to create a text node into a private member function and eliminated creating a document and saving it to use for creating the text nodes. Instead the function can just get the document from the element. It is also not necessary to Append the newly created text node to a document - I thought it was before but it is not (so you don't have to be confused by the AppendChild call anymore. :))
- Did NOT add assertions because I don't think it is necessary. I set the type internally when the value is set. We could possibly assert if the type set doesn't match the type being requested; ie if the type is set to NODE_TYPE and you call StringValue but I don't see much value in that as the return will be null anyway.
- I cannot use Init(aContextInfo->Count()) for the size of the contextinfo hashtable. Not all events have context info and we'd have to check if mContextInfo is null and initialize it. Naturally that is not simple as the hashtable doesn't define ! or == operators. It's just not worth it. The min size of the hashtable as defined in nsBaseHashtable is 16 - just as I had it before but now I just don't pass any size so it defaults to the min.

I will be happy to change things if I missed any AddRef type requirements or errors of that type but I do not want to change the inheritance hierarchy or data types anymore. Trying to address some of the review comments caused me to revisit problems I have already resolved and took several days. It works properly for the testsuite testcase.

Remember, this code must work for native code, XPath, and Javascript and that makes it difficult. As it stands now, all the cases will work properly.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 286244 [details] [diff] [review]
patch3

Almost there :)

>Index: nsIXFormsContextInfo.idl
>+[scriptable, uuid(b012a3e5-7dbc-42b4-99da-5e94c37dc44e)]
>+interface nsIXFormsContextInfo : nsISupports
>+{
>+    // Name of the event property; eg. 'resource-uri'
>+    readonly attribute DOMString name;
>+
>+    // Type of the property: String, Number, Node, Nodeset
>+    const unsigned short STRING_TYPE  = 1;
>+    const unsigned short NUMBER_TYPE  = 2;
>+    const unsigned short NODE_TYPE    = 3;
>+    const unsigned short NODESET_TYPE = 4;
>+    readonly attribute long type;
>+
>+    DOMString stringValue();
>+    long numberValue();
>+    nsIDOMNode nodeValue();
>+    nsIDOMXPathResult nodesetValue();

Nit, use 2 spaces as indentation, same also in nsIXFormsDOMEvent.idl
>+nsXFormsContextInfo::nsXFormsContextInfo(nsIDOMElement *aElement)
>+ : mElement(aElement), mType(0)
>+{
>+  mValue.mNode = nsnull;
>+}

Nit, if you drop mValue from the union, you can have mNode = nsnull;
Hmm, though now that there are only two types stored and both inherits 
nsISupports, using nsCOMPtr<nsISupports> mValue would be possible too, and then
just do_QueryInterface when needed. Both (nsCOMPtr<nsISupports> or union) are fine to me.

>+NS_IMETHODIMP
>+nsXFormsContextInfo::StringValue(nsAString &aResult)
>+{
>+  nsXFormsUtils::GetNodeValue(mValue.mNode, aResult);
>+  return NS_OK;
>+}

This is a scriptable method, so someone may ask stringValue even if the current type
is nodeset. That would crash, right?
Same problem with other scriptable getter methods too.
Somehow you need to ensure that the type is correct.

Btw, in the .idl, could those getter methods be
readonly attribute DOMString stringValue;
readonly attribute long numberValue;
etc.

>+nsXFormsContextInfo::~nsXFormsContextInfo()
>+{
>+}
You have to NS_IF_RELEASE mValue.mNode or mValue.mNodeset here depending
on the mType. (unless you decide to use nsCOMPtr<nsISupports>)

> class nsXFormsSetIndexElement : public nsXFormsActionModuleBase
> {
> public:
>-  NS_DECL_NSIXFORMSACTIONMODULEELEMENT
>+  NS_IMETHOD
>+  HandleAction(nsIDOMEvent *aEvent, nsIXFormsActionElement *aParentAction);
> };
Nit, the following style might be better.
  NS_IMETHOD HandleAction(nsIDOMEvent *aEvent,
                          nsIXFormsActionElement *aParentAction);

Comment 29

[Merle Sterling]

(In reply to comment #28)
> > Nit, use 2 spaces as indentation, same also in nsIXFormsDOMEvent.idl
Blame it on VisualSlick. I changed the code formatting options to match the Mozilla conventions for all types except .idl and I don't even notice that the spacing is 4 instead of 2. :)

> >+NS_IMETHODIMP
> >+nsXFormsContextInfo::StringValue(nsAString &aResult)
> >+{
> >+  nsXFormsUtils::GetNodeValue(mValue.mNode, aResult);
> >+  return NS_OK;
> >+}
> This is a scriptable method, so someone may ask stringValue even if the > current type is nodeset. That would crash, right?
> Same problem with other scriptable getter methods too.
> Somehow you need to ensure that the type is correct.
That case would be fine because a string is actually a node. It would be a problem if the caller did SetType(NODESET) and then called GetStringValue.

I've been thinking that SetType should be private and the type only be set via the SetString/Number/Node/NodesetValue methods so a caller cannot accidentally set a value and set its type to something different. I think we would still need to check the type anyway because it would be possible to set a nodeset value and then call GetStringValue.

So how to handle it - Assert or NS_ENSURE_TRUE or just return null if the type does not match?

> Btw, in the .idl, could those getter methods be
> readonly attribute DOMString stringValue;
> readonly attribute long numberValue;
> etc.
Sure and that is probably better because the method will then be Getxxx and match its Setxxx counterpart.

Comment 30

[Merle Sterling]

Attachment: patch4

Adding ns_assertion if a Get method is called and the type of the context property does not match.

GetNodeValue can be used to get a number or string property as a node (XPath Event relies on that fact) so it will only assert if the type is not node, string, or number.

Note that if the type is nodeset and you call GetStringValue you will get back an empty string, which should be expected, but the assert will fire.

Comment 31

[timeless]

if the trigger for this is dom scriptable, then it's highly inappropriate to use NS_ASSERTION.

That'd mean anytime i'm in venkman i have a high chance of abort()ing because i might type the key that triggers abort to the assertion dialog.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 286341 [details] [diff] [review]
patch4

>+NS_IMETHODIMP
>+nsXFormsContextInfo::GetStringValue(nsAString &aResult)
>+{
>+  NS_ASSERTION(mType == nsIXFormsContextInfo::STRING_TYPE,
>+               "GetStringValue: context type is not a string!");

Timeless is right, make this NS_WARN_IF_FALSE.
Same also in other getter methods.

>+
>+  nsXFormsUtils::GetNodeValue(mNode, aResult);
This will still crash if the type is nodeset. Same problem also elsewhere.

>+  union {
>+    nsIDOMNode *mNode;
>+    nsIDOMXPathResult *mNodeset;
>+  };

Align mNode and mNodeset

>@@ -205,16 +206,17 @@ const PRInt32 kDisabledIntrinsicState =
>   NS_EVENT_STATE_OPTIONAL |
>   NS_EVENT_STATE_MOZ_READWRITE;
> 
> struct EventItem
> {
>   nsXFormsEvent           event;
>   nsCOMPtr<nsIDOMNode>    eventTarget;
>   nsCOMPtr<nsIDOMElement> srcElement;
>+  nsCOMArray<nsIXFormsContextInfo> *contextInfo;

I hope contextInfo gets deleted somewhere properly.
Who owns contextInfo? Is it possible that whoever owns it deletes it
before event is dispatched in DispatchDeferredEvents?
Or is it guaranteed that scrElement always owns contextInfo and srcElement's mContextInfo can't get deleted
while contextInfo points to the old value?

Comment 33

[Merle Sterling]

(In reply to comment #32)
> >+  nsXFormsUtils::GetNodeValue(mNode, aResult);
> This will still crash if the type is nodeset. Same problem also elsewhere.
No, it will not crash. nsXFormsUtils::GetNodeValue will bail out and return an empty string. But, we can check the type and just return null to save a wasted call to utils::GetNodeValue.
> > struct EventItem
> > {
> >   nsXFormsEvent           event;
> >   nsCOMPtr<nsIDOMNode>    eventTarget;
> >   nsCOMPtr<nsIDOMElement> srcElement;
> >+  nsCOMArray<nsIXFormsContextInfo> *contextInfo;
> I hope contextInfo gets deleted somewhere properly.
> Who owns contextInfo? Is it possible that whoever owns it deletes it
> before event is dispatched in DispatchDeferredEvents?
> Or is it guaranteed that scrElement always owns contextInfo and srcElement's
> mContextInfo can't get deleted
> while contextInfo points to the old value?
Each element that can have context info has a member that is nsCOMArray<nsIXFormsContextInfo> and that is passed to DispatchEvent so mContextInfo will never be deleted during event processing. 

mContextInfo is never deleted though. So, where to do it? OnDestroyed() or the destructor for the element and can I just call mContextInfo.Clear()?

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #33
> No, it will not crash.
Why not? GetNodeValue takes nsIDOMNode* as parameter, but if the first paremeters isn't a pointer to nsIDOMNode* but pointer to nsIDOMXPathResult*, 
nsXFormsUtils::GetNodeValue would do something bad.

> mContextInfo is never deleted though. So, where to do it? OnDestroyed() or the
> destructor for the element and can I just call mContextInfo.Clear()?
> 
Ah, it is indeed always owned by some element. .Clear() will be automatically 
called when mContextInfo is deleted (which is done automatically when element 
is deleted), so everything should be ok there.

Comment 35

[Merle Sterling]

(In reply to comment #34)
> (In reply to comment #33
> > No, it will not crash.
> Why not? GetNodeValue takes nsIDOMNode* as parameter, but if the first
> paremeters isn't a pointer to nsIDOMNode* but pointer to nsIDOMXPathResult*, 
> nsXFormsUtils::GetNodeValue would do something bad.

Sorry, I was thinking of something else. If the type is nodeset but you call GetString/NumberResult, it won't crash because those methods will get the mNode pointer from the union and call nsXFormsUtils::GetNodeValue. It isn't really a node but the only method that will get called by utils is GetNodeType and then it doesn't match any of the types it can deal with. You're right that it isn't good to rely on that anyway.

So I will check the type and return null if the type is incorrect (for all of the Get methods).

Comment 36

[Merle Sterling]

Attachment: patch5

Comment 37

[Merle Sterling]

Attachment: testcase: xforms-submit-error with 'response-body'

This testcase is for context info 'response-body' in the case of xforms-submit-error'. It is also attached to bug400334 which was intended to implement the reamining submission context info that was not originally covered by the patches for this bug.

Comment 38

[Merle Sterling]

Attachment: patch6

This patch fixes the prior review comments (which were also included in patch5) and adds support for the context info 'response-body' when the event is xforms-submit-error on submission. Bug400334 was intended to be used to add the remaining context info for submission that was not originally included in the patch for this bug, but it's just too hard to keep separate patches for all of this stuff when they all depend on prior patches.

Therefore this patch includes all necessary changes for all context info across all elements. If this patch is deemed acceptable we can just dup bug40334 to this bug.

Comment 39

[Merle Sterling]

Attachment: patch7

Accidentally removed the code to set 'response-headers' into context info for submission. This patch is passing all of the W3C 1.1 testcases that use context info.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

I'll review this on Saturday or Sunday, or if very lucky already tomorrow.

Comment 41

[alexander :surkov (:asurkov)]

Merle, can you put up to dated patch where all Olli's comment are applied? I didn't follow your discussion, so I can't be sure about that Olli pointed already. For example, I see Olli requested to comment all new methods but this patch doesn't have them.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 287881 [details] [diff] [review]
patch7

>+    // Remember the src attribute so we can set it in the context info
>+    // for the xforms-link-error event.
>+    mSrc = src;
Btw, does XForms specify whether xforms-link-error contains the absolute URI?

>+      if (succeeded) {
>+        // XXX: Need to handle replace="text"
File a bug for XXX and add link here.

>+#ifdef DEBUG
>+      PRUint32 nodesetSize = 0;
>+      headerNodeset->GetSnapshotLength(&nodesetSize);
>+      for (PRUint32 i = 0; i < nodesetSize; i++) {
>+        nsCOMPtr<nsIDOMNode> headerNode, nameNode, valueNode;
>+        headerNodeset->SnapshotItem(i, getter_AddRefs(headerNode));
>+        headerNode->GetFirstChild(getter_AddRefs(nameNode));
>+        nsAutoString name, value;
>+        nsXFormsUtils::GetNodeValue(nameNode, name);
>+        nameNode->GetNextSibling(getter_AddRefs(valueNode));
>+        nsXFormsUtils::GetNodeValue(valueNode, value);
>+      }
>+    }
>+#endif // DEBUG
What is this going? Have you had some printfs here perhaps?

>+  // Submission body from xforms-submit-serialize.
>+  nsIDOMNode*                      mSubmissionBody;
Is it ensured that this points always to a valid value, never to a dead object?

>+#ifdef DEBUG
>+      PRInt32 type;
>+      contextInfo->GetType(&type);
>+      if (type == nsXFormsContextInfo::STRING_TYPE) {
>+        nsAutoString str;
>+        contextInfo->GetStringValue(str);
>+      } else if (type == nsXFormsContextInfo::NUMBER_TYPE) {
>+        PRInt32 number;
>+        contextInfo->GetNumberValue(&number);
>+      }
>+#endif
And what is this doing? Either remove these DEBUG blocks or make them do something useful.

r+ with comments addressed.

I hope the second review is quite careful, because of the size of the patch, but no need to be
a nitpicker ;)

Comment 43

[Merle Sterling]

(In reply to comment #42)
> >+#ifdef DEBUG
> >+      PRUint32 nodesetSize = 0;
> >+      headerNodeset->GetSnapshotLength(&nodesetSize);
> >+      for (PRUint32 i = 0; i < nodesetSize; i++) {
> >+        nsCOMPtr<nsIDOMNode> headerNode, nameNode, valueNode;
> >+        headerNodeset->SnapshotItem(i, getter_AddRefs(headerNode));
> >+        headerNode->GetFirstChild(getter_AddRefs(nameNode));
> >+        nsAutoString name, value;
> >+        nsXFormsUtils::GetNodeValue(nameNode, name);
> >+        nameNode->GetNextSibling(getter_AddRefs(valueNode));
> >+        nsXFormsUtils::GetNodeValue(valueNode, value);
> >+      }
> >+    }
> >+#endif // DEBUG
> What is this going? Have you had some printfs here perhaps?
> >+  // Submission body from xforms-submit-serialize.
> >+  nsIDOMNode*                      mSubmissionBody;
> Is it ensured that this points always to a valid value, never to a dead object?
> >+#ifdef DEBUG
> >+      PRInt32 type;
> >+      contextInfo->GetType(&type);
> >+      if (type == nsXFormsContextInfo::STRING_TYPE) {
> >+        nsAutoString str;
> >+        contextInfo->GetStringValue(str);
> >+      } else if (type == nsXFormsContextInfo::NUMBER_TYPE) {
> >+        PRInt32 number;
> >+        contextInfo->GetNumberValue(&number);
> >+      }
> >+#endif
> And what is this doing? Either remove these DEBUG blocks or make them do
> something useful.
I don't see why you'd say these debug blocks aren't 'useful'. They were useful to me when verifying that the http headers were read and saved correctly and the string/number type checks were useful to insure that strings and numbers were properly added to a text node. I'll remove them because we wouldn't want the code to be easier to debug. ;)

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Ah, if those are useful to you, just leave them there.

Comment 45

[Merle Sterling]

(In reply to comment #42)
> >+  // Submission body from xforms-submit-serialize.
> >+  nsIDOMNode*                      mSubmissionBody;
> Is it ensured that this points always to a valid value, never to a dead object?

Yes. The submit-serialize event sends an initially empty text node. The idea is that if an event handler modifies it, it becomes the new serialization data.

Submission will check if the text node is empty (in HandleDefault) and if not save it in mSubmissionBody. If it is still empty, mSubmissionBody will remain null. Just before performing the actual serialization, mSubmissionBody is checked to see if it is non-null and if so the text contents becomes the new serialization data; otherwise, the normal rules for determining the serialization data apply.

Comment 46

[Merle Sterling]

(In reply to comment #41)
> Merle, can you put up to dated patch where all Olli's comment are applied? I
> didn't follow your discussion, so I can't be sure about that Olli pointed
> already. For example, I see Olli requested to comment all new methods but this
> patch doesn't have them.

I'm not sure what you mean. I've been making minor changes at each step of the way and have addressed all of Olli's comments. 

If it is ok to leave the #ifdef DEBUG blocks, the only change I'd make is to remove the XXX comment about replace='text'. That was a note to self when I was working on something else. There are at least a dozen new submission features (not related to context info) that have not been implemented and we need many new bugs so I'll create bugs for that case and the others I know about.

Comment 47

[alexander :surkov (:asurkov)]

I meant it would be great to review the latest version you want to add nothing to. If this patch is latest then it's ok. Just iirc Olli somewhere asks to document interface methods. If I'm wrong and he didn't then I ask :) Please comment every interface method. There is no need new patch for this for sure but keep it in mind.

Comment 48

[Merle Sterling]

I just took a look at the .idl files and it's true I didn't put comments for the methods there. I did however comment them in the .h file of the class that implements those interfaces. I can add the same comments to the .idl files no problem and will do that for the final patch.

Comment 49

[alexander :surkov (:asurkov)]

Sure, please will do, idl should be very well documented, it's our face :)

Comment 50

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 283776 [details] [diff] [review] [details])
> > > # Temporary files that will be deleted on f.x. make clean
> > >-GARBAGE		+= install.js install.template
> > >+
> > Is this really for this bug?
> Aaron mentioned that we no longer build install.js or install.template so they
> don't need to be deleted. Just removing that line as part of this bug instead
> of opening a new one because this patch requires makefile changes.
> 
> > >+interface nsIXFormsContextInfo : nsISupports
> > Just wondering, could you combine nsIXFormsContextInfo to nsIXFormsDOMEvent?
> > And are there any better ways to implement contextinfo? Perhaps
> > nsXFormsContenxtInfo could
> > have just 4 members, mElement, mName, mType and mValue. mValue would be then
> > perhaps an union of nsIDOMDocument*, nsIDOMNode*, nsIDOMXPathResult*,
> > nsString*.
> > The first 3 types in union should be refcounted. This way the size of the
> > object wouldn't be so big.
> I suppose we could combine nsIXFormsContextInfo and nsIXFormsDOMEvent but I'd
> rather not. Right now you get the nsIXFormsContextInfo from the event and then
> set or get context properties using it. If they were combined I'd have to have
> methods like setContextInfoStringValue and the like. I think its better to keep
> them separate.
> 
> As for the union idea, it will not save any space. Name and Type are required.
> nsIDOMNode and nsIDOMXPathResult types would occupy 8 bytes. If I use an
> nsDiscriminatedUnion they would also occupy 8 bytes because the union uses a
> struct with two members. The downside is that I then need to implement the
> nsIVariant interface and add dozens of lines of code to deal with the union.
> That adds complexity to the code with no benefit because it will not save
> memory. I don't like that approach.
> 
> > >+  NS_ADDREF(mCurrentEvent = aEvent);
> > >+
> > So you're leaking events here; AddReffing but not Releasing.
> > Could mCurrentEvent be just nsCOMPtr<nsIDOMEvent>?
> Hmm...not so sure about that. Without the NS_ADDREF the event was released one
> too many times and the code traps in the event subsystem.
> 
> > > NS_IMETHODIMP
> > >+nsXFormsActionElement::GetCurrentEvent(nsIDOMEvent **aEvent)
> > >+{
> > >+  *aEvent = mCurrentEvent;
> > Out parameters must be AddReffed.
> > >+NS_IMETHODIMP
> > >+nsXFormsActionModuleBase::GetCurrentEvent(nsIDOMEvent **aEvent)
> > >+{
> > >+  *aEvent = mCurrentEvent;
> > Same here.
> Have to investigate that. Maybe if they were addref'd here I wouldn't have to
> addref when I save the current event in mCurrentEvent.
> 
> > >+
> > >+  // The element to which the context info refers.
> > >+  nsIDOMElement *mElement;
> > >+  // Document to create nodes for String and Number types.
> > >+  nsCOMPtr<nsIDOMDocument> mContextDoc;
> > Could you use mElement->GetOwnerDocument here?
> 
> As opposed to creating my own stand-alone doc as is done now? Probably, and it
> would save a few lines of code and a bit of memory, but that 'doc' is used
> solely to create elements to encapsulate string and number types (because they
> have to be nsIDOMNodes to set via txINodeset. I don't really need or want them
> to be related to the owner document.
> 
> I know this patch is large but it is difficult to separate all the different
> context info into separate bugs. There are a lot of files affected because of
> things like having to modify a dozen files to save the current event - but they
> are all the same.
> 

I would vote to have own bug for this. I was confused too by that line and I'm sure everyone who will browse through cvs history will be confused too.

Comment 51

[alexander :surkov (:asurkov)]

Why do you need nsXFormsContextInfo::SetType if it used locally only?

Comment 52

[Merle Sterling]

(In reply to comment #51)
> Why do you need nsXFormsContextInfo::SetType if it used locally only?

We have to set mType to the type of the context property and you can either assign mType directly in each set method or call a SetType method to do it. SetType was originally public until I realized that it would not be a good idea to allow the caller to set the type.

Would you like to make the method inline?

Comment 53

[alexander :surkov (:asurkov)]

(In reply to comment #52)
> (In reply to comment #51)
> Would you like to make the method inline?
> 

I would like to get rid this method because I don't see a sense to keep it.

Comment 54

[alexander :surkov (:asurkov)]

Merle, you store all items of context info and context infos as members of xforms element. Why you can't use event member for this propose?

Btw, I don't see where you free context info before you gather them again on new HandleAction, probably I missed something.

Olli, usually you are sensible to memory usage, here we make growth an xforms elements (for example, nsXFormsInsertDeleteElement, additional 7 members). What did I miss?

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #54)
> Olli, usually you are sensible to memory usage, here we make growth an xforms
> elements (for example, nsXFormsInsertDeleteElement, additional 7 members).
Hmm, I was thinking about that at some point. Apparently forgot to mention.
Most of those member could be saved, I think, and just use local variables
and pass those as parameters to SetContextInfo().

Comment 56

[alexander :surkov (:asurkov)]

(In reply to comment #55)
> (In reply to comment #54)
> > Olli, usually you are sensible to memory usage, here we make growth an xforms
> > elements (for example, nsXFormsInsertDeleteElement, additional 7 members).
> Hmm, I was thinking about that at some point. Apparently forgot to mention.
> Most of those member could be saved, I think, and just use local variables
> and pass those as parameters to SetContextInfo().
> 

Why do we need SetContextInfo() at all? We can create context info when it's needed and add it into event. Right?

Comment 57

[Merle Sterling]

What you guys are missing is the fact that events are created all over the place in response to different circumstances and the information that needs to be set in the context info differs for each event and also does not exist in one place. That is why member variables are required to store the information as it becomes available.

So far I think the following changes should be made:
1. Put back the GARBAGE= line in the makefile.
2. Remove SetType method in favor of setting mType = whatever.

I'm out this week and when I return next week I will be here for two more weeks. Let's wrap this up.

Comment 58

[alexander :surkov (:asurkov)]

Right. I missed about event, thanks :). But why you can't store all you need in context info array which is member of xforms element? But as I mentioned before can you point me code where you clean up that array before next event?

Comment 59

[Merle Sterling]

(In reply to comment #58)
> Right. I missed about event, thanks :). But why you can't store all you need > in context info array which is member of xforms element?
We do store each of the context info objects in the context info array. What you are proposing, I think, is to create the contextInfo object at every point where some piece of context info becomes available and it to the array then as opposed to storing them and then setting them all at one shot in a single method. Why litter the code with creation of contextInfo objects all over the place? 

A method should do one task and SetContextInfo creates the contextInfo objects and sets everything in one shot.

> But as I mentioned before can you point me code where you clean up that > array before next event?

I don't. The array will be destroyed when the element is destroyed. Keep in mind that there is overlap between events as to which context info should be available. 

Comment 60

[alexander :surkov (:asurkov)]

(In reply to comment #59)
> (In reply to comment #58)
> > Right. I missed about event, thanks :). But why you can't store all you need > in context info array which is member of xforms element?
> We do store each of the context info objects in the context info array. What
> you are proposing, I think, is to create the contextInfo object at every point
> where some piece of context info becomes available and it to the array then as
> opposed to storing them and then setting them all at one shot in a single
> method. Why litter the code with creation of contextInfo objects all over the
> place? 

because of less of memory usage

> A method should do one task and SetContextInfo creates the contextInfo objects
> and sets everything in one shot.

Olli suggested to use local variables to keep contextInfo object creating in one place though probably it's not possible evrywhere but it would work fine for simple cases.

> > But as I mentioned before can you point me code where you clean up that > array before next event?
> 
> I don't. The array will be destroyed when the element is destroyed. Keep in
> mind that there is overlap between events as to which context info should be
> available. 
> 

It means you will accumulate context infos for every event, for example, you will add 'resource-uri' info for each unsuccessful loading of label resource (when I change 'src' attribute for example). Correct?

What do events overlaps means? Give me an example.

Comment 61

[Merle Sterling]

The issue of extra member variables only applies to submission and insert/delete elements. We have three choices:
1. Leave it the way I have it and consume a small extra amount of memory due to the extra member variables.
2. Create the contextInfo object and add it to the array at the point where the info is available rather than saving it in a member variable and adding it at the end.
3. Have 4 overloaded SetContextInfo methods and pass the data as parameters to them. We'd need 4 methods because the context info can be one of 4 types: number, string, node, or nodeset.

I don't like option 3 and I chose option 1 over option 2 because I wanted the setting of context info to occur one time just before you send the event that it applies to. I can change it to option 2 to save a bit of memory if desired but not all of the member variables can be eliminated for submission element.

> It means you will accumulate context infos for every event, for example, you
> will add 'resource-uri' info for each unsuccessful loading of label resource
> (when I change 'src' attribute for example). Correct?

No. 'resource-uri' applies to xforms-link-exceptions which are fatal or the uri for a submission, whether it is successful or not (submit-error and submit-done). In either case, processing is complete.

If you are really concerned about the possibility of adding the same context info to the array multiple times (even though I don't see a scenario where it would occur) we can clear the mContextInfo array but only if we leave it the way I have it with a single method to set the context info. Note that the contextInfo object that you get back from the event will never have multiple context info properties of the same name because it uses a hashtable and the last one wins.

I had to re-do a lot of the patch due to the if/while code being checked in. So if we can decide on whether to leave the extra member variables and the single SetContextInfo method (for submission and insert) or to remove it and do it all inline, I can wrap this up.

Comment 62

[Merle Sterling]

Attachment: patch

- Eliminated all of the extra member variables for insert element.
- Eliminated as many of the extra member variables as possible for submission element.
- Clear the mContextInfo array after dispatching the event (even though it is not necessary. :))

This patch is a bit smaller because a lot of it had to be re-written after the patch for if/while was checked in.

Comment 63

[alexander :surkov (:asurkov)]

Comment on attachment 291544 [details] [diff] [review]
patch


> 		nsIXFormsXPathState.idl \
>+                nsIXFormsDOMEvent.idl \
>+                nsIXFormsContextInfo.idl \
> 		$(NULL)

use tabs instead of spaces

> 		nsXFormsXPathState.cpp \
>+                nsXFormsDOMEvent.cpp \
>+                nsXFormsContextInfo.cpp \
> 		$(NULL)

here too
> [uuid(b19c47e4-2478-4a73-91a3-d86f91d91ffd)]
> interface nsIXFormsActionModuleElement : nsISupports

change uuid

>   void handleAction(in nsIDOMEvent aEvent, in nsIXFormsActionElement aParentAction);
>+  nsIDOMEvent getCurrentEvent();
> };

document this one.

Comment 64

[alexander :surkov (:asurkov)]

I'm not sure I like GetCurrentEvent() approach. I don't remember similar things in Gecko where events are stored somewhere. Sounds like more correct approach is to pass event into nsXFormsUtils::EvaluateXPath() (where it's needed) and then into nsXFormsXPathState object. What do you think?

Comment 65

[Merle Sterling]

Attachment: patch

- Spaces to tabs in Makefile.in
- New uuid for nsIXFormsActionModuleElement.idl

Comment 66

[alexander :surkov (:asurkov)]

Merle I didn't get about 'submission-body'. In nsXFormsSubmissionElement::HandleDefault you check it on !(' '), in submit you set ' '. Where can another value be setted?

Comment 67

[Merle Sterling]

(In reply to comment #66)
> Merle I didn't get about 'submission-body'. In
> nsXFormsSubmissionElement::HandleDefault you check it on !(' '), in submit you
> set ' '. Where can another value be setted?

The xforms-submit-serialize event is dispatched before actual serialization begins. The submission-body property of the context info is initially an empty text node. If the form modifies it using setvalue in an <xforms:action ev:event="xforms-submit-serialize">, then that node becomes the new serialization data; if it remains empty, the normal serialization rules are followed.

Before you ask, I cannot use EmptyString() because an empty string is zero length and the XPath code bails when the string is empty.

Comment 69

[alexander :surkov (:asurkov)]

Comment on attachment 291724 [details] [diff] [review]
patch

Ok, I do not see problems with the patch excepting I probably don't like a lot GetCurrentEvent approach. But it's not worth to stop the patch. If you will agree with the comment #64 then it would great if you'll file a bug.

Comment 70

[alexander :surkov (:asurkov)]

(In reply to comment #67)

> The xforms-submit-serialize event is dispatched before actual serialization
> begins. The submission-body property of the context info is initially an empty
> text node. If the form modifies it using setvalue in an <xforms:action
> ev:event="xforms-submit-serialize">, then that node becomes the new
> serialization data; if it remains empty, the normal serialization rules are
> followed.

If you would file testcase then it will be excellent.

Comment 71

[Merle Sterling]

Attachment: testcase: xforms-submit-serialize

Testcase for xforms-submit-serialize from W3C 1.1 testsuite.

Comment 72

[Merle Sterling]

Attachment: patch for check-in

Sync with trunk changes from the libxul patch (348391).

Comment 73

[aaronr]

checked into trunk for msterlin.

Merle, please attach a 1.8 version of the patch so that we can get this in our next release.  Thanks.

Comment 74

[Merle Sterling]

(In reply to comment #69)
> (From update of attachment 291724 [details] [diff] [review])
> Ok, I do not see problems with the patch excepting I probably don't like a lot
> GetCurrentEvent approach. But it's not worth to stop the patch. If you will
> agree with the comment #64 then it would great if you'll file a bug.

I don't think replacing the GetCurrentEvent approach will be useful. The XPath
event() function needs to know the current event so it can get the context info
from it. We can get the action module element from the xpath evaluation context
passed in to event() and the current event is stored in the action element.

If you have an idea on how to improve it would you be willing to open a bug and
implement the changes you have in mind? 

Comment 75

[Olli Pettay [:smaug][[e-mail]]]

Attachment: const strings

The previous patch broke my build. Adding |const| to few places helps.
Hope that works on Windows too.

Comment 76

[Merle Sterling]

The patch with const strings builds cleanly on Windows.

r=me

Comment 77

[aaronr]

since Merle couldn't actually put his r+ on the patch, I put mine on it.

Comment 78

[Olli Pettay [:smaug][[e-mail]]]

Checked in the |const| patch

Comment 79

[Joshua Cranmer [:jcranmer]]

This patch breaks on non-debug builds: the #ifdef DEBUG in nsXFormsSubmissionElement.cpp contains one closing brace too much.

Comment 80

[aaronr]

Attachment: fix optimization break

fixes optimization break

Comment 81

[aaronr]

Comment on attachment 296892 [details] [diff] [review]
fix optimization break

simple fix.  Feel free to checkin before I get in.

Comment 82

[aaronr]

checked in optimized build break patch to trunk

Comment 83

[Merle Sterling]

Attachment: patch for 1.8 branch

Port to 1.8 branch

Comment 84

[aaronr]

Comment on attachment 298912 [details] [diff] [review]
patch for 1.8 branch


>Index: transformiix/source/xpath/XFormsFunctions.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xpath/Attic/XFormsFunctions.h,v
>retrieving revision 1.2.12.1
>diff -u -8 -p -r1.2.12.1 XFormsFunctions.h
>--- transformiix/source/xpath/XFormsFunctions.h	24 Jun 2007 00:00:32 -0000	1.2.12.1
>+++ transformiix/source/xpath/XFormsFunctions.h	24 Jan 2008 12:14:37 -0000
>@@ -62,17 +62,18 @@ public:
>         INSTANCE,             // instance()
>         MAX,                  // max()
>         MIN,                  // min()
>         MONTHS,               // months()
>         NOW,                  // now()
>         PROPERTY,             // property()
>         SECONDS,              // seconds()
>         SECONDSFROMDATETIME,  // seconds-from-dateTime()
>-        CURRENT               // current()
>+        CURRENT,              // current()
>+        EVENT,                // event()
>     };

nit: I think we ran into a build break on linux or something when we ended an enum with ','.  Remove the ',' after EVENT, please.

>Index: transformiix/source/xpath/nsIXFormsUtilityService.h
>===================================================================

>-  NS_IMETHOD GetDaysFromDateTime(const nsAString & aValue, PRInt32 *aDays);
>+  NS_IMETHOD GetDaysFromDateTime(const nsAString & aValue, PRInt32 *aDays); \
>+  NS_IMETHOD GetContextInfo(const nsAString & aContextName, nsIDOMNode *aNode, nsCOMArray<nsIDOMNode> *aResult);

I think I would prefer a name like GetEventContextInfo since it is more descriptive as to what it actually does.


>+NS_IMETHODIMP
>+nsXFormsUtilityService::GetContextInfo(const nsAString & aContextName,
>+                                       nsIDOMNode      *aNode,
>+                                       nsCOMArray<nsIDOMNode> *aResult)
>+{

nit: just to keep the style of the file as consistent as possible, please align aNode with aContextName and leave aResult where it is.

>+  nsresult rv;
>+
>+  nsCOMPtr<nsIXFormsContextInfo> contextInfo;
>+  nsCOMPtr<nsIXFormsActionModuleElement> actionElt = do_QueryInterface(aNode);

please use the constructor way rather than the = assignment

>+  if (actionElt) {

shouldn't you exit the function if !actionElt?  Otherwise we'll crash the way you have things now.

>+    nsCOMPtr<nsIDOMEvent> domEvent;
>+    actionElt->GetCurrentEvent(getter_AddRefs(domEvent));
>+    nsCOMPtr<nsIXFormsDOMEvent> xfEvent = do_QueryInterface(domEvent);

please use constructor way rather than = assignment

Most of my comments are all pretty minor.  With these changes, r=me

Comment 85

[Merle Sterling]

Attachment: patch2 for 1.8 branch

Fixes Aaron's review comments. Olli, you really only have to check the transformiix changes for the Event function as the rest of the code is practically identical to the trunk patch.

Comment 86

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 299016 [details] [diff] [review]
patch2 for 1.8 branch

> /* starting interface:    nsIXFormsUtilityService */
> #define NS_IXFORMSUTILITYSERVICE_IID_STR "1e51f8fb-22e1-4d5d-a5f3-ab34e76efe8e"
> 
> #define NS_IXFORMSUTILITYSERVICE_IID \
>   {0x1e51f8fb, 0x22e1, 0x4d5d, \
>     { 0xa5, 0xf3, 0xab, 0x34, 0xe7, 0x6e, 0xfe, 0x8e }}
How have we handled xpath interface changes on branch?
You should update the IID, right? Or, create a new interface
nsIXFormsUtilityService_MOZILLA_1_8_BRANCH?

>+  rv = parser->ParseFromStream(mPipeIn, contentCharset.get(), contentLength,
>+                          contentType.get(), getter_AddRefs(newDoc));
align the 2nd line properly

>+  // Submission body from xforms-submit-serialize.
>+  nsIDOMNode*                      mSubmissionBody;
Hmm, I commented something about this already for trunk patch...
But is there an issue actually that mSubmissionBody is set to addrefed value and never released?
In that case the type should be nsCOMPtr<nsIDOMNode>

r- for now, until comments answered or addressed.

Comment 87

[Merle Sterling]

Attachment: patch3 for 1.8 branch

Addresses Olli's comments.

Comment 88

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

In @@ -410,47 +442,67 @@ nsXFormsSubmissionElement::OnStopRequest

>           else
>+            // replace="none"
>             rv = NS_OK;

This is generally a bad idea to do without {}. It's very easy to mistake that for a block rather than a single line and insert more stuff later.

Doesn't really matter much given that this is a branch patch though.

Comment 89

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 299671 [details] [diff] [review]
patch3 for 1.8 branch

Looks good otherwise, sorry about the lag :(

Comment 90

[aaronr]

None of these changes to transformiix will impact any xpath user outside of xforms since they can only be reached using an xforms xpath evaluator which normal xpath doesn't use.

Comment 91

[Daniel Veditz [:dveditz]]

Comment on attachment 299671 [details] [diff] [review]
patch3 for 1.8 branch

approved for 1.8.1.13, a=dveditz for release-drivers

Comment 92

[aaronr]

checked into branch for msterlin