Replace EntryArray type by an Entry[]

Replace EntryArray type by an Entry[]

Replace EntryArray type by an Entry[] to match the specification:
http://www.w3.org/TR/file-system-api/#the-entriescallback-interface

As a consequence, regular JavaScript Array methods can now be used.
There is no longer an item() method exposed but developers usually
access items using [] operator.

EntryArraySync is not converted to an EntrySync[] in this patch
to reduce patch size. It should be converted in a follow-up patch.

BUG=268356
R=arv@chromium.org, haraken@chromium.org, kinuko@chromium.org, tkent@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155749

[haraken, 2013-08-07 00:00:53]

In terms of implementation, LGTM.

Given the following points, it sounds reasonable to make this change:

- File APIs are implemented only in Blink and thus there are no concerns about
cross-browser compatibility
- Developers usually won't use item() to access items
- File API experts are happy with making this change
(https://code.google.com/p/chromium/issues/detail?id=268356)


Please wait for an approval from an API owner.

[kinuko, 2013-08-07 10:16:08]

lgtm

https://codereview.chromium.org/22436002/diff/1/Source/modules/filesystem/DOM...
File Source/modules/filesystem/DOMFileSystem.h (right):

https://codereview.chromium.org/22436002/diff/1/Source/modules/filesystem/DOM...
Source/modules/filesystem/DOMFileSystem.h:89: class DispatchCallbackTaskBase :
public ScriptExecutionContext::Task {
nit: having this base class may not add much benefit?

[do-not-use, 2013-08-07 10:28:45]

https://codereview.chromium.org/22436002/diff/1/Source/modules/filesystem/DOM...
File Source/modules/filesystem/DOMFileSystem.h (right):

https://codereview.chromium.org/22436002/diff/1/Source/modules/filesystem/DOM...
Source/modules/filesystem/DOMFileSystem.h:89: class DispatchCallbackTaskBase :
public ScriptExecutionContext::Task {
On 2013/08/07 10:16:08, kinuko wrote:
> nit: having this base class may not add much benefit?

Yes, I have to agree with you. It looked like a bit too much duplication before
so I added a base class. However, the base class does not really share as much
code as I initially hoped.

I will kill this base class and re-upload.

[do-not-use, 2013-08-07 10:41:42]

On 2013/08/07 10:16:08, kinuko wrote:
> lgtm
> 
>
https://codereview.chromium.org/22436002/diff/1/Source/modules/filesystem/DOM...
> File Source/modules/filesystem/DOMFileSystem.h (right):
> 
>
https://codereview.chromium.org/22436002/diff/1/Source/modules/filesystem/DOM...
> Source/modules/filesystem/DOMFileSystem.h:89: class DispatchCallbackTaskBase :
> public ScriptExecutionContext::Task {
> nit: having this base class may not add much benefit?

Fixed in patch set 2.

[kinuko, 2013-08-07 11:12:21]

patch set lgtm (still)

[arv (Not doing code reviews), 2013-08-07 14:24:43]

LGTM

https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesyste...
File LayoutTests/fast/filesystem/resources/op-tests-helper.js (right):

https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesyste...
LayoutTests/fast/filesystem/resources/op-tests-helper.js:210: this.readEntries =
this.readEntries.concat(entries);
concat is evil. How about:

this.readEntries.push.apply(this.readEntries, entries);

https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesyste...
File LayoutTests/fast/filesystem/script-tests/read-directory.js (right):

https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesyste...
LayoutTests/fast/filesystem/script-tests/read-directory.js:51: resultEntries =
resultEntries.concat(entries);
same

[apavlov, 2013-08-07 14:29:27]

https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesyste...
File LayoutTests/fast/filesystem/resources/op-tests-helper.js (right):

https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesyste...
LayoutTests/fast/filesystem/resources/op-tests-helper.js:210: this.readEntries =
this.readEntries.concat(entries);
On 2013/08/07 14:24:43, arv wrote:
> concat is evil. How about:
> 
> this.readEntries.push.apply(this.readEntries, entries);

On a related note, we on the DevTools team have seen V8 stack overflows with
push.apply if |entries| is _sufficiently_ long...

[arv (Not doing code reviews), 2013-08-07 15:10:07]

On 2013/08/07 14:29:27, apavlov wrote:
>
https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesyste...
> File LayoutTests/fast/filesystem/resources/op-tests-helper.js (right):
> 
>
https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesyste...
> LayoutTests/fast/filesystem/resources/op-tests-helper.js:210: this.readEntries
=
> this.readEntries.concat(entries);
> On 2013/08/07 14:24:43, arv wrote:
> > concat is evil. How about:
> > 
> > this.readEntries.push.apply(this.readEntries, entries);
> 
> On a related note, we on the DevTools team have seen V8 stack overflows with
> push.apply if |entries| is _sufficiently_ long...

Do you have a bug number?

[apavlov, 2013-08-07 15:15:09]

On 2013/08/07 15:10:07, arv wrote:
> On 2013/08/07 14:29:27, apavlov wrote:
> >
>
https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesyste...
> > File LayoutTests/fast/filesystem/resources/op-tests-helper.js (right):
> > 
> >
>
https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesyste...
> > LayoutTests/fast/filesystem/resources/op-tests-helper.js:210:
this.readEntries
> =
> > this.readEntries.concat(entries);
> > On 2013/08/07 14:24:43, arv wrote:
> > > concat is evil. How about:
> > > 
> > > this.readEntries.push.apply(this.readEntries, entries);
> > 
> > On a related note, we on the DevTools team have seen V8 stack overflows with
> > push.apply if |entries| is _sufficiently_ long...
> 
> Do you have a bug number?

Actually, it was pfeldman@ who hit it. I'm not sure about the bug number, but my
guess is none was filed.

[do-not-use, 2013-08-07 18:12:28]

https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesyste...
File LayoutTests/fast/filesystem/resources/op-tests-helper.js (right):

https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesyste...
LayoutTests/fast/filesystem/resources/op-tests-helper.js:210: this.readEntries =
this.readEntries.concat(entries);
On 2013/08/07 14:24:43, arv wrote:
> concat is evil. How about:
> 
> this.readEntries.push.apply(this.readEntries, entries);

Honestly, as a non-javascript person, it is really not obvious what this line is
doing. I find the concat() to be much more readable (not to say that it is
better / not evil).

[do-not-use, 2013-08-07 18:22:09]

On 2013/08/07 18:12:28, Christophe Dumez wrote:
>
https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesyste...
> File LayoutTests/fast/filesystem/resources/op-tests-helper.js (right):
> 
>
https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesyste...
> LayoutTests/fast/filesystem/resources/op-tests-helper.js:210: this.readEntries
=
> this.readEntries.concat(entries);
> On 2013/08/07 14:24:43, arv wrote:
> > concat is evil. How about:
> > 
> > this.readEntries.push.apply(this.readEntries, entries);
> 
> Honestly, as a non-javascript person, it is really not obvious what this line
is
> doing. I find the concat() to be much more readable (not to say that it is
> better / not evil).

I stopped using concat() is patch set 3.

[arv (Not doing code reviews), 2013-08-07 18:32:41]

concat is broken for non array instances (and it creates a new array but
that sometimes you want that).

In ES6 you can write:

this.readEntries.push(...entries);

Carrots are better than sticks.



On Wed, Aug 7, 2013 at 2:22 PM, <ch.dumez@sisa.samsung.com> wrote:

> On 2013/08/07 18:12:28, Christophe Dumez wrote:
>
> https://codereview.chromium.**org/22436002/diff/9001/**
>
LayoutTests/fast/filesystem/**resources/op-tests-helper.js<https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesystem/resources/op-tests-helper.js>
>
>> File LayoutTests/fast/filesystem/**resources/op-tests-helper.js (right):
>>
>
>
> https://codereview.chromium.**org/22436002/diff/9001/**
>
LayoutTests/fast/filesystem/**resources/op-tests-helper.js#**newcode210<https://codereview.chromium.org/22436002/diff/9001/LayoutTests/fast/filesystem/resources/op-tests-helper.js#newcode210>
>
>> LayoutTests/fast/filesystem/**resources/op-tests-helper.js:**210:
>> this.readEntries
>>
> =
>
>> this.readEntries.concat(**entries);
>> On 2013/08/07 14:24:43, arv wrote:
>> > concat is evil. How about:
>> >
>> > this.readEntries.push.apply(**this.readEntries, entries);
>>
>
>  Honestly, as a non-javascript person, it is really not obvious what this
>> line
>>
> is
>
>> doing. I find the concat() to be much more readable (not to say that it is
>> better / not evil).
>>
>
> I stopped using concat() is patch set 3.
>
>
https://codereview.chromium.**org/22436002/<https://codereview.chromium.org/2...
>



-- 
erik

[tkent, 2013-08-07 21:26:45]

API change LGTM

[do-not-use, 2013-08-08 07:55:01]

Committed patchset #4 manually as r155749 (presubmit successful).

[f(malita), 2013-08-08 14:42:39]

On 2013/08/08 07:55:01, Christophe Dumez wrote:
> Committed patchset #4 manually as r155749 (presubmit successful).

This broke browser_tests/GetFileStatuses, PTAL:

http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/302...

[25000:25000:0808/071619:2779187310:INFO:CONSOLE(113)] "Uncaught Error:
Invocation of form syncFileSystem.getFileStatuses(array, function) doesn't match
definition syncFileSystem.getFileStatuses(object fileEntries, function
callback)", source: schemaUtils (113)

[kinuko, 2013-08-08 14:44:52]

On 2013/08/08 14:42:39, Florin Malita wrote:
> On 2013/08/08 07:55:01, Christophe Dumez wrote:
> > Committed patchset #4 manually as r155749 (presubmit successful).
> 
> This broke browser_tests/GetFileStatuses, PTAL:
> 
>
http://build.chromium.org/p/chromium.webkit/builders/Linux%2520Tests/builds/3...
> 
> [25000:25000:0808/071619:2779187310:INFO:CONSOLE(113)] "Uncaught Error:
> Invocation of form syncFileSystem.getFileStatuses(array, function) doesn't
match
> definition syncFileSystem.getFileStatuses(object fileEntries, function
> callback)", source: schemaUtils (113)

I think we can/should fix this in chrome side.  Can we disable the test for now
until I fix it?  I can make a patch soonish (but tomorrow in our time, it's mid
night here)

[do-not-use, 2013-08-08 14:49:28]

On 2013/08/08 14:44:52, kinuko wrote:
> On 2013/08/08 14:42:39, Florin Malita wrote:
> > On 2013/08/08 07:55:01, Christophe Dumez wrote:
> > > Committed patchset #4 manually as r155749 (presubmit successful).
> > 
> > This broke browser_tests/GetFileStatuses, PTAL:
> > 
> >
>
http://build.chromium.org/p/chromium.webkit/builders/Linux%252520Tests/builds...
> > 
> > [25000:25000:0808/071619:2779187310:INFO:CONSOLE(113)] "Uncaught Error:
> > Invocation of form syncFileSystem.getFileStatuses(array, function) doesn't
> match
> > definition syncFileSystem.getFileStatuses(object fileEntries, function
> > callback)", source: schemaUtils (113)
> 
> I think we can/should fix this in chrome side.  Can we disable the test for
now
> until I fix it?  I can make a patch soonish (but tomorrow in our time, it's
mid
> night here)

I'm giving it a shot now since it is not too late yet in my timezone. I'm not
very familiar with chrome tests though so we'll see :)