388976 - [1.1] Support conditional actions (i.e. the `if` attribute)

Status: RESOLVED FIXED

(Core Graveyard :: XForms, enhancement)

Description

[John L. Clark]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5
Build Identifier: 0.8.0.3

XForms 1.1 supports a general-purpose `if` attribute on all actions.  This attribute is an XPath expression that is evaluated and interpreted as a boolean value before the parent action is performed.  If the value is false, then the action is not performed.  The reference to the specification (last call working draft as of this writing) is:

http://www.w3.org/TR/xforms11/#action-conditional

This bug report represents a request that this feature be implemented.

Reproducible: Always

Comment 1

[aaronr]

could you attach a testcase or two, please?

Comment 2

[John L. Clark]

Attachment: Simple test of `if` functionality

This test case is very simplistic, providing a basic test of the functionality.  If you need something more elaborate or less contrived, please ask and I will work on crafting one.

Comment 3

[Kostis Anagnostopoulos]

(without disregarding your great efforts)
I would like to see this RFE implemented, or else, 
a lot of JS code needed...

Comment 4

[John L. Clark]

Attachment: Patch to provide this feature

This patch implements the functionality described in this bug as well as that for bug 389554 (support for the `while` attribute), as the two pieces of functionality are strongly coupled and implementing them together was natural.  

There are a couple "interesting" things to note about this patch.  First, the patch changes references for node-set size and context position to use unsigned integers (PRUint32) instead of signed integers (PRInt32), which I believe is a safe change that creates more consistency across the various interfaces.  In working on this implementation, I noticed that for a variety of actions, iterating the action simply doesn't make sense.  (Either the action is idempotent or does not change the model.)  As a result, you will note that for these actions (for example `xf:rebuild` or `xf:message`), the presence of a `while` attribute does not cause that action to iterate (although its condition is still used as a test).  I intend to post to the XForms mailing list about this.

This patch enables the Mozilla XForms extension to pass the test cases posted in these bug reports, although I don't know if it introduces any regressions.  I will post more test cases to these bug reports as I work through some additional tests.

Comment 5

[alexander :surkov (:asurkov)]

John, you can move CanPerformAction check into nsXFormsActionModuleBase, right?

Comment 6

[John L. Clark]

Attachment: Alternative patch for this feature

Alexander and I had a good IRC discussion centered around his previous comment, and this patch attempts to implement this feature using the more object-oriented approach that he encouraged me to take.  In this approach, all actions are subclasses of `nsXFormsActionModuleBase`; instead of defining their own `HandleAction` method, actions define a `HandleSingleAction` method that implements a single iteration of a particular action.  This method is then managed by a master `HandleAction` method (defined in the `nsXFormsActionModuleBase` parent class).  This reorganization required the class definition changes that are present in this patch.

Two interesting cases are `nsXFormsInsertDeleteElement` and `nsXFormsMessageElement`.  The first is interesting because it sets up additional context before evaluating `if` and `while`, so it overrides the `HandleAction` method to get full control over the execution of its action.  The second is interesting because it is a subclass of both `nsXFormsActionModuleBase` and `nsXFormsDelegateStub`, which both descend from `nsXFormsStubElement`, so I had to make this last class a virtual superclass to avoid ambiguity.

This patch shares other characteristics of interest with the previous patch; see the comment on that patch for details.

Comment 7

[alexander :surkov (:asurkov)]

Btw, when you change interface you must change uuid.

If you file new patch then mark the previous one as obsolete

I would recommend to file new bug/patch for your interface changes. It will bring more easy cvs tracking and your patches will be easy for review.

Comment 8

[John L. Clark]

Attachment: Patch to change interfaces to use unsigned longs

This patch breaks out the changes to the interfaces and updates the UUIDs of the interfaces as requested by Alexander.  Note that the required updates to implementers of these interfaces are included in a separate patch (to be posted shortly).

Comment 9

[John L. Clark]

Attachment: Update to the alternative patch for this feature

This patch is the same as the "alternative patch" above, but with the changes to the interfaces divided into a separate patch.  As a result, this patch is dependent on that patch, attachment 285093 [details] [diff] [review].

Taken together, this patch set does not obsolete the first patch to this bug, as that patch provides an alternate implementation for this feature.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> Created an attachment (id=285093) [details]
> Patch to change interfaces to use unsigned longs
> 
> This patch breaks out the changes to the interfaces and updates the UUIDs of
> the interfaces as requested by Alexander.  Note that the required updates to
> implementers of these interfaces are included in a separate patch (to be posted
> shortly).
> 

Sorry, I probably was unclear. I meant first patch should be "First, the
patch changes references for node-set size and context position to use unsigned
integers (PRUint32) instead of signed integers (PRInt32), which I believe is a
safe change that creates more consistency across the various interfaces." And probably it's worth to have separate bug for this.

Comment 11

[John L. Clark]

Attachment: Update 2 to the alternative patch for this feature

After some discussion on IRC, the interface changes seemed like a bad idea.  This new patch provides a local solution to the problem instead of working to make the various interfaces consistent.  This means that there is no need to divide the patch into two components.

Comment 12

[aaronr]

So there are two patches marked for review.  We need both patches to fix this bug?

Comment 13

[John L. Clark]

No, the two patches still left up for review take alternative approaches.  I think the later one (attachment 285144 [details] [diff] [review]) is the better patch, but a reviewer might prefer the approach taken in the earlier patch (attachment 284533 [details] [diff] [review]).

Comment 14

[aaronr]

Comment on attachment 285144 [details] [diff] [review]
Update 2 to the alternative patch for this feature

My first comment is that this patch doesn't build on Windows.  Has a problem with you saying that HandleSingleAction returns nsresult in the headers but in the .cpp you say that it returns NS_IMETHODIMP.  Should probably change that to nsresult.

Also, keep in mind that xforms on the 1.8 branch doesn't have exactly the same inheritance chain.  You'll have to do a separate patch for the 1.8 branch once this one passes both reviews.  It might be worth taking a look at now so you can keep the code as common as possible between the trees.  But you don't need to submit it for review until the trunk one is ok'd.

Please use cvs diff -u8pN .... when you diff so that we get more context information in the patch.  Easier to find the part of the code that you are changing without actually applying the patch.

Apart from that, here are my other review comments.

>Index: extensions/xforms/nsXFormsActionModuleBase.cpp
>===================================================================

>+PRBool
>+nsXFormsActionModuleBase::CanPerformAction(PRBool     *usesWhile,
>+                                           nsIDOMNode *context,
>+                                           PRInt32     contextSize,
>+                                           PRInt32     contextPosition)
>+{
>+  *usesWhile = PR_FALSE;
>+
>+  nsresult rv;
>+  nsCOMPtr<nsIDOMXPathResult> res;
>+  PRBool condTrue;
>+
>+  nsCOMPtr<nsIDOMNode> contextNode;
>+
>+  if (context) {
>+    contextNode = do_QueryInterface(context);

you don't need the do_QueryInterface

>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }

the signature for this file is to return Boolean, but if the NS_ENSURE_SUCCESS fails, you'll be returning a nsresult.  You need to fix this in other places below, too.  Use if (NS_SUCCEEDED(rv)) to do the test instead.

>+
>+  nsAutoString ifExpr;
>+  nsAutoString whileExpr;
>+  mElement->GetAttribute(NS_LITERAL_STRING("if"), ifExpr);
>+  mElement->GetAttribute(NS_LITERAL_STRING("while"), whileExpr);
>+

should do the test for "if" and "while" before doing all the work of GetNodeContext.  They won't be there the vast majority of the time and if they aren't there, you can just return PR_TRUE.

>+  if (!whileExpr.IsEmpty()) {
>+    *usesWhile = PR_TRUE;
>+
>+    rv = nsXFormsUtils::EvaluateXPath(whileExpr, contextNode, mElement, 
>+                                      nsIDOMXPathResult::BOOLEAN_TYPE,
>+                                      getter_AddRefs(res),
>+                                      contextPosition, contextSize);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    
>+    rv = res->GetBooleanValue(&condTrue);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    if (!condTrue) {
>+      return PR_FALSE;
>+    }
>+  }
>+
>+  if (!ifExpr.IsEmpty()) {
>+    rv = nsXFormsUtils::EvaluateXPath(ifExpr, contextNode, mElement, 

nit: extra whitespace at end of line

>Index: extensions/xforms/nsXFormsActionModuleBase.h
>===================================================================

>+  /**
>+   * This signals whether or not this action can iterate.  Technically, all
>+   * XForms 1.1 actions are allowed to iterate, but for some of them it
>+   * simply doesn't make sense.
>+   */
>+  PRBool mCanIterate;
> };

I would argue that we should follow the spec unless we are trying to be compatible with a majority of implementations.  You can always open a holder bug to add this functionality once we get an answer from your question to the working group.  Please place a link to the query to the WG in the bug so that we can keep track of its progress.


>Index: extensions/xforms/nsXFormsInsertDeleteElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsInsertDeleteElement.cpp,v
>retrieving revision 1.19
>diff -u -p -r1.19 nsXFormsInsertDeleteElement.cpp
>--- extensions/xforms/nsXFormsInsertDeleteElement.cpp	3 Oct 2007 09:08:41 -0000	1.19
>+++ extensions/xforms/nsXFormsInsertDeleteElement.cpp	16 Oct 2007 20:53:44 -0000
>@@ -112,8 +112,15 @@ public:
> 
>   /** Constructor */
>   nsXFormsInsertDeleteElement(PRBool aIsInsert) :
>+    nsXFormsActionModuleBase(PR_TRUE),
>     mIsInsert(aIsInsert)
>     {}
>+protected:
>+  nsresult HandleSingleAction(nsIDOMEvent* aEvent,
>+                              nsIXFormsActionElement *aParentAction)
>+  {
>+    return NS_OK;
>+  }

Please comment here why you chose not to follow the normal flow of HandleAction calling HandleSingleAction.

If you keep the current structure where multiple insertions could happen yet still return an error, you need to put that comment somewhere, too.

>Index: extensions/xforms/nsXFormsMessageElement.cpp
>===================================================================

>@@ -262,6 +266,12 @@ nsXFormsMessageElement::WillChangeDocume
> }
> 
> NS_IMETHODIMP
>+nsXFormsMessageElement::DocumentChanged(nsIDOMDocument *aNewDocument)
>+{
>+  return nsXFormsControlStub::DocumentChanged(aNewDocument);
>+}
>+
>+NS_IMETHODIMP

Why did you need to add this override of DocumentChanged here?  Isn't nsXFormsControlStub where it would fall anyhow?

> nsXFormsMessageElement::OnDestroyed()
> {
>   mChannel = nsnull;
>@@ -440,12 +450,9 @@ nsXFormsMessageElement::DoneAddingChildr
> }
> 
> NS_IMETHODIMP
>-nsXFormsMessageElement::HandleAction(nsIDOMEvent* aEvent, 
>-                                     nsIXFormsActionElement *aParentAction)
>+nsXFormsMessageElement::HandleSingleAction(nsIDOMEvent* aEvent, 

nit: extra whitespace at end of the line

Comment 15

[John L. Clark]

(In reply to comment #14)
> I would argue that we should follow the spec unless we are trying to be
> compatible with a majority of implementations.  You can always open a holder
> bug to add this functionality once we get an answer from your question to the
> working group.  Please place a link to the query to the WG in the bug so that
> we can keep track of its progress.

Actually, after I mentioned sending a query to the WG, I forgot to do it, so I did so just now.  Here's the first message:

  http://lists.w3.org/Archives/Public/www-forms-editor/2007Nov/0004.html

The new patch (which I will post after this message) will turn iteration on for all actions.

Comment 16

[John L. Clark]

Attachment: Update 3 to the alternative patch for this feature

Aaron,

Thank you for your review of the previous patch.  This new patch fixes the problems you mentioned and includes comments that hopefully answer the questions that you raised.

Comment 17

[aaronr]

Comment on attachment 288360 [details] [diff] [review]
Update 3 to the alternative patch for this feature


>Index: extensions/xforms/nsXFormsActionModuleBase.cpp
>===================================================================

>+
>+PRBool
>+nsXFormsActionModuleBase::CanPerformAction(PRBool     *usesWhile,
>+                                           nsIDOMNode *context,
>+                                           PRInt32     contextSize,
>+                                           PRInt32     contextPosition)
>+{

nit: parameters to a function in Mozilla should start with 'a' (for argument).  So change *usesWhile to *aUsesWhile, etc. please.


>+  nsCOMPtr<nsIDOMNode> contextNode;
>+
>+  if (context) {
>+    //contextNode = do_QueryInterface(context);
>+    contextNode = context;
>+  }

nit: remove commented out code since we don't need it.

with those, r=me

Comment 18

[John L. Clark]

Attachment: Update 4 to the alternative patch for this feature

This patch should address the issues from Aaron's latest review.

Comment 19

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

Comment on attachment 288709 [details] [diff] [review]
Update 4 to the alternative patch for this feature

>+nsXFormsActionModuleBase::HandleAction(nsIDOMEvent            *aEvent,
...
>+  nsAutoString elementName;
>+  mElement->GetNodeName(elementName);
You aren't using elementName anywhere?

>+  // Repeat this action if it can iterate and if it uses the `while`
>+  // attribute (which must have evaluated to true to arrive here).
>+  if (mCanIterate && usesWhile) {
>+    return HandleAction(aEvent, aParentAction);
>+  }
So you're iterating in stack in which case stack overflow happens pretty easily.
Please change recursion to iteration.

>+nsXFormsActionModuleBase::CanPerformAction(PRBool     *aUsesWhile,
...
>+  if (aContext) {
>+    contextNode = aContext;
>+  }
>+  else {
Nit, I'd prefer |else| in the same line as |}|, so:
if (expr) {

} else {

}

>+  /**
>+   * Determine whether this action element should be executed, based upon
>+   * optional `if` and `while` attributes.  For each of these attributes
>+   * that are present on an action element, the action is only performed if
>+   * the boolean value of the XPath expression contained in the attribute is
>+   * true.  In addition, if the `while` attribute is used, the action is
>+   * "executed repeatedly" until one of these attributes evaluates to false.
>+   * This method indicates to the caller whether the action element uses a
>+   * `while` attribute through the `usesWhile` parameter.
>+   */
Hmm, we need to have some kind of limit for "while", to prevent endless loops and
DoS, right?

Comment 20

[John L. Clark]

Attachment: Update 5 to the alternative patch for this feature

This patch should address the issues from Olli's latest review.

Comment 21

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

Comment on attachment 289507 [details] [diff] [review]
Update 5 to the alternative patch for this feature

>+  PRInt32 seconds = 10;
>+  nsCOMPtr<nsIPrefBranch> prefBranch =
>+    do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+  if (NS_SUCCEEDED(rv) && prefBranch) {
>+    rv = prefBranch->GetIntPref("dom.max_script_run_time", &seconds);
>+  }
>+  PRTime microseconds = seconds * PR_USEC_PER_SEC;
>+
>+  PRTime runTime = 0, start = PR_Now();
>+  PRBool ignoreTimeLimit = PR_FALSE;
What is ignoreTimeLimit? Where is it used or set to PR_TRUE?

Could you upload some testcases for the loop handling?

Comment 22

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

Comment on attachment 289507 [details] [diff] [review]
Update 5 to the alternative patch for this feature

>+  PRInt32 seconds = 10;
>+  nsCOMPtr<nsIPrefBranch> prefBranch =
>+    do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+  if (NS_SUCCEEDED(rv) && prefBranch) {
>+    rv = prefBranch->GetIntPref("dom.max_script_run_time", &seconds);
>+  }
You might want to cache the value somewhere in nsXFormsUtils, like the other pref(s)

Comment 23

[John L. Clark]

Attachment: Update 6 to the alternative patch for this feature

This patch should address the issues from Olli's latest review.

Comment 24

[John L. Clark]

(In reply to comment #21)
> Could you upload some testcases for the loop handling?

There are a couple of test cases attached to bug 389554.  I had initially divided the `if` and `while` functionality into separate bug reports, only to discover later that the solutions to both bugs would be tightly coupled.  If there is a good way to merge the two bug reports, let me know.

Comment 25

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

Comment on attachment 290389 [details] [diff] [review]
Update 6 to the alternative patch for this feature

>+    else if (strcmp(aPref, "dom.max_script_run_time") == 0) {
>+      PRInt32 val;
>+      if (NS_SUCCEEDED(pref->GetIntPref("dom.max_script_run_time", &val))) {
>+        nsXFormsUtils::maxScriptRunTime = val;
>+      }
>+    }
...
>+    PRInt32 intval;
>+    rv = prefBranch->GetIntPref("dom.max_script_run_time", &intval);
>+    if (NS_SUCCEEDED(rv)) {
>+      nsXFormsUtils::maxScriptRunTime = intval;
>+    }

One nit, define some macro which you use instead of "dom.max_script_run_time"
r=me

Comment 26

[John L. Clark]

Attachment: Update 7 to the alternative patch for this feature

This patch should address the nit from Olli's latest review.

Comment 27

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

checked in

Comment 28

[John L. Clark]

Attachment: Version of final patch for branch MOZILLA_1_8_BRANCH

This is a port of the final patch (attachment 290532 [details] [diff] [review]) to Mozilla CVS branch MOZILLA_1_8_BRANCH.

Comment 29

[aaronr]

Comment on attachment 296018 [details] [diff] [review]
Version of final patch for branch MOZILLA_1_8_BRANCH

I see a couple of other bug fixes mixed up in your patch that don't belong, like the patches for 391586 and 394023.  What we need is a patch that is exactly like your trunk patch, just ported to 1.8.  We can't have anything extra in there otherwise when we go to apply your patch to the 1.8 branch that will already contain those fixes we'll get conflicts.

If you patch absolutely requires these other patches, let me know and I'll get them checked into 1.8 and then you can build your patch off of that.

Comment 30

[John L. Clark]

Attachment: Update 1 for version of patch for branch MOZILLA_1_8_BRANCH

This version of the patch should address Aaron's review comments.

Comment 31

[aaronr]

Comment on attachment 296175 [details] [diff] [review]
Update 1 for version of patch for branch MOZILLA_1_8_BRANCH

Sorry, you won't be able to just change the inheritance hierarchy of nsXFormsActionElement that easily.  Look at bug 377880 for the reason that it inherits from the binding side of things.  With your current patch, the testcase attached to bug 377880 will now fail.

Comment 32

[John L. Clark]

Attachment: Update 2 for version of patch for branch MOZILLA_1_8_BRANCH

This patch should address Aaron's most recent comments.

Comment 33

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

Attachment: endless loop with insert/delete

Comment 34

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

Attachment: Remove virtual inheritance

This patch removes virtual inheritance by introducing nsXFormsActionModuleHelper
and using static helper methods DoHandleAction and CanPerformAction.
Fixes also the endless loop problem with Insert/Delete using the
same technique as in DoHandleAction.
I did the patch by first backing out the previous changes and then adding back 
those parts which didn't change the class hierarchy.
IMO, with this patch the class hierarchy stays easier to understand.
And John, I'm *really* sorry that I didn't think about this when reviewing your patch.

I'll attach also a -w patch, which is perhaps easier to review, because it shows
that changes to Insert/Delete are actually pretty small.

Comment 35

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

Attachment: -w

Comment 36

[John L. Clark]

(In reply to comment #34)
> Created an attachment (id=296703) [details]
> Remove virtual inheritance

>Index: extensions/xforms/nsXFormsActionElement.cpp
>===================================================================
...
>+nsXFormsActionElement::HandleAction(nsIDOMEvent* aEvent,
>+                                    nsIXFormsActionElement *aParentAction)

Nit: use consistent pointer syntax; it seems like `type *aThing` is more
common.  This applies to other aspects of the patch as well.

>Index: extensions/xforms/nsXFormsInsertDeleteElement.cpp
>===================================================================
...
>+    rv = nsXFormsUtils::GetNodeContext(mElement,
...
>+    mElement->GetAttribute(NS_LITERAL_STRING("bind"), bindExpr);
>+    mElement->GetAttribute(NS_LITERAL_STRING("context"), contextExpr);
...
>+      rv = nsXFormsUtils::EvaluateXPath(contextExpr, contextNode, mElement,

Did you want to use `element` in these places instead of `mElement` (and
throughout the same method)?

>+    // Test the `if` and `while` attributes to determine whether this action
>+    // can be performed and should be repeated.  As the insert and delete
>+    // actions can change their context, we do this testing here instead of
>+    // relying on the default logic from our parent class' `HandleAction`
>+    // definition.
>+    PRBool usesWhile;
>+    if (!nsXFormsActionModuleBase::CanPerformAction(mElement, &usesWhile,
>+                                                    contextNode,
>+                                                    contextNodesetSize,
>+                                                    contextPosition)) {

Please update the comment to reflect the new interaction structure.

With these comments addressed, r=me.

Thanks for cleaning up after me, in particular for catching the
inconsistency in the implementation of `xf:insert`/`xf:delete` with respect
to the other actions.  Also, thanks for the `-w` patch; that is certainly easier to read, and I didn't know about that option before!

Comment 37

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

> Did you want to use `element` in these places instead of `mElement` (and
> throughout the same method)?
Doesn't really matter because mElement points to the valid object while
element is alive.

> Please update the comment to reflect the new interaction structure.
Will update the comment to talk about DoHandleAction etc.

Comment 38

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

> >+nsXFormsActionElement::HandleAction(nsIDOMEvent* aEvent,
> >+                                    nsIXFormsActionElement *aParentAction)
> 
> Nit: use consistent pointer syntax; it seems like `type *aThing` is more
> common.  This applies to other aspects of the patch as well.
We aren't very consistent, but sure, I should fix this case.
I usually prefer nsObjType* aParamName

Comment 39

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

Attachment: v2

Comment 40

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

Attachment: v2 -w

Comment 41

[aaronr]

Comment on attachment 296545 [details] [diff] [review]
Update 2 for version of patch for branch MOZILLA_1_8_BRANCH

removing old review request since new approach is going to be used.

Comment 42

[aaronr]

Comment on attachment 297055 [details] [diff] [review]
v2


>Index: extensions/xforms/nsXFormsActionModuleBase.cpp
>===================================================================

>+/* static */ nsresult
>+nsXFormsActionModuleBase::DoHandleAction(nsXFormsActionModuleHelper* aXFormsAction,
>+                                         nsIDOMEvent*                aEvent,
>+                                         nsIXFormsActionElement*     aParentAction)
>+{

nit: the *'s in this file seem to all be right next to the parameter names and not next to their types.

>Index: extensions/xforms/nsXFormsActionModuleBase.h
>===================================================================

nit: it would be nice to have some comments here about why

nsXFormsActionModuleHelper is needed and how it fits into things.

nit: it would be nice to have some comments here about how nsXFormsActionModuleBase fits into things and what types of actions should inherit from it and which shouldn't (if any).  Just afraid without some comments here and above in this header file we'll have to re-figure it out every time we go to modify this file.  I should have made this nit in the previous reviews.

>+class nsXFormsActionModuleBase : public nsIDOMEventListener,
>+                                 public nsXFormsStubElement,
>+                                 public nsIXFormsActionModuleElement,
>+                                 public nsXFormsActionModuleHelper
>+{

>+  static nsresult DoHandleAction(nsXFormsActionModuleHelper* aXFormsAction,
>+                                 nsIDOMEvent*                aEvent,
>+                                 nsIXFormsActionElement*     aParentAction);

nit: consistency with the *'s again compared to what is already in this file.  I don't care which way it goes as long as it is consistent within the file.

nice change in insert/delete.  r=me

Comment 43

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

May not have time to land this before Thursday, or in worst case Friday.

Comment 44

[aaronr]

(In reply to comment #43)
> May not have time to land this before Thursday, or in worst case Friday.
> 

I'll get it checked in tonight sometime.  John, please get a 1.8 version of the patch ready for review as soon as you can unless you want Merle to get his context patch into 1.8 first.  Since you are both hitting the action stuff, one of you needs to go first.

Comment 45

[aaronr]

Attachment: final non-virtual patch

fixes my nits.  This is what I'm checking into the trunk.

Comment 46

[aaronr]

(In reply to comment #45)
> Created an attachment (id=297410) [details]
> final non-virtual patch
> 
> fixes my nits.  This is what I'm checking into the trunk.
> 

checked into the trunk

Comment 47

[John L. Clark]

Attachment: Initial combined patch for branch MOZILLA_1_8_BRANCH

This is a port, to the MOZILLA_1_8_BRANCH branch, of the combined work to implement this feature on the trunk, including Olli and Aaron's updates to my initial patch.

Comment 48

[aaronr]

Comment on attachment 298341 [details] [diff] [review]
Initial combined patch for branch MOZILLA_1_8_BRANCH

>Index: extensions/xforms/nsXFormsActionElement.cpp
>===================================================================

> 
>+nsXFormsActionElement::nsXFormsActionElement() : mElement(nsnull)
>+{
>+}
>+

Ummm, do you need this?  nsXFormsActionElement inherits from nsXFormsBindableControlStub which inherits from nsXFormsControlStubBase which contains mElement.  So doesn't nsXFormsActionElement already have mElement?

>Index: extensions/xforms/nsXFormsActionElement.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsActionElement.h,v
>retrieving revision 1.2.12.3
>diff -u -8 -p -r1.2.12.3 nsXFormsActionElement.h
>--- extensions/xforms/nsXFormsActionElement.h	1 May 2007 16:28:14 -0000	1.2.12.3
>+++ extensions/xforms/nsXFormsActionElement.h	21 Jan 2008 22:15:40 -0000
>@@ -44,30 +44,35 @@
> #include "nsDataHashtable.h"
> #include "nsXFormsControlStub.h"
> 
> class nsIXTFBindableElementWrapper;
> 
> class nsXFormsActionElement : public nsXFormsBindableControlStub,
>                               public nsIXFormsActionElement,
>                               public nsIXFormsActionModuleElement,
>-                              public nsIDOMEventListener
>+                              public nsIDOMEventListener,
>+                              public nsXFormsActionModuleHelper
> {
> public:
>-
>+  nsXFormsActionElement();

probably don't need this

> private:
>+  nsIDOMElement*                                mElement;

probably don't need

>Index: extensions/xforms/nsXFormsActionModuleBase.h
>===================================================================

> class nsXFormsActionModuleBase : public nsIDOMEventListener,
>                                  public nsXFormsStubElement,
>-                                 public nsIXFormsActionModuleElement
>+                                 public nsIXFormsActionModuleElement,
>+                                 public nsXFormsActionModuleHelper
> {
> public:
>   nsXFormsActionModuleBase();
>   virtual ~nsXFormsActionModuleBase();
>   NS_DECL_ISUPPORTS_INHERITED
>-  NS_DECL_NSIXTFGENERICELEMENT
>+  NS_DECL_NSIXFORMSACTIONMODULEELEMENT
>   NS_DECL_NSIDOMEVENTLISTENER
>+  NS_IMETHOD OnCreated(nsIXTFGenericElementWrapper *aWrapper);

why remove NS_DECL_NSIXTFGENERICELEMENT and then add signature for OnCreated?  Isn't that the same thing?

 
>Index: extensions/xforms/nsXFormsActionModuleBase.cpp
>===================================================================

>+/* static */ nsresult
>+nsXFormsActionModuleBase::DoHandleAction(nsXFormsActionModuleHelper *aXFormsAction,
>+                                         nsIDOMEvent                *aEvent,
>+                                         nsIXFormsActionElement     *aParentAction)
>+{
>+  nsCOMPtr<nsIDOMElement> element = aXFormsAction->GetElement();
>+  NS_ENSURE_STATE(element);
>+

You should probably return NS_OK if !element rather than returning NS_ERROR_UNEXPECTED.  If you notice most of the HandleActions that you are removing in your patch, they just returned NS_OK if !mElement.  There are, if I remember correctly, timing places where mElement isn't set up, yet.  I don't think we want to return an error in those cases.

>Index: extensions/xforms/nsXFormsInsertDeleteElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsInsertDeleteElement.cpp,v
>retrieving revision 1.3.2.10
>diff -u -8 -p -r1.3.2.10 nsXFormsInsertDeleteElement.cpp
>--- extensions/xforms/nsXFormsInsertDeleteElement.cpp	9 Jan 2008 02:47:30 -0000	1.3.2.10
>+++ extensions/xforms/nsXFormsInsertDeleteElement.cpp	21 Jan 2008 22:15:40 -0000
>@@ -103,542 +103,605 @@ private:
>    *  @return aResult        result node
>    */
>   nsresult InsertNode(nsIDOMNode *aTargetNode, nsIDOMNode *aNewNode,
>                       Location aLocation, nsIDOMNode **aResNode);
> 
>   nsresult RefreshRepeats(nsCOMArray<nsIDOMNode> *aNodes);
> 
> public:
>-  NS_DECL_NSIXFORMSACTIONMODULEELEMENT
>-
>   /** Constructor */
>   nsXFormsInsertDeleteElement(PRBool aIsInsert) :
>     mIsInsert(aIsInsert)
>     {}
>+
>+  NS_IMETHOD
>+  HandleAction(nsIDOMEvent* aEvent, nsIXFormsActionElement *aParentAction);
>+

why remove NS_DECL_NSIXFORMSACTIONMODULEELEMENT and add signature for HandleAction?  Isn't that the same thing?

>Index: extensions/xforms/nsXFormsSetIndexElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSetIndexElement.cpp,v
>retrieving revision 1.2.8.3
>diff -u -8 -p -r1.2.8.3 nsXFormsSetIndexElement.cpp
>--- extensions/xforms/nsXFormsSetIndexElement.cpp	25 Apr 2007 19:41:22 -0000	1.2.8.3
>+++ extensions/xforms/nsXFormsSetIndexElement.cpp	21 Jan 2008 22:15:40 -0000
>@@ -55,27 +55,29 @@
>  * To set the index for a \<repeat\>, we find the (DOM) \<repeat\> with the
>  * given @id, and call nsIXFormsRepeatElement::SetIndex() on it.
>  *
>  * @see http://www.w3.org/TR/xforms/slice9.html#action-setRepeatCursor
>  */
> class nsXFormsSetIndexElement : public nsXFormsActionModuleBase
> {
> public:
>-  NS_DECL_NSIXFORMSACTIONMODULEELEMENT
>+  nsXFormsSetIndexElement();

why did you need to add the default constructor?

>Index: extensions/xforms/nsXFormsToggleElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsToggleElement.cpp,v
>retrieving revision 1.1.18.2
>diff -u -8 -p -r1.1.18.2 nsXFormsToggleElement.cpp
>--- extensions/xforms/nsXFormsToggleElement.cpp	25 Apr 2007 19:41:22 -0000	1.1.18.2
>+++ extensions/xforms/nsXFormsToggleElement.cpp	21 Jan 2008 22:15:40 -0000
>@@ -46,26 +46,29 @@
> 
> /**
>  * XForms Toggle element. According to the the XForms Schema it can be handled
>  * like Action Module elements.
>  */
> class nsXFormsToggleElement : public nsXFormsActionModuleBase
> {
> public:
>-  NS_DECL_NSIXFORMSACTIONMODULEELEMENT
>+  nsXFormsToggleElement();

why do you need the default constructor now?


r-'ing because I didn't understand why some of these changes were made.  But most of them are pretty minor.

Comment 49

[John L. Clark]

Attachment: Update 1 of combined patch for branch MOZILLA_1_8_BRANCH

This version of the patch should address Aaron's comments.

Comment 50

[John L. Clark]

Attachment: Update 2 of combined patch for branch MOZILLA_1_8_BRANCH

Update 1 had some extra chunks that were unneeded.  This version of the patch cleans those up.  Sorry about that.

Comment 51

[aaronr]

Comment on attachment 298470 [details] [diff] [review]
Update 2 of combined patch for branch MOZILLA_1_8_BRANCH

>Index: extensions/xforms/nsXFormsActionElement.cpp
>===================================================================

> NS_IMETHODIMP
> nsXFormsActionElement::OnCreated(nsIXTFBindableElementWrapper* aWrapper)
> {
>   nsresult rv = nsXFormsBindableControlStub::OnCreated(aWrapper);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  // It's ok to keep a weak pointer to mElement.  mElement will have an
>+  // owning reference to this object, so as long as we null out mElement in
>+  // OnDestroyed, it will always be valid.
>+  nsCOMPtr<nsIDOMElement> node;
>+  aWrapper->GetElementNode(getter_AddRefs(node));
>+  mElement = node;
>+  NS_ASSERTION(mElement, "Wrapper is not an nsIDOMElement, we'll crash soon");
>+

won't mElement get assigned in nsXFormsControlStubBase::Create already when it is called from nsXFormsBindableControlStub::OnCreated?

>Index: extensions/xforms/nsXFormsActionModuleBase.cpp
>===================================================================

>+/* static */ nsresult
>+nsXFormsActionModuleBase::DoHandleAction(nsXFormsActionModuleHelper *aXFormsAction,
>+                                         nsIDOMEvent                *aEvent,
>+                                         nsIXFormsActionElement     *aParentAction)
>+{
>+  nsCOMPtr<nsIDOMElement> element = aXFormsAction->GetElement();
>+  NS_ENSURE_TRUE(element, NS_OK);

I'd rather you just use an 'if' and return NS_OK if !element.  Using NS_ENSURE_TRUE will throw a warning to the console.  And we try to keep it so that only real failures are reported to the console otherwise if there is extraneous stuff we'll just ignore all warnings and probably miss something important.

>Index: extensions/xforms/nsXFormsActionModuleBase.h
>===================================================================

> class nsXFormsActionModuleBase : public nsIDOMEventListener,
>                                  public nsXFormsStubElement,
>-                                 public nsIXFormsActionModuleElement
>+                                 public nsIXFormsActionModuleElement,
>+                                 public nsXFormsActionModuleHelper
> {
> public:
>   nsXFormsActionModuleBase();
>   virtual ~nsXFormsActionModuleBase();
>   NS_DECL_ISUPPORTS_INHERITED
>+  NS_DECL_NSIXFORMSACTIONMODULEELEMENT
>   NS_DECL_NSIXTFGENERICELEMENT
>   NS_DECL_NSIDOMEVENTLISTENER
>+  //NS_IMETHOD OnCreated(nsIXTFGenericElementWrapper *aWrapper);

nit: you probably shouldn't leave this commented out.  If we don't need it, chuck it.

>Index: extensions/xforms/nsXFormsMessageElement.cpp
>===================================================================
> 
> NS_IMETHODIMP
>-nsXFormsMessageElement::HandleAction(nsIDOMEvent* aEvent, 
>+nsXFormsMessageElement::HandleAction(nsIDOMEvent *aEvent, 
>                                      nsIXFormsActionElement 

nit: please remove the extraneous space after aEvent (I know that you didn't do it, but since you are in the code anyhow).

With those changes, r=me

Comment 52

[John L. Clark]

Attachment: Update 3 of combined patch for branch MOZILLA_1_8_BRANCH

This version of the patch should address Aaron's latest comments.

Comment 53

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

Comment on attachment 298503 [details] [diff] [review]
Update 3 of combined patch for branch MOZILLA_1_8_BRANCH

>+++ extensions/xforms/nsXFormsInsertDeleteElement.cpp	22 Jan 2008 18:47:11 -0000
...>   NS_DECL_NSIXFORMSACTIONMODULEELEMENT
>-
>+ 

Nit, extra space

Comment 54

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

Comment on attachment 298503 [details] [diff] [review]
Update 3 of combined patch for branch MOZILLA_1_8_BRANCH

>+++ extensions/xforms/nsXFormsInsertDeleteElement.cpp	22 Jan 2008 18:47:11 -0000
...>   NS_DECL_NSIXFORMSACTIONMODULEELEMENT
>-
>+ 

Nit, extra space

Comment 55

[aaronr]

removed extra space and checked into 1.8 branch for John

Comment 56

[aaronr]

Comment on attachment 284533 [details] [diff] [review]
Patch to provide this feature

marking an old version of the patch obsolete and removing the review request for it.