PPAPI: Refactor renderer side of browser host creation

content/renderer/pepper/resource_converter.cc

Index: content/renderer/pepper/resource_converter.cc
diff --git a/content/renderer/pepper/resource_converter.cc b/content/renderer/pepper/resource_converter.cc
index 2af3bba3e7799d5864281bc05397c88e052477e3..0aba1423dafe85d89c946a69ce8db0ebd5c6fbb8 100644
--- a/content/renderer/pepper/resource_converter.cc
+++ b/content/renderer/pepper/resource_converter.cc
@@ -5,6 +5,7 @@
 #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"
@@ -29,17 +30,6 @@ 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) {
@@ -198,7 +188,8 @@ bool DOMMediaStreamTrackToResource(
 ResourceConverter::~ResourceConverter() {}
 
 ResourceConverterImpl::ResourceConverterImpl(PP_Instance instance)
-    : instance_(instance) {}
+    : instance_(instance),
+      weak_ptr_factory_(this) {}
 
 ResourceConverterImpl::~ResourceConverterImpl() {
   // Verify Flush() was called.
@@ -276,12 +267,18 @@ bool ResourceConverterImpl::NeedsFlush() {
 }
 
 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,
@@ -332,6 +329,9 @@ bool ResourceConverterImpl::ToV8Value(const PP_Var& var,
   }
 }
 
+ResourceConverterImpl::PendingConversion::PendingConversion() {}
+ResourceConverterImpl::PendingConversion::~PendingConversion() {}
+
 scoped_refptr<HostResourceVar> ResourceConverterImpl::CreateResourceVar(
     int pending_renderer_id,
     const IPC::Message& create_message) {
@@ -350,4 +350,24 @@ ResourceConverterImpl::CreateResourceVarWithBrowserHost(
   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