PPAPI: Refactor renderer side of browser host creation

content/renderer/pepper/resource_converter.cc

 // Copyright 2013 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 "content/renderer/pepper/resource_converter.h"
 
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/message_loop/message_loop.h"
 #include "content/public/renderer/renderer_ppapi_host.h"
 #include "content/renderer/pepper/pepper_file_system_host.h"
 #include "content/renderer/pepper/pepper_media_stream_audio_track_host.h"
 #include "content/renderer/pepper/pepper_media_stream_video_track_host.h"
 #include "ipc/ipc_message.h"
 #include "ppapi/host/ppapi_host.h"
 #include "ppapi/host/resource_host.h"
 #include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/shared_impl/resource_var.h"
 #include "ppapi/shared_impl/scoped_pp_var.h"
 #include "storage/common/fileapi/file_system_util.h"
 #include "third_party/WebKit/public/platform/WebFileSystem.h"
 #include "third_party/WebKit/public/platform/WebMediaStreamSource.h"
 #include "third_party/WebKit/public/platform/WebMediaStreamTrack.h"
 #include "third_party/WebKit/public/web/WebDOMFileSystem.h"
 #include "third_party/WebKit/public/web/WebDOMMediaStreamTrack.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
 
 using ppapi::ResourceVar;
 
 namespace content {
 namespace {
 
-void FlushComplete(
-    const base::Callback<void(bool)>& callback,
-    const std::vector<scoped_refptr<content::HostResourceVar> >& browser_vars,
-    const std::vector<int>& pending_host_ids) {
-  CHECK(browser_vars.size() == pending_host_ids.size());
-  for (size_t i = 0; i < browser_vars.size(); ++i) {
-    browser_vars[i]->set_pending_browser_host_id(pending_host_ids[i]);
-  }
-  callback.Run(true);
-}
-
 // Converts a blink::WebFileSystem::Type to a PP_FileSystemType.
 PP_FileSystemType WebFileSystemTypeToPPAPI(blink::WebFileSystem::Type type) {
   switch (type) {
     case blink::WebFileSystem::TypeTemporary:
       return PP_FILESYSTEMTYPE_LOCALTEMPORARY;
     case blink::WebFileSystem::TypePersistent:
       return PP_FILESYSTEMTYPE_LOCALPERSISTENT;
     case blink::WebFileSystem::TypeIsolated:
       return PP_FILESYSTEMTYPE_ISOLATED;
     case blink::WebFileSystem::TypeExternal:
@@ skipping 138 matching lines @@
   }
 #endif
   return false;
 }
 
 }  // namespace
 
 ResourceConverter::~ResourceConverter() {}
 
 ResourceConverterImpl::ResourceConverterImpl(PP_Instance instance)
-    : instance_(instance) {}
+    : instance_(instance),
+      weak_ptr_factory_(this) {}
 
 ResourceConverterImpl::~ResourceConverterImpl() {
   // Verify Flush() was called.
   DCHECK(browser_host_create_messages_.empty());
   DCHECK(browser_vars_.empty());
 }
 
 bool ResourceConverterImpl::FromV8Value(v8::Handle<v8::Object> val,
                                         v8::Handle<v8::Context> context,
                                         PP_Var* result,
@@ skipping 57 matching lines @@
 void ResourceConverterImpl::Reset() {
   browser_host_create_messages_.clear();
   browser_vars_.clear();
 }
 
 bool ResourceConverterImpl::NeedsFlush() {
   return !browser_host_create_messages_.empty();
 }
 
 void ResourceConverterImpl::Flush(const base::Callback<void(bool)>& callback) {
-  RendererPpapiHost::GetForPPInstance(instance_)->CreateBrowserResourceHosts(
-      instance_,
-      browser_host_create_messages_,
-      base::Bind(&FlushComplete, callback, browser_vars_));
+  pending_conversions_.push(PendingConversion());
+  pending_conversions_.back().callback = callback;
+  pending_conversions_.back().browser_vars.swap(browser_vars_);
+  pending_conversions_.back().sequence_num =
+      RendererPpapiHost::GetForPPInstance(instance_)->
+          CreateBrowserResourceHosts(
+              instance_,
+              browser_host_create_messages_,
+              base::Bind(&ResourceConverterImpl::FlushComplete,
+                         weak_ptr_factory_.GetWeakPtr()));
+
   browser_host_create_messages_.clear();
-  browser_vars_.clear();
 }
 
 bool ResourceConverterImpl::ToV8Value(const PP_Var& var,
                                       v8::Handle<v8::Context> context,
                                       v8::Handle<v8::Value>* result) {
   DCHECK(var.type == PP_VARTYPE_RESOURCE);
 
   ResourceVar* resource = ResourceVar::FromPPVar(var);
   if (!resource) {
     NOTREACHED();
@@ skipping 30 matching lines @@
         static_cast<content::PepperMediaStreamVideoTrackHost*>(resource_host),
         context,
         result);
   } else {
     LOG(ERROR) << "The type of resource #" << resource_id
                << " cannot be converted to a JavaScript object.";
     return false;
   }
 }
 
+ResourceConverterImpl::PendingConversion::PendingConversion() {}
+ResourceConverterImpl::PendingConversion::~PendingConversion() {}
+
 scoped_refptr<HostResourceVar> ResourceConverterImpl::CreateResourceVar(
     int pending_renderer_id,
     const IPC::Message& create_message) {
   return new HostResourceVar(pending_renderer_id, create_message);
 }
 
 scoped_refptr<HostResourceVar>
 ResourceConverterImpl::CreateResourceVarWithBrowserHost(
     int pending_renderer_id,
     const IPC::Message& create_message,
     const IPC::Message& browser_host_create_message) {
   scoped_refptr<HostResourceVar> result =
       CreateResourceVar(pending_renderer_id, create_message);
   browser_host_create_messages_.push_back(browser_host_create_message);
   browser_vars_.push_back(result);
   return result;
 }
 
+void ResourceConverterImpl::FlushComplete(
+    const ppapi::proxy::CompletedBrowserResourceHosts& hosts) {
+  if (pending_conversions_.empty() ||
+      hosts.sequence_num < pending_conversions_.front().sequence_num) {

[raymes, 2014/11/17 02:48:11]

should the sequence numbers be equal?

[dmichael (off chromium), 2014/11/17 22:26:20]

For now :) I improved my check in the latest patch, thanks.

Later, when responses can come back either via the usual async reply or by the
response to GetAllPendingBrowserHosts, FlushComplete might happen twice for a
given sequence number. I.e., the browser had already sent the async reply, but
the renderer hasn't gotten it yet when it does GetAllPendingBrowserHosts. In
that case, we'll process the FlushComplete in response to
GetAllPendingBrowserHosts. Later, when we get the async reply message, we'll see
that the sequence number must have happened already, so we can ignore it.

See FlushComplete in:
https://codereview.chromium.org/605593002/diff/320001/content/renderer/pepper...

(And to answer your other question, this check is the only current use of the
sequence number. But hopefully the above comment helps explain what it will be
used for.)

+    NOTREACHED();
+    return;
+  }
+  const PendingConversion& conversion = pending_conversions_.front();

[raymes, 2014/11/17 02:48:11]

Will FlushComplete() always come back in-order?

[dmichael (off chromium), 2014/11/17 22:26:20]

Yes. The order of async messages is guaranteed. When we make the Sync call, the
browser-side handler will also return them in order.

If you can think of a counter-example, let me know.

+  if (conversion.browser_vars.size() != hosts.host_ids.size()) {
+    NOTREACHED();
+    pending_conversions_.pop();
+    return;
+  }
+  for (size_t i = 0; i < conversion.browser_vars.size(); ++i) {
+    conversion.browser_vars[i]->set_pending_browser_host_id(hosts.host_ids[i]);
+  }
+  conversion.callback.Run(true);
+  pending_conversions_.pop();
+}
+
 }  // namespace content