Pepper: Simplify OpenResource() for Non-SFI.

components/nacl/renderer/ppb_nacl_private_impl.cc

Index: components/nacl/renderer/ppb_nacl_private_impl.cc
diff --git a/components/nacl/renderer/ppb_nacl_private_impl.cc b/components/nacl/renderer/ppb_nacl_private_impl.cc
index e7150e6ddfce0c89d4b757c7681ba088dbdc9495..cc35f92566183aff4f0518d5690a87239feec439 100644
--- a/components/nacl/renderer/ppb_nacl_private_impl.cc
+++ b/components/nacl/renderer/ppb_nacl_private_impl.cc
@@ -113,6 +113,14 @@ nacl::NexeLoadManager* GetNexeLoadManager(PP_Instance instance) {
   return NULL;
 }
 
+PP_NaClFileInfo MakeInvalidNaClFileInfo() {

[hidehiko, 2014/06/18 04:43:11]

Just FYI:
How about using a constant here?

const PP_NaClFileInfo kInvalidNaClFileInfo = {
  PP_kInvalidFileHandle,
  0,  // token_lo
  0,  // token_hi
};

[teravest, 2014/06/18 20:13:43]

Done.

+  PP_NaClFileInfo info;
+  info.handle = PP_kInvalidFileHandle;
+  info.token_lo = 0;
+  info.token_hi = 0;
+  return info;
+}
+
 int GetRoutingID(PP_Instance instance) {
   // Check that we are on the main renderer thread.
   DCHECK(content::RenderThread::Get());
@@ -145,16 +153,29 @@ void PostPPCompletionCallback(PP_CompletionCallback callback,
       base::Bind(callback.func, callback.user_data, status));
 }
 
+bool ManifestResolveKey(PP_Instance instance,

[hidehiko, 2014/06/18 04:43:11]

I'm rarely see this kind of forward declaration. Can we avoid?

[teravest, 2014/06/18 20:13:43]

I don't think so; I could try to reorganize the file so implementations come
before they're used, but I'd like for the function declarations to roughly match
the interface order.

I hope to get remove ManifestResolveKey from PPB_NaCl_Private soon, which should
make things better.

+                        bool is_helper_process,
+                        const std::string& key,
+                        std::string* full_url,
+                        PP_PNaClOptions* pnacl_options);
+
+void DownloadFile(PP_Instance instance,
+                  const char* url,
+                  base::Callback<void(int32_t, PP_NaClFileInfo)> callback);

[hidehiko, 2014/06/18 04:43:11]

const PP_NaClFileInfo& ? Ditto for below.

[teravest, 2014/06/18 20:13:44]

Done.

+
 // Thin adapter from PPP_ManifestService to ManifestServiceChannel::Delegate.
 // Note that user_data is managed by the caller of LaunchSelLdr. Please see
 // also PP_ManifestService's comment for more details about resource
 // management.
 class ManifestServiceProxy : public ManifestServiceChannel::Delegate {
  public:
-  ManifestServiceProxy(const PPP_ManifestService* manifest_service,
+  ManifestServiceProxy(PP_Instance pp_instance,
+                       const PPP_ManifestService* manifest_service,
                        void* user_data)
-      : manifest_service_(*manifest_service),
-        user_data_(user_data) {
+      : pp_instance_(pp_instance),
+        manifest_service_(*manifest_service),
+        user_data_(user_data),
+        weak_factory_(this) {
   }
 
   virtual ~ManifestServiceProxy() {
@@ -177,22 +198,30 @@ class ManifestServiceProxy : public ManifestServiceChannel::Delegate {
     if (!user_data_)
       return;
 
-    // The allocated callback will be freed in DidOpenResource, which is always
-    // called regardless whether OpenResource() succeeds or fails.
-    if (!PP_ToBool(manifest_service_.OpenResource(
-            user_data_,
-            key.c_str(),
-            DidOpenResource,
-            new ManifestServiceChannel::OpenResourceCallback(callback)))) {
-      user_data_ = NULL;
+    std::string url;
+    PP_PNaClOptions pnacl_options = {PP_FALSE, PP_FALSE, 2};

[dmichael (off chromium), 2014/06/17 21:52:28]

nit: This would be more readable if you zero-initialized and then set the fields
by name...
PP_PNaClOptions pnacl_options = {};
pnacl_options.foo = PP_FALSE;
etc

[teravest, 2014/06/18 20:13:44]

Done.

+    if (!ManifestResolveKey(pp_instance_, false, key, &url, &pnacl_options)) {
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(callback, PP_kInvalidFileHandle));
+      return;
     }
+
+    DownloadFile(pp_instance_, url.c_str(),
+                 base::Bind(&ManifestServiceProxy::DidDownloadFile,
+                            weak_factory_.GetWeakPtr(),
+                            callback));
   }
 
  private:
-  static void DidOpenResource(void* user_data, PP_FileHandle file_handle) {
-    scoped_ptr<ManifestServiceChannel::OpenResourceCallback> callback(
-        static_cast<ManifestServiceChannel::OpenResourceCallback*>(user_data));
-    callback->Run(file_handle);
+  void DidDownloadFile(ManifestServiceChannel::OpenResourceCallback callback,

[hidehiko, 2014/06/18 04:43:11]

Can be static? Or is it intentional change that the callback won't be called
after this instance is destructed?

Would be great to document the cancellation policy.

Also, if this instance is deleted during the file downloading, the opened file
handle will be leaked.

[teravest, 2014/06/18 20:13:44]

Changed this to be static. It's better without the weak ref issue.

+                       int32_t pp_error,
+                       PP_NaClFileInfo file_info) {
+    if (pp_error != PP_OK) {
+      callback.Run(base::kInvalidPlatformFileValue);
+      return;
+    }
+    callback.Run(file_info.handle);
   }
 
   void Quit() {
@@ -204,8 +233,11 @@ class ManifestServiceProxy : public ManifestServiceChannel::Delegate {
     user_data_ = NULL;
   }
 
+  PP_Instance pp_instance_;
   PPP_ManifestService manifest_service_;
   void* user_data_;
+  base::WeakPtrFactory<ManifestServiceProxy> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ManifestServiceProxy);
 };
 
@@ -271,7 +303,8 @@ void LaunchSelLdr(PP_Instance instance,
   // will be called in its destructor so that the caller of this function
   // can free manifest_service_user_data properly.
   scoped_ptr<ManifestServiceChannel::Delegate> manifest_service_proxy(
-      new ManifestServiceProxy(manifest_service_interface,
+      new ManifestServiceProxy(instance,
+                               manifest_service_interface,
                                manifest_service_user_data));
 
   FileDescriptor result_socket;
@@ -1368,34 +1401,31 @@ void DownloadNexeCompletion(const DownloadNexeRequest& request,
   request.callback.func(request.callback.user_data, pp_error);
 }
 
-void DownloadFileCompletion(PP_NaClFileInfo* file_info,
-                            PP_CompletionCallback callback,
-                            FileDownloader::Status status,
-                            base::File file,
-                            int http_status) {
+void DownloadFileCompletion(
+    base::Callback<void(int32_t, PP_NaClFileInfo)> callback,
+    FileDownloader::Status status,
+    base::File file,
+    int http_status) {
   int32_t pp_error = FileDownloaderToPepperError(status);
-  if (pp_error == PP_OK) {
-    file_info->handle = file.TakePlatformFile();
-    file_info->token_lo = 0;
-    file_info->token_hi = 0;
-  }
-  callback.func(callback.user_data, pp_error);
+  PP_NaClFileInfo file_info = MakeInvalidNaClFileInfo();
+  if (pp_error == PP_OK)
+    file_info.handle = file.TakePlatformFile();

[hidehiko, 2014/06/18 04:43:11]

Setting the platform file handle to invalid file info looks a bit strange.
Can we create:
- a valid file info with the platform file, or
- an invalid file info iff an operation fails.
?

[teravest, 2014/06/18 20:13:44]

Done.

+  callback.Run(pp_error, file_info);
 }
 
 void DownloadFile(PP_Instance instance,
                   const char* url,
-                  struct PP_NaClFileInfo* file_info,
-                  struct PP_CompletionCallback callback) {
+                  base::Callback<void(int32_t, PP_NaClFileInfo)> callback) {
   CHECK(url);
-  CHECK(file_info);
 
   NexeLoadManager* load_manager = GetNexeLoadManager(instance);
   DCHECK(load_manager);
   if (!load_manager) {
-    ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
+    base::MessageLoop::current()->PostTask(

[dmichael (off chromium), 2014/06/17 21:52:28]

Is this only a cosmetic change, or are you trying to make this callable from a
background thread?

I'm pretty sure we haven't made accessing and using NexeLoadManager thread-safe.

If it's just cosmetic, it might be good to DCHECK that you're on the main thread
at the beginning of the method.

[teravest, 2014/06/18 20:13:44]

Done.

         FROM_HERE,
-        base::Bind(callback.func, callback.user_data,
-                   static_cast<int32_t>(PP_ERROR_FAILED)));
+        base::Bind(callback,
+                   static_cast<int32_t>(PP_ERROR_FAILED),
+                   MakeInvalidNaClFileInfo()));
     return;
   }
 
@@ -1405,21 +1435,22 @@ void DownloadFile(PP_Instance instance,
   if (url_string.find(kPNaClTranslatorBaseUrl, 0) == 0) {
     PP_FileHandle handle = GetReadonlyPnaclFd(url);
     if (handle == PP_kInvalidFileHandle) {
-      ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
+      base::MessageLoop::current()->PostTask(
           FROM_HERE,
-          base::Bind(callback.func, callback.user_data,
-                     static_cast<int32_t>(PP_ERROR_FAILED)));
+          base::Bind(callback,
+                     static_cast<int32_t>(PP_ERROR_FAILED),
+                     MakeInvalidNaClFileInfo()));
       return;
     }
     // TODO(ncbray): enable the fast loading and validation paths for this type
     // of file.
-    file_info->handle = handle;
-    file_info->token_lo = 0;
-    file_info->token_hi = 0;
-    ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
+    PP_NaClFileInfo file_info;
+    file_info.handle = handle;
+    file_info.token_lo = 0;
+    file_info.token_hi = 0;
+    base::MessageLoop::current()->PostTask(
         FROM_HERE,
-        base::Bind(callback.func, callback.user_data,
-                   static_cast<int32_t>(PP_OK)));
+        base::Bind(callback, static_cast<int32_t>(PP_OK), file_info));
     return;
   }
 
@@ -1427,10 +1458,11 @@ void DownloadFile(PP_Instance instance,
   // before downloading it.
   const GURL& test_gurl = load_manager->plugin_base_url().Resolve(url);
   if (!test_gurl.is_valid()) {
-    ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
+    base::MessageLoop::current()->PostTask(
         FROM_HERE,
-        base::Bind(callback.func, callback.user_data,
-                   static_cast<int32_t>(PP_ERROR_FAILED)));
+        base::Bind(callback,
+                   static_cast<int32_t>(PP_ERROR_FAILED),
+                   MakeInvalidNaClFileInfo()));
     return;
   }
 
@@ -1442,13 +1474,13 @@ void DownloadFile(PP_Instance instance,
                                                  &file_token_lo,
                                                  &file_token_hi);
   if (file_handle != PP_kInvalidFileHandle) {
-    file_info->handle = file_handle;
-    file_info->token_lo = file_token_lo;
-    file_info->token_hi = file_token_hi;
-    ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
+    PP_NaClFileInfo file_info;
+    file_info.handle = file_handle;
+    file_info.token_lo = file_token_lo;
+    file_info.token_hi = file_token_hi;
+    base::MessageLoop::current()->PostTask(
         FROM_HERE,
-        base::Bind(callback.func, callback.user_data,
-                   static_cast<int32_t>(PP_OK)));
+        base::Bind(callback, static_cast<int32_t>(PP_OK), file_info));
     return;
   }
 
@@ -1460,10 +1492,11 @@ void DownloadFile(PP_Instance instance,
   content::PepperPluginInstance* plugin_instance =
       content::PepperPluginInstance::Get(instance);
   if (!plugin_instance) {
-    ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
+    base::MessageLoop::current()->PostTask(
         FROM_HERE,
-        base::Bind(callback.func, callback.user_data,
-                   static_cast<int32_t>(PP_ERROR_FAILED)));
+        base::Bind(callback,
+                   static_cast<int32_t>(PP_ERROR_FAILED),
+                   MakeInvalidNaClFileInfo()));
   }
   const blink::WebDocument& document =
       plugin_instance->GetContainer()->element().document();
@@ -1477,12 +1510,37 @@ void DownloadFile(PP_Instance instance,
   FileDownloader* file_downloader = new FileDownloader(
       url_loader.Pass(),
       target_file.Pass(),
-      base::Bind(&DownloadFileCompletion, file_info, callback),
+      base::Bind(&DownloadFileCompletion, callback),
       base::Bind(&ProgressEventRateLimiter::ReportProgress,
                  base::Owned(tracker), url));
   file_downloader->Load(url_request);
 }
 
+void ExternalDownloadFileCompletion(struct PP_NaClFileInfo* out_file_info,
+                                    struct PP_CompletionCallback callback,
+                                    int32_t pp_error,
+                                    PP_NaClFileInfo file_info) {
+  DCHECK(callback.func);

[dmichael (off chromium), 2014/06/17 21:52:28]

nit: This DCHECK seems either unnecessary or like it should be inside
ExternalDownloadFile (to make it easier to catch a bad call at the right place)

[teravest, 2014/06/18 20:13:43]

Done.

+  if (pp_error == PP_OK) {
+    out_file_info->handle = file_info.handle;

[hidehiko, 2014/06/18 04:43:11]

Maybe:
*out_file_info = file_info; 

[teravest, 2014/06/18 20:13:43]

Done.

+    out_file_info->token_lo = file_info.token_lo;
+    out_file_info->token_hi = file_info.token_hi;
+  }
+  callback.func(callback.user_data, pp_error);
+}
+
+void ExternalDownloadFile(PP_Instance instance,
+                          const char* url,
+                          struct PP_NaClFileInfo* file_info,
+                          struct PP_CompletionCallback callback) {
+  CHECK(ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->
+            BelongsToCurrentThread());
+  DownloadFile(instance, url,
+               base::Bind(&ExternalDownloadFileCompletion,
+                          file_info,
+                          callback));
+}
+
 void ReportSelLdrStatus(PP_Instance instance,
                         int32_t load_status,
                         int32_t max_status) {
@@ -1546,7 +1604,7 @@ const PPB_NaCl_Private nacl_interface = {
   &GetCpuFeatureAttrs,
   &PostMessageToJavaScript,
   &DownloadNexe,
-  &DownloadFile,
+  &ExternalDownloadFile,
   &ReportSelLdrStatus,
   &LogTranslateTime
 };