Bug 77899 - IETC: FileList.item(-1) should return null instead of raising
Summary: IETC: FileList.item(-1) should return null instead of raising
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 87611 (view as bug list)
Depends on:
Blocks: 76198
  Show dependency treegraph
 
Reported: 2012-02-06 14:14 PST by Eric Seidel
Modified: 2012-06-05 01:13 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.78 KB, patch)
2012-06-01 07:17 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (5.10 KB, patch)
2012-06-01 08:11 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (5.14 KB, patch)
2012-06-03 21:20 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (4.60 KB, patch)
2012-06-04 18:00 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (5.09 KB, patch)
2012-06-04 19:23 PDT, Li Yin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2012-02-06 14:14:02 PST
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.
Comment 1 Eric Seidel 2012-02-06 14:41:48 PST
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
Comment 2 Li Yin 2012-05-29 20:31:34 PDT
*** Bug 87611 has been marked as a duplicate of this bug. ***
Comment 3 Li Yin 2012-05-29 21:21:22 PDT
Both of Firefox11 and IE10 will return null, if index is negative or largger than length.
Comment 4 Li Yin 2012-05-29 22:01:06 PDT
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.
Comment 5 Li Yin 2012-05-29 22:14:50 PDT
(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.
Comment 6 Li Yin 2012-05-31 18:31:32 PDT
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
Comment 7 Kinuko Yasuda 2012-05-31 22:56:36 PDT
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
Comment 8 Kinuko Yasuda 2012-06-01 01:47:20 PDT
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.
Comment 9 Li Yin 2012-06-01 07:17:02 PDT
Created attachment 145301 [details]
Patch
Comment 10 Li Yin 2012-06-01 07:24:21 PDT
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.
Comment 11 Kinuko Yasuda 2012-06-01 07:42:51 PDT
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.
Comment 12 Li Yin 2012-06-01 07:54:46 PDT
(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.
Comment 13 Li Yin 2012-06-01 08:11:23 PDT
Created attachment 145312 [details]
Patch
Comment 14 Kinuko Yasuda 2012-06-03 20:13:11 PDT
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" ?
Comment 15 Li Yin 2012-06-03 21:20:38 PDT
Created attachment 145513 [details]
Patch
Comment 16 Li Yin 2012-06-03 21:26:38 PDT
(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.
Comment 17 Kinuko Yasuda 2012-06-03 23:22:06 PDT
(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);
Comment 18 Kinuko Yasuda 2012-06-04 00:09:08 PDT
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)
Comment 19 Li Yin 2012-06-04 18:00:46 PDT
Created attachment 145671 [details]
Patch
Comment 20 Li Yin 2012-06-04 18:05:09 PDT
(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.
Comment 21 Kentaro Hara 2012-06-04 18:18:44 PDT
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)?
Comment 22 Li Yin 2012-06-04 18:44:02 PDT
(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.
Comment 23 Kentaro Hara 2012-06-04 18:45:37 PDT
(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).
Comment 24 Kentaro Hara 2012-06-04 18:47:41 PDT
(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
Comment 25 Li Yin 2012-06-04 19:23:36 PDT
Created attachment 145680 [details]
Patch
Comment 26 Li Yin 2012-06-04 19:24:43 PDT
(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.
Comment 27 Kentaro Hara 2012-06-04 19:30:39 PDT
Comment on attachment 145680 [details]
Patch

Looks OK
Comment 28 WebKit Review Bot 2012-06-05 01:13:16 PDT
Comment on attachment 145680 [details]
Patch

Clearing flags on attachment: 145680

Committed r119466: <http://trac.webkit.org/changeset/119466>
Comment 29 WebKit Review Bot 2012-06-05 01:13:23 PDT
All reviewed patches have been landed.  Closing bug.