index() with no repeat should return NaN

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
a year ago

People

(Reporter: Steve Speicher, Assigned: aaronr)

Tracking

({fixed1.8.0.5, fixed1.8.1})

Trunk
x86
All
fixed1.8.0.5, fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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
(Reporter)

Comment 1

12 years ago
Created attachment 218444 [details]
testcase
(Assignee)

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

12 years ago
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.
(Assignee)

Updated

12 years ago
Blocks: 322255
(Assignee)

Comment 3

12 years ago
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.
(Assignee)

Comment 4

12 years ago
Created attachment 218557 [details]
testcase 2

original testcase plus test for index using non-repeat node's ID.
Attachment #218444 - Attachment is obsolete: true
(Assignee)

Comment 5

12 years ago
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.
Attachment #218560 - Flags: review?(bugmail)
(Assignee)

Updated

12 years ago
Attachment #218560 - Flags: review?(allan)
(Assignee)

Comment 6

12 years ago
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.
(Assignee)

Comment 7

12 years ago
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.
OS: Windows XP → All

Updated

12 years ago
Attachment #218560 - Flags: review?(allan) → review+

Updated

12 years ago
Status: NEW → ASSIGNED
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.
Attachment #218560 - Flags: review?(bugmail) → review-
(Assignee)

Comment 9

12 years ago
(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.
(Assignee)

Comment 10

12 years ago
Created attachment 220088 [details] [diff] [review]
trunk patch with -1 return

patch with the -1 return talked about earlier
Attachment #218560 - Attachment is obsolete: true
Attachment #218561 - Attachment is obsolete: true
Attachment #220088 - Flags: review?(bugmail)
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
Attachment #220088 - Flags: review?(bugmail) → review+
(Assignee)

Comment 12

12 years ago
Created attachment 220135 [details] [diff] [review]
trunk patch with jonas fix

updated patch with jonas' comment fixed.
Attachment #220088 - Attachment is obsolete: true
Attachment #220135 - Flags: review?(allan)

Updated

11 years ago
Attachment #220135 - Flags: review?(allan) → review+
(Assignee)

Comment 13

11 years ago
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.
Attachment #220135 - Flags: superreview?(peterv)
Attachment #220135 - Flags: superreview?(peterv) → superreview+

Comment 14

11 years ago
(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

11 years ago
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 :-) ).
Attachment #220135 - Flags: approval1.8.0.5?
Comment on attachment 220135 [details] [diff] [review]
trunk patch with jonas fix

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #220135 - Flags: approval1.8.0.5? → approval1.8.0.5+
checked into 1.8.0 for aaron
Keywords: fixed1.8.0.5
(Assignee)

Updated

11 years ago
Attachment #220135 - Flags: approval-branch-1.8.1?
(Assignee)

Updated

11 years ago
Attachment #220135 - Flags: approval-branch-1.8.1? → approval-branch-1.8.1?(bugmail)
Attachment #220135 - Flags: approval-branch-1.8.1?(bugmail) → approval-branch-1.8.1+
(Assignee)

Comment 18

11 years ago
checked into 1.8.1
Keywords: fixed1.8.1
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.