Implement Pepper proxy for PPB_DirectoryReader

Implement Pepper proxy for PPB_DirectoryReader

This patch includes:
- Adding proxy implementation (DircetoryReaderResource and PepperDirectoryReaderHost)
- Merging PPB_DirectoryReader_impl into PepperDirectoryReaderHost

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


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

[nhiroki, 2013-01-18 02:39:13]

Hi, could you take a early look at this? Thanks!

[kinuko, 2013-01-21 05:00:00]

David, Yuzhu,
while API discussion is going on on the other CL
(https://codereview.chromium.org/12026008/) is it possible to get earlier review
comments for this proxy implementation based on the old API?  There's a project
who really wants to use DirectoryReader (in any API format) and we want to help
delivering usable implementation asap. Thanks!

[nhiroki, 2013-01-21 11:15:07]

Updated. I'll add tests for the resource and the host later.

[yzshen1, 2013-01-24 21:55:43]

Haven't read pepper_directory_reader_host.h.

Thanks!

https://codereview.chromium.org/11958033/diff/5001/ppapi/ppapi_proxy.gypi
File ppapi/ppapi_proxy.gypi (right):

https://codereview.chromium.org/11958033/diff/5001/ppapi/ppapi_proxy.gypi#new...
ppapi/ppapi_proxy.gypi:40: 'proxy/directory_reader_resource.cc',
alphabetically, please.

https://codereview.chromium.org/11958033/diff/5001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/11958033/diff/5001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1398: ppapi::HostResource /* file_ref_resource */)
Now that the implementation at the host side gets all the entries and send them
all at once, the message names should be GetEntries[Reply] instead of
GetNextEntry[Reply].

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.h (right):

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:15: #include
"ppapi/proxy/enter_proxy.h"
This can be moved to the .cc file. (And some other includes.)

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:17: #include
"ppapi/shared_impl/ppb_file_ref_shared.h"
Could you please forward declare things like this and move the includes to the
.cc file?

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:18: #include
"ppapi/thunk/resource_creation_api.h"
you don't need this.

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:25: class CONTENT_EXPORT
PepperDirectoryReaderHost
Is it better to remove the ppb_directory_reader_impl entirely and move necessary
things into this class?
(I haven't read the rest of the class. I will wait till we have an answer to
this question.)

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:27: public
base::SupportsWeakPtr<PepperDirectoryReaderHost> {
add include, please.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.cc (right):

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:31: has_more_(true) {}
please init |entry_|.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:66: DCHECK_EQ(infos.size(),
file_types.size());
Please make it a CHECK. Something must be really wrong.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:71: entry.file_ref =
PPB_FileRef_Proxy::DeserializeFileRef(infos[i]);
Where do you release the ref? If the ref is not returned to the plugin by any
GetNextEntry() call, you have to release it yourself.

Please be careful with refs, it is very hard to catch issues later.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:81: FillUpEntry();
[optional]
Please check that callback_ is still pending before filing up the entry.

Currently it should be safe, when the last plugin ref is deleted (which causes
the callback to be aborted), I think this object should be destroyed and this
method won't be called after that. But I would rather not rely on this fact.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:96: entry_->file_ref = 0;
Please also init the other field in |entry_|.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.h (right):

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:5: #ifndef
PPAPI_PROXY_DIRECTORY_READER_H_
Please name it after the path exactly.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:30: virtual
thunk::PPB_DirectoryReader_API* AsPPB_DirectoryReader_API() OVERRIDE;
Please add include for OVERRIDE.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:40: const
std::vector<ppapi::PPB_FileRef_CreateInfo> infos,
Please use const &

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:41: const std::vector<PP_FileType>
file_types);
ditto

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:44: PP_Resource directory_ref_;
This might go away at any time because you don't hold a ref to it.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:52:
DISALLOW_COPY_AND_ASSIGN(DirectoryReaderResource);
Please add include.

https://codereview.chromium.org/11958033/diff/36002/ppapi/thunk/ppb_directory...
File ppapi/thunk/ppb_directory_reader_thunk.cc (right):

https://codereview.chromium.org/11958033/diff/36002/ppapi/thunk/ppb_directory...
ppapi/thunk/ppb_directory_reader_thunk.cc:24: if (!object)
You could move this check into CreateDirectoryReader(), right?
Then you don't have to GetResource() twice.

[nhiroki, 2013-01-25 12:27:16]

Updated. Thanks for your review!

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.h (right):

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:15: #include
"ppapi/proxy/enter_proxy.h"
On 2013/01/24 21:55:43, yzshen1 wrote:
> This can be moved to the .cc file. (And some other includes.)

Done.

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:17: #include
"ppapi/shared_impl/ppb_file_ref_shared.h"
On 2013/01/24 21:55:43, yzshen1 wrote:
> Could you please forward declare things like this and move the includes to the
> .cc file?

Done.

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:18: #include
"ppapi/thunk/resource_creation_api.h"
On 2013/01/24 21:55:43, yzshen1 wrote:
> you don't need this.

Done.

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:25: class CONTENT_EXPORT
PepperDirectoryReaderHost
On 2013/01/24 21:55:43, yzshen1 wrote:
> Is it better to remove the ppb_directory_reader_impl entirely and move
necessary
> things into this class?
> (I haven't read the rest of the class. I will wait till we have an answer to
> this question.)

I think so too! There is some redundant codes here. I'll try to merge like
FileIO later.

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:27: public
base::SupportsWeakPtr<PepperDirectoryReaderHost> {
On 2013/01/24 21:55:43, yzshen1 wrote:
> add include, please.

Done.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.cc (right):

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:31: has_more_(true) {}
On 2013/01/24 21:55:43, yzshen1 wrote:
> please init |entry_|.

Done.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:66: DCHECK_EQ(infos.size(),
file_types.size());
On 2013/01/24 21:55:43, yzshen1 wrote:
> Please make it a CHECK. Something must be really wrong.

Done.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:71: entry.file_ref =
PPB_FileRef_Proxy::DeserializeFileRef(infos[i]);
On 2013/01/24 21:55:43, yzshen1 wrote:
> Where do you release the ref? If the ref is not returned to the plugin by any
> GetNextEntry() call, you have to release it yourself.
> 
> Please be careful with refs, it is very hard to catch issues later.

Added release routine into the dtor.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:81: FillUpEntry();
On 2013/01/24 21:55:43, yzshen1 wrote:
> [optional]
> Please check that callback_ is still pending before filing up the entry.
> 
> Currently it should be safe, when the last plugin ref is deleted (which causes
> the callback to be aborted), I think this object should be destroyed and this
> method won't be called after that. But I would rather not rely on this fact.

Done.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:96: entry_->file_ref = 0;
On 2013/01/24 21:55:43, yzshen1 wrote:
> Please also init the other field in |entry_|.

Done.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.h (right):

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:5: #ifndef
PPAPI_PROXY_DIRECTORY_READER_H_
On 2013/01/24 21:55:43, yzshen1 wrote:
> Please name it after the path exactly.

Good eye, thanks!

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:30: virtual
thunk::PPB_DirectoryReader_API* AsPPB_DirectoryReader_API() OVERRIDE;
On 2013/01/24 21:55:43, yzshen1 wrote:
> Please add include for OVERRIDE.

Done.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:40: const
std::vector<ppapi::PPB_FileRef_CreateInfo> infos,
On 2013/01/24 21:55:43, yzshen1 wrote:
> Please use const &

Done.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:41: const std::vector<PP_FileType>
file_types);
On 2013/01/24 21:55:43, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:44: PP_Resource directory_ref_;
On 2013/01/24 21:55:43, yzshen1 wrote:
> This might go away at any time because you don't hold a ref to it.

Added AddRefResource() and ReleaseResource().

https://codereview.chromium.org/11958033/diff/36002/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:52:
DISALLOW_COPY_AND_ASSIGN(DirectoryReaderResource);
On 2013/01/24 21:55:43, yzshen1 wrote:
> Please add include.

Done.

https://codereview.chromium.org/11958033/diff/36002/ppapi/thunk/ppb_directory...
File ppapi/thunk/ppb_directory_reader_thunk.cc (right):

https://codereview.chromium.org/11958033/diff/36002/ppapi/thunk/ppb_directory...
ppapi/thunk/ppb_directory_reader_thunk.cc:24: if (!object)
On 2013/01/24 21:55:43, yzshen1 wrote:
> You could move this check into CreateDirectoryReader(), right?
> Then you don't have to GetResource() twice.

I see. I removed GetResource() from CreateDirectoryReader() and changed
CreateDirectoryReader() to take PP_Instance object as an argument.

[yzshen1, 2013-01-28 17:52:34]

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.cc (right):

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:106:
PpapiPluginMsg_DirectoryReader_GetNextEntryReply(host_resources_,
Do you need to clear this and file_types_?

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.h (right):

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:25: class CONTENT_EXPORT
PepperDirectoryReaderHost
Do you mean you will do it in this CL or a separate CL?
I prefer changing it in this CL, because it is about the same amout of work
comparing with this.
What do you think?

On 2013/01/25 12:27:17, nhiroki wrote:
> On 2013/01/24 21:55:43, yzshen1 wrote:
> > Is it better to remove the ppb_directory_reader_impl entirely and move
> necessary
> > things into this class?
> > (I haven't read the rest of the class. I will wait till we have an answer to
> > this question.)
> 
> I think so too! There is some redundant codes here. I'll try to merge like
> FileIO later.

https://codereview.chromium.org/11958033/diff/39005/ppapi/ppapi_proxy.gypi
File ppapi/ppapi_proxy.gypi (right):

https://codereview.chromium.org/11958033/diff/39005/ppapi/ppapi_proxy.gypi#ne...
ppapi/ppapi_proxy.gypi:40: 'proxy/directory_reader_resource.cc',
I think you haven't seen my review comments on patchset 2. (Please reorder.)

https://codereview.chromium.org/11958033/diff/39005/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.cc (right):

https://codereview.chromium.org/11958033/diff/39005/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:78: if
(!TrackedCallback::IsPending(callback_))
This is a little bit tricky: when we send a host resource to the plugin side,
usually one ref is carried to the plugin side (otherwise, the resource may be
destroyed in the middle). And the plugin side will create a plugin-side resource
object to take that host-side ref. When the plugin-side resource object goes
away, that host-side ref is released by sending a message back to the host side.

In this case, if we return early, the host-side resource ref is leaked.

https://codereview.chromium.org/11958033/diff/39005/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:95: output_ = NULL;
It is better to reset |output_| within FillUpEntry() if necessary, so that you
don't need to do it here and line 59.

https://codereview.chromium.org/11958033/diff/39005/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/11958033/diff/39005/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1331:
IPC_MESSAGE_CONTROL1(PpapiHostMsg_DirectoryReader_GetNextEntry,
Please see me comment on patchset 2.

[brettw, 2013-01-28 20:29:56]

LGTM for content owners gypi. I did not look at any details, I'll have that to
yzshen and dmichael.

[nhiroki, 2013-01-29 05:10:34]

Thank you for reviewing! PTAL

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.cc (right):

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:106:
PpapiPluginMsg_DirectoryReader_GetNextEntryReply(host_resources_,
On 2013/01/28 17:52:34, yzshen1 wrote:
> Do you need to clear this and file_types_?

Done.

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.h (right):

https://codereview.chromium.org/11958033/diff/36002/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:25: class CONTENT_EXPORT
PepperDirectoryReaderHost
On 2013/01/28 17:52:34, yzshen1 wrote:
> Do you mean you will do it in this CL or a separate CL?
> I prefer changing it in this CL, because it is about the same amout of work
> comparing with this.
> What do you think?
> 
> On 2013/01/25 12:27:17, nhiroki wrote:
> > On 2013/01/24 21:55:43, yzshen1 wrote:
> > > Is it better to remove the ppb_directory_reader_impl entirely and move
> > necessary
> > > things into this class?
> > > (I haven't read the rest of the class. I will wait till we have an answer
to
> > > this question.)
> > 
> > I think so too! There is some redundant codes here. I'll try to merge like
> > FileIO later.
> 

I intended to make a separate CL. Okay, let me see if I can simply do so in this
CL.

https://codereview.chromium.org/11958033/diff/39005/ppapi/ppapi_proxy.gypi
File ppapi/ppapi_proxy.gypi (right):

https://codereview.chromium.org/11958033/diff/39005/ppapi/ppapi_proxy.gypi#ne...
ppapi/ppapi_proxy.gypi:40: 'proxy/directory_reader_resource.cc',
On 2013/01/28 17:52:34, yzshen1 wrote:
> I think you haven't seen my review comments on patchset 2. (Please reorder.)

Sorry for missing this. Done.

https://codereview.chromium.org/11958033/diff/39005/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.cc (right):

https://codereview.chromium.org/11958033/diff/39005/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:78: if
(!TrackedCallback::IsPending(callback_))
On 2013/01/28 17:52:34, yzshen1 wrote:
> This is a little bit tricky: when we send a host resource to the plugin side,
> usually one ref is carried to the plugin side (otherwise, the resource may be
> destroyed in the middle). And the plugin side will create a plugin-side
resource
> object to take that host-side ref. When the plugin-side resource object goes
> away, that host-side ref is released by sending a message back to the host
side.
> 
> In this case, if we return early, the host-side resource ref is leaked.

I see, thanks for your understandable explanations! I moved this check after
pushing entries to release them correctly in the dtor.

https://codereview.chromium.org/11958033/diff/39005/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:95: output_ = NULL;
On 2013/01/28 17:52:34, yzshen1 wrote:
> It is better to reset |output_| within FillUpEntry() if necessary, so that you
> don't need to do it here and line 59.

Done.

https://codereview.chromium.org/11958033/diff/39005/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/11958033/diff/39005/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1331:
IPC_MESSAGE_CONTROL1(PpapiHostMsg_DirectoryReader_GetNextEntry,
On 2013/01/28 17:52:34, yzshen1 wrote:
> Please see me comment on patchset 2.

Done. These messages and related functions were renamed as GetEntries*.

[nhiroki, 2013-01-29 17:18:06]

I merged PPB_DirectoryReader_Impl into PepperDirectoryReaderHost.
Could you take another look?

Thanks!

[dmichael (off chromium), 2013-01-29 21:22:42]

https://codereview.chromium.org/11958033/diff/39005/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.h (right):

https://codereview.chromium.org/11958033/diff/39005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:27: public
base::SupportsWeakPtr<PepperDirectoryReaderHost> {
^^^ Please just use a WeakPtrFactory, since you only need WeakPtrs internally.

https://codereview.chromium.org/11958033/diff/53008/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.cc (right):

https://codereview.chromium.org/11958033/diff/53008/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:60: host_(host) {}
^ nit: line up host_ with callback_

https://codereview.chromium.org/11958033/diff/53008/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:145: entries.begin(); it
!= entries.end(); ++it) {
nit: entries.begin() should be indented. I'd suggest either using a typedef to
get the first part shorter, or putting the second and third parts of the for on
their own line.

https://codereview.chromium.org/11958033/diff/53008/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:146: PPB_FileRef_Impl*
file_ref = PPB_FileRef_Impl::CreateInternal(
You should usually wrap this immediately in a scoped_refptr (it has a ref-count
of 0 until you stick it in a scoped_refptr).

You should probably be storing a vector of scoped_ptr<PPB_FileRef_Impl>; using
the external PP_Resource as you do is usually risky. E.g., the plugin can
decrement an extra ref-count and make your resource disappear out from under
you. That's why we use ref-counted objects internally; the internal ref-count
(from RefCounted) is the one that actually determines if the object is still
alive. The reference count in the tracker is primarily used by the plugin to
tell us when it's done with a resource.

https://codereview.chromium.org/11958033/diff/53008/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:157:
PPB_FileRef_Proxy::SerializeFileRef(file_ref->GetReference(), &info);
We generally shouldn't be using stuff from ppapi/proxy here in the host side.

https://codereview.chromium.org/11958033/diff/53008/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:198: AsWeakPtr()));
Why are you using completion callbacks instead of base::Callback?

https://codereview.chromium.org/11958033/diff/53008/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.cc (right):

https://codereview.chromium.org/11958033/diff/53008/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:91: DCHECK(!has_more_);
If we never use this parameter currently, maybe we shouldn't send it in the
message, and shouldn't have code that checks has_more_?

https://codereview.chromium.org/11958033/diff/53008/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.h (right):

https://codereview.chromium.org/11958033/diff/53008/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:50: PP_Resource directory_ref_;
Probably should be a scoped_refptr instead?

https://codereview.chromium.org/11958033/diff/53008/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:53: std::queue<PP_DirectoryEntry_Dev>
entries_;
ditto, scoped_refptr

[dmichael (off chromium), 2013-01-29 21:25:31]

https://codereview.chromium.org/11958033/diff/53008/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.h (right):

https://codereview.chromium.org/11958033/diff/53008/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:50: PP_Resource directory_ref_;
On 2013/01/29 21:22:42, dmichael wrote:
> Probably should be a scoped_refptr instead?
Let me clarify a bit...  you should just hold a
scoped_refptr<PPB_FileRef_Shared>, or whatever the internal type is. You
shouldn't rely on the reference count in the ResourceTracker being sane, since
the plugin can muck with it.

[nhiroki, 2013-01-30 09:24:51]

Thank you for comments! Updated.

https://codereview.chromium.org/11958033/diff/39005/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.h (right):

https://codereview.chromium.org/11958033/diff/39005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:27: public
base::SupportsWeakPtr<PepperDirectoryReaderHost> {
On 2013/01/29 21:22:42, dmichael wrote:
> ^^^ Please just use a WeakPtrFactory, since you only need WeakPtrs internally.

Done.

https://codereview.chromium.org/11958033/diff/53008/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.cc (right):

https://codereview.chromium.org/11958033/diff/53008/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:60: host_(host) {}
On 2013/01/29 21:22:42, dmichael wrote:
> ^ nit: line up host_ with callback_

Removed host_.

https://codereview.chromium.org/11958033/diff/53008/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:145: entries.begin(); it
!= entries.end(); ++it) {
On 2013/01/29 21:22:42, dmichael wrote:
> nit: entries.begin() should be indented. I'd suggest either using a typedef to
> get the first part shorter, or putting the second and third parts of the for
on
> their own line.

Done.

https://codereview.chromium.org/11958033/diff/53008/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:146: PPB_FileRef_Impl*
file_ref = PPB_FileRef_Impl::CreateInternal(
On 2013/01/29 21:22:42, dmichael wrote:
> You should usually wrap this immediately in a scoped_refptr (it has a
ref-count
> of 0 until you stick it in a scoped_refptr).
> 
> You should probably be storing a vector of scoped_ptr<PPB_FileRef_Impl>; using
> the external PP_Resource as you do is usually risky. E.g., the plugin can
> decrement an extra ref-count and make your resource disappear out from under
> you. That's why we use ref-counted objects internally; the internal ref-count
> (from RefCounted) is the one that actually determines if the object is still
> alive. The reference count in the tracker is primarily used by the plugin to
> tell us when it's done with a resource.

Done.

https://codereview.chromium.org/11958033/diff/53008/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:157:
PPB_FileRef_Proxy::SerializeFileRef(file_ref->GetReference(), &info);
On 2013/01/29 21:22:42, dmichael wrote:
> We generally shouldn't be using stuff from ppapi/proxy here in the host side.

Done.

https://codereview.chromium.org/11958033/diff/53008/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:198: AsWeakPtr()));
On 2013/01/29 21:22:42, dmichael wrote:
> Why are you using completion callbacks instead of base::Callback?

Done.

https://codereview.chromium.org/11958033/diff/53008/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.cc (right):

https://codereview.chromium.org/11958033/diff/53008/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:91: DCHECK(!has_more_);
On 2013/01/29 21:22:42, dmichael wrote:
> If we never use this parameter currently, maybe we shouldn't send it in the
> message, and shouldn't have code that checks has_more_?

Done.

https://codereview.chromium.org/11958033/diff/53008/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.h (right):

https://codereview.chromium.org/11958033/diff/53008/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:50: PP_Resource directory_ref_;
On 2013/01/29 21:25:31, dmichael wrote:
> On 2013/01/29 21:22:42, dmichael wrote:
> > Probably should be a scoped_refptr instead?
> Let me clarify a bit...  you should just hold a
> scoped_refptr<PPB_FileRef_Shared>, or whatever the internal type is. You
> shouldn't rely on the reference count in the ResourceTracker being sane, since
> the plugin can muck with it.

I see, thanks for your clear explanation! Done.

https://codereview.chromium.org/11958033/diff/53008/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:53: std::queue<PP_DirectoryEntry_Dev>
entries_;
On 2013/01/29 21:22:42, dmichael wrote:
> ditto, scoped_refptr

Done.

[yzshen1, 2013-01-31 08:50:54]

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.cc (right):

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:57: explicit
ReadDirectoryCallback(OnReadDirectoryCallback callback)
Please use const ref.

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:124:
ppapi::host::HostMessageContext* host_context,
wrong indent.

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:154: 
Is it better to simplify the code like this?

if (result == PP_OK && !AddNewEntries(entries, has_more))
  result = PP_ERROR_FAILED;
SendGetEntriesReply(result);

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:174:
DCHECK(!entries.empty());
It is possible, according to the logic of the previous method, that
entries.empty() && !has_more. Right?

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:176: for
(Entries::const_iterator it = entries.begin();
The ref count handling is wrong in the code below. I recommend you to read
resource_tracker and plugin_resource_tracker, and always consider ref counting
carefully when writing Pepper code.

- There are two kinds of ref here: ref count of the resource impl object; ref
count of PP_Resource.
- When we send a HostResource to the plugin side, we should add 1 ref for
PP_Resource on behalf of the plugin side. (And the plugin side is responsible to
notify the host side when it is done with the ref.)
- GetReference() on line 191 adds 1 PP_Resource ref. If we don't send it to the
plugin side (e.g., we hit line 188 or 196), it is leaked. (Btw, GetCreateInfo()
on line 199 doesn't add ref.)
- There is no need to EnterResource on line 191, PPB_FileRef_Impl is a subclass
of PPB_FileRef_API.

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.h (right):

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:48: ppapi::HostResource
resource);
Maybe use const ref?

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:56:
scoped_refptr<webkit::ppapi::PPB_FileRef_Impl> directory_ref_;
nit: please include what you use.

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:65: PP_Resource
PepperInProcessResourceCreation::CreateDirectoryReader(
If nobody is using it in-process, we don't have to support it. (You could return
0 directly in resource_creation_impl instead of here.)

https://codereview.chromium.org/11958033/diff/70005/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.h (right):

https://codereview.chromium.org/11958033/diff/70005/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:43: scoped_refptr<Resource>
file_resource;
nit: please include what you use. (ref_counted.h)

[nhiroki, 2013-02-01 08:19:47]

PTAL, thanks!

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.cc (right):

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:57: explicit
ReadDirectoryCallback(OnReadDirectoryCallback callback)
On 2013/01/31 08:50:54, yzshen1 wrote:
> Please use const ref.

Done.

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:124:
ppapi::host::HostMessageContext* host_context,
On 2013/01/31 08:50:54, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:154: 
On 2013/01/31 08:50:54, yzshen1 wrote:
> Is it better to simplify the code like this?
> 
> if (result == PP_OK && !AddNewEntries(entries, has_more))
>   result = PP_ERROR_FAILED;
> SendGetEntriesReply(result);

Done.

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:174:
DCHECK(!entries.empty());
On 2013/01/31 08:50:54, yzshen1 wrote:
> It is possible, according to the logic of the previous method, that
> entries.empty() && !has_more. Right?

It's an intended case. Removed this check.

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:176: for
(Entries::const_iterator it = entries.begin();
On 2013/01/31 08:50:54, yzshen1 wrote:
> The ref count handling is wrong in the code below. I recommend you to read
> resource_tracker and plugin_resource_tracker, and always consider ref counting
> carefully when writing Pepper code.
> 
> - There are two kinds of ref here: ref count of the resource impl object; ref
> count of PP_Resource.
> - When we send a HostResource to the plugin side, we should add 1 ref for
> PP_Resource on behalf of the plugin side. (And the plugin side is responsible
to
> notify the host side when it is done with the ref.)
> - GetReference() on line 191 adds 1 PP_Resource ref. If we don't send it to
the
> plugin side (e.g., we hit line 188 or 196), it is leaked. (Btw,
GetCreateInfo()
> on line 199 doesn't add ref.)
> - There is no need to EnterResource on line 191, PPB_FileRef_Impl is a
subclass
> of PPB_FileRef_API.

Thanks for the explanation. I read related sourcecodes and realized my mistakes.
I fixed here to increment just one refcount on behalf of the plugin side and to
take care of breaking out of the loop.

In my understanding, PluginResourceTracker will notify the host side when the
refcount of the plugin's PP_Resource becomes 0, so I don't have to decrement the
refcount of host's PP_Resource explicitly. Is this right?

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.h (right):

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:48: ppapi::HostResource
resource);
On 2013/01/31 08:50:54, yzshen1 wrote:
> Maybe use const ref?

Done.

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:56:
scoped_refptr<webkit::ppapi::PPB_FileRef_Impl> directory_ref_;
On 2013/01/31 08:50:54, yzshen1 wrote:
> nit: please include what you use.

Done.

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:65: PP_Resource
PepperInProcessResourceCreation::CreateDirectoryReader(
On 2013/01/31 08:50:54, yzshen1 wrote:
> If nobody is using it in-process, we don't have to support it. (You could
return
> 0 directly in resource_creation_impl instead of here.)

Yes, nobody is using this except browser_tests. I made resource_creation_impl to
return 0 directly and changed the test from in-process to out-of-process.

https://codereview.chromium.org/11958033/diff/70005/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.h (right):

https://codereview.chromium.org/11958033/diff/70005/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.h:43: scoped_refptr<Resource>
file_resource;
On 2013/01/31 08:50:54, yzshen1 wrote:
> nit: please include what you use. (ref_counted.h)

Done.

[yzshen1, 2013-02-01 23:57:47]

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.cc (right):

https://codereview.chromium.org/11958033/diff/70005/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:176: for
(Entries::const_iterator it = entries.begin();
> In my understanding, PluginResourceTracker will notify the host side when the
> refcount of the plugin's PP_Resource becomes 0, so I don't have to decrement
the
> refcount of host's PP_Resource explicitly. Is this right?
Correct.

Thanks! :)

https://codereview.chromium.org/11958033/diff/58047/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.cc (right):

https://codereview.chromium.org/11958033/diff/58047/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:183:
file_ref->GetReference();
Is it better to do line 181-186 in SendGetEntriesReply()?
We don't have to have |host_resources_| and |file_types_| as class members. And
ReleaseResources() is not needed.

https://codereview.chromium.org/11958033/diff/58047/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.cc (right):

https://codereview.chromium.org/11958033/diff/58047/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:33:
tracker->AddRefResource(directory_ref);
You don't have to add a PP_Resource ref, now that |directory_resource_| has held
a resource object ref. (And so you don't need line 43 either.)

https://codereview.chromium.org/11958033/diff/58047/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:84: entry.file_resource =
It seems better to use ScopedPPResource here, because DeserializeFileRef()
returns a PP_Resource:

// |file_resource| is ScopedPPResource instead of scoped_refptr.
entry.file_resource = ScopedPPResource(
    PassRef, PPB_FileRef_Proxy::DeserializeFileRef(infos[i]));

In FillUpEntry(), you use:
// Pass the ref to |output_|.
output_->file_ref = entry.file_resource.Release();

And you don't need to manually release in destructor.

[nhiroki, 2013-02-02 05:14:57]

Updated, thanks!

https://codereview.chromium.org/11958033/diff/58047/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.cc (right):

https://codereview.chromium.org/11958033/diff/58047/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:183:
file_ref->GetReference();
On 2013/02/01 23:57:47, yzshen1 wrote:
> Is it better to do line 181-186 in SendGetEntriesReply()?
> We don't have to have |host_resources_| and |file_types_| as class members.
And
> ReleaseResources() is not needed.

Done.

https://codereview.chromium.org/11958033/diff/58047/ppapi/proxy/directory_rea...
File ppapi/proxy/directory_reader_resource.cc (right):

https://codereview.chromium.org/11958033/diff/58047/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:33:
tracker->AddRefResource(directory_ref);
On 2013/02/01 23:57:47, yzshen1 wrote:
> You don't have to add a PP_Resource ref, now that |directory_resource_| has
held
> a resource object ref. (And so you don't need line 43 either.)

Done.

https://codereview.chromium.org/11958033/diff/58047/ppapi/proxy/directory_rea...
ppapi/proxy/directory_reader_resource.cc:84: entry.file_resource =
On 2013/02/01 23:57:47, yzshen1 wrote:
> It seems better to use ScopedPPResource here, because DeserializeFileRef()
> returns a PP_Resource:
> 
> // |file_resource| is ScopedPPResource instead of scoped_refptr.
> entry.file_resource = ScopedPPResource(
>     PassRef, PPB_FileRef_Proxy::DeserializeFileRef(infos[i]));
> 
> In FillUpEntry(), you use:
> // Pass the ref to |output_|.
> output_->file_ref = entry.file_resource.Release();
> 
> And you don't need to manually release in destructor.

Done.

[yzshen1, 2013-02-05 05:55:44]

LGTM

Only a few more nits. Thanks for all the work in response to my comments.

https://codereview.chromium.org/11958033/diff/89001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.cc (right):

https://codereview.chromium.org/11958033/diff/89001/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:191:
it->file_ref->GetReference();
nit: would you please add a comment about this ref is added on behalf of the
plugin side?

https://codereview.chromium.org/11958033/diff/89001/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:194: }
nit: you could clear entry_data_ after creating host_resources and file_types.

https://codereview.chromium.org/11958033/diff/89001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.h (right):

https://codereview.chromium.org/11958033/diff/89001/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:44: EntryData();
nit: do we have to have constructor and destructor?

[nhiroki, 2013-02-05 08:24:37]

Thank you for a lot of informative comments!

https://codereview.chromium.org/11958033/diff/89001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.cc (right):

https://codereview.chromium.org/11958033/diff/89001/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:191:
it->file_ref->GetReference();
On 2013/02/05 05:55:44, yzshen1 wrote:
> nit: would you please add a comment about this ref is added on behalf of the
> plugin side?

Done.

https://codereview.chromium.org/11958033/diff/89001/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.cc:194: }
On 2013/02/05 05:55:44, yzshen1 wrote:
> nit: you could clear entry_data_ after creating host_resources and file_types.

Done.

https://codereview.chromium.org/11958033/diff/89001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_directory_reader_host.h (right):

https://codereview.chromium.org/11958033/diff/89001/content/renderer/pepper/p...
content/renderer/pepper/pepper_directory_reader_host.h:44: EntryData();
On 2013/02/05 05:55:44, yzshen1 wrote:
> nit: do we have to have constructor and destructor?

I think these are necessary. Without them, a compiler emits errors since
Chromium-Style requires such ctor and dtor.

http://dev.chromium.org/developers/coding-style/chromium-style-checker-errors...

[nhiroki, 2013-02-05 08:43:29]

+jln@, could you review "ppapi_messages.h"? Thanks!

[jln (very slow on Chromium), 2013-02-06 06:36:28]

On 2013/02/05 08:43:29, nhiroki wrote:
> +jln@, could you review "ppapi_messages.h"? Thanks!

Hello,

sorry for the delay. I might be able to take a look tomorrow, but it's not sure
at all. If you need this review very soon, you could either:
- pick up a different owner
- explain to me which this is obviously safe

Otherwise I'll take a look early next week.

Julien

[nhiroki, 2013-02-06 08:37:32]

On 2013/02/06 06:36:28, Julien Tinnes wrote:
> On 2013/02/05 08:43:29, nhiroki wrote:
> > +jln@, could you review "ppapi_messages.h"? Thanks!
> 
> Hello,
> 
> sorry for the delay. I might be able to take a look tomorrow, but it's not
sure
> at all. If you need this review very soon, you could either:
> - pick up a different owner
> - explain to me which this is obviously safe
> 
> Otherwise I'll take a look early next week.
> 
> Julien

Julien, thank you for the reply!

Let me explain this.

This patch enables plugin developers to read a directory in the sandboxed
filesystem (that is, temporary or persistent filesystem) from the plugin.

The plugin-side send PP_Resource (actually HostResource) for the directory to be
read. This resource does not contain any risky values and the plugin cannot get
the directory resource from out of the sandboxed filesystem, so it is safe to
send it.

On the other hand, the renderer-side send vectors of PPB_FileRef_Info and
PP_FileType for files in the given Directory. This is also safe since these
objects contain only information about files in the sandboxed filesystem
(FileChooser does the similar thing at line 1344 in ppapi_messages.h).

Please let me know if you have any questions. Thanks!

[jln (very slow on Chromium), 2013-02-06 22:29:25]

Thanks. I think this warrants a closer look, which I won't be able to provide
before early next week (going to the team's offsite).

If you want to make the branch cut, please message one of the other owners
personally and see if they would be available.

Otherwise, I'll review it next week, sorry about that.

[dmichael (off chromium), 2013-02-06 22:44:21]

lgtm

mostly nits

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
File ppapi/proxy/directory_reader_resource.cc (right):

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
ppapi/proxy/directory_reader_resource.cc:81: has_more_ = false;
has_more_ is a confusing name to me, given that we get all the entries in one
message. Maybe it would be clearer if it was instead
"did_receive_get_entries_reply_" (obviously true/false would be swapped)?

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
ppapi/proxy/directory_reader_resource.cc:92: 
Couldn't you bail early if you haven't received the message? I think your code
is correct; I just found it a little hard to follow at first, so this is just an
optional suggestion for restructuring this method...
i.e.,
if (!did_receive_get_entries_reply_)
  return false;

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
ppapi/proxy/directory_reader_resource.cc:96: output_->file_ref =
entry.file_resource.Release();
The API doc says:
"If entry->file_ref is non-zero when passed to GetNextEntry, it will be released
before the next file_ref is stored."

So should we release a ResourceTracker reference for output_file_ref?

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
ppapi/proxy/directory_reader_resource.cc:102: if (!has_more_) {
This could instead be an else if you bailed early as I suggested above.  i.e.,
if we get here, we know we have received the message, but our queue is now
empty.

That seems clearer to me personally.

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
ppapi/proxy/directory_reader_resource.cc:103: output_->file_ref = 0;
I think you also need to release output_->file_ref here.

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
File ppapi/proxy/directory_reader_resource.h (right):

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
ppapi/proxy/directory_reader_resource.h:53: bool FillUpEntry();
This function probably deserves a comment, including explaining the meaning of
the return value

[nhiroki, 2013-02-07 09:37:26]

Updated, thanks

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
File ppapi/proxy/directory_reader_resource.cc (right):

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
ppapi/proxy/directory_reader_resource.cc:81: has_more_ = false;
On 2013/02/06 22:44:21, dmichael wrote:
> has_more_ is a confusing name to me, given that we get all the entries in one
> message. Maybe it would be clearer if it was instead
> "did_receive_get_entries_reply_" (obviously true/false would be swapped)?

Done.

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
ppapi/proxy/directory_reader_resource.cc:92: 
On 2013/02/06 22:44:21, dmichael wrote:
> Couldn't you bail early if you haven't received the message? I think your code
> is correct; I just found it a little hard to follow at first, so this is just
an
> optional suggestion for restructuring this method...
> i.e.,
> if (!did_receive_get_entries_reply_)
>   return false;

Done.

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
ppapi/proxy/directory_reader_resource.cc:96: output_->file_ref =
entry.file_resource.Release();
On 2013/02/06 22:44:21, dmichael wrote:
> The API doc says:
> "If entry->file_ref is non-zero when passed to GetNextEntry, it will be
released
> before the next file_ref is stored."
> 
> So should we release a ResourceTracker reference for output_file_ref?

Oops... I missed it. Thank you for catching. Done.

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
ppapi/proxy/directory_reader_resource.cc:102: if (!has_more_) {
On 2013/02/06 22:44:21, dmichael wrote:
> This could instead be an else if you bailed early as I suggested above.  i.e.,
> if we get here, we know we have received the message, but our queue is now
> empty.
> 
> That seems clearer to me personally.

Done.

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
ppapi/proxy/directory_reader_resource.cc:103: output_->file_ref = 0;
On 2013/02/06 22:44:21, dmichael wrote:
> I think you also need to release output_->file_ref here.

Done.

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
File ppapi/proxy/directory_reader_resource.h (right):

https://codereview.chromium.org/11958033/diff/100001/ppapi/proxy/directory_re...
ppapi/proxy/directory_reader_resource.h:53: bool FillUpEntry();
On 2013/02/06 22:44:21, dmichael wrote:
> This function probably deserves a comment, including explaining the meaning of
> the return value

Done.

[nhiroki, 2013-02-07 09:52:10]

On 2013/02/06 22:29:25, Julien Tinnes wrote:
> If you want to make the branch cut, please message one of the other owners
> personally and see if they would be available.

Wow, it looks to me that all owners are going to the offsite. Okay, please
review this next week and enjoy the offsite!

[nhiroki, 2013-02-08 04:03:37]

+jschuh@

Julien, thanks for your suggestion on chat!

Hi Justin, could you review ppapi_messages.h on behalf of Julien?
Please see the following explanation:

> This patch enables plugin developers to read a directory in the sandboxed
> filesystem (that is, temporary or persistent filesystem) from the plugin.
> 
> The plugin-side send PP_Resource (actually HostResource) for the directory to
be
> read. This resource does not contain any risky values and the plugin cannot
get
> the directory resource from out of the sandboxed filesystem, so it is safe to
> send it.
> 
> On the other hand, the renderer-side send vectors of PPB_FileRef_Info and
> PP_FileType for files in the given Directory. This is also safe since these
> objects contain only information about files in the sandboxed filesystem
> (FileChooser does the similar thing at line 1344 in ppapi_messages.h).

Thanks,
nhiroki

[nhiroki, 2013-02-13 02:19:47]

Ping jschuh@, do you have time to look at this? Thanks.

[jln (very slow on Chromium), 2013-02-13 03:33:28]

I've spent some time looking at this. As all changes related to PPAPI and the
PPAPI proxy, this is quite difficult to follow for people outside of the PPAPI
development loop.

This seems ok, but I'm far from understanding all of this code. This particular
CL only involves PPAPI <-> renderer communication, so it's not something we
absolutely to look at deeply.

We've already held this CL for a while, and I don't think any of us (security)
will give it a more thorough review, so LGTM.

Please, if there is anything you'd want to be checked by the security-team,
follow-up with us and ask explicitly.

At some point it may make sense for someone in Security to spend a bit of time
reviewing all of the PPAPI proxy logic.

[commit-bot: I haz the power, 2013-02-13 03:51:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/11958033/110001

[commit-bot: I haz the power, 2013-02-13 07:39:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/11958033/107011

[commit-bot: I haz the power, 2013-02-13 11:41:42]

Change committed as 182189