Move DirectoryReader::ReadEntries to FileRef::ReadDirectoryEntries

Move DirectoryReader::ReadEntries to FileRef::ReadDirectoryEntries

This also means this API becomes a stable API.

While DirectoryReader was using the new pepper proxy API, FileRef is
using the old one. As updating FileRef would take some time, the
implementation of ReadEntries is converted from the new design to the
old one for now.

BUG=234513
TEST=browser_tests
R=avi@chromium.org, binji@chromium.org, dmichael@chromium.org, palmer@chromium.org, raymes@chromium.org, teravest@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198204

[jam, 2013-05-01 19:34:40]

redirecting to avi

[Avi (use Gerrit), 2013-05-01 19:40:34]

Please specify who should be reviewing what. content/ code lgtm.

[hamaji, 2013-05-01 19:50:13]

Sorry,

avi (OWNERS): content/content_renderer.gypi
palmer (OWNERS) and aedla: ppapi/proxy/ppapi_messsages.h
dmichael, yzshen1: everything else

[palmer, 2013-05-01 19:57:40]

I don't understand the security implications of this large change, and the bug
doesn't have much in the way of detail. Is there a design document?

[hamaji, 2013-05-01 20:31:39]

This change just moves an existing PPAPI which returns the list of filenames in
an HTML5 directory from DirectoryReader_Dev::ReadEntries to
FileRef::ReadEntries. It's unfortunate a CL which just moves a function becomes
that big.

Let me elaborate. There are three changes:

1). Move DirectoryReader::ReadEntries to FileRef::ReadEntries and remove
DirectoryReader class
2). Make a "listdir" API stable
3). Use pepper old design instead of new design

The 1) requires the change to ppapi_messages.h. 1) was requested in a CL
(https://codereview.chromium.org/14419003/) which does 2), and 3) is required
because FileRef uses the old design.

[hamaji, 2013-05-02 03:41:35]

+binji

Hi Ben, could you review the changes for native_client_sdk/*?

I took over this work from nhiroki (https://codereview.chromium.org/14419003/).
Now we have FileRef::ReadEntries instead of DirectoryReader::ReadEntries.

Thanks!

[binji, 2013-05-02 05:18:27]

native_client_sdk lgtm, thanks!

[teravest, 2013-05-02 17:32:09]

It's weird to see so much use of OwnedWrapper in this patch; it's not used very
often in the Chrome codebase at all (with the exception of the one existing use
in ppb_file_ref_proxy). Are you sure this is the right approach?

https://codereview.chromium.org/14784002/diff/8001/ppapi/api/ppb_file_ref.idl
File ppapi/api/ppb_file_ref.idl (right):

https://codereview.chromium.org/14784002/diff/8001/ppapi/api/ppb_file_ref.idl...
ppapi/api/ppb_file_ref.idl:200: [version=1.1]
You'll want to make this version 1.2=M29 unless this gets in really soon.

https://codereview.chromium.org/14784002/diff/8001/ppapi/proxy/ppb_file_ref_p...
File ppapi/proxy/ppb_file_ref_proxy.cc (right):

https://codereview.chromium.org/14784002/diff/8001/ppapi/proxy/ppb_file_ref_p...
ppapi/proxy/ppb_file_ref_proxy.cc:488: int32_t set_result =
Why bother setting the read entries if the result != PP_OK?

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
File webkit/plugins/ppapi/file_callbacks.cc (right):

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
webkit/plugins/ppapi/file_callbacks.cc:29: std::string
FilePathStringToUTF8String(const base::FilePath::StringType& str) {
This is a weird place to have platform-specific treatment of file paths. Can you
add a comment about what's going on here?

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
webkit/plugins/ppapi/file_callbacks.cc:126: std::vector<
::ppapi::PPB_FileRef_CreateInfo>* files
'=' should be on the first line here.

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
webkit/plugins/ppapi/file_callbacks.cc:138:
UTF8StringToFilePathString(dir_path);
I feel like there has to be a better way to handle the string manipulation of
these paths. Why the converstion back and forth to FilePathString?

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
File webkit/plugins/ppapi/file_callbacks.h (right):

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
webkit/plugins/ppapi/file_callbacks.h:42: // Don't take the ownership of
members.
s/Don't/Doesn't/ ?

[yzshen1, 2013-05-02 17:39:14]

On 2013/05/02 05:18:27, binji wrote:
> native_client_sdk lgtm, thanks!

Redirecting to Raymes. (Thanks a lot, Raymes!)

[hamaji, 2013-05-02 18:49:39]

As for Owned, I didn't no this isn't so popular. I just followed the way Query
did (you wrote Query, right?). I think Owned was a bad choice for
file_callbacks, where ownership move was actually unnecessary, so I've fixed it
in file_callbacks. As for ppb_file_ref_proxy, I'm feeling Owned is optimal,
given how pepper old design works. As we won't need Owned once we move FileRef
to the new world, it might be OK as is?

https://codereview.chromium.org/14784002/diff/8001/ppapi/api/ppb_file_ref.idl
File ppapi/api/ppb_file_ref.idl (right):

https://codereview.chromium.org/14784002/diff/8001/ppapi/api/ppb_file_ref.idl...
ppapi/api/ppb_file_ref.idl:200: [version=1.1]
We want this real soon and I'd like to land this before the branch cut.

https://codereview.chromium.org/14784002/diff/8001/ppapi/proxy/ppb_file_ref_p...
File ppapi/proxy/ppb_file_ref_proxy.cc (right):

https://codereview.chromium.org/14784002/diff/8001/ppapi/proxy/ppb_file_ref_p...
ppapi/proxy/ppb_file_ref_proxy.cc:488: int32_t set_result =
On 2013/05/02 17:32:10, teravest wrote:
> Why bother setting the read entries if the result != PP_OK?

Done.

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
File webkit/plugins/ppapi/file_callbacks.cc (right):

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
webkit/plugins/ppapi/file_callbacks.cc:29: std::string
FilePathStringToUTF8String(const base::FilePath::StringType& str) {
Removed, by using fileapi::FilePathToString.

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
webkit/plugins/ppapi/file_callbacks.cc:126: std::vector<
::ppapi::PPB_FileRef_CreateInfo>* files
On 2013/05/02 17:32:10, teravest wrote:
> '=' should be on the first line here.

Done.

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
webkit/plugins/ppapi/file_callbacks.cc:138:
UTF8StringToFilePathString(dir_path);
I just moved this code from pepper_directory_reader_host.cc, but I agree. Now we
have only one conversion.

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
File webkit/plugins/ppapi/file_callbacks.h (right):

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
webkit/plugins/ppapi/file_callbacks.h:42: // Don't take the ownership of
members.
On 2013/05/02 17:32:10, teravest wrote:
> s/Don't/Doesn't/ ?

Done.

[teravest, 2013-05-02 19:30:46]

On Thu, May 2, 2013 at 12:49 PM,  <hamaji@chromium.org> wrote:
> As for Owned, I didn't no this isn't so popular. I just followed the way
> Query
> did (you wrote Query, right?). I think Owned was a bad choice for
> file_callbacks, where ownership move was actually unnecessary, so I've fixed
> it
> in file_callbacks. As for ppb_file_ref_proxy, I'm feeling Owned is optimal,
> given how pepper old design works. As we won't need Owned once we move
> FileRef
> to the new world, it might be OK as is?

Yes, you're right. I had to talk to dmichael to understand why we were
using OwnedWrapper, but I forgot. Hopefully this will be cleaned up in
the resource refactor.

>
>
>
> https://codereview.chromium.org/14784002/diff/8001/ppapi/api/ppb_file_ref.idl
> File ppapi/api/ppb_file_ref.idl (right):
>
>
https://codereview.chromium.org/14784002/diff/8001/ppapi/api/ppb_file_ref.idl...
> ppapi/api/ppb_file_ref.idl:200: [version=1.1]
> We want this real soon and I'd like to land this before the branch cut.
>
>
>
https://codereview.chromium.org/14784002/diff/8001/ppapi/proxy/ppb_file_ref_p...
> File ppapi/proxy/ppb_file_ref_proxy.cc (right):
>
>
https://codereview.chromium.org/14784002/diff/8001/ppapi/proxy/ppb_file_ref_p...
> ppapi/proxy/ppb_file_ref_proxy.cc:488: int32_t set_result =
> On 2013/05/02 17:32:10, teravest wrote:
>>
>> Why bother setting the read entries if the result != PP_OK?
>
>
> Done.
>
>
>
https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
> File webkit/plugins/ppapi/file_callbacks.cc (right):
>
>
https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
> webkit/plugins/ppapi/file_callbacks.cc:29: std::string
> FilePathStringToUTF8String(const base::FilePath::StringType& str) {
> Removed, by using fileapi::FilePathToString.
>
>
>
https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
> webkit/plugins/ppapi/file_callbacks.cc:126: std::vector<
> ::ppapi::PPB_FileRef_CreateInfo>* files
> On 2013/05/02 17:32:10, teravest wrote:
>>
>> '=' should be on the first line here.
>
>
> Done.
>
>
>
https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
> webkit/plugins/ppapi/file_callbacks.cc:138:
> UTF8StringToFilePathString(dir_path);
> I just moved this code from pepper_directory_reader_host.cc, but I
> agree. Now we have only one conversion.
>
>
>
https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
> File webkit/plugins/ppapi/file_callbacks.h (right):
>
>
https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
> webkit/plugins/ppapi/file_callbacks.h:42: // Don't take the ownership of
> members.
> On 2013/05/02 17:32:10, teravest wrote:
>>
>> s/Don't/Doesn't/ ?
>
>
> Done.
>
> https://codereview.chromium.org/14784002/

[dmichael (off chromium), 2013-05-02 19:43:53]

Thanks for doing this!!

https://codereview.chromium.org/14784002/diff/1/ppapi/api/pp_directory_entry.idl
File ppapi/api/pp_directory_entry.idl (right):

https://codereview.chromium.org/14784002/diff/1/ppapi/api/pp_directory_entry....
ppapi/api/pp_directory_entry.idl:12: };
^^ Not sure if this part is necessary for just a struct.

https://codereview.chromium.org/14784002/diff/8001/ppapi/api/ppb_file_ref.idl
File ppapi/api/ppb_file_ref.idl (right):

https://codereview.chromium.org/14784002/diff/8001/ppapi/api/ppb_file_ref.idl...
ppapi/api/ppb_file_ref.idl:189: /* Reads all entries in a directory.
nit: please use doxygen comment style: /**
(see others)

https://codereview.chromium.org/14784002/diff/8001/ppapi/api/ppb_file_ref.idl...
ppapi/api/ppb_file_ref.idl:203: [in] PP_CompletionCallback callback);
This is a little bit tricky...  We would usually rev the interface for adding a
function. Maybe we can get away with this if it ships in 28 with Query. The risk
is that some app (e.g. in NaCl) decides to start using it because it showed up
in the SDK, but runs against a version of Chrome that doesn't actually have
support yet. Given that the SDK will probably not have this any sooner than Dev
channel/Canary...  we can probably take that chance this time.

But if we miss 28, we'll have to make FileRef go to version 1.2 for this
function.

https://codereview.chromium.org/14784002/diff/8001/ppapi/proxy/ppb_file_ref_p...
File ppapi/proxy/ppb_file_ref_proxy.cc (right):

https://codereview.chromium.org/14784002/diff/8001/ppapi/proxy/ppb_file_ref_p...
ppapi/proxy/ppb_file_ref_proxy.cc:519:
base::internal::OwnedWrapper<HostCallbackParams> params,
Can you just pass this one by value? It doesn't seem worth the complication to
pass it by pointer.

https://codereview.chromium.org/14784002/diff/8001/ppapi/proxy/ppb_file_ref_p...
ppapi/proxy/ppb_file_ref_proxy.cc:522:
base::internal::OwnedWrapper<std::vector<PP_FileType> > file_types) {
^^^ For these, you might consider linked_ptr<> or auto_ptr<> so you don't have
to use base::internal stuff.

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref.cc
File ppapi/tests/test_file_ref.cc (right):

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:47: sprintf(result, "%d", value);
nit: please use snprintf (or just delete this function; see below)

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:94: it != entries.end(); ++it) {
nit: I prefer each statement in the loop have its own line if they don't fit on
1

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:668: return
ReportError("DeleteDirectoryRecursively", rv);
nit: I'd prefer just:
ASSERT_TRUE(rv == PP_OK || rv == PP_ERROR_FILENOTFOUND);

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:677: sprintf(buffer, "%s/%s%d", test_dir_name,
file_prefix, i);
snprintf

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:690: for (int i = 1; i < 4; ++i) {
suggestion: May make sense to use a constant like:
const int kNumDirectories = 3;
And loop while "i <= kNumDirectories" for readability and to keep the lopos in
sync

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:692: sprintf(buffer, "%s/%s%d", test_dir_name,
dir_prefix, i);
snprintf

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:717: IntegerToString(entries.size());
How about just ASSERT_EQ(entries.size(), sum);
Then you don't even need IntegerToString.

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:739: return "Expected more file paths.";
ASSERT_TRUE(expected_file_names.empty());
ASSERT_TRUE(expected_dir_names.empty());

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
File webkit/plugins/ppapi/file_callbacks.h (right):

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
webkit/plugins/ppapi/file_callbacks.h:58:
base::internal::OwnedWrapper<ReadEntriesParams> params);
Can you just use scoped_ptr and scoped_ptr::Pass? Or, maybe simpler...  It's
just a bundle of 3 pointers. You could just pass it by const-ref & own it by
value.

[teravest, 2013-05-02 20:08:24]

lgtm

[hamaji, 2013-05-02 20:14:48]

Thanks guys for your comments!

https://codereview.chromium.org/14784002/diff/1/ppapi/api/pp_directory_entry.idl
File ppapi/api/pp_directory_entry.idl (right):

https://codereview.chromium.org/14784002/diff/1/ppapi/api/pp_directory_entry....
ppapi/api/pp_directory_entry.idl:12: };
Ah, removed.

https://codereview.chromium.org/14784002/diff/8001/ppapi/api/ppb_file_ref.idl
File ppapi/api/ppb_file_ref.idl (right):

https://codereview.chromium.org/14784002/diff/8001/ppapi/api/ppb_file_ref.idl...
ppapi/api/ppb_file_ref.idl:189: /* Reads all entries in a directory.
On 2013/05/02 19:43:54, dmichael wrote:
> nit: please use doxygen comment style: /**
> (see others)

Done.

https://codereview.chromium.org/14784002/diff/8001/ppapi/api/ppb_file_ref.idl...
ppapi/api/ppb_file_ref.idl:203: [in] PP_CompletionCallback callback);
OK, I'll do my best to land this soon.

https://codereview.chromium.org/14784002/diff/8001/ppapi/proxy/ppb_file_ref_p...
File ppapi/proxy/ppb_file_ref_proxy.cc (right):

https://codereview.chromium.org/14784002/diff/8001/ppapi/proxy/ppb_file_ref_p...
ppapi/proxy/ppb_file_ref_proxy.cc:519:
base::internal::OwnedWrapper<HostCallbackParams> params,
On 2013/05/02 19:43:54, dmichael wrote:
> Can you just pass this one by value? It doesn't seem worth the complication to
> pass it by pointer.

Done.

https://codereview.chromium.org/14784002/diff/8001/ppapi/proxy/ppb_file_ref_p...
ppapi/proxy/ppb_file_ref_proxy.cc:522:
base::internal::OwnedWrapper<std::vector<PP_FileType> > file_types) {
On 2013/05/02 19:43:54, dmichael wrote:
> ^^^ For these, you might consider linked_ptr<> or auto_ptr<> so you don't have
> to use base::internal stuff.

Done. I used linked_ptr because auto_ptr is not allowed by google's C++ style.

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref.cc
File ppapi/tests/test_file_ref.cc (right):

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:47: sprintf(result, "%d", value);
Removed

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:94: it != entries.end(); ++it) {
Yeah, I actually have the same preference :) (as you might know, I just copied
most code from test_directory_reader.cc)

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:668: return
ReportError("DeleteDirectoryRecursively", rv);
On 2013/05/02 19:43:54, dmichael wrote:
> nit: I'd prefer just:
> ASSERT_TRUE(rv == PP_OK || rv == PP_ERROR_FILENOTFOUND);

Done.

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:677: sprintf(buffer, "%s/%s%d", test_dir_name,
file_prefix, i);
On 2013/05/02 19:43:54, dmichael wrote:
> snprintf

Done.

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:690: for (int i = 1; i < 4; ++i) {
On 2013/05/02 19:43:54, dmichael wrote:
> suggestion: May make sense to use a constant like:
> const int kNumDirectories = 3;
> And loop while "i <= kNumDirectories" for readability and to keep the lopos in
> sync

It seems this loop creates directories but the loop above creates files. That
said, I agree adding kNum* would improve readability. Added kNumFiles and
kNumDirectories.

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:692: sprintf(buffer, "%s/%s%d", test_dir_name,
dir_prefix, i);
On 2013/05/02 19:43:54, dmichael wrote:
> snprintf

Done.

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:717: IntegerToString(entries.size());
On 2013/05/02 19:43:54, dmichael wrote:
> How about just ASSERT_EQ(entries.size(), sum);
> Then you don't even need IntegerToString.

Done.

https://codereview.chromium.org/14784002/diff/8001/ppapi/tests/test_file_ref....
ppapi/tests/test_file_ref.cc:739: return "Expected more file paths.";
On 2013/05/02 19:43:54, dmichael wrote:
> ASSERT_TRUE(expected_file_names.empty());
> ASSERT_TRUE(expected_dir_names.empty());

Done.

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
File webkit/plugins/ppapi/file_callbacks.h (right):

https://codereview.chromium.org/14784002/diff/8001/webkit/plugins/ppapi/file_...
webkit/plugins/ppapi/file_callbacks.h:58:
base::internal::OwnedWrapper<ReadEntriesParams> params);
Yeah, I've modified this code to just pass |params| by value after teravest's
comment.

[dmichael (off chromium), 2013-05-02 20:39:12]

Looking pretty good, just a couple of things I noticed...

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

https://codereview.chromium.org/14784002/diff/33003/ppapi/cpp/directory_entry...
ppapi/cpp/directory_entry.h:13: class DirectoryEntry {
Oops, didn't notice before...  please add some doxygen-style C++ comments
("///") for this class. You can probably use what nhiroki had here:
https://codereview.chromium.org/14419003/diff/16001/ppapi/cpp/directory_entry.h

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
File ppapi/proxy/ppb_file_ref_proxy.cc (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:451: enter.callback()));
Hmm, it looks like if enter fails, the completion callback that owns files and
file_types might get destroyed before the FileCallback executes. I think that
would cause the FileCallback to write to the vectors after they're deleted. I'm
pretty sure all this happens on the main WebKit thread, so it should be safe to
use linked_ptr inside the FileCallbacks too to prevent this situation.

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:492: callback_id, infos, file_types);
nit: please use curly braces for >1 line (even when it's 1 statement)

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.h (right):

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.h:85: std::vector<PP_FileType>*
read_entries_file_types_;
As I noted elsewhere, I think these may need to be linked_ptr to avoid
use-after-free.

(This is kind of ugly, but probably good enough for the interim time before
FileRef is refactored)

[raymes, 2013-05-02 20:44:14]

https://codereview.chromium.org/14784002/diff/26001/ppapi/api/ppb_file_ref.idl
File ppapi/api/ppb_file_ref.idl (right):

https://codereview.chromium.org/14784002/diff/26001/ppapi/api/ppb_file_ref.id...
ppapi/api/ppb_file_ref.idl:201: int32_t ReadEntries([in] PP_Resource file_ref,
Now that this has moved to FileRef, maybe we should rename this to
ReadDirectoryEntries (here and elsewhere) so that it is clear.

[raymes, 2013-05-02 21:45:20]

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
File ppapi/proxy/ppb_file_ref_proxy.cc (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:252: return PP_OK;
should this return PP_OK or a failure code? I think this case should never
normally be reached (possibly during resource deletion) so I think a failure
code would be appropriate.

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:274: entries.clear();
nit: I don't think this (or the above entries.clear()) are needed because the
vector will be deallocated when it goes out of scope.

https://codereview.chromium.org/14784002/diff/33003/ppapi/tests/test_file_ref.cc
File ppapi/tests/test_file_ref.cc (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/tests/test_file_ref...
ppapi/tests/test_file_ref.cc:120: // FileRef::ReadEntries is in-process only.
I think you mean out-of-process only?

[hamaji, 2013-05-02 22:45:14]

Renamed ReadEntries to ReadDirectoryEntries. Confirmed browser_tests passes for
FileRef and git grep ReadEntries returns empty.

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

https://codereview.chromium.org/14784002/diff/33003/ppapi/cpp/directory_entry...
ppapi/cpp/directory_entry.h:13: class DirectoryEntry {
On 2013/05/02 20:39:13, dmichael wrote:
> Oops, didn't notice before...  please add some doxygen-style C++ comments
> ("///") for this class. You can probably use what nhiroki had here:
>
https://codereview.chromium.org/14419003/diff/16001/ppapi/cpp/directory_entry.h

Done. Thanks for the URL!

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
File ppapi/proxy/ppb_file_ref_proxy.cc (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:252: return PP_OK;
Oops, I think I tried to follow the logic of Query. I think Query should be
fixed. Done.

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:274: entries.clear();
On 2013/05/02 21:45:21, raymes wrote:
> nit: I don't think this (or the above entries.clear()) are needed because the
> vector will be deallocated when it goes out of scope.

Done.

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:451: enter.callback()));
Done. See the comment in file_callbacks.h for detail.

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:492: callback_id, infos, file_types);
On 2013/05/02 20:39:13, dmichael wrote:
> nit: please use curly braces for >1 line (even when it's 1 statement)

Done.

https://codereview.chromium.org/14784002/diff/33003/ppapi/tests/test_file_ref.cc
File ppapi/tests/test_file_ref.cc (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/tests/test_file_ref...
ppapi/tests/test_file_ref.cc:120: // FileRef::ReadEntries is in-process only.
Oops, done!

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.h (right):

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.h:85: std::vector<PP_FileType>*
read_entries_file_types_;
Done. As we've chatted, I also modified Query as well. Query was actually more
tricky than ReadEntries because it also supports in-process mode as well. I
think I managed to solve the problem by introducing a hack called
maybe_linked_ptr. Please let me know if there is a better solution for this.

[dmichael (off chromium), 2013-05-02 23:20:43]

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

https://codereview.chromium.org/14784002/diff/33003/ppapi/cpp/directory_entry...
ppapi/cpp/directory_entry.h:13: class DirectoryEntry {
On 2013/05/02 22:45:14, hamaji wrote:
> On 2013/05/02 20:39:13, dmichael wrote:
> > Oops, didn't notice before...  please add some doxygen-style C++ comments
> > ("///") for this class. You can probably use what nhiroki had here:
> >
>
https://codereview.chromium.org/14419003/diff/16001/ppapi/cpp/directory_entry.h
> 
> Done. Thanks for the URL!
Please have another look at the link; there was more documentation. You might be
able to just take his version.

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.h (right):

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.h:85: std::vector<PP_FileType>*
read_entries_file_types_;
On 2013/05/02 22:45:14, hamaji wrote:
> Done. As we've chatted, I also modified Query as well. Query was actually more
> tricky than ReadEntries because it also supports in-process mode as well. I
> think I managed to solve the problem by introducing a hack called
> maybe_linked_ptr. Please let me know if there is a better solution for this.
Nobody actually uses FileRef::Query in-process; we should just kill it off now
and not add maybe_linked_ptr.

https://codereview.chromium.org/14784002/diff/55001/ppapi/api/ppb_file_ref.idl
File ppapi/api/ppb_file_ref.idl (right):

https://codereview.chromium.org/14784002/diff/55001/ppapi/api/ppb_file_ref.id...
ppapi/api/ppb_file_ref.idl:190: * READS all entries in a directory.
nit: why all-caps?

https://codereview.chromium.org/14784002/diff/55001/ppapi/c/ppb_file_ref.h
File ppapi/c/ppb_file_ref.h (right):

https://codereview.chromium.org/14784002/diff/55001/ppapi/c/ppb_file_ref.h#ne...
ppapi/c/ppb_file_ref.h:199: * READS all entries in a directory.
why all-caps?

[hamaji, 2013-05-02 23:36:33]

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

https://codereview.chromium.org/14784002/diff/33003/ppapi/cpp/directory_entry...
ppapi/cpp/directory_entry.h:13: class DirectoryEntry {
Oops, sorry :(

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.h (right):

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.h:85: std::vector<PP_FileType>*
read_entries_file_types_;
A good news! Done

https://codereview.chromium.org/14784002/diff/55001/ppapi/api/ppb_file_ref.idl
File ppapi/api/ppb_file_ref.idl (right):

https://codereview.chromium.org/14784002/diff/55001/ppapi/api/ppb_file_ref.id...
ppapi/api/ppb_file_ref.idl:190: * READS all entries in a directory.
On 2013/05/02 23:20:43, dmichael wrote:
> nit: why all-caps?

Done.

https://codereview.chromium.org/14784002/diff/55001/ppapi/c/ppb_file_ref.h
File ppapi/c/ppb_file_ref.h (right):

https://codereview.chromium.org/14784002/diff/55001/ppapi/c/ppb_file_ref.h#ne...
ppapi/c/ppb_file_ref.h:199: * READS all entries in a directory.
On 2013/05/02 23:20:43, dmichael wrote:
> why all-caps?

Done.

[palmer, 2013-05-03 00:07:24]

https://codereview.chromium.org/14784002/diff/33003/ppapi/api/ppb_file_ref.idl
File ppapi/api/ppb_file_ref.idl (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/api/ppb_file_ref.id...
ppapi/api/ppb_file_ref.idl:194: * @param[in] output An output array which will
receive
Shouldn't this be [out], then?

https://codereview.chromium.org/14784002/diff/33003/ppapi/c/ppb_file_ref.h
File ppapi/c/ppb_file_ref.h (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/c/ppb_file_ref.h#ne...
ppapi/c/ppb_file_ref.h:203: * @param[in] output An output array which will
receive
[out] ?

https://codereview.chromium.org/14784002/diff/33003/ppapi/cpp/file_ref.cc
File ppapi/cpp/file_ref.cc (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/cpp/file_ref.cc#new...
ppapi/cpp/file_ref.cc:174: const CompletionCallbackWithOutput<
std::vector<DirectoryEntry> >&
Style nit: No space between < std

https://codereview.chromium.org/14784002/diff/33003/ppapi/cpp/file_ref.h
File ppapi/cpp/file_ref.h (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/cpp/file_ref.h#newc...
ppapi/cpp/file_ref.h:179: const CompletionCallbackWithOutput<
std::vector<DirectoryEntry> >&
Style nit: No space between < std

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
File ppapi/proxy/ppb_file_ref_proxy.cc (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:110: typedef std::map<uint32_t,
PP_ArrayOutput> PendingReadEntriesOutputMap;
So, these are the uint32_ts that you elsewhere call |callback_id|, right?
Wouldn't |pending_read_id| be a better name? To me, |callback_id| suggests an
opaque reference to an actual callback closure/function, rather than an opaque
reference to a PP_ArrayOutput.

Since integers are the keys, it would be more space- and time-efficient to use a
std::vector<PP_ArrayOutput>. (Although you'd want to sanity-check the keys
before using at or operator[], of course.)

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:199: int id = SendCallback(callback);
Should be uint32_t; or in ay case, this type and the type of the keys in
pending_read_entries_outputs_ should be made to match.

I suppose the viability of the vector approach (as opposed to using a map)
depends on whether or not SendCallback returns incrementing numbers starting
with 0, or some random/sparse sequence. Yeah, looks like incrementing.

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:258: for
(std::vector<ppapi::PPB_FileRef_CreateInfo>::size_type i = 0;
Style guide says to just use size_t instead of the fully-qualified
....::size_type.

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:489: if (enter.succeeded()) {
Style nit: You could save a level of indentation by doing:

if (!enter.succeeded())
  return;

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.cc (right):

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.cc:113: if (dir_path[dir_path.size() - 1] !=
'/')
Could dir_path.size() ever return 0? I would do this:

if (dir_path.empty() || dir_path.back() != '/')
  dir_path.append('/');

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.h (right):

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.h:44: std::vector<
::ppapi::PPB_FileRef_CreateInfo>* files;
extra space

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.h:56: scoped_refptr<
::ppapi::TrackedCallback> callback,
space

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.h:84: std::vector<
::ppapi::PPB_FileRef_CreateInfo>* read_entries_files_;
space

[palmer, 2013-05-03 00:09:08]

Also, best practice is to either check the return value of snprintf to make sure
it did not have to truncate the output string, or use base::StringPrintf.

[hamaji, 2013-05-03 01:10:34]

Thanks palmer for your review! As for StringPrintf, I think we cannot use it
from ppapi/tests unfortunately. I added extra checks in addition to memset which
makes sure the strings are NULL terminated.

dmichael, do you have any opinion about the type of PP_ArrayOutput? I'm feeling
[in] would be OK for this parameter, but I'm not sure.

https://codereview.chromium.org/14784002/diff/33003/ppapi/api/ppb_file_ref.idl
File ppapi/api/ppb_file_ref.idl (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/api/ppb_file_ref.id...
ppapi/api/ppb_file_ref.idl:194: * @param[in] output An output array which will
receive
I'm not sure, but I think this should be [in], because PP_ArrayOutput is a
handle like file descriptors. We pass a file descriptor by value to write(2),
though write(2) will modify underlying data structures.

As you may know, this is code rather than a comment because we are in an IDL
file. If we change this, the interface of this function will change.

Anyway, I think I want inputs from pepper folks because they know how the
original code (DirectoryReader) is introduced. Dave, do you have any opinion?

https://codereview.chromium.org/14784002/diff/33003/ppapi/c/ppb_file_ref.h
File ppapi/c/ppb_file_ref.h (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/c/ppb_file_ref.h#ne...
ppapi/c/ppb_file_ref.h:203: * @param[in] output An output array which will
receive
See the comment in IDL file.

https://codereview.chromium.org/14784002/diff/33003/ppapi/cpp/file_ref.cc
File ppapi/cpp/file_ref.cc (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/cpp/file_ref.cc#new...
ppapi/cpp/file_ref.cc:174: const CompletionCallbackWithOutput<
std::vector<DirectoryEntry> >&
On 2013/05/03 00:07:24, Chromium Palmer wrote:
> Style nit: No space between < std

Done.

https://codereview.chromium.org/14784002/diff/33003/ppapi/cpp/file_ref.h
File ppapi/cpp/file_ref.h (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/cpp/file_ref.h#newc...
ppapi/cpp/file_ref.h:179: const CompletionCallbackWithOutput<
std::vector<DirectoryEntry> >&
On 2013/05/03 00:07:24, Chromium Palmer wrote:
> Style nit: No space between < std

Done.

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
File ppapi/proxy/ppb_file_ref_proxy.cc (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:110: typedef std::map<uint32_t,
PP_ArrayOutput> PendingReadEntriesOutputMap;
They are actually opaque reference to callbacks. This callback IDs are shared
among all functions in this class. |pending_callbacks_| is the main storage for
the callback IDs, and |pending_file_infos_| and |pending_read_entries_outputs_|
are just additional info.

As both |pending_file_infos_| and |pending_read_entries_outputs_| will be sparse
because other functions may increment |next_callback_id_|, it won't make sense
to make |pending_file_infos_| and |pending_read_entries_outputs_| vector.

As for |pending_callbacks_|, current |next_callback_id_| won't decrease so we
cannot use vector without modifying SendCallback to reuse finished callback IDs.
I think it's not a good idea to make this change in this big change.

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:199: int id = SendCallback(callback);
On 2013/05/03 00:07:24, Chromium Palmer wrote:
> Should be uint32_t; or in ay case, this type and the type of the keys in
> pending_read_entries_outputs_ should be made to match.
> 
> I suppose the viability of the vector approach (as opposed to using a map)
> depends on whether or not SendCallback returns incrementing numbers starting
> with 0, or some random/sparse sequence. Yeah, looks like incrementing.

Done.

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:258: for
(std::vector<ppapi::PPB_FileRef_CreateInfo>::size_type i = 0;
On 2013/05/03 00:07:24, Chromium Palmer wrote:
> Style guide says to just use size_t instead of the fully-qualified
> ....::size_type.

Done.

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:489: if (enter.succeeded()) {
On 2013/05/03 00:07:24, Chromium Palmer wrote:
> Style nit: You could save a level of indentation by doing:
> 
> if (!enter.succeeded())
>   return;

Done.

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.cc (right):

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.cc:113: if (dir_path[dir_path.size() - 1] !=
'/')
Done. I believe it won't be an empty string, but I agree this check is a great
idea.

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.h (right):

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.h:44: std::vector<
::ppapi::PPB_FileRef_CreateInfo>* files;
I think we don't use <:: because it's a trigraph :(

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.h:56: scoped_refptr<
::ppapi::TrackedCallback> callback,
ditto

https://codereview.chromium.org/14784002/diff/33003/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.h:84: std::vector<
::ppapi::PPB_FileRef_CreateInfo>* read_entries_files_;
ditto

[hamaji, 2013-05-03 01:18:48]

I've just remembered windows doesn't have snprintf. I think sstream would be a
cleaner solution here.

[dmichael (off chromium), 2013-05-03 02:54:57]

lgtm

https://codereview.chromium.org/14784002/diff/33003/ppapi/api/ppb_file_ref.idl
File ppapi/api/ppb_file_ref.idl (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/api/ppb_file_ref.id...
ppapi/api/ppb_file_ref.idl:194: * @param[in] output An output array which will
receive
On 2013/05/03 01:10:34, hamaji wrote:
> I'm not sure, but I think this should be [in], because PP_ArrayOutput is a
> handle like file descriptors. We pass a file descriptor by value to write(2),
> though write(2) will modify underlying data structures.
> 
> As you may know, this is code rather than a comment because we are in an IDL
> file. If we change this, the interface of this function will change.
> 
> Anyway, I think I want inputs from pepper folks because they know how the
> original code (DirectoryReader) is introduced. Dave, do you have any opinion?

We use [in] for PP_ArrayOutput parameters. It's a little confusing, given that
PP_ArrayOutput is used to send data out, but the call doesn't actually write to
the PP_ArrayOutput. The PP_ArrayOutput just gives PPAPI a way to allocate a
buffer to pass to the plugin.

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
File ppapi/proxy/ppb_file_ref_proxy.cc (right):

https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
ppapi/proxy/ppb_file_ref_proxy.cc:110: typedef std::map<uint32_t,
PP_ArrayOutput> PendingReadEntriesOutputMap;
On 2013/05/03 01:10:34, hamaji wrote:
> They are actually opaque reference to callbacks. This callback IDs are shared
> among all functions in this class. |pending_callbacks_| is the main storage
for
> the callback IDs, and |pending_file_infos_| and
|pending_read_entries_outputs_|
> are just additional info.
> 
> As both |pending_file_infos_| and |pending_read_entries_outputs_| will be
sparse
> because other functions may increment |next_callback_id_|, it won't make sense
> to make |pending_file_infos_| and |pending_read_entries_outputs_| vector.
> 
> As for |pending_callbacks_|, current |next_callback_id_| won't decrease so we
> cannot use vector without modifying SendCallback to reuse finished callback
IDs.
> I think it's not a good idea to make this change in this big change.
+1, map is more appropriate here.

https://codereview.chromium.org/14784002/diff/64069/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.cc (right):

https://codereview.chromium.org/14784002/diff/64069/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.cc:106: if (dir_path.empty() ||
dir_path[dir_path.size() - 1] != '/')
nit/suggestion: could replace "dir_path[dir_path.size() - 1]" with
"dir_path.back()"
(Definitely need the empty() check, though, sorry I missed that!)

[raymes, 2013-05-03 03:06:48]

lgtm

On Thu, May 2, 2013 at 7:54 PM,  <dmichael@chromium.org> wrote:
> lgtm
>
>
>
>
>
>
https://codereview.chromium.org/14784002/diff/33003/ppapi/api/ppb_file_ref.idl
> File ppapi/api/ppb_file_ref.idl (right):
>
>
https://codereview.chromium.org/14784002/diff/33003/ppapi/api/ppb_file_ref.id...
> ppapi/api/ppb_file_ref.idl:194: * @param[in] output An output array
> which will receive
> On 2013/05/03 01:10:34, hamaji wrote:
>>
>> I'm not sure, but I think this should be [in], because PP_ArrayOutput
>
> is a
>>
>> handle like file descriptors. We pass a file descriptor by value to
>
> write(2),
>>
>> though write(2) will modify underlying data structures.
>
>
>> As you may know, this is code rather than a comment because we are in
>
> an IDL
>>
>> file. If we change this, the interface of this function will change.
>
>
>> Anyway, I think I want inputs from pepper folks because they know how
>
> the
>>
>> original code (DirectoryReader) is introduced. Dave, do you have any
>
> opinion?
>
> We use [in] for PP_ArrayOutput parameters. It's a little confusing,
> given that PP_ArrayOutput is used to send data out, but the call doesn't
> actually write to the PP_ArrayOutput. The PP_ArrayOutput just gives
> PPAPI a way to allocate a buffer to pass to the plugin.
>
>
>
https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
> File ppapi/proxy/ppb_file_ref_proxy.cc (right):
>
>
https://codereview.chromium.org/14784002/diff/33003/ppapi/proxy/ppb_file_ref_...
> ppapi/proxy/ppb_file_ref_proxy.cc:110: typedef std::map<uint32_t,
> PP_ArrayOutput> PendingReadEntriesOutputMap;
> On 2013/05/03 01:10:34, hamaji wrote:
>>
>> They are actually opaque reference to callbacks. This callback IDs are
>
> shared
>>
>> among all functions in this class. |pending_callbacks_| is the main
>
> storage for
>>
>> the callback IDs, and |pending_file_infos_| and
>
> |pending_read_entries_outputs_|
>>
>> are just additional info.
>
>
>> As both |pending_file_infos_| and |pending_read_entries_outputs_| will
>
> be sparse
>>
>> because other functions may increment |next_callback_id_|, it won't
>
> make sense
>>
>> to make |pending_file_infos_| and |pending_read_entries_outputs_|
>
> vector.
>
>> As for |pending_callbacks_|, current |next_callback_id_| won't
>
> decrease so we
>>
>> cannot use vector without modifying SendCallback to reuse finished
>
> callback IDs.
>>
>> I think it's not a good idea to make this change in this big change.
>
> +1, map is more appropriate here.
>
>
https://codereview.chromium.org/14784002/diff/64069/webkit/plugins/ppapi/file...
> File webkit/plugins/ppapi/file_callbacks.cc (right):
>
>
https://codereview.chromium.org/14784002/diff/64069/webkit/plugins/ppapi/file...
> webkit/plugins/ppapi/file_callbacks.cc:106: if (dir_path.empty() ||
>
> dir_path[dir_path.size() - 1] != '/')
> nit/suggestion: could replace "dir_path[dir_path.size() - 1]" with
> "dir_path.back()"
> (Definitely need the empty() check, though, sorry I missed that!)
>
> https://codereview.chromium.org/14784002/

[hamaji, 2013-05-03 03:40:49]

Thanks comments especially for [in] or [out]! I wasn't sure which is the better.

https://codereview.chromium.org/14784002/diff/64069/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.cc (right):

https://codereview.chromium.org/14784002/diff/64069/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.cc:106: if (dir_path.empty() ||
dir_path[dir_path.size() - 1] != '/')
I think std::string doesn't have back() for some reason. Though I slightly
prefer the current code, I'll use *dir_path.rbegin() if you like it.

[palmer, 2013-05-03 17:34:02]

lgtm

[commit-bot: I haz the power, 2013-05-03 17:35:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hamaji@chromium.org/14784002/64069

[commit-bot: I haz the power, 2013-05-03 17:35:45]

Failed to apply patch for ppapi/cpp/array_output.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ppapi/cpp/array_output.h
  Hunk #1 FAILED at 7.
  Hunk #2 FAILED at 270.
  Hunk #3 FAILED at 279.
  3 out of 3 hunks FAILED -- saving rejects to file ppapi/cpp/array_output.h.rej

Patch:       ppapi/cpp/array_output.h
Index: ppapi/cpp/array_output.h
diff --git a/ppapi/cpp/array_output.h b/ppapi/cpp/array_output.h
index
3a52190feac93d6bdba66fbdb49665b748812b31..7818f0796589976a264261bc61d0486855be183f
100644
--- a/ppapi/cpp/array_output.h
+++ b/ppapi/cpp/array_output.h
@@ -7,10 +7,9 @@
 
 #include <vector>
 
-#include "ppapi/c/dev/ppb_directory_reader_dev.h"
 #include "ppapi/c/pp_array_output.h"
 #include "ppapi/c/pp_resource.h"
-#include "ppapi/cpp/dev/directory_entry_dev.h"
+#include "ppapi/cpp/directory_entry.h"
 #include "ppapi/cpp/logging.h"
 #include "ppapi/cpp/pass_ref.h"
 #include "ppapi/cpp/var.h"
@@ -270,7 +269,7 @@ class ResourceArrayOutputAdapterWithStorage
 };
 
 class DirectoryEntryArrayOutputAdapterWithStorage
-    : public ArrayOutputAdapter<PP_DirectoryEntry_Dev> {
+    : public ArrayOutputAdapter<PP_DirectoryEntry> {
  public:
   DirectoryEntryArrayOutputAdapterWithStorage() {
     set_output(&temp_storage_);
@@ -279,35 +278,35 @@ class DirectoryEntryArrayOutputAdapterWithStorage
   virtual ~DirectoryEntryArrayOutputAdapterWithStorage() {
     if (!temp_storage_.empty()) {
       // An easy way to release the resource references held by
|temp_storage_|.
-      // A destructor for PP_DirectoryEntry_Dev will release them.
+      // A destructor for PP_DirectoryEntry will release them.
       output();
     }
   };
 
   // Returns the final array of resource objects, converting the
-  // PP_DirectoryEntry_Dev written by the browser to pp::DirectoryEntry_Dev
+  // PP_DirectoryEntry written by the browser to pp::DirectoryEntry
   // objects.
   //
   // This function should only be called once or we would end up converting
   // the array more than once, which would mess up the refcounting.
-  std::vector<pp::DirectoryEntry_Dev>& output() {
+  std::vector<pp::DirectoryEntry>& output() {
     PP_DCHECK(output_storage_.empty());
-    typedef std::vector<PP_DirectoryEntry_Dev> Entries;
+    typedef std::vector<PP_DirectoryEntry> Entries;
     for (Entries::iterator it = temp_storage_.begin();
          it != temp_storage_.end(); ++it)
-      output_storage_.push_back(DirectoryEntry_Dev(PASS_REF, *it));
+      output_storage_.push_back(DirectoryEntry(PASS_REF, *it));
     temp_storage_.clear();
     return output_storage_;
   }
 
  private:
-  // The browser will write the PP_DirectoryEntry_Devs into this array.
-  std::vector<PP_DirectoryEntry_Dev> temp_storage_;
+  // The browser will write the PP_DirectoryEntrys into this array.
+  std::vector<PP_DirectoryEntry> temp_storage_;
 
-  // When asked for the output, the PP_DirectoryEntry_Devs above will be
-  // converted to the pp::DirectoryEntry_Devs in this array for passing to the
+  // When asked for the output, the PP_DirectoryEntrys above will be
+  // converted to the pp::DirectoryEntrys in this array for passing to the
   // calling code.
-  std::vector<pp::DirectoryEntry_Dev> output_storage_;
+  std::vector<pp::DirectoryEntry> output_storage_;
 };
 
 }  // namespace pp

[commit-bot: I haz the power, 2013-05-03 18:01:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hamaji@chromium.org/14784002/14002

[palmer, 2013-05-03 18:02:32]

https://codereview.chromium.org/14784002/diff/64069/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.cc (right):

https://codereview.chromium.org/14784002/diff/64069/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.cc:106: if (dir_path.empty() ||
dir_path[dir_path.size() - 1] != '/')
http://www.cplusplus.com/reference/string/string/back/

But apparently it is new and not yet supported. Weird.

[hamaji, 2013-05-03 18:11:18]

Thanks again for reviewers!

https://codereview.chromium.org/14784002/diff/64069/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.cc (right):

https://codereview.chromium.org/14784002/diff/64069/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.cc:106: if (dir_path.empty() ||
dir_path[dir_path.size() - 1] != '/')
Ah, it seems C++11 added it! Nice. "21.4.5 basic_string element access" (C++11)
vs 21.3.4 (C++98) if you have both standards.

[dmichael (off chromium), 2013-05-03 18:28:16]

https://codereview.chromium.org/14784002/diff/64069/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.cc (right):

https://codereview.chromium.org/14784002/diff/64069/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.cc:106: if (dir_path.empty() ||
dir_path[dir_path.size() - 1] != '/')
On 2013/05/03 18:11:18, hamaji wrote:
> Ah, it seems C++11 added it! Nice. "21.4.5 basic_string element access"
(C++11)
> vs 21.3.4 (C++98) if you have both standards.
Hah, weird. I looked at cppreference.com when I suggested it, but I guess I
didn't look carefully enough; it just made sense that it would exist! Oh well.
By index is fine; probably slightly more readable than rbegin(), but I don't
care.

[hamaji, 2013-05-03 21:52:01]

Committed patchset #9 manually as r198204 (presubmit successful).