Bug 77899 – IETC: FileList.item(-1) should return null instead of raising

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric Seidel (no email)]

IETC: FileList.item(-1) should return null instead of raising

http://www.w3.org/TR/FileAPI/#filelist-methods-params

This is covered by http://samples.msdn.microsoft.com/ietestcenter/fileapi/filelist.htm (and is the only subtest in that page that we fail).

I'm not sure that the spec is correct.  It's possible we should get the spec changed.

[Eric Seidel (no email)]

I believe this exception is auto-generated by the IsIndex flag here:
        File item(in [IsIndex] unsigned long index);
http://trac.webkit.org/browser/trunk/Source/WebCore/fileapi/FileList.idl#L33

[Li Yin]

*** Bug 87611 has been marked as a duplicate of this bug. ***

[Li Yin]

Both of Firefox11 and IE10 will return null, if index is negative or largger than length.

[Li Yin]

From FileList.idl of Spec
interface FileList {
      getter File? item(unsigned long index);
      readonly attribute unsigned long length;
};

"unsigned long index" represents negative index is not expected.

Maybe webkit is correct behavior.

[Li Yin]

(In reply to comment #1)
> I believe this exception is auto-generated by the IsIndex flag here:
>         File item(in [IsIndex] unsigned long index);
> http://trac.webkit.org/browser/trunk/Source/WebCore/fileapi/FileList.idl#L33

Although we delete the IsIndex flag, it is still a problem when index is negative.
index is expected to be unsigned long data type, negative index will be translated to be a long positive index. 
For example, file.item(-1) will return the 4294967295th item in the file list.

[Li Yin]

Spec is not clear to describe that.
I filed a bug in W3C to track that.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=17277

[Kinuko Yasuda]

We have wondered the same, webkit seems to be following the Web IDL's unsigned long handling for negative values.  Thanks for filing the w3c bug.
http://dev.w3.org/2006/webapi/WebIDL/#es-unsigned-long

[Kinuko Yasuda]

Rereading the spec now, it looks like we should run the procedure specified by ToUint32 (http://es5.github.com/#x9.6) if it's not annotated EnforceRange nor Clamp.

Therefore the index needs to be converted to modulo 2**32.

[Li Yin]

Created attachment 145301 [details]
Patch

[Li Yin]

Delete the [IsIndex], the auto-generated code will doesn't raise the index exception.

Hi jianli,
  I also updated the file-list-test.html test as you expected last time, please have a look. Thanks in advance.

[Kinuko Yasuda]

Comment on attachment 145301 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145301&action=review

> LayoutTests/ChangeLog:9
> +        undefined, null, normal index(0~length-1), more than length index.

Can we add a link to the IETC test and the corresponding spec section so that readers can have an idea why undefined and null are mapped to +0?

> LayoutTests/fast/files/file-list-test.html:21
> +    }

If we're only testing single test case why did you add this and the recursion call 'doTest(++globalIndex)'?

> LayoutTests/fast/files/file-list-test.html:28
> +    shouldBeNull("files.item(2)");

nit: you're mixing single quote and double quota in the same file.

[Li Yin]

(In reply to comment #11)
> (From update of attachment 145301 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145301&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        undefined, null, normal index(0~length-1), more than length index.
> 
> Can we add a link to the IETC test and the corresponding spec section so that readers can have an idea why undefined and null are mapped to +0?

Okay, I will do that.

> 
> > LayoutTests/fast/files/file-list-test.html:21
> > +    }
> 
> If we're only testing single test case why did you add this and the recursion call 'doTest(++globalIndex)'?
> 

In fact, there are two sub test case, one for two files, another for no file in FileList.

> > LayoutTests/fast/files/file-list-test.html:28
> > +    shouldBeNull("files.item(2)");
> 
> nit: you're mixing single quote and double quota in the same file.

Okay, I will update it.

[Li Yin]

Created attachment 145312 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 145312 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145312&action=review

> LayoutTests/fast/files/file-list-test.html:13
> +var caseArray = [["resources/UTF8.txt", "resources/UTF8-2.txt"], []];

Can we add a comment 'caseArray must end with an empty item' or something like that since the test code seems to rely on the assumption?

> LayoutTests/fast/files/file-list-test.html:14
> +var globalIndex = 0;

naming-nit: How about naming it caseIndex to indicate it is the index of caseArray?

> LayoutTests/fast/files/file-list-test.html:18
> +    if (globalIndex == 1) {

if (caseArray[globalIndex]].length == 0)

might make this code more generic?

> LayoutTests/fast/files/file-list-test.html:19
> +        testFailed("onInputFileChange was called.");

"... was called for an empty file list" ?

[Li Yin]

Created attachment 145513 [details]
Patch

[Li Yin]

(In reply to comment #14)
> (From update of attachment 145312 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145312&action=review
> 
> > LayoutTests/fast/files/file-list-test.html:13
> > +var caseArray = [["resources/UTF8.txt", "resources/UTF8-2.txt"], []];
> 
> Can we add a comment 'caseArray must end with an empty item' or something like that since the test code seems to rely on the assumption?

Using the expression "if (caseArray[globalIndex]].length == 0)" as you refered, It will not be a must that array ends with an empty.

> 
> > LayoutTests/fast/files/file-list-test.html:14
> > +var globalIndex = 0;
> 
> naming-nit: How about naming it caseIndex to indicate it is the index of caseArray?
> 
> > LayoutTests/fast/files/file-list-test.html:18
> > +    if (globalIndex == 1) {
> 
> if (caseArray[globalIndex]].length == 0)
> 
> might make this code more generic?
>

Done. 

> > LayoutTests/fast/files/file-list-test.html:19
> > +        testFailed("onInputFileChange was called.");
> 
> "... was called for an empty file list" ?

Done.

Thanks.

[Kinuko Yasuda]

(In reply to comment #16)
> (In reply to comment #14)
> > > LayoutTests/fast/files/file-list-test.html:13
> > > +var caseArray = [["resources/UTF8.txt", "resources/UTF8-2.txt"], []];
> > 
> > Can we add a comment 'caseArray must end with an empty item' or something like that since the test code seems to rely on the assumption?
> 
> Using the expression "if (caseArray[globalIndex]].length == 0)" as you refered, It will not be a must that array ends with an empty.

I might want to check if ++caseIndex < caseArray.length before calling doTest then.

By the way I noticed something else. You're calling the next beginDragWithFiles directly from the file change event handler which is supposed to clear the dragged data 'after' the handler runs, and I think it could screw up the internal drag status (i.e. it'd fail on assertion if it runs in debug build).

I think you can simply loop over the test cases like:

 while (++caseIndex < caseArray.length)
   doTest(caseIndex);

[Kinuko Yasuda]

Comment on attachment 145513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145513&action=review

> LayoutTests/fast/files/file-list-test.html:25
> +    shouldBeEqualToString("files.item(0).name", "UTF8.txt");

One more comment.  We're testing the 'files' against constant values in this handler while the handler is registered for multiple test cases now (though the only non-empty test case is [UTF8.txt, UTF8-2.txt]).  If we really want to make this handler generic we should give the expected values also from caseArray, or maybe in this case we can just drop the caseArray but instead calling each case directly with different handler?  Like:

doTest(["resources/UTF8.txt", "resources/UTF8-2.txt"], onInputFileChange);
doTest([], shouldNotBeCalled);

(By the way do we really need to test empty file list case in this test?  It doesn't look to be a part of FileList tests)

[Li Yin]

Created attachment 145671 [details]
Patch

[Li Yin]

(In reply to comment #18)
> (By the way do we really need to test empty file list case in this test?  It doesn't look to be a part of FileList tests)

Yeah, empty file list case is not necessary, delete it.

Thanks.

[Kentaro Hara]

Comment on attachment 145671 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145671&action=review

Let me confirm my understanding:

- Per the Web IDL spec, ToUint32() is called. -1 is converted to 4294967295.
- files.item(-1) will return the same item as files.item(4294967295).
- Given that there is no item in the 4294967295th index, files.item(-1) returns null.
- Firefox11 and IE10 also return null for files.item(-1).
- Consequently, this change follows the spec and does not violate cross-browser compatibility.

Is my understanding correct? (If so, the change looks reasonable to me.)

> LayoutTests/fast/files/file-list-test.html:24
> +    shouldBeNull("files.item(-1)");

Shall we add a test for files.item(4294967295)?

[Li Yin]

(In reply to comment #21)
> (From update of attachment 145671 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145671&action=review
> 
> Let me confirm my understanding:
> 
> - Per the Web IDL spec, ToUint32() is called. -1 is converted to 4294967295.
> - files.item(-1) will return the same item as files.item(4294967295).
> - Given that there is no item in the 4294967295th index, files.item(-1) returns null.
> - Firefox11 and IE10 also return null for files.item(-1).
> - Consequently, this change follows the spec and does not violate cross-browser compatibility.
> 
> Is my understanding correct? (If so, the change looks reasonable to me.)
> 
> > LayoutTests/fast/files/file-list-test.html:24
> > +    shouldBeNull("files.item(-1)");
> 
> Shall we add a test for files.item(4294967295)?

Yeah, -1 is converted to 2^32 - 1, do you mean we should test file.item(-4294967295) or file.item(-4294967296)?
file.item(-4294967295) equals file.item(1);
file.item(-4294967296) equals file.item(0);

I will add these tests.

Thanks for your review.

[Kentaro Hara]

(In reply to comment #22)
> Yeah, -1 is converted to 2^32 - 1, do you mean we should test file.item(-4294967295) or file.item(-4294967296)?
> file.item(-4294967295) equals file.item(1);
> file.item(-4294967296) equals file.item(0);
> 
> I will add these tests.

That's a good idea. And plus files.item(4294967295), which should equal to files.item(-1).

[Kentaro Hara]

(In reply to comment #23)
> (In reply to comment #22)
> > Yeah, -1 is converted to 2^32 - 1, do you mean we should test file.item(-4294967295) or file.item(-4294967296)?
> > file.item(-4294967295) equals file.item(1);
> > file.item(-4294967296) equals file.item(0);
> > 
> > I will add these tests.
> 
> That's a good idea. And plus files.item(4294967295), which should equal to files.item(-1).

So in summary we might want to check the followings:

files.item(4294967295) == files.item(-1) == null
files.item(4294967296) == files.item(0) == "UTF8.txt"
files.item(4294967297) == files.item(1) == "UTF8-2.txt"
files.item(4294967298) == files.item(2) == null

[Li Yin]

Created attachment 145680 [details]
Patch

[Li Yin]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > Yeah, -1 is converted to 2^32 - 1, do you mean we should test file.item(-4294967295) or file.item(-4294967296)?
> > > file.item(-4294967295) equals file.item(1);
> > > file.item(-4294967296) equals file.item(0);
> > > 
> > > I will add these tests.
> > 
> > That's a good idea. And plus files.item(4294967295), which should equal to files.item(-1).
> 
> So in summary we might want to check the followings:
> 
> files.item(4294967295) == files.item(-1) == null
> files.item(4294967296) == files.item(0) == "UTF8.txt"
> files.item(4294967297) == files.item(1) == "UTF8-2.txt"
> files.item(4294967298) == files.item(2) == null

Done.

Thanks.

[Kentaro Hara]

Comment on attachment 145680 [details]
Patch

Looks OK

[WebKit Review Bot]

Comment on attachment 145680 [details]
Patch

Clearing flags on attachment: 145680

Committed r119466: <http://trac.webkit.org/changeset/119466>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.