Move DirectoryReader::ReadEntries to FileRef::ReadDirectoryEntries

ppapi/proxy/ppb_file_ref_proxy.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ppapi/proxy/ppb_file_ref_proxy.h"
 
 #include <map>
 
 #include "base/bind.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/c/ppb_file_ref.h"
 #include "ppapi/c/private/ppb_file_ref_private.h"
 #include "ppapi/c/private/ppb_proxy_private.h"
 #include "ppapi/proxy/enter_proxy.h"
 #include "ppapi/proxy/host_dispatcher.h"
 #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"
 #include "ppapi/thunk/resource_creation_api.h"
 #include "ppapi/thunk/thunk.h"
 
 using ppapi::thunk::EnterResourceNoLock;
 using ppapi::thunk::PPB_FileRef_API;
 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);
   virtual ~FileRef();
 
   // Resource overrides.
   virtual void LastPluginRefWasDeleted() OVERRIDE;
 
   // PPB_FileRef_API implementation (not provided by PPB_FileRef_Shared).
   virtual PP_Resource GetParent() OVERRIDE;
   virtual int32_t MakeDirectory(
       PP_Bool make_ancestors,
       scoped_refptr<TrackedCallback> callback) OVERRIDE;
   virtual int32_t Touch(PP_Time last_access_time,
                         PP_Time last_modified_time,
                         scoped_refptr<TrackedCallback> callback) OVERRIDE;
   virtual int32_t Delete(scoped_refptr<TrackedCallback> callback) OVERRIDE;
   virtual int32_t Rename(PP_Resource new_file_ref,
                          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 {
     return PluginDispatcher::GetForResource(this);
   }
 
   // Adds a callback to the list and returns its ID.
   uint32_t SendCallback(scoped_refptr<TrackedCallback> callback);
 
   // This class can have any number of out-standing requests with completion
   // callbacks, in contrast to most resources which have one possible pending
   // callback pending (like a Flush callback).
   //
   // To keep track of them, assign integer IDs to the callbacks, which is how
   // the callback will be identified when it's passed to the host and then
   // back here. Use unsigned so that overflow is well-defined.
   uint32_t next_callback_id_;
   typedef std::map<uint32_t,
                    scoped_refptr<TrackedCallback> > PendingCallbackMap;
   PendingCallbackMap pending_callbacks_;
 
   // Used to keep pointers to PP_FileInfo instances that are written before
   // callbacks are invoked. The id of a pending file info will match that of
   // the corresponding callback.
   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
   // be no longer needed after FileRef refactoring.
   ScopedPPResource file_system_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(FileRef);
 };
 
 FileRef::FileRef(const PPB_FileRef_CreateInfo& info)
     : PPB_FileRef_Shared(OBJECT_IS_PROXY, info),
       next_callback_id_(0u),
       file_system_(info.file_system_plugin_resource) {
 }
 
 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() {
   PPB_FileRef_CreateInfo create_info;
   GetDispatcher()->Send(new PpapiHostMsg_PPBFileRef_GetParent(
       API_ID_PPB_FILE_REF, host_resource(), &create_info));
   return PPB_FileRef_Proxy::DeserializeFileRef(create_info);
 }
 
 int32_t FileRef::MakeDirectory(PP_Bool make_ancestors,
@@ skipping 36 matching lines @@
 int32_t FileRef::Query(PP_FileInfo* info,
                        scoped_refptr<TrackedCallback> callback) {
   // Store the pending file info id.
   int id = SendCallback(callback);
   pending_file_infos_[id] = info;
   GetDispatcher()->Send(new PpapiHostMsg_PPBFileRef_Query(
       API_ID_PPB_FILE_REF, host_resource(), id));
   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(
       API_ID_PPB_FILE_REF, host_resource(), &result));
   return result.Return(GetDispatcher());
 }
 
 void FileRef::ExecuteCallback(uint32_t callback_id, int32_t result) {
   PendingCallbackMap::iterator found = pending_callbacks_.find(callback_id);
   if (found == pending_callbacks_.end()) {
@@ skipping 11 matching lines @@
 
 void FileRef::SetFileInfo(uint32_t callback_id, const PP_FileInfo& info) {
   PendingFileInfoMap::iterator found = pending_file_infos_.find(callback_id);
   if (found == pending_file_infos_.end())
     return;
   PP_FileInfo* target_info = found->second;
   *target_info = 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_))
     ++next_callback_id_;
 
   pending_callbacks_[next_callback_id_] = callback;
   return next_callback_id_++;
 }
 
 PPB_FileRef_Proxy::PPB_FileRef_Proxy(Dispatcher* dispatcher)
@@ skipping 20 matching lines @@
   IPC_BEGIN_MESSAGE_MAP(PPB_FileRef_Proxy, msg)
 #if !defined(OS_NACL)
     IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBFileRef_Create, OnMsgCreate)
     IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBFileRef_GetParent, OnMsgGetParent)
     IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBFileRef_MakeDirectory,
                         OnMsgMakeDirectory)
     IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBFileRef_Touch, OnMsgTouch)
     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)
 
     IPC_MESSAGE_HANDLER(PpapiMsg_PPBFileRef_CallbackComplete,
                         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;
 }
 
 // static
 void PPB_FileRef_Proxy::SerializeFileRef(PP_Resource file_ref,
                                          PPB_FileRef_CreateInfo* result) {
   EnterResourceNoLock<PPB_FileRef_API> enter(file_ref, false);
   if (enter.succeeded())
@@ skipping 72 matching lines @@
       file_ref, callback_factory_,
       &PPB_FileRef_Proxy::OnCallbackCompleteInHost, file_ref, callback_id);
   if (enter.succeeded()) {
     enter.SetResult(enter.object()->Rename(new_file_ref.host_resource(),
                                            enter.callback()));
   }
 }
 
 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,
                                              SerializedVarReturnValue result) {
   EnterHostFromHostResource<PPB_FileRef_API> enter(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(
     const HostResource& host_resource,
     uint32_t callback_id,
     int32_t result) {
   // Forward the callback info to the plugin resource.
   EnterPluginFromHostResource<PPB_FileRef_API> enter(host_resource);
   if (enter.succeeded())
     static_cast<FileRef*>(enter.object())->ExecuteCallback(callback_id, result);
 }
 
 void PPB_FileRef_Proxy::OnMsgQueryCallbackComplete(
     const HostResource& host_resource,
     const PP_FileInfo& info,
     uint32_t callback_id,
     int32_t result) {
   EnterPluginFromHostResource<PPB_FileRef_API> enter(host_resource);
   if (enter.succeeded()) {
     // Set the FileInfo output parameter.
     static_cast<FileRef*>(enter.object())->SetFileInfo(callback_id, info);
     static_cast<FileRef*>(enter.object())->ExecuteCallback(callback_id, result);
   }
 }
 
+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,
     const HostResource& host_resource,
     uint32_t callback_id) {
   // Execute OnMsgCallbackComplete in the plugin process.
   Send(new PpapiMsg_PPBFileRef_CallbackComplete(
       API_ID_PPB_FILE_REF, host_resource, callback_id, result));
 }
 
 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
 }  // namespace ppapi