Add ReadEntries interface for PPB_DirectoryReader (won't commit)

Add ReadEntries interface for PPB_DirectoryReader

I won't commit this CL since this is just for API discussion.

BUG=113086

[nhiroki, 2013-01-18 05:00:30]

This is just proposal for adding new API.
Could you review this and give us some advice?

Thanks!

[brettw, 2013-01-18 05:06:30]

I'll defer my review to dmichael.

[darin (slow to review), 2013-01-18 06:26:19]

https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
File ppapi/api/dev/ppb_directory_reader_dev.idl (right):

https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
ppapi/api/dev/ppb_directory_reader_dev.idl:67: [in] PP_ArrayOutput output,
Most operating systems don't provide APIs like this because they can take a long
time to execute and consume a large amount of memory.  Consider large
directories.  Instead, they provide APIs to progressively fetch items (e.g.,
readdir and FindNextFile).

Also, we could easily implement ReadEntries in terms of GetNextEntry, so it
seems like ReadEntries could be a helper function if applications wish to have a
simpler way to read all of the entries in a directory.  It just seems like the
core API should be more flexible.

(It could be the case that we temporarily implement GetNextEntry using a
sub-optimal IPC to fetch all entries in one go, but that could be fixed in the
future.)

[brettw, 2013-01-18 07:12:59]

There was some discussion on the thread titled "Does anything use
PPB_DirectoryReader_Dev"

Brett

On Thu, Jan 17, 2013 at 10:26 PM,  <darin@chromium.org> wrote:
>
>
https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
> File ppapi/api/dev/ppb_directory_reader_dev.idl (right):
>
>
https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
> ppapi/api/dev/ppb_directory_reader_dev.idl:67: [in] PP_ArrayOutput
> output,
> Most operating systems don't provide APIs like this because they can
> take a long time to execute and consume a large amount of memory.
> Consider large directories.  Instead, they provide APIs to progressively
> fetch items (e.g., readdir and FindNextFile).
>
> Also, we could easily implement ReadEntries in terms of GetNextEntry, so
> it seems like ReadEntries could be a helper function if applications
> wish to have a simpler way to read all of the entries in a directory.
> It just seems like the core API should be more flexible.
>
> (It could be the case that we temporarily implement GetNextEntry using a
> sub-optimal IPC to fetch all entries in one go, but that could be fixed
> in the future.)
>
> https://codereview.chromium.org/12026008/

[dmichael (off chromium), 2013-01-18 16:59:47]

On 2013/01/18 07:12:59, brettw wrote:
> There was some discussion on the thread titled "Does anything use
> PPB_DirectoryReader_Dev"
I think the simple way is fine while this is still in Dev. But I worry that if
returning everything becomes a problem, we'll be saddled with supporting the old
interface even if we update the API.

> 
> Brett
> 
> On Thu, Jan 17, 2013 at 10:26 PM,  <mailto:darin@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
> > File ppapi/api/dev/ppb_directory_reader_dev.idl (right):
> >
> >
>
https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
> > ppapi/api/dev/ppb_directory_reader_dev.idl:67: [in] PP_ArrayOutput
> > output,
> > Most operating systems don't provide APIs like this because they can
> > take a long time to execute and consume a large amount of memory.
> > Consider large directories.  Instead, they provide APIs to progressively
> > fetch items (e.g., readdir and FindNextFile).
> >
> > Also, we could easily implement ReadEntries in terms of GetNextEntry, so
> > it seems like ReadEntries could be a helper function if applications
> > wish to have a simpler way to read all of the entries in a directory.
> > It just seems like the core API should be more flexible.
> >
> > (It could be the case that we temporarily implement GetNextEntry using a
> > sub-optimal IPC to fetch all entries in one go, but that could be fixed
> > in the future.)
> >
> > https://codereview.chromium.org/12026008/

[dmichael (off chromium), 2013-01-18 17:10:18]

https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
File ppapi/api/dev/ppb_directory_reader_dev.idl (right):

https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
ppapi/api/dev/ppb_directory_reader_dev.idl:52: [in] PP_CompletionCallback
callback);
Since nobody is using this API right now, let's remove this function entirely
and pretend 0.5 didn't happen :-)

[darin (slow to review), 2013-01-18 19:27:00]

Imagine enumerating the files on a network share.  An app might like to
incrementally render the contents of a folder.  With ReadEntries you are
unable to do that and moreover left showing the user a busy indicator with
no possible way of estimating time to completion.  This API forces bad UI,
and we'd be stuck with apps using it potentially for a long time.

With the former API, you can do a suboptimal impl that you can seamlessly
improve later.  Seems like the right compromise to me.
On Jan 18, 2013 8:59 AM, <dmichael@chromium.org> wrote:

> On 2013/01/18 07:12:59, brettw wrote:
>
>> There was some discussion on the thread titled "Does anything use
>> PPB_DirectoryReader_Dev"
>>
> I think the simple way is fine while this is still in Dev. But I worry
> that if
> returning everything becomes a problem, we'll be saddled with supporting
> the old
> interface even if we update the API.
>
>
>  Brett
>>
>
>  On Thu, Jan 17, 2013 at 10:26 PM,  <mailto:darin@chromium.org> wrote:
>> >
>> >
>>
>
> https://codereview.chromium.**org/12026008/diff/1002/ppapi/**
>
api/dev/ppb_directory_reader_**dev.idl<https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_directory_reader_dev.idl>
>
>> > File ppapi/api/dev/ppb_directory_**reader_dev.idl (right):
>> >
>> >
>>
>
> https://codereview.chromium.**org/12026008/diff/1002/ppapi/**
>
api/dev/ppb_directory_reader_**dev.idl#newcode67<https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_directory_reader_dev.idl#newcode67>
>
>> > ppapi/api/dev/ppb_directory_**reader_dev.idl:67: [in] PP_ArrayOutput
>> > output,
>> > Most operating systems don't provide APIs like this because they can
>> > take a long time to execute and consume a large amount of memory.
>> > Consider large directories.  Instead, they provide APIs to progressively
>> > fetch items (e.g., readdir and FindNextFile).
>> >
>> > Also, we could easily implement ReadEntries in terms of GetNextEntry, so
>> > it seems like ReadEntries could be a helper function if applications
>> > wish to have a simpler way to read all of the entries in a directory.
>> > It just seems like the core API should be more flexible.
>> >
>> > (It could be the case that we temporarily implement GetNextEntry using a
>> > sub-optimal IPC to fetch all entries in one go, but that could be fixed
>> > in the future.)
>> >
>> >
https://codereview.chromium.**org/12026008/<https://codereview.chromium.org/1...
>>
>
>
>
>
https://codereview.chromium.**org/12026008/<https://codereview.chromium.org/1...
>

[brettw, 2013-01-18 20:20:55]

I don't disagree that the incremental approach is theoretically better.

My concern was practical: just thinking about using this API makes my
mind boggle. The basic way to do this is to do an async callback for
each result. This seems pretty insane and will make everything slower,
forcing a trip back to the message loop for each result, and is super
annoying to program to.

Doing the "handle synchronous results" thing like we do for the URL
loader is exceedingly tricky to get right, so much so that the initial
URLLoader examples we wrote were wrong! I don't think we should
encourage this pattern in anything but the url loader, where you can
copy our example.

Plus, the backend is currently one-shot. This means that anybody that
does actually implement the approach where synchronous results are
returned from the query function will see one async callback, and then
all results are returned synchronously. They either won't write the
ability to continue getting async results, or that code path will be
completely untested in their code.

If we do actually support incremental querying in our backend later,
it will light up all of this code in existing plugins. If people
actually start using this API and we care about not breaking them, I
suspect this means we can never change it to deliver results in an
incremental manner.

So my inclination is to make the current API do what 98% of all users
want now anyway: "give me all the results in the simplest possible way
and tell me when you're done." Later, if we add an incremental
backend, we can add an incremental API to it and the remaining 2% of
programmers who want to do an incremental interface can program to
that API and test their code properly.

Brett

On Fri, Jan 18, 2013 at 11:26 AM, Darin Fisher <darin@chromium.org> wrote:
> Imagine enumerating the files on a network share.  An app might like to
> incrementally render the contents of a folder.  With ReadEntries you are
> unable to do that and moreover left showing the user a busy indicator with
> no possible way of estimating time to completion.  This API forces bad UI,
> and we'd be stuck with apps using it potentially for a long time.
>
> With the former API, you can do a suboptimal impl that you can seamlessly
> improve later.  Seems like the right compromise to me.
>
> On Jan 18, 2013 8:59 AM, <dmichael@chromium.org> wrote:
>>
>> On 2013/01/18 07:12:59, brettw wrote:
>>>
>>> There was some discussion on the thread titled "Does anything use
>>> PPB_DirectoryReader_Dev"
>>
>> I think the simple way is fine while this is still in Dev. But I worry
>> that if
>> returning everything becomes a problem, we'll be saddled with supporting
>> the old
>> interface even if we update the API.
>>
>>
>>> Brett
>>
>>
>>> On Thu, Jan 17, 2013 at 10:26 PM,  <mailto:darin@chromium.org> wrote:
>>> >
>>> >
>>
>>
>>
>>
https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
>>>
>>> > File ppapi/api/dev/ppb_directory_reader_dev.idl (right):
>>> >
>>> >
>>
>>
>>
>>
https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
>>>
>>> > ppapi/api/dev/ppb_directory_reader_dev.idl:67: [in] PP_ArrayOutput
>>> > output,
>>> > Most operating systems don't provide APIs like this because they can
>>> > take a long time to execute and consume a large amount of memory.
>>> > Consider large directories.  Instead, they provide APIs to
>>> > progressively
>>> > fetch items (e.g., readdir and FindNextFile).
>>> >
>>> > Also, we could easily implement ReadEntries in terms of GetNextEntry,
>>> > so
>>> > it seems like ReadEntries could be a helper function if applications
>>> > wish to have a simpler way to read all of the entries in a directory.
>>> > It just seems like the core API should be more flexible.
>>> >
>>> > (It could be the case that we temporarily implement GetNextEntry using
>>> > a
>>> > sub-optimal IPC to fetch all entries in one go, but that could be fixed
>>> > in the future.)
>>> >
>>> > https://codereview.chromium.org/12026008/
>>
>>
>>
>>
>> https://codereview.chromium.org/12026008/

[darin (slow to review), 2013-01-18 21:26:30]

That makes a lot of sense.  Last shot by me:  I suppose we could also
provide a helper function that implements ReadEntries efficiently on top of
GetNextEntry.  Why not do that?  Isn't that a have-our-cake-and-eat-it-too
solution?

-Darin


On Fri, Jan 18, 2013 at 12:20 PM, Brett Wilson <brettw@chromium.org> wrote:

> I don't disagree that the incremental approach is theoretically better.
>
> My concern was practical: just thinking about using this API makes my
> mind boggle. The basic way to do this is to do an async callback for
> each result. This seems pretty insane and will make everything slower,
> forcing a trip back to the message loop for each result, and is super
> annoying to program to.
>
> Doing the "handle synchronous results" thing like we do for the URL
> loader is exceedingly tricky to get right, so much so that the initial
> URLLoader examples we wrote were wrong! I don't think we should
> encourage this pattern in anything but the url loader, where you can
> copy our example.
>
> Plus, the backend is currently one-shot. This means that anybody that
> does actually implement the approach where synchronous results are
> returned from the query function will see one async callback, and then
> all results are returned synchronously. They either won't write the
> ability to continue getting async results, or that code path will be
> completely untested in their code.
>
> If we do actually support incremental querying in our backend later,
> it will light up all of this code in existing plugins. If people
> actually start using this API and we care about not breaking them, I
> suspect this means we can never change it to deliver results in an
> incremental manner.
>
> So my inclination is to make the current API do what 98% of all users
> want now anyway: "give me all the results in the simplest possible way
> and tell me when you're done." Later, if we add an incremental
> backend, we can add an incremental API to it and the remaining 2% of
> programmers who want to do an incremental interface can program to
> that API and test their code properly.
>
> Brett
>
> On Fri, Jan 18, 2013 at 11:26 AM, Darin Fisher <darin@chromium.org> wrote:
> > Imagine enumerating the files on a network share.  An app might like to
> > incrementally render the contents of a folder.  With ReadEntries you are
> > unable to do that and moreover left showing the user a busy indicator
> with
> > no possible way of estimating time to completion.  This API forces bad
> UI,
> > and we'd be stuck with apps using it potentially for a long time.
> >
> > With the former API, you can do a suboptimal impl that you can seamlessly
> > improve later.  Seems like the right compromise to me.
> >
> > On Jan 18, 2013 8:59 AM, <dmichael@chromium.org> wrote:
> >>
> >> On 2013/01/18 07:12:59, brettw wrote:
> >>>
> >>> There was some discussion on the thread titled "Does anything use
> >>> PPB_DirectoryReader_Dev"
> >>
> >> I think the simple way is fine while this is still in Dev. But I worry
> >> that if
> >> returning everything becomes a problem, we'll be saddled with supporting
> >> the old
> >> interface even if we update the API.
> >>
> >>
> >>> Brett
> >>
> >>
> >>> On Thu, Jan 17, 2013 at 10:26 PM,  <mailto:darin@chromium.org> wrote:
> >>> >
> >>> >
> >>
> >>
> >>
> >>
>
https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
> >>>
> >>> > File ppapi/api/dev/ppb_directory_reader_dev.idl (right):
> >>> >
> >>> >
> >>
> >>
> >>
> >>
>
https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
> >>>
> >>> > ppapi/api/dev/ppb_directory_reader_dev.idl:67: [in] PP_ArrayOutput
> >>> > output,
> >>> > Most operating systems don't provide APIs like this because they can
> >>> > take a long time to execute and consume a large amount of memory.
> >>> > Consider large directories.  Instead, they provide APIs to
> >>> > progressively
> >>> > fetch items (e.g., readdir and FindNextFile).
> >>> >
> >>> > Also, we could easily implement ReadEntries in terms of GetNextEntry,
> >>> > so
> >>> > it seems like ReadEntries could be a helper function if applications
> >>> > wish to have a simpler way to read all of the entries in a directory.
> >>> > It just seems like the core API should be more flexible.
> >>> >
> >>> > (It could be the case that we temporarily implement GetNextEntry
> using
> >>> > a
> >>> > sub-optimal IPC to fetch all entries in one go, but that could be
> fixed
> >>> > in the future.)
> >>> >
> >>> > https://codereview.chromium.org/12026008/
> >>
> >>
> >>
> >>
> >> https://codereview.chromium.org/12026008/
>

[brettw, 2013-01-18 21:33:31]

If we're guaranteed that everybody either writes their code properly
to handle the case that Chrome doesn't do yet, or uses the helper,
that sounds OK to me. But I'm not super comfortable with this.

Thinking about "real" OS APIs. Microsoft would never change this kind
of thing in a Win32 function, and it doesn't matter if the ATL/MFC
wrapper for it does the right thing because there are always people
that write their own. I think we need to get more into this mindset or
we'll be stuck later.

Brett



On Fri, Jan 18, 2013 at 1:26 PM, Darin Fisher <darin@chromium.org> wrote:
> That makes a lot of sense.  Last shot by me:  I suppose we could also
> provide a helper function that implements ReadEntries efficiently on top of
> GetNextEntry.  Why not do that?  Isn't that a have-our-cake-and-eat-it-too
> solution?
>
> -Darin
>
>
> On Fri, Jan 18, 2013 at 12:20 PM, Brett Wilson <brettw@chromium.org> wrote:
>>
>> I don't disagree that the incremental approach is theoretically better.
>>
>> My concern was practical: just thinking about using this API makes my
>> mind boggle. The basic way to do this is to do an async callback for
>> each result. This seems pretty insane and will make everything slower,
>> forcing a trip back to the message loop for each result, and is super
>> annoying to program to.
>>
>> Doing the "handle synchronous results" thing like we do for the URL
>> loader is exceedingly tricky to get right, so much so that the initial
>> URLLoader examples we wrote were wrong! I don't think we should
>> encourage this pattern in anything but the url loader, where you can
>> copy our example.
>>
>> Plus, the backend is currently one-shot. This means that anybody that
>> does actually implement the approach where synchronous results are
>> returned from the query function will see one async callback, and then
>> all results are returned synchronously. They either won't write the
>> ability to continue getting async results, or that code path will be
>> completely untested in their code.
>>
>> If we do actually support incremental querying in our backend later,
>> it will light up all of this code in existing plugins. If people
>> actually start using this API and we care about not breaking them, I
>> suspect this means we can never change it to deliver results in an
>> incremental manner.
>>
>> So my inclination is to make the current API do what 98% of all users
>> want now anyway: "give me all the results in the simplest possible way
>> and tell me when you're done." Later, if we add an incremental
>> backend, we can add an incremental API to it and the remaining 2% of
>> programmers who want to do an incremental interface can program to
>> that API and test their code properly.
>>
>> Brett
>>
>> On Fri, Jan 18, 2013 at 11:26 AM, Darin Fisher <darin@chromium.org> wrote:
>> > Imagine enumerating the files on a network share.  An app might like to
>> > incrementally render the contents of a folder.  With ReadEntries you are
>> > unable to do that and moreover left showing the user a busy indicator
>> > with
>> > no possible way of estimating time to completion.  This API forces bad
>> > UI,
>> > and we'd be stuck with apps using it potentially for a long time.
>> >
>> > With the former API, you can do a suboptimal impl that you can
>> > seamlessly
>> > improve later.  Seems like the right compromise to me.
>> >
>> > On Jan 18, 2013 8:59 AM, <dmichael@chromium.org> wrote:
>> >>
>> >> On 2013/01/18 07:12:59, brettw wrote:
>> >>>
>> >>> There was some discussion on the thread titled "Does anything use
>> >>> PPB_DirectoryReader_Dev"
>> >>
>> >> I think the simple way is fine while this is still in Dev. But I worry
>> >> that if
>> >> returning everything becomes a problem, we'll be saddled with
>> >> supporting
>> >> the old
>> >> interface even if we update the API.
>> >>
>> >>
>> >>> Brett
>> >>
>> >>
>> >>> On Thu, Jan 17, 2013 at 10:26 PM,  <mailto:darin@chromium.org> wrote:
>> >>> >
>> >>> >
>> >>
>> >>
>> >>
>> >>
>> >>
https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
>> >>>
>> >>> > File ppapi/api/dev/ppb_directory_reader_dev.idl (right):
>> >>> >
>> >>> >
>> >>
>> >>
>> >>
>> >>
>> >>
https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
>> >>>
>> >>> > ppapi/api/dev/ppb_directory_reader_dev.idl:67: [in] PP_ArrayOutput
>> >>> > output,
>> >>> > Most operating systems don't provide APIs like this because they can
>> >>> > take a long time to execute and consume a large amount of memory.
>> >>> > Consider large directories.  Instead, they provide APIs to
>> >>> > progressively
>> >>> > fetch items (e.g., readdir and FindNextFile).
>> >>> >
>> >>> > Also, we could easily implement ReadEntries in terms of
>> >>> > GetNextEntry,
>> >>> > so
>> >>> > it seems like ReadEntries could be a helper function if applications
>> >>> > wish to have a simpler way to read all of the entries in a
>> >>> > directory.
>> >>> > It just seems like the core API should be more flexible.
>> >>> >
>> >>> > (It could be the case that we temporarily implement GetNextEntry
>> >>> > using
>> >>> > a
>> >>> > sub-optimal IPC to fetch all entries in one go, but that could be
>> >>> > fixed
>> >>> > in the future.)
>> >>> >
>> >>> > https://codereview.chromium.org/12026008/
>> >>
>> >>
>> >>
>> >>
>> >> https://codereview.chromium.org/12026008/
>
>

[dmichael (off chromium), 2013-01-18 21:55:10]

On Fri, Jan 18, 2013 at 2:33 PM, Brett Wilson <brettw@chromium.org> wrote:

> If we're guaranteed that everybody either writes their code properly
> to handle the case that Chrome doesn't do yet, or uses the helper,
> that sounds OK to me. But I'm not super comfortable with this.
>
> Thinking about "real" OS APIs. Microsoft would never change this kind
> of thing in a Win32 function, and it doesn't matter if the ATL/MFC
> wrapper for it does the right thing because there are always people
> that write their own. I think we need to get more into this mindset or
> we'll be stuck later.
>
> Brett
>
The suggested API here is definitely simpler and what developers would
want. I was just worried about supporting it long term.

Would it be stupid to put some high cap on how many entries we'll retrieve
with this API? And if that's ever too small for some app, we can consider
adding the "expert" version that can actually get all entries.

Or just hope for the best. It does look like the API for JavaScript has the
exact same problem:
https://developer.mozilla.org/en-US/docs/DOM/File_API/File_System_API/Directo...


>
>
>
> On Fri, Jan 18, 2013 at 1:26 PM, Darin Fisher <darin@chromium.org> wrote:
> > That makes a lot of sense.  Last shot by me:  I suppose we could also
> > provide a helper function that implements ReadEntries efficiently on top
> of
> > GetNextEntry.  Why not do that?  Isn't that a
> have-our-cake-and-eat-it-too
> > solution?
>
I don't think it would be that easy to implement ReadEntries on top of
GetNextEntry. You could make it work on background threads with blocking
completion callbacks, but I don't see how you could make it work in general
on the main thread?




> >
> > -Darin
> >
> >
> > On Fri, Jan 18, 2013 at 12:20 PM, Brett Wilson <brettw@chromium.org>
> wrote:
> >>
> >> I don't disagree that the incremental approach is theoretically better.
> >>
> >> My concern was practical: just thinking about using this API makes my
> >> mind boggle. The basic way to do this is to do an async callback for
> >> each result. This seems pretty insane and will make everything slower,
> >> forcing a trip back to the message loop for each result, and is super
> >> annoying to program to.
> >>
> >> Doing the "handle synchronous results" thing like we do for the URL
> >> loader is exceedingly tricky to get right, so much so that the initial
> >> URLLoader examples we wrote were wrong! I don't think we should
> >> encourage this pattern in anything but the url loader, where you can
> >> copy our example.
> >>
> >> Plus, the backend is currently one-shot. This means that anybody that
> >> does actually implement the approach where synchronous results are
> >> returned from the query function will see one async callback, and then
> >> all results are returned synchronously. They either won't write the
> >> ability to continue getting async results, or that code path will be
> >> completely untested in their code.
> >>
> >> If we do actually support incremental querying in our backend later,
> >> it will light up all of this code in existing plugins. If people
> >> actually start using this API and we care about not breaking them, I
> >> suspect this means we can never change it to deliver results in an
> >> incremental manner.
> >>
> >> So my inclination is to make the current API do what 98% of all users
> >> want now anyway: "give me all the results in the simplest possible way
> >> and tell me when you're done." Later, if we add an incremental
> >> backend, we can add an incremental API to it and the remaining 2% of
> >> programmers who want to do an incremental interface can program to
> >> that API and test their code properly.
> >>
> >> Brett
> >>
> >> On Fri, Jan 18, 2013 at 11:26 AM, Darin Fisher <darin@chromium.org>
> wrote:
> >> > Imagine enumerating the files on a network share.  An app might like
> to
> >> > incrementally render the contents of a folder.  With ReadEntries you
> are
> >> > unable to do that and moreover left showing the user a busy indicator
> >> > with
> >> > no possible way of estimating time to completion.  This API forces bad
> >> > UI,
> >> > and we'd be stuck with apps using it potentially for a long time.
> >> >
> >> > With the former API, you can do a suboptimal impl that you can
> >> > seamlessly
> >> > improve later.  Seems like the right compromise to me.
> >> >
> >> > On Jan 18, 2013 8:59 AM, <dmichael@chromium.org> wrote:
> >> >>
> >> >> On 2013/01/18 07:12:59, brettw wrote:
> >> >>>
> >> >>> There was some discussion on the thread titled "Does anything use
> >> >>> PPB_DirectoryReader_Dev"
> >> >>
> >> >> I think the simple way is fine while this is still in Dev. But I
> worry
> >> >> that if
> >> >> returning everything becomes a problem, we'll be saddled with
> >> >> supporting
> >> >> the old
> >> >> interface even if we update the API.
> >> >>
> >> >>
> >> >>> Brett
> >> >>
> >> >>
> >> >>> On Thu, Jan 17, 2013 at 10:26 PM,  <mailto:darin@chromium.org>
> wrote:
> >> >>> >
> >> >>> >
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>
>
https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
> >> >>>
> >> >>> > File ppapi/api/dev/ppb_directory_reader_dev.idl (right):
> >> >>> >
> >> >>> >
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>
>
https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
> >> >>>
> >> >>> > ppapi/api/dev/ppb_directory_reader_dev.idl:67: [in] PP_ArrayOutput
> >> >>> > output,
> >> >>> > Most operating systems don't provide APIs like this because they
> can
> >> >>> > take a long time to execute and consume a large amount of memory.
> >> >>> > Consider large directories.  Instead, they provide APIs to
> >> >>> > progressively
> >> >>> > fetch items (e.g., readdir and FindNextFile).
> >> >>> >
> >> >>> > Also, we could easily implement ReadEntries in terms of
> >> >>> > GetNextEntry,
> >> >>> > so
> >> >>> > it seems like ReadEntries could be a helper function if
> applications
> >> >>> > wish to have a simpler way to read all of the entries in a
> >> >>> > directory.
> >> >>> > It just seems like the core API should be more flexible.
> >> >>> >
> >> >>> > (It could be the case that we temporarily implement GetNextEntry
> >> >>> > using
> >> >>> > a
> >> >>> > sub-optimal IPC to fetch all entries in one go, but that could be
> >> >>> > fixed
> >> >>> > in the future.)
> >> >>> >
> >> >>> > https://codereview.chromium.org/12026008/
> >> >>
> >> >>
> >> >>
> >> >>
> >> >> https://codereview.chromium.org/12026008/
> >
> >
>

[nhiroki, 2013-01-24 04:44:14]

Thank you for a lot of comments.

Could we start to implement new API? There's a project who needs (old or new)
DircetoryReader API and we'd like to deliver it to them asap.

Let me get the discussion straight:
- The ReadEntries API is okay (without no helper function on GetNextEntry).
- GetNextEntry should be removed.


> Would it be stupid to put some high cap on how many entries we'll retrieve
> with this API? And if that's ever too small for some app, we can consider
> adding the "expert" version that can actually get all entries.
> 
> Or just hope for the best. It does look like the API for JavaScript has the
> exact same problem:
>
https://developer.mozilla.org/en-US/docs/DOM/File_API/File_System_API/Directo...

I think that the latter way is better, too. For now, we'll implement the new API
to return all entries at once, and then make it possible to return entries in
multiple chunks. Is it okay with you?

Thanks!


On 2013/01/18 21:55:10, dmichael wrote:
> On Fri, Jan 18, 2013 at 2:33 PM, Brett Wilson <mailto:brettw@chromium.org>
wrote:
> 
> > If we're guaranteed that everybody either writes their code properly
> > to handle the case that Chrome doesn't do yet, or uses the helper,
> > that sounds OK to me. But I'm not super comfortable with this.
> >
> > Thinking about "real" OS APIs. Microsoft would never change this kind
> > of thing in a Win32 function, and it doesn't matter if the ATL/MFC
> > wrapper for it does the right thing because there are always people
> > that write their own. I think we need to get more into this mindset or
> > we'll be stuck later.
> >
> > Brett
> >
> The suggested API here is definitely simpler and what developers would
> want. I was just worried about supporting it long term.
> 
> Would it be stupid to put some high cap on how many entries we'll retrieve
> with this API? And if that's ever too small for some app, we can consider
> adding the "expert" version that can actually get all entries.
> 
> Or just hope for the best. It does look like the API for JavaScript has the
> exact same problem:
>
https://developer.mozilla.org/en-US/docs/DOM/File_API/File_System_API/Directo...
> 
> 
> >
> >
> >
> > On Fri, Jan 18, 2013 at 1:26 PM, Darin Fisher <mailto:darin@chromium.org>
wrote:
> > > That makes a lot of sense.  Last shot by me:  I suppose we could also
> > > provide a helper function that implements ReadEntries efficiently on top
> > of
> > > GetNextEntry.  Why not do that?  Isn't that a
> > have-our-cake-and-eat-it-too
> > > solution?
> >
> I don't think it would be that easy to implement ReadEntries on top of
> GetNextEntry. You could make it work on background threads with blocking
> completion callbacks, but I don't see how you could make it work in general
> on the main thread?
> 
> 
> 
> 
> > >
> > > -Darin
> > >
> > >
> > > On Fri, Jan 18, 2013 at 12:20 PM, Brett Wilson
<mailto:brettw@chromium.org>
> > wrote:
> > >>
> > >> I don't disagree that the incremental approach is theoretically better.
> > >>
> > >> My concern was practical: just thinking about using this API makes my
> > >> mind boggle. The basic way to do this is to do an async callback for
> > >> each result. This seems pretty insane and will make everything slower,
> > >> forcing a trip back to the message loop for each result, and is super
> > >> annoying to program to.
> > >>
> > >> Doing the "handle synchronous results" thing like we do for the URL
> > >> loader is exceedingly tricky to get right, so much so that the initial
> > >> URLLoader examples we wrote were wrong! I don't think we should
> > >> encourage this pattern in anything but the url loader, where you can
> > >> copy our example.
> > >>
> > >> Plus, the backend is currently one-shot. This means that anybody that
> > >> does actually implement the approach where synchronous results are
> > >> returned from the query function will see one async callback, and then
> > >> all results are returned synchronously. They either won't write the
> > >> ability to continue getting async results, or that code path will be
> > >> completely untested in their code.
> > >>
> > >> If we do actually support incremental querying in our backend later,
> > >> it will light up all of this code in existing plugins. If people
> > >> actually start using this API and we care about not breaking them, I
> > >> suspect this means we can never change it to deliver results in an
> > >> incremental manner.
> > >>
> > >> So my inclination is to make the current API do what 98% of all users
> > >> want now anyway: "give me all the results in the simplest possible way
> > >> and tell me when you're done." Later, if we add an incremental
> > >> backend, we can add an incremental API to it and the remaining 2% of
> > >> programmers who want to do an incremental interface can program to
> > >> that API and test their code properly.
> > >>
> > >> Brett
> > >>
> > >> On Fri, Jan 18, 2013 at 11:26 AM, Darin Fisher
<mailto:darin@chromium.org>
> > wrote:
> > >> > Imagine enumerating the files on a network share.  An app might like
> > to
> > >> > incrementally render the contents of a folder.  With ReadEntries you
> > are
> > >> > unable to do that and moreover left showing the user a busy indicator
> > >> > with
> > >> > no possible way of estimating time to completion.  This API forces bad
> > >> > UI,
> > >> > and we'd be stuck with apps using it potentially for a long time.
> > >> >
> > >> > With the former API, you can do a suboptimal impl that you can
> > >> > seamlessly
> > >> > improve later.  Seems like the right compromise to me.
> > >> >
> > >> > On Jan 18, 2013 8:59 AM, <mailto:dmichael@chromium.org> wrote:
> > >> >>
> > >> >> On 2013/01/18 07:12:59, brettw wrote:
> > >> >>>
> > >> >>> There was some discussion on the thread titled "Does anything use
> > >> >>> PPB_DirectoryReader_Dev"
> > >> >>
> > >> >> I think the simple way is fine while this is still in Dev. But I
> > worry
> > >> >> that if
> > >> >> returning everything becomes a problem, we'll be saddled with
> > >> >> supporting
> > >> >> the old
> > >> >> interface even if we update the API.
> > >> >>
> > >> >>
> > >> >>> Brett
> > >> >>
> > >> >>
> > >> >>> On Thu, Jan 17, 2013 at 10:26 PM,  <mailto:darin@chromium.org>
> > wrote:
> > >> >>> >
> > >> >>> >
> > >> >>
> > >> >>
> > >> >>
> > >> >>
> > >> >>
> >
>
https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
> > >> >>>
> > >> >>> > File ppapi/api/dev/ppb_directory_reader_dev.idl (right):
> > >> >>> >
> > >> >>> >
> > >> >>
> > >> >>
> > >> >>
> > >> >>
> > >> >>
> >
>
https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_director...
> > >> >>>
> > >> >>> > ppapi/api/dev/ppb_directory_reader_dev.idl:67: [in] PP_ArrayOutput
> > >> >>> > output,
> > >> >>> > Most operating systems don't provide APIs like this because they
> > can
> > >> >>> > take a long time to execute and consume a large amount of memory.
> > >> >>> > Consider large directories.  Instead, they provide APIs to
> > >> >>> > progressively
> > >> >>> > fetch items (e.g., readdir and FindNextFile).
> > >> >>> >
> > >> >>> > Also, we could easily implement ReadEntries in terms of
> > >> >>> > GetNextEntry,
> > >> >>> > so
> > >> >>> > it seems like ReadEntries could be a helper function if
> > applications
> > >> >>> > wish to have a simpler way to read all of the entries in a
> > >> >>> > directory.
> > >> >>> > It just seems like the core API should be more flexible.
> > >> >>> >
> > >> >>> > (It could be the case that we temporarily implement GetNextEntry
> > >> >>> > using
> > >> >>> > a
> > >> >>> > sub-optimal IPC to fetch all entries in one go, but that could be
> > >> >>> > fixed
> > >> >>> > in the future.)
> > >> >>> >
> > >> >>> > https://codereview.chromium.org/12026008/
> > >> >>
> > >> >>
> > >> >>
> > >> >>
> > >> >> https://codereview.chromium.org/12026008/
> > >
> > >
> >

[darin (slow to review), 2013-01-24 09:19:24]

I'm supportive.  What convinced me was the argument that GetNextEntry
really isn't the ideal interface for people who like to use non-optional
callbacks.  Instead, we'd want some interface that delivers arrays of
entries, so that callbacks are generated per chunk.

The original interface was designed back before non-optional callbacks
existed.

-Darin



On Wed, Jan 23, 2013 at 8:44 PM, <nhiroki@chromium.org> wrote:

> Thank you for a lot of comments.
>
> Could we start to implement new API? There's a project who needs (old or
> new)
> DircetoryReader API and we'd like to deliver it to them asap.
>
> Let me get the discussion straight:
> - The ReadEntries API is okay (without no helper function on GetNextEntry).
> - GetNextEntry should be removed.
>
>
>
>  Would it be stupid to put some high cap on how many entries we'll retrieve
>> with this API? And if that's ever too small for some app, we can consider
>> adding the "expert" version that can actually get all entries.
>>
>
>  Or just hope for the best. It does look like the API for JavaScript has
>> the
>> exact same problem:
>>
>
> https://developer.mozilla.org/**en-US/docs/DOM/File_API/File_**
>
System_API/DirectoryReader<https://developer.mozilla.org/en-US/docs/DOM/File_API/File_System_API/DirectoryReader>
>
> I think that the latter way is better, too. For now, we'll implement the
> new API
> to return all entries at once, and then make it possible to return entries
> in
> multiple chunks. Is it okay with you?
>
> Thanks!
>
>
> On 2013/01/18 21:55:10, dmichael wrote:
>
>> On Fri, Jan 18, 2013 at 2:33 PM, Brett Wilson <mailto:brettw@chromium.org
>> >
>>
> wrote:
>
>  > If we're guaranteed that everybody either writes their code properly
>> > to handle the case that Chrome doesn't do yet, or uses the helper,
>> > that sounds OK to me. But I'm not super comfortable with this.
>> >
>> > Thinking about "real" OS APIs. Microsoft would never change this kind
>> > of thing in a Win32 function, and it doesn't matter if the ATL/MFC
>> > wrapper for it does the right thing because there are always people
>> > that write their own. I think we need to get more into this mindset or
>> > we'll be stuck later.
>> >
>> > Brett
>> >
>> The suggested API here is definitely simpler and what developers would
>> want. I was just worried about supporting it long term.
>>
>
>  Would it be stupid to put some high cap on how many entries we'll retrieve
>> with this API? And if that's ever too small for some app, we can consider
>> adding the "expert" version that can actually get all entries.
>>
>
>  Or just hope for the best. It does look like the API for JavaScript has
>> the
>> exact same problem:
>>
>
> https://developer.mozilla.org/**en-US/docs/DOM/File_API/File_**
>
System_API/DirectoryReader<https://developer.mozilla.org/en-US/docs/DOM/File_API/File_System_API/DirectoryReader>
>
>
>  >
>> >
>> >
>> > On Fri, Jan 18, 2013 at 1:26 PM, Darin Fisher <mailto:
>> darin@chromium.org>
>>
> wrote:
>
>> > > That makes a lot of sense.  Last shot by me:  I suppose we could also
>> > > provide a helper function that implements ReadEntries efficiently on
>> top
>> > of
>> > > GetNextEntry.  Why not do that?  Isn't that a
>> > have-our-cake-and-eat-it-too
>> > > solution?
>> >
>> I don't think it would be that easy to implement ReadEntries on top of
>> GetNextEntry. You could make it work on background threads with blocking
>> completion callbacks, but I don't see how you could make it work in
>> general
>> on the main thread?
>>
>
>
>
>
>  > >
>> > > -Darin
>> > >
>> > >
>> > > On Fri, Jan 18, 2013 at 12:20 PM, Brett Wilson
>>
> <mailto:brettw@chromium.org>
>
>  > wrote:
>> > >>
>> > >> I don't disagree that the incremental approach is theoretically
>> better.
>> > >>
>> > >> My concern was practical: just thinking about using this API makes my
>> > >> mind boggle. The basic way to do this is to do an async callback for
>> > >> each result. This seems pretty insane and will make everything
>> slower,
>> > >> forcing a trip back to the message loop for each result, and is super
>> > >> annoying to program to.
>> > >>
>> > >> Doing the "handle synchronous results" thing like we do for the URL
>> > >> loader is exceedingly tricky to get right, so much so that the
>> initial
>> > >> URLLoader examples we wrote were wrong! I don't think we should
>> > >> encourage this pattern in anything but the url loader, where you can
>> > >> copy our example.
>> > >>
>> > >> Plus, the backend is currently one-shot. This means that anybody that
>> > >> does actually implement the approach where synchronous results are
>> > >> returned from the query function will see one async callback, and
>> then
>> > >> all results are returned synchronously. They either won't write the
>> > >> ability to continue getting async results, or that code path will be
>> > >> completely untested in their code.
>> > >>
>> > >> If we do actually support incremental querying in our backend later,
>> > >> it will light up all of this code in existing plugins. If people
>> > >> actually start using this API and we care about not breaking them, I
>> > >> suspect this means we can never change it to deliver results in an
>> > >> incremental manner.
>> > >>
>> > >> So my inclination is to make the current API do what 98% of all users
>> > >> want now anyway: "give me all the results in the simplest possible
>> way
>> > >> and tell me when you're done." Later, if we add an incremental
>> > >> backend, we can add an incremental API to it and the remaining 2% of
>> > >> programmers who want to do an incremental interface can program to
>> > >> that API and test their code properly.
>> > >>
>> > >> Brett
>> > >>
>> > >> On Fri, Jan 18, 2013 at 11:26 AM, Darin Fisher
>>
> <mailto:darin@chromium.org>
>
>> > wrote:
>> > >> > Imagine enumerating the files on a network share.  An app might
>> like
>> > to
>> > >> > incrementally render the contents of a folder.  With ReadEntries
>> you
>> > are
>> > >> > unable to do that and moreover left showing the user a busy
>> indicator
>> > >> > with
>> > >> > no possible way of estimating time to completion.  This API forces
>> bad
>> > >> > UI,
>> > >> > and we'd be stuck with apps using it potentially for a long time.
>> > >> >
>> > >> > With the former API, you can do a suboptimal impl that you can
>> > >> > seamlessly
>> > >> > improve later.  Seems like the right compromise to me.
>> > >> >
>> > >> > On Jan 18, 2013 8:59 AM, <mailto:dmichael@chromium.org> wrote:
>> > >> >>
>> > >> >> On 2013/01/18 07:12:59, brettw wrote:
>> > >> >>>
>> > >> >>> There was some discussion on the thread titled "Does anything use
>> > >> >>> PPB_DirectoryReader_Dev"
>> > >> >>
>> > >> >> I think the simple way is fine while this is still in Dev. But I
>> > worry
>> > >> >> that if
>> > >> >> returning everything becomes a problem, we'll be saddled with
>> > >> >> supporting
>> > >> >> the old
>> > >> >> interface even if we update the API.
>> > >> >>
>> > >> >>
>> > >> >>> Brett
>> > >> >>
>> > >> >>
>> > >> >>> On Thu, Jan 17, 2013 at 10:26 PM,  <mailto:darin@chromium.org>
>> > wrote:
>> > >> >>> >
>> > >> >>> >
>> > >> >>
>> > >> >>
>> > >> >>
>> > >> >>
>> > >> >>
>> >
>>
>
> https://codereview.chromium.**org/12026008/diff/1002/ppapi/**
>
api/dev/ppb_directory_reader_**dev.idl<https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_directory_reader_dev.idl>
>
>> > >> >>>
>> > >> >>> > File ppapi/api/dev/ppb_directory_**reader_dev.idl (right):
>> > >> >>> >
>> > >> >>> >
>> > >> >>
>> > >> >>
>> > >> >>
>> > >> >>
>> > >> >>
>> >
>>
>
> https://codereview.chromium.**org/12026008/diff/1002/ppapi/**
>
api/dev/ppb_directory_reader_**dev.idl#newcode67<https://codereview.chromium.org/12026008/diff/1002/ppapi/api/dev/ppb_directory_reader_dev.idl#newcode67>
>
>> > >> >>>
>> > >> >>> > ppapi/api/dev/ppb_directory_**reader_dev.idl:67: [in]
>> PP_ArrayOutput
>> > >> >>> > output,
>> > >> >>> > Most operating systems don't provide APIs like this because
>> they
>> > can
>> > >> >>> > take a long time to execute and consume a large amount of
>> memory.
>> > >> >>> > Consider large directories.  Instead, they provide APIs to
>> > >> >>> > progressively
>> > >> >>> > fetch items (e.g., readdir and FindNextFile).
>> > >> >>> >
>> > >> >>> > Also, we could easily implement ReadEntries in terms of
>> > >> >>> > GetNextEntry,
>> > >> >>> > so
>> > >> >>> > it seems like ReadEntries could be a helper function if
>> > applications
>> > >> >>> > wish to have a simpler way to read all of the entries in a
>> > >> >>> > directory.
>> > >> >>> > It just seems like the core API should be more flexible.
>> > >> >>> >
>> > >> >>> > (It could be the case that we temporarily implement
>> GetNextEntry
>> > >> >>> > using
>> > >> >>> > a
>> > >> >>> > sub-optimal IPC to fetch all entries in one go, but that could
>> be
>> > >> >>> > fixed
>> > >> >>> > in the future.)
>> > >> >>> >
>> > >> >>> >
https://codereview.chromium.**org/12026008/<https://codereview.chromium.org/1...
>> > >> >>
>> > >> >>
>> > >> >>
>> > >> >>
>> > >> >>
https://codereview.chromium.**org/12026008/<https://codereview.chromium.org/1...
>> > >
>> > >
>> >
>>
>
>
>
https://codereview.chromium.**org/12026008/<https://codereview.chromium.org/1...
>

[yzshen1, 2013-01-24 19:16:13]

One more thing about GetNextEntry(): (not quite sure whether others have
mentioned it elsewhere.)

Unless we have a method to abort GetNextEntry() explicitly, it is not easy for
the user to manage the memory allocated for the output parameter. Currently if
the callback never fires until the resource ref count reaches 0 and it gets
aborted, the user is forced to retain that memory block. (Using
CompletionCallbackFactory doesn't help in this case.)

We have met this problem several times with other APIs.

[dmichael (off chromium), 2013-01-24 20:08:26]

On 2013/01/24 09:19:24, darin wrote:
> I'm supportive.  What convinced me was the argument that GetNextEntry
> really isn't the ideal interface for people who like to use non-optional
> callbacks.  Instead, we'd want some interface that delivers arrays of
> entries, so that callbacks are generated per chunk.
> 
> The original interface was designed back before non-optional callbacks
> existed.
> 
> -Darin
Sure, sounds good to me too. The API is so much easier to get right this way,
it's probably worth the risk. If it is ever a performance problem, we can start
capping the number of files you get from this API or something.

If you want to check this in before implementing it, you can do that...  just
get rid of the old version of the API from the IDL first.