Last Comment Bug 334018 - index() with no repeat should return NaN
: index() with no repeat should return NaN
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: aaronr
: Stephen Pride
:
Mentors:
http://www.w3.org/MarkUp/Forms/Test/X...
Depends on:
Blocks: 322255
  Show dependency treegraph
 
Reported: 2006-04-14 10:19 PDT by Steve Speicher
Modified: 2016-07-15 14:46 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase (893 bytes, application/xhtml+xml)
2006-04-14 10:20 PDT, Steve Speicher
no flags Details
testcase 2 (1.21 KB, application/xhtml+xml)
2006-04-15 15:18 PDT, aaronr
no flags Details
patch for trunk (2.88 KB, patch)
2006-04-15 15:38 PDT, aaronr
jonas: review-
allan: review+
Details | Diff | Splinter Review
patch for 1.8.x branches (2.68 KB, patch)
2006-04-15 15:55 PDT, aaronr
no flags Details | Diff | Splinter Review
trunk patch with -1 return (6.31 KB, patch)
2006-04-27 18:21 PDT, aaronr
jonas: review+
Details | Diff | Splinter Review
trunk patch with jonas fix (6.29 KB, patch)
2006-04-28 09:49 PDT, aaronr
allan: review+
jonas: superreview+
jonas: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review

Description Steve Speicher 2006-04-14 10:19:48 PDT
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 2006-04-14 10:20:14 PDT
Created attachment 218444 [details]
testcase
Comment 2 aaronr 2006-04-14 12:09:23 PDT
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 2006-04-14 17:39:10 PDT
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 2006-04-15 15:18:43 PDT
Created attachment 218557 [details]
testcase 2

original testcase plus test for index using non-repeat node's ID.
Comment 5 aaronr 2006-04-15 15:38:55 PDT
Created attachment 218560 [details] [diff] [review]
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 2006-04-15 15:55:44 PDT
Created attachment 218561 [details] [diff] [review]
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 2006-04-15 16:01:58 PDT
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 2006-04-20 15:30:53 PDT
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 2006-04-20 16:34:51 PDT
(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 2006-04-27 18:21:03 PDT
Created attachment 220088 [details] [diff] [review]
trunk patch with -1 return

patch with the -1 return talked about earlier
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-27 21:07:14 PDT
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 2006-04-28 09:49:39 PDT
Created attachment 220135 [details] [diff] [review]
trunk patch with jonas fix

updated patch with jonas' comment fixed.
Comment 13 aaronr 2006-05-04 13:18:04 PDT
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 2006-05-29 01:45:00 PDT
(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 2006-05-29 02:34:02 PDT
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] 2006-06-12 12:05:19 PDT
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) 2006-06-13 11:30:19 PDT
checked into 1.8.0 for aaron
Comment 18 aaronr 2006-06-20 11:56:19 PDT
checked into 1.8.1

Note You need to log in before you can comment on or make changes to this bug.