Make the Graphics2D flush call an asynchronous operation. This tracks the

callback in the plugin and adds a new message when a flush is complete in
the renderer.

I adapted the callback factory so it could be used for tracking the flush
callbacks in the proxy in the renderer, and also changed the URL loader
stuff (which had some custom management) around so that it also uses the
factory.

This also fixes a bug in font proxying where we would assert if there was a
font creation error because the dispatcher wasn't set on the SerializedVar in
the font decription.

TEST=none
BUG=none
Review URL: http://codereview.chromium.org/4876003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@65995 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 197 insertions, 63 deletions

diff --git a/ppapi/cpp/completion_callback.h b/ppapi/cpp/completion_callback.h
index 394271a..3cec39a 100644
--- a/ppapi/cpp/completion_callback.h
+++ b/ppapi/cpp/completion_callback.h
@@ -101,7 +101,8 @@ class CompletionCallback {
 //
 //     void ReadMore() {
 //       CompletionCallback cc = factory_.NewCallback(&MyHandler::DidRead);
-//       int32_t rv = fio_.Read(offset_, buf_, sizeof(buf_), cc);
+//       int32_t rv = fio_.Read(offset_, buf_, sizeof(buf_),
+//                              cc.pp_completion_callback());
 //       if (rv != PP_ERROR_WOULDBLOCK)
 //         cc.Run(rv);
 //     }
diff --git a/ppapi/proxy/DEPS b/ppapi/proxy/DEPS
index 377524e..d5d2f9c 100644
--- a/ppapi/proxy/DEPS
+++ b/ppapi/proxy/DEPS
@@ -4,4 +4,11 @@ include_rules = [
   # headers (which don't depend on anything else).
   "+webkit/glue/plugins/ppb_private.h",
   "+webkit/glue/plugins/ppb_private2.h",
+
+  # We don't want the proxy to depend on the C++ layer, which is appropriate
+  # for plugins only. However, the completion callback factory is a very useful
+  # tool that we would otherwise have to duplicate, and has no other
+  # dependencies, so we allow that.
+  "-ppapi/cpp",
+  "+ppapi/cpp/completion_callback.h"
 ]
diff --git a/ppapi/proxy/ppapi_messages_internal.h b/ppapi/proxy/ppapi_messages_internal.h
index 3e85955..e80586a 100644
--- a/ppapi/proxy/ppapi_messages_internal.h
+++ b/ppapi/proxy/ppapi_messages_internal.h
@@ -88,6 +88,11 @@ IPC_BEGIN_MESSAGES(Ppapi)
                       int64 /* ppp_class */,
                       int64 /* object */)
 
+  // PPB_Graphics2D.
+  IPC_MESSAGE_ROUTED2(PpapiMsg_PPBGraphics2D_FlushACK,
+                      PP_Resource /* graphics_2d */,
+                      int32_t /* pp_error */)
+
   // PPP_Instance.
   IPC_SYNC_MESSAGE_ROUTED3_1(PpapiMsg_PPPInstance_DidCreate,
                              PP_Instance /* instance */,
@@ -294,10 +299,8 @@ IPC_BEGIN_MESSAGES(PpapiHost)
   IPC_MESSAGE_ROUTED2(PpapiHostMsg_PPBGraphics2D_ReplaceContents,
                       PP_Resource /* graphics_2d */,
                       PP_Resource /* image_data */)
-  IPC_SYNC_MESSAGE_ROUTED2_1(PpapiHostMsg_PPBGraphics2D_Flush,
-                             PP_Resource /* graphics_2d */,
-                             uint32_t /* serialized_callback */,
-                             int32_t /* result */)
+  IPC_MESSAGE_ROUTED1(PpapiHostMsg_PPBGraphics2D_Flush,
+                      PP_Resource /* graphics_2d */)
 
   // PPB_ImageData.
   IPC_SYNC_MESSAGE_ROUTED0_1(
diff --git a/ppapi/proxy/ppb_font_proxy.cc b/ppapi/proxy/ppb_font_proxy.cc
index 2313c34..4f10956 100644
--- a/ppapi/proxy/ppb_font_proxy.cc
+++ b/ppapi/proxy/ppb_font_proxy.cc
@@ -246,18 +246,24 @@ void PPB_Font_Proxy::OnMsgCreate(
   in_desc.letter_spacing = in_description.letter_spacing;
   in_desc.word_spacing = in_description.word_spacing;
 
+  PP_FontDescription_Dev out_desc;
+  SerializedVarOutParam out_face_name(&out_description->face);
+
   *result = ppb_font_target()->Create(pp_module, &in_desc);
-  if (!*result)
+  if (!*result) {
+    // Even in the failure case we need to set the var for the name or it
+    // will have no dispatcher set, and the serialization layer will assert
+    // when we're sending it to the browser.
+    *out_face_name.OutParam(dispatcher()) = PP_MakeUndefined();
     return;
+  }
 
   // Get the metrics and resulting description to return to the browser.
-  PP_FontDescription_Dev out_desc;
   PP_FontMetrics_Dev metrics;
   ppb_font_target()->Describe(*result, &out_desc, &metrics);
 
   // Convert the PP_Var in the resulting description to an out param. This
   // should clean up the ref |Describe| assigned to our font name.
-  SerializedVarOutParam out_face_name(&out_description->face);
   *out_face_name.OutParam(dispatcher()) = out_desc.face;
 
   // The rest of the font description is easy.
diff --git a/ppapi/proxy/ppb_graphics_2d_proxy.cc b/ppapi/proxy/ppb_graphics_2d_proxy.cc
index 0567369..74f8b91 100644
--- a/ppapi/proxy/ppb_graphics_2d_proxy.cc
+++ b/ppapi/proxy/ppb_graphics_2d_proxy.cc
@@ -4,8 +4,9 @@
 
 #include "ppapi/proxy/ppb_graphics_2d_proxy.h"
 
-#include <string.h>  // For memset
+#include <string.h>  // For memset.
 
+#include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "ppapi/c/pp_completion_callback.h"
 #include "ppapi/c/pp_errors.h"
@@ -21,7 +22,9 @@ namespace proxy {
 class Graphics2D : public PluginResource {
  public:
   Graphics2D(const PP_Size& size, PP_Bool is_always_opaque)
-      : size_(size), is_always_opaque_(is_always_opaque) {
+      : size_(size),
+        is_always_opaque_(is_always_opaque),
+        current_flush_callback_(PP_BlockUntilComplete()) {
   }
 
   // Resource overrides.
@@ -30,10 +33,23 @@ class Graphics2D : public PluginResource {
   const PP_Size& size() const { return size_; }
   PP_Bool is_always_opaque() const { return is_always_opaque_; }
 
+  bool is_flush_pending() const { return !!current_flush_callback_.func; }
+
+  PP_CompletionCallback current_flush_callback() const {
+    return current_flush_callback_;
+  }
+  void set_current_flush_callback(PP_CompletionCallback cb) {
+    current_flush_callback_ = cb;
+  }
+
  private:
   PP_Size size_;
   PP_Bool is_always_opaque_;
 
+  // In the plugin, this is the current callback set for Flushes. When the
+  // callback function pointer is non-NULL, we're waiting for a flush ACK.
+  PP_CompletionCallback current_flush_callback_;
+
   DISALLOW_COPY_AND_ASSIGN(Graphics2D);
 };
 
@@ -103,13 +119,22 @@ void ReplaceContents(PP_Resource graphics_2d, PP_Resource image_data) {
 
 int32_t Flush(PP_Resource graphics_2d,
               PP_CompletionCallback callback) {
-  PluginDispatcher* dispatcher = PluginDispatcher::Get();
-  int32_t result = PP_ERROR_FAILED;
-  dispatcher->Send(new PpapiHostMsg_PPBGraphics2D_Flush(
-      INTERFACE_ID_PPB_GRAPHICS_2D, graphics_2d,
-      dispatcher->callback_tracker().SendCallback(callback),
-      &result));
-  return result;
+  Graphics2D* object = PluginResource::GetAs<Graphics2D>(graphics_2d);
+  if (!object)
+    return PP_ERROR_BADRESOURCE;
+
+  // For now, disallow blocking calls. We'll need to add support for other
+  // threads to this later.
+  if (!callback.func)
+    return PP_ERROR_BADARGUMENT;
+
+  if (object->is_flush_pending())
+    return PP_ERROR_INPROGRESS;  // Can't have >1 flush pending.
+  object->set_current_flush_callback(callback);
+
+  PluginDispatcher::Get()->Send(new PpapiHostMsg_PPBGraphics2D_Flush(
+      INTERFACE_ID_PPB_GRAPHICS_2D, graphics_2d));
+  return PP_ERROR_WOULDBLOCK;
 }
 
 const PPB_Graphics2D ppb_graphics_2d = {
@@ -126,7 +151,8 @@ const PPB_Graphics2D ppb_graphics_2d = {
 
 PPB_Graphics2D_Proxy::PPB_Graphics2D_Proxy(Dispatcher* dispatcher,
                                            const void* target_interface)
-    : InterfaceProxy(dispatcher, target_interface) {
+    : InterfaceProxy(dispatcher, target_interface),
+      callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
 }
 
 PPB_Graphics2D_Proxy::~PPB_Graphics2D_Proxy() {
@@ -152,6 +178,9 @@ void PPB_Graphics2D_Proxy::OnMessageReceived(const IPC::Message& msg) {
                         OnMsgReplaceContents)
     IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBGraphics2D_Flush,
                         OnMsgFlush)
+
+    IPC_MESSAGE_HANDLER(PpapiMsg_PPBGraphics2D_FlushACK,
+                        OnMsgFlushACK)
   IPC_END_MESSAGE_MAP()
   // FIXME(brettw) handle bad messages!
 }
@@ -188,14 +217,39 @@ void PPB_Graphics2D_Proxy::OnMsgReplaceContents(PP_Resource graphics_2d,
   ppb_graphics_2d_target()->ReplaceContents(graphics_2d, image_data);
 }
 
-void PPB_Graphics2D_Proxy::OnMsgFlush(PP_Resource graphics_2d,
-                                      uint32_t serialized_callback,
-                                      int32_t* result) {
-  // TODO(brettw) this should be a non-sync function. Ideally it would call
-  // the callback with a failure code from here if you weren't allowed to
-  // call Flush there.
-  *result = ppb_graphics_2d_target()->Flush(
-      graphics_2d, ReceiveCallback(serialized_callback));
+void PPB_Graphics2D_Proxy::OnMsgFlush(PP_Resource graphics_2d) {
+  CompletionCallback callback = callback_factory_.NewCallback(
+      &PPB_Graphics2D_Proxy::SendFlushACKToPlugin, graphics_2d);
+  int32_t result = ppb_graphics_2d_target()->Flush(
+      graphics_2d, callback.pp_completion_callback());
+  if (result != PP_ERROR_WOULDBLOCK) {
+    // There was some error, so we won't get a flush callback. We need to now
+    // issue the ACK to the plugin hears about the error. This will also clean
+    // up the data associated with the callback.
+    callback.Run(result);
+  }
+}
+
+void PPB_Graphics2D_Proxy::OnMsgFlushACK(PP_Resource resource,
+                                         int32_t pp_error) {
+  Graphics2D* object = PluginResource::GetAs<Graphics2D>(resource);
+  if (!object) {
+    // The plugin has released the graphics 2D object so don't issue the
+    // callback.
+    return;
+  }
+
+  // Be careful to make the callback NULL again before issuing the callback
+  // since the plugin might want to flush from within the callback.
+  PP_CompletionCallback callback = object->current_flush_callback();
+  object->set_current_flush_callback(PP_BlockUntilComplete());
+  PP_RunCompletionCallback(&callback, pp_error);
+}
+
+void PPB_Graphics2D_Proxy::SendFlushACKToPlugin(int32_t result,
+                                                PP_Resource graphics_2d) {
+  dispatcher()->Send(new PpapiMsg_PPBGraphics2D_FlushACK(
+      INTERFACE_ID_PPB_GRAPHICS_2D, graphics_2d, result));
 }
 
 }  // namespace proxy
diff --git a/ppapi/proxy/ppb_graphics_2d_proxy.h b/ppapi/proxy/ppb_graphics_2d_proxy.h
index db678eb..66226b9 100644
--- a/ppapi/proxy/ppb_graphics_2d_proxy.h
+++ b/ppapi/proxy/ppb_graphics_2d_proxy.h
@@ -12,7 +12,9 @@
 #include "ppapi/c/pp_resource.h"
 #include "ppapi/c/pp_size.h"
 #include "ppapi/c/pp_var.h"
+#include "ppapi/cpp/completion_callback.h"
 #include "ppapi/proxy/interface_proxy.h"
+#include "ppapi/proxy/proxy_non_thread_safe_ref_count.h"
 
 struct PPB_Graphics2D;
 struct PP_Point;
@@ -36,7 +38,7 @@ class PPB_Graphics2D_Proxy : public InterfaceProxy {
   virtual void OnMessageReceived(const IPC::Message& msg);
 
  private:
-  // Message handlers.
+  // Plugin->renderer message handlers.
   void OnMsgCreate(PP_Module module,
                    const PP_Size& size,
                    PP_Bool is_always_opaque,
@@ -52,9 +54,16 @@ class PPB_Graphics2D_Proxy : public InterfaceProxy {
                    const PP_Point& amount);
   void OnMsgReplaceContents(PP_Resource graphics_2d,
                             PP_Resource image_data);
-  void OnMsgFlush(PP_Resource graphics_2d,
-                  uint32_t serialized_callback,
-                  int32_t* result);
+  void OnMsgFlush(PP_Resource graphics_2d);
+
+  // Renderer->plugin message handlers.
+  void OnMsgFlushACK(PP_Resource graphics_2d, int32_t pp_error);
+
+  // Called in the renderer to send the given flush ACK to the plugin.
+  void SendFlushACKToPlugin(int32_t result, PP_Resource graphics_2d);
+
+  CompletionCallbackFactory<PPB_Graphics2D_Proxy,
+                            ProxyNonThreadSafeRefCount> callback_factory_;
 };
 
 }  // namespace proxy
diff --git a/ppapi/proxy/ppb_url_loader_proxy.cc b/ppapi/proxy/ppb_url_loader_proxy.cc
index 230535d7..55409fb 100644
--- a/ppapi/proxy/ppb_url_loader_proxy.cc
+++ b/ppapi/proxy/ppb_url_loader_proxy.cc
@@ -201,40 +201,19 @@ const PPB_URLLoader ppb_urlloader = {
   &Close
 };
 
-// Renderer status updates -----------------------------------------------------
+}  // namespace
+
+// PPB_URLLoader_Proxy ---------------------------------------------------------
 
-// Data associated with callbacks for ReadResponseBody.
-struct ReadCallbackInfo {
-  base::WeakPtr<PPB_URLLoader_Proxy> loader;
+struct PPB_URLLoader_Proxy::ReadCallbackInfo {
   PP_Resource pp_resource;
   std::string read_buffer;
 };
 
-// Callback for renderer calls to ReadResponseBody. This function will forward
-// the result to the plugin and clean up the callback info.
-void ReadCallbackHandler(void* user_data, int32_t result) {
-  scoped_ptr<ReadCallbackInfo> info(static_cast<ReadCallbackInfo*>(user_data));
-  if (!info->loader)
-    return;
-
-  int32_t bytes_read = 0;
-  if (result > 0)
-    bytes_read = result;  // Positive results indicate bytes read.
-  info->read_buffer.resize(bytes_read);
-
-  info->loader->dispatcher()->Send(
-      new PpapiMsg_PPBURLLoader_ReadResponseBody_Ack(
-          INTERFACE_ID_PPB_URL_LOADER, info->pp_resource,
-          result, info->read_buffer));
-}
-
-}  // namespace
-
-// PPB_URLLoader_Proxy ---------------------------------------------------------
-
 PPB_URLLoader_Proxy::PPB_URLLoader_Proxy(Dispatcher* dispatcher,
                                          const void* target_interface)
-    : InterfaceProxy(dispatcher, target_interface) {
+    : InterfaceProxy(dispatcher, target_interface),
+      callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
 }
 
 PPB_URLLoader_Proxy::~PPB_URLLoader_Proxy() {
@@ -326,18 +305,20 @@ void PPB_URLLoader_Proxy::OnMsgReadResponseBody(
   // destroyed. Depending on the cleanup ordering, we may not need the weak
   // pointer here.)
   ReadCallbackInfo* info = new ReadCallbackInfo;
-  info->loader = AsWeakPtr();
   info->pp_resource = loader;
   info->read_buffer.resize(bytes_to_read);
 
+  CompletionCallback callback = callback_factory_.NewCallback(
+      &PPB_URLLoader_Proxy::OnReadCallback, info);
+
   int32_t result = ppb_url_loader_target()->ReadResponseBody(
       loader, const_cast<char*>(info->read_buffer.c_str()), bytes_to_read,
-      PP_MakeCompletionCallback(&ReadCallbackHandler, info));
+      callback.pp_completion_callback());
   if (result != PP_ERROR_WOULDBLOCK) {
     // Send error (or perhaps success for synchronous reads) back to plugin.
     // The callback function is already set up to do this and also delete the
     // callback info.
-    ReadCallbackHandler(info, result);
+    callback.Run(result);
   }
 }
 
@@ -399,5 +380,19 @@ void PPB_URLLoader_Proxy::OnMsgReadResponseBodyAck(PP_Resource pp_resource,
   PP_RunCompletionCallback(&temp_callback, result);
 }
 
+void PPB_URLLoader_Proxy::OnReadCallback(int32_t result,
+                                         ReadCallbackInfo* info) {
+  int32_t bytes_read = 0;
+  if (result > 0)
+    bytes_read = result;  // Positive results indicate bytes read.
+  info->read_buffer.resize(bytes_read);
+
+  dispatcher()->Send(new PpapiMsg_PPBURLLoader_ReadResponseBody_Ack(
+      INTERFACE_ID_PPB_URL_LOADER, info->pp_resource,
+      result, info->read_buffer));
+
+  delete info;
+}
+
 }  // namespace proxy
 }  // namespace pp
diff --git a/ppapi/proxy/ppb_url_loader_proxy.h b/ppapi/proxy/ppb_url_loader_proxy.h
index cd767b0..b5671ef 100644
--- a/ppapi/proxy/ppb_url_loader_proxy.h
+++ b/ppapi/proxy/ppb_url_loader_proxy.h
@@ -5,22 +5,22 @@
 #ifndef PPAPI_PPB_URL_LOADER_PROXY_H_
 #define PPAPI_PPB_URL_LOADER_PROXY_H_
 
-#include "base/weak_ptr.h"
 #include "ppapi/c/pp_completion_callback.h"
 #include "ppapi/c/pp_instance.h"
 #include "ppapi/c/pp_module.h"
 #include "ppapi/c/pp_resource.h"
 #include "ppapi/c/pp_size.h"
 #include "ppapi/c/pp_var.h"
+#include "ppapi/cpp/completion_callback.h"
 #include "ppapi/proxy/interface_proxy.h"
+#include "ppapi/proxy/proxy_non_thread_safe_ref_count.h"
 
 struct PPB_URLLoader;
 
 namespace pp {
 namespace proxy {
 
-class PPB_URLLoader_Proxy : public InterfaceProxy,
-                            public base::SupportsWeakPtr<PPB_URLLoader_Proxy> {
+class PPB_URLLoader_Proxy : public InterfaceProxy {
  public:
   PPB_URLLoader_Proxy(Dispatcher* dispatcher, const void* target_interface);
   virtual ~PPB_URLLoader_Proxy();
@@ -41,6 +41,9 @@ class PPB_URLLoader_Proxy : public InterfaceProxy,
   virtual void OnMessageReceived(const IPC::Message& msg);
 
  private:
+  // Data associated with callbacks for ReadResponseBody.
+  struct ReadCallbackInfo;
+
   // Plugin->renderer message handlers.
   void OnMsgCreate(PP_Instance instance,
                    PP_Resource* result);
@@ -66,6 +69,13 @@ class PPB_URLLoader_Proxy : public InterfaceProxy,
   void OnMsgReadResponseBodyAck(PP_Resource pp_resource,
                                 int32_t result,
                                 const std::string& data);
+
+  // Handles callbacks for read complete messages. Takes ownership of the info
+  // pointer.
+  void OnReadCallback(int32_t result, ReadCallbackInfo* info);
+
+  CompletionCallbackFactory<PPB_URLLoader_Proxy,
+                            ProxyNonThreadSafeRefCount> callback_factory_;
 };
 
 }  // namespace proxy
diff --git a/ppapi/proxy/proxy_non_thread_safe_ref_count.h b/ppapi/proxy/proxy_non_thread_safe_ref_count.h
new file mode 100644
index 0000000..cfcb720
--- /dev/null
+++ b/ppapi/proxy/proxy_non_thread_safe_ref_count.h
@@ -0,0 +1,49 @@
+// Copyright (c) 2010 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.
+
+#ifndef PPAPI_PROXY_PROXY_NON_THREAD_SAFE_REF_COUNT_H_
+#define PPAPI_PROXY_PROXY_NON_THREAD_SAFE_REF_COUNT_H_
+
+#include "base/message_loop.h"
+#include "ppapi/cpp/completion_callback.h"
+
+namespace pp {
+namespace proxy {
+
+// This class is just like ppapi/cpp/non_thread_safe_ref_count.h but rather
+// than using pp::Module::core (which doesn't exist), it uses Chrome threads
+// which do.
+class ProxyNonThreadSafeRefCount {
+ public:
+  ProxyNonThreadSafeRefCount() : ref_(0) {
+#ifndef NDEBUG
+    message_loop_ = MessageLoop::current();
+#endif
+  }
+
+  ~ProxyNonThreadSafeRefCount() {
+    PP_DCHECK(message_loop_ == MessageLoop::current());
+  }
+
+  int32_t AddRef() {
+    PP_DCHECK(message_loop_ == MessageLoop::current());
+    return ++ref_;
+  }
+
+  int32_t Release() {
+    PP_DCHECK(message_loop_ == MessageLoop::current());
+    return --ref_;
+  }
+
+ private:
+  int32_t ref_;
+#ifndef NDEBUG
+  MessageLoop* message_loop_;
+#endif
+};
+
+}  // namespace proxy
+}  // namespace pp
+
+#endif  // PPAPI_PROXY_PROXY_NON_THREAD_SAFE_REF_COUNT_H_