Add Query() support to FileRef

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 49dac58d60ada7d43589bd10a201186336c6adf8..1c9836c266a370f3610af422f3a41e9d824c10aa 100644
--- a/ppapi/proxy/ppb_file_ref_proxy.cc
+++ b/ppapi/proxy/ppb_file_ref_proxy.cc
@@ -7,6 +7,7 @@
 #include <map>
 
 #include "base/bind.h"
+#include "base/debug/stack_trace.h"

[dmichael (off chromium), 2013/03/18 23:16:16]

^^ leftover from debugging

 #include "ppapi/c/pp_errors.h"
 #include "ppapi/c/ppb_file_ref.h"
 #include "ppapi/c/private/ppb_file_ref_private.h"
@@ -47,10 +48,13 @@ class FileRef : public PPB_FileRef_Shared {
   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 PP_Var GetAbsolutePath() OVERRIDE;
 
   // Executes the pending callback with the given ID. See pending_callbacks_.
   void ExecuteCallback(int callback_id, int32_t result);
+  void SetFileInfo(int callback_id, const PP_FileInfo &info);
 
  private:
   PluginDispatcher* GetDispatcher() const {
@@ -72,6 +76,12 @@ class FileRef : public PPB_FileRef_Shared {
                    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<unsigned int, PP_FileInfo*> PendingFileInfoMap;

[dmichael (off chromium), 2013/03/18 23:16:16]

It looks like you're using |int| instead of |unsigned int| for the callback id
elsewhere.

[teravest, 2013/03/21 16:45:04]

This is a little gross. PpapiMsg_PPBFileRef_CallbackComplete uses an int for the
callback_id, but we're obviously using unsigned int internally here.

Do you want me to clean this all up as part of this patch?

[dmichael (off chromium), 2013/03/21 22:11:29]

Hmm, I guess you aren't making it worse, so you could do it separately if you
want. But it should probably be changed. I'm guessing you could make them it
uint32_t everywhere and be more consistent with what other messages do.

+  PendingFileInfoMap pending_file_infos_;
+
   DISALLOW_IMPLICIT_CONSTRUCTORS(FileRef);
 };
 
@@ -134,6 +144,16 @@ int32_t FileRef::Rename(PP_Resource new_file_ref,
   return PP_OK_COMPLETIONPENDING;
 }
 
+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;
+}
+
 PP_Var FileRef::GetAbsolutePath() {
   ReceiveSerializedVarReturnValue result;
   GetDispatcher()->Send(new PpapiHostMsg_PPBFileRef_GetAbsolutePath(
@@ -156,6 +176,15 @@ void FileRef::ExecuteCallback(int callback_id, int32_t result) {
   callback->Run(result);
 }
 
+void FileRef::SetFileInfo(int 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;

[dmichael (off chromium), 2013/03/18 23:16:16]

nit: '*' should be with PP_FileInfo

[teravest, 2013/03/21 16:45:04]

Done.

+  *target_info = info;
+  pending_file_infos_.erase(found);
+}
+
 int FileRef::SendCallback(scoped_refptr<TrackedCallback> callback) {
   // In extreme cases the IDs may wrap around, so avoid duplicates.
   while (pending_callbacks_.count(next_callback_id_))
@@ -200,12 +229,15 @@ bool PPB_FileRef_Proxy::OnMessageReceived(const IPC::Message& msg) {
     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_GetAbsolutePath,
                         OnMsgGetAbsolutePath)
 #endif  // !defined(OS_NACL)
 
     IPC_MESSAGE_HANDLER(PpapiMsg_PPBFileRef_CallbackComplete,
                         OnMsgCallbackComplete)
+    IPC_MESSAGE_HANDLER(PpapiMsg_PPBFileRef_QueryCallbackComplete,
+                        OnMsgQueryCallbackComplete)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -294,6 +326,17 @@ void PPB_FileRef_Proxy::OnMsgRename(const HostResource& file_ref,
   }
 }
 
+void PPB_FileRef_Proxy::OnMsgQuery(const HostResource& file_ref,
+                                   int callback_id) {
+  PP_FileInfo info;
+  EnterHostFromHostResourceForceCallback<PPB_FileRef_API> enter(
+      file_ref, callback_factory_,
+      &PPB_FileRef_Proxy::OnQueryCallbackCompleteInHost, file_ref, &info,
+      callback_id);
+  if (enter.succeeded())
+    enter.SetResult(enter.object()->Query(&info, enter.callback()));
+}
+
 void PPB_FileRef_Proxy::OnMsgGetAbsolutePath(const HostResource& host_resource,
                                              SerializedVarReturnValue result) {
   EnterHostFromHostResource<PPB_FileRef_API> enter(host_resource);
@@ -312,6 +355,19 @@ void PPB_FileRef_Proxy::OnMsgCallbackComplete(
     static_cast<FileRef*>(enter.object())->ExecuteCallback(callback_id, result);
 }
 
+void PPB_FileRef_Proxy::OnMsgQueryCallbackComplete(
+    const HostResource& host_resource,
+    PP_FileInfo info,
+    int 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);
+  }
+}
+
 #if !defined(OS_NACL)
 void PPB_FileRef_Proxy::OnCallbackCompleteInHost(
     int32_t result,
@@ -321,6 +377,22 @@ void PPB_FileRef_Proxy::OnCallbackCompleteInHost(
   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,
+    PP_FileInfo* info,
+    int callback_id) {
+  if (result == PP_OK) {
+    Send(new PpapiMsg_PPBFileRef_QueryCallbackComplete(
+        API_ID_PPB_FILE_REF, host_resource, *info, callback_id, result));
+  } else {
+    PP_FileInfo tmp_info;
+    memset(&tmp_info, 0, sizeof(tmp_info));

[dmichael (off chromium), 2013/03/18 23:16:16]

Shouldn't this just be done in PPB_FileRef_Impl? It seems like you shouldn't
need to special case this here

[teravest, 2013/03/21 16:45:04]

I'm special casing it in the proxy because we can't send uninitialized data over
IPC. I don't think we need to zero-out the output parameter in every case.

[dmichael (off chromium), 2013/03/21 22:11:29]

What if you just implement the host side so that it's guaranteed not to modify
the PP_FileInfo (it's probably that way anyway). I think that's consistent with
what we do elsewhere. In that case, you can initialize PP_FileInfo to 0 before
the call unconditionally, and get rid of the if.

WDYT?

+    Send(new PpapiMsg_PPBFileRef_QueryCallbackComplete(
+        API_ID_PPB_FILE_REF, host_resource, tmp_info, callback_id, result));
+  }
+}
 #endif  // !defined(OS_NACL)
 
 }  // namespace proxy