334018 - index() with no repeat should return NaN

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Steve Speicher]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060411 Firefox/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060411 Firefox/3.0a1

See 1.0 2nd ed test suite test case 7.8.5.b.  According to section 7.8.5 "If the specified argument does not identify a repeat, the function returns NaN."

Reproducible: Always

Comment 1

[Steve Speicher]

Attachment: testcase

Comment 2

[aaronr]

The rub about this is whether we can get it into transformiix on the branches.  I'm looking into if it is possible to return NaN from nsXFormsUtilityService::GetRepeatIndex, but since NaN is a double in transformiix, I don't think I can without a change to transformiix anyhow.

Comment 3

[aaronr]

talked to Jonas.  Getting a change into transformiix in 1.8.0 will be difficult.  I think that we can work around this by returning an error from GetRepeatIndex that will propogate back through xpath to us (in nsXFormsUtils::EvaluateXPath, I think).  We can check the return code and if it is NS_XFORMS_GENERATE_NAN or something like that, then we'll have to build an xpath result that is the same as NaN (or force an xpath evaluation that we know will result in a NaN and return that.

Comment 4

[aaronr]

Attachment: testcase 2

original testcase plus test for index using non-repeat node's ID.

Comment 5

[aaronr]

Attachment: patch for trunk

Here is the most correct approach...generating the NaN xpathresult in the xforms xpath source code.  This is the patch for the trunk.  The fixes for the branches will be exactly the same but in a different file since the directory structure for xpath is different between trunk and the 1.8 and 1.8.0 branches.

If this fix (branches version) isn't accepted for both branches, then we should probably scrap this approach altogether and go with the rv approach where we'll propogate a error back through xpath to xforms and then have xforms generate the NaN.  To keep the xforms code common between all branches and the trunk.

Comment 6

[aaronr]

Attachment: patch for 1.8.x branches

patch for the 1.8.x branches.  Same code as the trunk patch.  Just in the correct files for 1.8.x branches.

Comment 7

[aaronr]

Just to be clear, while this change is in the xpath code, the changed code can only be called by the xforms xpath evaluator and thus only used by the xforms extension.

Comment 8

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

Comment on attachment 218560 [details] [diff] [review]
patch for trunk

I don't like the idea of turning all errors into NaN.

For example if instantiating the xformsService fails you probably want to propagate that error.

One solution might be to make GetRepeatIndex return a |PRInt32| instead and make -1 indicate that NaN should be returned.

Of course, you could also simply make it return a double.

Comment 9

[aaronr]

(In reply to comment #8)
> (From update of attachment 218560 [details] [diff] [review] [edit])
> I don't like the idea of turning all errors into NaN.
> 
> For example if instantiating the xformsService fails you probably want to
> propagate that error.
> 
> One solution might be to make GetRepeatIndex return a |PRInt32| instead and
> make -1 indicate that NaN should be returned.
> 
> Of course, you could also simply make it return a double.
> 


If instantiating the xformsService fails, we do propogate that error.  We just won't propogate any error from GetRepeatIndex.  But that is fine.  I can change the signature of GetRepeatIndex to PRUInt32 and return a -1 to trigger the NaN.

Comment 10

[aaronr]

Attachment: trunk patch with -1 return

patch with the -1 return talked about earlier

Comment 11

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

Comment on attachment 220088 [details] [diff] [review]
trunk patch with -1 return

>+nsXFormsUtilityService::GetRepeatIndex(nsIDOMNode *aRepeat, PRInt32 *aIndex)
...
>-  /// 
>-  /// @bug This should somehow end up in a NaN per the XForms 1.0 Errata (XXX)
>-  return repeatEle ? repeatEle->GetIndex(aIndex) : NS_OK;
>+  return rv;

Change this to |return NS_OK;|

With that, r=me

Comment 12

[aaronr]

Attachment: trunk patch with jonas fix

updated patch with jonas' comment fixed.

Comment 13

[aaronr]

Comment on attachment 220135 [details] [diff] [review]
trunk patch with jonas fix

If we get peterv's signoff for the trunk change, then I'll port the same patch for 1_8_BRANCH and 1_8_0_BRANCH, attach it, and ask approvals.

Comment 14

[Allan Beaufour]

(In reply to comment #13)
> (From update of attachment 220135 [details] [diff] [review] [edit])
> If we get peterv's signoff for the trunk change, then I'll port the same patch
> for 1_8_BRANCH and 1_8_0_BRANCH, attach it, and ask approvals.

I just checked it in on trunk.

Comment 15

[Allan Beaufour]

Comment on attachment 220135 [details] [diff] [review]
trunk patch with jonas fix

Requesting approval1.8.0.5. 

Although this patch includes an interface change, it is only used by XForms (and we live in our own little parallel world :-) ).

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 220135 [details] [diff] [review]
trunk patch with jonas fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 17

[Doron Rosenberg (IBM)]

checked into 1.8.0 for aaron

Comment 18

[aaronr]

checked into 1.8.1