PPAPI: Move PPB_DirectoryReader_Dev to stable (won't commit)

PPAPI: Move PPB_DirectoryReader_Dev to stable

A separate patch for the same issue was landed, so I'd like to close this without commit.
https://codereview.chromium.org/14784002/


BUG=234513
TEST=browser_tests --gtest_filter=\*DirectoryReader\*

[nhiroki, 2013-04-23 05:24:42]

Hi,

yusukes@'d like this change *ASAP*. I think that he or someone in his team will
ask pepper folks (pepper-team@ ??) whether we can move the reader to stable.

binji: I'm sorry for stealing your task... Could you review native_client_sdk/? 
yzshen: Could you review ppapi/ and webkit/plugins/ppapi/?

Thanks,
nhiroki

[raymes1, 2013-04-23 07:04:55]

yzshen is OOO until next week. How urgent is this?

On Mon, Apr 22, 2013 at 10:24 PM,  <nhiroki@chromium.org> wrote:
> Reviewers: yzshen1, binji,
>
> Message:
> Hi,
>
> yusukes@'d like this change *ASAP*. I think that he or someone in his team
> will
> ask pepper folks (pepper-team@ ??) whether we can move the reader to stable.
>
> binji: I'm sorry for stealing your task... Could you review
> native_client_sdk/?
> yzshen: Could you review ppapi/ and webkit/plugins/ppapi/?
>
> Thanks,
> nhiroki
>
> Description:
> PPAPI: Move PPB_DirectoryReader_Dev to stable
>
>
> BUG=113086
>
> Please review this at https://codereview.chromium.org/14419003/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   M native_client_sdk/src/libraries/nacl_io/mount_node_html5fs.cc
>   M native_client_sdk/src/libraries/nacl_io/pepper/all_interfaces.h
>   M native_client_sdk/src/libraries/nacl_io/pepper_interface.h
>   M native_client_sdk/src/libraries/nacl_io_test/mount_html5fs_test.cc
>   D ppapi/api/dev/ppb_directory_reader_dev.idl
>   A + ppapi/api/ppb_directory_reader.idl
>   D ppapi/c/dev/ppb_directory_reader_dev.h
>   A + ppapi/c/ppb_directory_reader.h
>   M ppapi/cpp/array_output.h
>   D ppapi/cpp/dev/directory_entry_dev.h
>   D ppapi/cpp/dev/directory_entry_dev.cc
>   D ppapi/cpp/dev/directory_reader_dev.h
>   D ppapi/cpp/dev/directory_reader_dev.cc
>   M ppapi/cpp/dev/file_chooser_dev.h
>   A + ppapi/cpp/directory_entry.h
>   A + ppapi/cpp/directory_entry.cc
>   A + ppapi/cpp/directory_reader.h
>   A + ppapi/cpp/directory_reader.cc
>   M ppapi/cpp/output_traits.h
>   M ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c
>   M ppapi/ppapi_sources.gypi
>   M ppapi/proxy/directory_reader_resource.cc
>   M ppapi/proxy/interface_list.cc
>   M ppapi/proxy/ppapi_messages.h
>   M ppapi/tests/all_c_includes.h
>   M ppapi/tests/all_cpp_includes.h
>   M ppapi/tests/test_directory_reader.cc
>   M ppapi/tests/test_file_ref.cc
>   M ppapi/thunk/interfaces_ppb_public_dev.h
>   M ppapi/thunk/interfaces_ppb_public_stable.h
>   M ppapi/thunk/ppb_directory_reader_api.h
>   M ppapi/thunk/ppb_directory_reader_thunk.cc
>   M webkit/plugins/ppapi/plugin_module.cc
>
>

[nhiroki, 2013-04-23 07:40:47]

On 2013/04/23 07:04:55, raymes1 wrote:
> yzshen is OOO until next week. How urgent is this?

Thanks for your information!

He desires that it'll be available within this week. Okay, I'll find another
reviewer.

[nhiroki, 2013-04-23 07:49:19]

+dmichael@

Hi dmichael, could you review ppapi/ and webkit/plugins/ppapi/? Thanks.

[dmichael (off chromium), 2013-04-23 16:08:59]

Thank you for doing this! Looks good overall. I just have some nits because we
have a higher bar for the documentation of stable interfaces, and we need to
maintain at least binary-level compatibility for the Dev interface for a while.

Raymes: Do we also need the C++ wrappers to live for a little while? I guess
Chromoting might use this as well...  so we should probably just leave the C++
wrappers for now.

https://codereview.chromium.org/14419003/diff/1/ppapi/api/dev/ppb_directory_r...
File ppapi/api/dev/ppb_directory_reader_dev.idl (left):

https://codereview.chromium.org/14419003/diff/1/ppapi/api/dev/ppb_directory_r...
ppapi/api/dev/ppb_directory_reader_dev.idl:46: };
I think we may need this to stick around for a little while for Flash. You might
check with Raymes. At a minimum, we need to make sure that the Dev interface
still works, even if we don't publish the header.

https://codereview.chromium.org/14419003/diff/1/ppapi/api/ppb_directory_reade...
File ppapi/api/ppb_directory_reader.idl (right):

https://codereview.chromium.org/14419003/diff/1/ppapi/api/ppb_directory_reade...
ppapi/api/ppb_directory_reader.idl:20: interface PPB_DirectoryReader {
Please add a brief interface-level comment explaining what capability this
interface provides.

https://codereview.chromium.org/14419003/diff/1/ppapi/api/ppb_directory_reade...
ppapi/api/ppb_directory_reader.idl:23: // DirectoryReader object until such time
as the DirectoryReader object is
nit: All the comments should probably be C-style here (see other IDL files)

https://codereview.chromium.org/14419003/diff/1/ppapi/cpp/dev/directory_entry...
File ppapi/cpp/dev/directory_entry_dev.h (left):

https://codereview.chromium.org/14419003/diff/1/ppapi/cpp/dev/directory_entry...
ppapi/cpp/dev/directory_entry_dev.h:38: #endif  //
PPAPI_CPP_DEV_DIRECTORY_ENTRY_DEV_H_
Ditto; check with Raymes. We may want to keep the "Dev" version around for a
short bit so Flash doesn't need to change to the non-dev interface and recompile
immediately.

https://codereview.chromium.org/14419003/diff/1/ppapi/cpp/directory_entry.h
File ppapi/cpp/directory_entry.h (right):

https://codereview.chromium.org/14419003/diff/1/ppapi/cpp/directory_entry.h#n...
ppapi/cpp/directory_entry.h:13: class DirectoryEntry {
Please add some documentation for the class and constructors.

https://codereview.chromium.org/14419003/diff/1/ppapi/cpp/directory_entry.h#n...
ppapi/cpp/directory_entry.h:23: // Returns true if the DirectoryEntry is invalid
or uninitialized.
The comments here should be in doxygen-style (three slashes: "///"). See other
headers in ppapi/cpp

https://codereview.chromium.org/14419003/diff/1/ppapi/thunk/interfaces_ppb_pu...
File ppapi/thunk/interfaces_ppb_public_dev.h (left):

https://codereview.chromium.org/14419003/diff/1/ppapi/thunk/interfaces_ppb_pu...
ppapi/thunk/interfaces_ppb_public_dev.h:16: PPB_DirectoryReader_Dev_0_6)
This is the part that I think you will still need to provide for some amount of
time, to provide binary-compatibility for Flash.

You'll want to keep at least the C header so that you have the
PPB_DIRECTORYREADER_DEV_INTERFACE_0_6 string and PPB_DirectoryReader_Dev_0_6
struct.

https://codereview.chromium.org/14419003/diff/1/ppapi/thunk/ppb_directory_rea...
File ppapi/thunk/ppb_directory_reader_thunk.cc (right):

https://codereview.chromium.org/14419003/diff/1/ppapi/thunk/ppb_directory_rea...
ppapi/thunk/ppb_directory_reader_thunk.cc:58: }
Here, you will want a vtable struct for the stable and dev, and a function for
getting each. They can use the same 3 implementation functions.

[dmichael (off chromium), 2013-04-23 16:21:54]

I apologize, I was just re-reading the discussion we had around this API and
realized that Flash and Chromoting almost certainly do not use this.

Raymes: can you confirm Flash still does not use this?

I take back what I said about backwards compatibility. We can probably delete
all traces of the Dev version as you have done.

[yzshen1, 2013-04-23 17:14:01]

Thanks for taking care of this, David!

On 2013/04/23 16:21:54, dmichael wrote:
> I apologize, I was just re-reading the discussion we had around this API and
> realized that Flash and Chromoting almost certainly do not use this.
> 
> Raymes: can you confirm Flash still does not use this?
> 
> I take back what I said about backwards compatibility. We can probably delete
> all traces of the Dev version as you have done.

[binji, 2013-04-23 17:26:27]

native_client_sdk lgtm, thanks!

[raymes1, 2013-04-24 00:02:20]

I confirmed with ihf that this is not used in Flash. In general a manifest check
should fail on the buildbots if we try to remove an interface used by Flash.

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

Thank you for your review and comments!
Please take another look, thanks.

https://codereview.chromium.org/14419003/diff/1/ppapi/api/dev/ppb_directory_r...
File ppapi/api/dev/ppb_directory_reader_dev.idl (left):

https://codereview.chromium.org/14419003/diff/1/ppapi/api/dev/ppb_directory_r...
ppapi/api/dev/ppb_directory_reader_dev.idl:46: };
On 2013/04/23 16:08:59, dmichael wrote:
> I think we may need this to stick around for a little while for Flash. You
might
> check with Raymes. At a minimum, we need to make sure that the Dev interface
> still works, even if we don't publish the header.

Raymes confirmed that Flash doesn't use this. Wont fix.

https://codereview.chromium.org/14419003/diff/1/ppapi/api/ppb_directory_reade...
File ppapi/api/ppb_directory_reader.idl (right):

https://codereview.chromium.org/14419003/diff/1/ppapi/api/ppb_directory_reade...
ppapi/api/ppb_directory_reader.idl:20: interface PPB_DirectoryReader {
On 2013/04/23 16:08:59, dmichael wrote:
> Please add a brief interface-level comment explaining what capability this
> interface provides.

Done.

https://codereview.chromium.org/14419003/diff/1/ppapi/api/ppb_directory_reade...
ppapi/api/ppb_directory_reader.idl:23: // DirectoryReader object until such time
as the DirectoryReader object is
On 2013/04/23 16:08:59, dmichael wrote:
> nit: All the comments should probably be C-style here (see other IDL files)

Done.

https://codereview.chromium.org/14419003/diff/1/ppapi/cpp/dev/directory_entry...
File ppapi/cpp/dev/directory_entry_dev.h (left):

https://codereview.chromium.org/14419003/diff/1/ppapi/cpp/dev/directory_entry...
ppapi/cpp/dev/directory_entry_dev.h:38: #endif  //
PPAPI_CPP_DEV_DIRECTORY_ENTRY_DEV_H_
On 2013/04/23 16:08:59, dmichael wrote:
> Ditto; check with Raymes. We may want to keep the "Dev" version around for a
> short bit so Flash doesn't need to change to the non-dev interface and
recompile
> immediately.

Ditto: Confirmed Flash does not use this. Wont fix.

https://codereview.chromium.org/14419003/diff/1/ppapi/cpp/directory_entry.h
File ppapi/cpp/directory_entry.h (right):

https://codereview.chromium.org/14419003/diff/1/ppapi/cpp/directory_entry.h#n...
ppapi/cpp/directory_entry.h:13: class DirectoryEntry {
On 2013/04/23 16:08:59, dmichael wrote:
> Please add some documentation for the class and constructors.

Done.

https://codereview.chromium.org/14419003/diff/1/ppapi/cpp/directory_entry.h#n...
ppapi/cpp/directory_entry.h:23: // Returns true if the DirectoryEntry is invalid
or uninitialized.
On 2013/04/23 16:08:59, dmichael wrote:
> The comments here should be in doxygen-style (three slashes: "///"). See other
> headers in ppapi/cpp

Done.

https://codereview.chromium.org/14419003/diff/1/ppapi/thunk/interfaces_ppb_pu...
File ppapi/thunk/interfaces_ppb_public_dev.h (left):

https://codereview.chromium.org/14419003/diff/1/ppapi/thunk/interfaces_ppb_pu...
ppapi/thunk/interfaces_ppb_public_dev.h:16: PPB_DirectoryReader_Dev_0_6)
On 2013/04/23 16:08:59, dmichael wrote:
> This is the part that I think you will still need to provide for some amount
of
> time, to provide binary-compatibility for Flash.
> 
> You'll want to keep at least the C header so that you have the
> PPB_DIRECTORYREADER_DEV_INTERFACE_0_6 string and PPB_DirectoryReader_Dev_0_6
> struct.

Ditto: Confirmed Flash does not use this. Wont fix.

https://codereview.chromium.org/14419003/diff/1/ppapi/thunk/ppb_directory_rea...
File ppapi/thunk/ppb_directory_reader_thunk.cc (right):

https://codereview.chromium.org/14419003/diff/1/ppapi/thunk/ppb_directory_rea...
ppapi/thunk/ppb_directory_reader_thunk.cc:58: }
On 2013/04/23 16:08:59, dmichael wrote:
> Here, you will want a vtable struct for the stable and dev, and a function for
> getting each. They can use the same 3 implementation functions.

Ditto: Confirmed Flash does not use this. Wont fix.

[nhiroki, 2013-04-24 02:06:31]

+jln@

Hi Julien, could you review ppapi/proxy/ppapi_messages.h?
This is one line include change. Thanks.

[jln (very slow on Chromium), 2013-04-24 05:20:51]

On 2013/04/24 02:06:31, nhiroki wrote:
> +jln@
> 
> Hi Julien, could you review ppapi/proxy/ppapi_messages.h?
> This is one line include change. Thanks.

ppapi/proxy/ppapi_messages.h lgtm

[dmichael (off chromium), 2013-04-24 16:10:20]

I'm thinking now that this is going to stable, we could just put ReadEntries on
PPB_FileRef. It seems cleaner.

Does anybody object to that?

[nhiroki, 2013-04-25 02:28:00]

On 2013/04/24 16:10:20, dmichael wrote:
> I'm thinking now that this is going to stable, we could just put ReadEntries
on
> PPB_FileRef. It seems cleaner.
> 
> Does anybody object to that?

It looks simpler and easier to use. Anyway I'll make a CL for moving ReadEntries
in parallel with this. Please review it.

[dmichael (off chromium), 2013-04-25 02:48:17]

On Wed, Apr 24, 2013 at 8:28 PM, <nhiroki@chromium.org> wrote:

> On 2013/04/24 16:10:20, dmichael wrote:
>
>> I'm thinking now that this is going to stable, we could just put
>> ReadEntries
>>
> on
>
>> PPB_FileRef. It seems cleaner.
>>
>
>  Does anybody object to that?
>>
>
> It looks simpler and easier to use. Anyway I'll make a CL for moving
> ReadEntries
> in parallel with this. Please review it.
>
>
https://codereview.chromium.**org/14419003/<https://codereview.chromium.org/1...
>
I'm sorry, I'm not sure I understand what you mean. I believe we should
either take PPB_DirectoryReader out of dev _or_ add ReadEntries to
PPB_FileRef (which is a stable interface). We should not do both, because
we don't want to have to maintain both. I think I prefer adding ReadEntries
to PPB_FileRef. Are you saying you will do a separate CL for that, so that
we can compare them and land whichever one we decide on? That sounds like a
good idea. Or you can try it in this CL; that would be fine as well.

[nhiroki, 2013-04-25 03:40:52]

On 2013/04/25 02:48:17, dmichael wrote:
> I'm sorry, I'm not sure I understand what you mean.

Sorry for confusing you.

> Are you saying you will do a separate CL for that, so that
> we can compare them and land whichever one we decide on?

Yes, I meant to say this.

[kinuko, 2013-04-25 04:16:26]

On 2013/04/25 03:40:52, nhiroki wrote:
> On 2013/04/25 02:48:17, dmichael wrote:
> > I'm sorry, I'm not sure I understand what you mean.
> 
> Sorry for confusing you.
> 
> > Are you saying you will do a separate CL for that, so that
> > we can compare them and land whichever one we decide on?
> 
> Yes, I meant to say this.

Can we decide one of either before spending too much resource?

[nhiroki, 2013-04-25 04:36:55]

On 2013/04/25 04:16:26, kinuko wrote:
> Can we decide one of either before spending too much resource?

You are right. dmichael, do pepper folks have any consensus about moving
ReadEntries?

FYI: Although I said "make ... in parallel", I think this patch has been almost
done, so there is no tasks to do for now.

[dmichael (off chromium), 2013-04-25 22:14:29]

I think we should put it on PPB_FileRef. This seems like the best thing from an
API perspective. I apologize for not suggesting this sooner.