Move DirectoryReader::ReadEntries to FileRef::ReadDirectoryEntries

ppapi/proxy/ppb_file_ref_proxy.cc

Index: ppapi/proxy/ppb_file_ref_proxy.cc
diff --git a/ppapi/proxy/ppb_file_ref_proxy.cc b/ppapi/proxy/ppb_file_ref_proxy.cc
index da621598e24dd6ed24ef138b0790f8aa006214c8..d578e5dd43ed730e4fa89eeb035187a718306db9 100644
--- a/ppapi/proxy/ppb_file_ref_proxy.cc
+++ b/ppapi/proxy/ppb_file_ref_proxy.cc
@@ -16,6 +16,7 @@
 #include "ppapi/proxy/plugin_dispatcher.h"
 #include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/proxy/serialized_var.h"
+#include "ppapi/shared_impl/array_writer.h"
 #include "ppapi/shared_impl/ppb_file_ref_shared.h"
 #include "ppapi/shared_impl/scoped_pp_resource.h"
 #include "ppapi/shared_impl/tracked_callback.h"
@@ -29,6 +30,17 @@ using ppapi::thunk::ResourceCreationAPI;
 namespace ppapi {
 namespace proxy {
 
+namespace {
+
+void ReleaseEntries(const std::vector<PP_DirectoryEntry>& entries) {
+  ResourceTracker* tracker = PpapiGlobals::Get()->GetResourceTracker();
+  for (std::vector<PP_DirectoryEntry>::const_iterator it = entries.begin();
+       it != entries.end(); ++it)
+    tracker->ReleaseResource(it->file_ref);
+}
+
+}  // namespace
+
 class FileRef : public PPB_FileRef_Shared {
  public:
   explicit FileRef(const PPB_FileRef_CreateInfo& info);
@@ -50,11 +62,21 @@ class FileRef : public PPB_FileRef_Shared {
                          scoped_refptr<TrackedCallback> callback) OVERRIDE;
   virtual int32_t Query(PP_FileInfo* info,
                         scoped_refptr<TrackedCallback> callback) OVERRIDE;
+  virtual int32_t ReadEntries(const PP_ArrayOutput& output,
+                              scoped_refptr<TrackedCallback> callback) OVERRIDE;
+  virtual int32_t ReadEntriesInHost(
+      std::vector<ppapi::PPB_FileRef_CreateInfo>* files,
+      std::vector<PP_FileType>* file_types,
+      scoped_refptr<TrackedCallback> callback) OVERRIDE;
   virtual PP_Var GetAbsolutePath() OVERRIDE;
 
   // Executes the pending callback with the given ID. See pending_callbacks_.
   void ExecuteCallback(uint32_t callback_id, int32_t result);
   void SetFileInfo(uint32_t callback_id, const PP_FileInfo& info);
+  int32_t SetReadEntriesOutput(
+      uint32_t callback_id,
+      const std::vector<ppapi::PPB_FileRef_CreateInfo>& infos,
+      const std::vector<PP_FileType>& file_types);
 
  private:
   PluginDispatcher* GetDispatcher() const {
@@ -82,6 +104,12 @@ class FileRef : public PPB_FileRef_Shared {
   typedef std::map<uint32_t, PP_FileInfo*> PendingFileInfoMap;
   PendingFileInfoMap pending_file_infos_;
 
+  // Used to keep PP_ArrayOutput instances that are written before callbacks
+  // are invoked. The id of a pending array output will match that of the
+  // corresponding callback.
+  typedef std::map<uint32_t, PP_ArrayOutput> PendingReadEntriesOutputMap;

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

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.)

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

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.

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

+1, map is more appropriate here.

+  PendingReadEntriesOutputMap pending_read_entries_outputs_;
+
   // Holds a reference on plugin side when running out of process, so that
   // FileSystem won't die before FileRef.  See PPB_FileRef_Impl for
   // corresponding code for in-process mode.  Note that this workaround will
@@ -101,12 +129,14 @@ FileRef::~FileRef() {
   // The callbacks map should have been cleared by LastPluginRefWasDeleted.
   DCHECK(pending_callbacks_.empty());
   DCHECK(pending_file_infos_.empty());
+  DCHECK(pending_read_entries_outputs_.empty());
 }
 
 void FileRef::LastPluginRefWasDeleted() {
   // The callback tracker will abort our callbacks for us.
   pending_callbacks_.clear();
   pending_file_infos_.clear();
+  pending_read_entries_outputs_.clear();
 }
 
 PP_Resource FileRef::GetParent() {
@@ -163,6 +193,24 @@ int32_t FileRef::Query(PP_FileInfo* info,
   return PP_OK_COMPLETIONPENDING;
 }
 
+int32_t FileRef::ReadEntries(const PP_ArrayOutput& output,
+                             scoped_refptr<TrackedCallback> callback) {
+  // Store the pending read entries output id.
+  int id = SendCallback(callback);

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

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.

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

Done.

+  pending_read_entries_outputs_[id] = output;
+  GetDispatcher()->Send(new PpapiHostMsg_PPBFileRef_ReadEntries(
+      API_ID_PPB_FILE_REF, host_resource(), id));
+  return PP_OK_COMPLETIONPENDING;
+}
+
+int32_t FileRef::ReadEntriesInHost(
+    std::vector<ppapi::PPB_FileRef_CreateInfo>* files,
+    std::vector<PP_FileType>* file_types,
+    scoped_refptr<TrackedCallback> callback) {
+  NOTREACHED();
+  return PP_ERROR_FAILED;
+}
+
 PP_Var FileRef::GetAbsolutePath() {
   ReceiveSerializedVarReturnValue result;
   GetDispatcher()->Send(new PpapiHostMsg_PPBFileRef_GetAbsolutePath(
@@ -194,6 +242,39 @@ void FileRef::SetFileInfo(uint32_t callback_id, const PP_FileInfo& info) {
   pending_file_infos_.erase(found);
 }
 
+int32_t FileRef::SetReadEntriesOutput(
+    uint32_t callback_id,
+    const std::vector<ppapi::PPB_FileRef_CreateInfo>& infos,
+    const std::vector<PP_FileType>& file_types) {
+  PendingReadEntriesOutputMap::iterator found =
+      pending_read_entries_outputs_.find(callback_id);
+  if (found == pending_read_entries_outputs_.end())
+    return PP_OK;

[raymes, 2013/05/02 21:45:21]

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.

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

Oops, I think I tried to follow the logic of Query. I think Query should be
fixed. Done.

+
+  PP_ArrayOutput output = found->second;
+  pending_read_entries_outputs_.erase(found);
+
+  std::vector<PP_DirectoryEntry> entries;
+  for (std::vector<ppapi::PPB_FileRef_CreateInfo>::size_type i = 0;

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

Style guide says to just use size_t instead of the fully-qualified
....::size_type.

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

Done.

+       i < infos.size(); ++i) {
+    PP_DirectoryEntry entry;
+    entry.file_ref = PPB_FileRef_Proxy::DeserializeFileRef(infos[i]);
+    entry.file_type = file_types[i];
+    entries.push_back(entry);
+  }
+
+  ArrayWriter writer(output);
+  if (!writer.is_valid()) {
+    ReleaseEntries(entries);
+    entries.clear();
+    return PP_ERROR_BADARGUMENT;
+  }
+
+  writer.StoreVector(entries);
+  entries.clear();

[raymes, 2013/05/02 21:45:21]

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.

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

Done.

+  return PP_OK;
+}
+
 uint32_t FileRef::SendCallback(scoped_refptr<TrackedCallback> callback) {
   // In extreme cases the IDs may wrap around, so avoid duplicates.
   while (pending_callbacks_.count(next_callback_id_))
@@ -234,6 +315,7 @@ bool PPB_FileRef_Proxy::OnMessageReceived(const IPC::Message& msg) {
     IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBFileRef_Delete, OnMsgDelete)
     IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBFileRef_Rename, OnMsgRename)
     IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBFileRef_Query, OnMsgQuery)
+    IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBFileRef_ReadEntries, OnMsgReadEntries)
     IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBFileRef_GetAbsolutePath,
                         OnMsgGetAbsolutePath)
 #endif  // !defined(OS_NACL)
@@ -242,6 +324,8 @@ bool PPB_FileRef_Proxy::OnMessageReceived(const IPC::Message& msg) {
                         OnMsgCallbackComplete)
     IPC_MESSAGE_HANDLER(PpapiMsg_PPBFileRef_QueryCallbackComplete,
                         OnMsgQueryCallbackComplete)
+    IPC_MESSAGE_HANDLER(PpapiMsg_PPBFileRef_ReadEntriesCallbackComplete,
+                        OnMsgReadEntriesCallbackComplete)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -334,13 +418,13 @@ void PPB_FileRef_Proxy::OnMsgRename(const HostResource& file_ref,
 
 void PPB_FileRef_Proxy::OnMsgQuery(const HostResource& file_ref,
                                    uint32_t callback_id) {
-  PP_FileInfo* info = new PP_FileInfo();
+  linked_ptr<PP_FileInfo> info(new PP_FileInfo());
   EnterHostFromHostResourceForceCallback<PPB_FileRef_API> enter(
       file_ref, callback_factory_,
       &PPB_FileRef_Proxy::OnQueryCallbackCompleteInHost, file_ref,
-      base::Owned(info), callback_id);
+      info, callback_id);
   if (enter.succeeded())
-    enter.SetResult(enter.object()->Query(info, enter.callback()));
+    enter.SetResult(enter.object()->Query(info.get(), enter.callback()));
 }
 
 void PPB_FileRef_Proxy::OnMsgGetAbsolutePath(const HostResource& host_resource,
@@ -349,6 +433,25 @@ void PPB_FileRef_Proxy::OnMsgGetAbsolutePath(const HostResource& host_resource,
   if (enter.succeeded())
     result.Return(dispatcher(), enter.object()->GetAbsolutePath());
 }
+
+void PPB_FileRef_Proxy::OnMsgReadEntries(const HostResource& file_ref,
+                                         uint32_t callback_id) {
+  linked_ptr<std::vector<ppapi::PPB_FileRef_CreateInfo> > files(
+      new std::vector<ppapi::PPB_FileRef_CreateInfo>());
+  linked_ptr<std::vector<PP_FileType> > file_types(
+      new std::vector<PP_FileType>());
+  HostCallbackParams params(file_ref, callback_id);
+  EnterHostFromHostResourceForceCallback<PPB_FileRef_API> enter(
+      file_ref, callback_factory_,
+      &PPB_FileRef_Proxy::OnReadEntriesCallbackCompleteInHost,
+      params, files, file_types);
+  if (enter.succeeded()) {
+    enter.SetResult(enter.object()->ReadEntriesInHost(files.get(),
+                                                      file_types.get(),
+                                                      enter.callback()));

[dmichael (off chromium), 2013/05/02 20:39:13]

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.

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

Done. See the comment in file_callbacks.h for detail.

+  }
+}
+
 #endif  // !defined(OS_NACL)
 
 void PPB_FileRef_Proxy::OnMsgCallbackComplete(
@@ -374,6 +477,24 @@ void PPB_FileRef_Proxy::OnMsgQueryCallbackComplete(
   }
 }
 
+void PPB_FileRef_Proxy::OnMsgReadEntriesCallbackComplete(
+    const HostResource& host_resource,
+    const std::vector<ppapi::PPB_FileRef_CreateInfo>& infos,
+    const std::vector<PP_FileType>& file_types,
+    uint32_t callback_id,
+    int32_t result) {
+  CHECK_EQ(infos.size(), file_types.size());
+
+  EnterPluginFromHostResource<PPB_FileRef_API> enter(host_resource);
+  if (enter.succeeded()) {

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

Style nit: You could save a level of indentation by doing:

if (!enter.succeeded())
  return;

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

Done.

+    if (result == PP_OK)
+      result = static_cast<FileRef*>(enter.object())->SetReadEntriesOutput(
+          callback_id, infos, file_types);

[dmichael (off chromium), 2013/05/02 20:39:13]

nit: please use curly braces for >1 line (even when it's 1 statement)

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

Done.

+    static_cast<FileRef*>(enter.object())->ExecuteCallback(
+        callback_id, result);
+  }
+}
+
 #if !defined(OS_NACL)
 void PPB_FileRef_Proxy::OnCallbackCompleteInHost(
     int32_t result,
@@ -387,11 +508,22 @@ void PPB_FileRef_Proxy::OnCallbackCompleteInHost(
 void PPB_FileRef_Proxy::OnQueryCallbackCompleteInHost(
     int32_t result,
     const HostResource& host_resource,
-    base::internal::OwnedWrapper<PP_FileInfo> info,
+    linked_ptr<PP_FileInfo> info,
     uint32_t callback_id) {
   Send(new PpapiMsg_PPBFileRef_QueryCallbackComplete(
       API_ID_PPB_FILE_REF, host_resource, *info.get(), callback_id, result));
 }
+
+void PPB_FileRef_Proxy::OnReadEntriesCallbackCompleteInHost(
+    int32_t result,
+    HostCallbackParams params,
+    linked_ptr<std::vector<ppapi::PPB_FileRef_CreateInfo> > files,
+    linked_ptr<std::vector<PP_FileType> > file_types) {
+  Send(new PpapiMsg_PPBFileRef_ReadEntriesCallbackComplete(
+      API_ID_PPB_FILE_REF, params.host_resource,
+      *files, *file_types, params.callback_id, result));
+}
+
 #endif  // !defined(OS_NACL)
 
 }  // namespace proxy