Converted PluginResource reply message handling to use base::Callback

Converted PluginResource reply message handling to use base::Callback

Previously each PluginResource had to write a reply handler (|OnReplyReceived|) for any replies to resource messages. This approach had several problems including the fact that the PluginResource had to track the state of any outstanding calls.

This change allows you to register a base::Callback when calling CallToBrowser/CallToRenderer. The callback will be run when a reply message is received with a sequence number matching the call. The parameters of the reply will be passed to the callback. An example of usage:

CallBrowser<PpapiPluginMsg_MyResourceType_MyReplyMessage>(
    PpapiHostMsg_MyResourceType_MyRequestMessage(),
    base::Bind(&MyPluginResource::ReplyHandler, this));

If a reply message to this call is received whose type does not match the expected reply message (for example, in the case of an error), the callback will still be invoked but with the default values of the message parameters.


BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160015

[yzshen1, 2012-10-01 23:41:24]

Only some nits.

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/dispatch_repl...
File ppapi/proxy/dispatch_reply_message.h (right):

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/dispatch_repl...
ppapi/proxy/dispatch_reply_message.h:74: // generate a reply in error cases
(when the call handle returns error rather
handle -> handler?

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/dispatch_repl...
ppapi/proxy/dispatch_reply_message.h:103: // the nested message is empty because
the host handler didn't explicitly
nit: "// the" -> "// The"

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/plugin_resour...
File ppapi/proxy/plugin_resource.h (right):

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/plugin_resour...
ppapi/proxy/plugin_resource.h:69: template<typename MsgClass, typename
CallbackType>
nit: ReplyMsgClass might be more clear. (Here and elsewhere)

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/plugin_resour...
File ppapi/proxy/plugin_resource_callback.h (right):

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/plugin_resour...
ppapi/proxy/plugin_resource_callback.h:24: const IPC::Message& msg) = 0;
indent to align with the first parameter, please.

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/plugin_resour...
ppapi/proxy/plugin_resource_callback.h:33: PluginResourceCallback(const
CallbackType& callback) : callback_(callback) {}
explicit, please.

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/plugin_resour...
ppapi/proxy/plugin_resource_callback.h:37: const IPC::Message& msg) {
Please use OVERRIDE.

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/plugin_resour...
ppapi/proxy/plugin_resource_callback.h:43: friend class
base::RefCounted<PluginResourceCallback>;
Is this necessary?

[raymes, 2012-10-02 16:13:36]

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/dispatch_repl...
File ppapi/proxy/dispatch_reply_message.h (right):

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/dispatch_repl...
ppapi/proxy/dispatch_reply_message.h:74: // generate a reply in error cases
(when the call handle returns error rather
On 2012/10/01 23:41:24, yzshen1 wrote:
> handle -> handler?

Done.

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/dispatch_repl...
ppapi/proxy/dispatch_reply_message.h:103: // the nested message is empty because
the host handler didn't explicitly
On 2012/10/01 23:41:24, yzshen1 wrote:
> nit: "// the" -> "// The"

Done.

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/plugin_resour...
File ppapi/proxy/plugin_resource.h (right):

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/plugin_resour...
ppapi/proxy/plugin_resource.h:69: template<typename MsgClass, typename
CallbackType>
On 2012/10/01 23:41:24, yzshen1 wrote:
> nit: ReplyMsgClass might be more clear. (Here and elsewhere)

Done.

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/plugin_resour...
File ppapi/proxy/plugin_resource_callback.h (right):

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/plugin_resour...
ppapi/proxy/plugin_resource_callback.h:24: const IPC::Message& msg) = 0;
On 2012/10/01 23:41:24, yzshen1 wrote:
> indent to align with the first parameter, please.

Done.

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/plugin_resour...
ppapi/proxy/plugin_resource_callback.h:33: PluginResourceCallback(const
CallbackType& callback) : callback_(callback) {}
On 2012/10/01 23:41:24, yzshen1 wrote:
> explicit, please.

Done.

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/plugin_resour...
ppapi/proxy/plugin_resource_callback.h:37: const IPC::Message& msg) {
On 2012/10/01 23:41:24, yzshen1 wrote:
> Please use OVERRIDE.

Done.

https://codereview.chromium.org/11022005/diff/12002/ppapi/proxy/plugin_resour...
ppapi/proxy/plugin_resource_callback.h:43: friend class
base::RefCounted<PluginResourceCallback>;
On 2012/10/01 23:41:24, yzshen1 wrote:
> Is this necessary?

Done.

[raymes, 2012-10-02 16:14:21]

Adding Brett

[yzshen1, 2012-10-02 16:59:04]

lgtm

[brettw, 2012-10-02 19:40:00]

This is great stuff. LGTM

https://codereview.chromium.org/11022005/diff/14002/ppapi/proxy/plugin_resour...
File ppapi/proxy/plugin_resource_callback.h (right):

https://codereview.chromium.org/11022005/diff/14002/ppapi/proxy/plugin_resour...
ppapi/proxy/plugin_resource_callback.h:34: const CallbackType& callback) :
callback_(callback) {}
The : should go on the next line when not everything fits on the same line.

[raymes, 2012-10-03 21:43:50]

Thanks for the quick review.

http://codereview.chromium.org/11022005/diff/14002/ppapi/proxy/plugin_resourc...
File ppapi/proxy/plugin_resource_callback.h (right):

http://codereview.chromium.org/11022005/diff/14002/ppapi/proxy/plugin_resourc...
ppapi/proxy/plugin_resource_callback.h:34: const CallbackType& callback) :
callback_(callback) {}
On 2012/10/02 19:40:00, brettw wrote:
> The : should go on the next line when not everything fits on the same line.

Done.

[commit-bot: I haz the power, 2012-10-03 21:44:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11022005/22001

[commit-bot: I haz the power, 2012-10-03 21:44:19]

Failed to apply patch for ppapi/proxy/plugin_resource.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ppapi/proxy/plugin_resource.h
  Hunk #3 FAILED at 49.
  Hunk #4 succeeded at 72 (offset 5 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
ppapi/proxy/plugin_resource.h.rej

Patch:       ppapi/proxy/plugin_resource.h
Index: ppapi/proxy/plugin_resource.h
diff --git a/ppapi/proxy/plugin_resource.h b/ppapi/proxy/plugin_resource.h
index
972908a6ddfdc9d99bdcb5f185f211116909f228..ce50fddabc15775cc7ca4dd16fe4733bfbf6c594
100644
--- a/ppapi/proxy/plugin_resource.h
+++ b/ppapi/proxy/plugin_resource.h
@@ -5,9 +5,12 @@
 #ifndef PPAPI_PROXY_PLUGIN_RESOURCE_H_
 #define PPAPI_PROXY_PLUGIN_RESOURCE_H_
 
+#include <map>
+
 #include "base/compiler_specific.h"
 #include "ipc/ipc_sender.h"
 #include "ppapi/proxy/connection.h"
+#include "ppapi/proxy/plugin_resource_callback.h"
 #include "ppapi/proxy/ppapi_proxy_export.h"
 #include "ppapi/shared_impl/resource.h"
 
@@ -31,6 +34,11 @@ class PPAPI_PROXY_EXPORT PluginResource : public Resource {
   bool sent_create_to_browser() const { return sent_create_to_browser_; }
   bool sent_create_to_renderer() const { return sent_create_to_renderer_; }
 
+  // This handles a reply to a resource call. It works by looking up the
+  // callback that was registered when CallBrowser/CallRenderer was called
+  // and calling it with |params| and |msg|.
+  virtual void OnReplyReceived(const proxy::ResourceMessageReplyParams& params,
+                               const IPC::Message& msg) OVERRIDE;
  protected:
   // Sends a create message to the browser or renderer for the current
resource.
   void SendCreateToBrowser(const IPC::Message& msg);
@@ -41,17 +49,41 @@ class PPAPI_PROXY_EXPORT PluginResource : public Resource {
   void PostToBrowser(const IPC::Message& msg);
   void PostToRenderer(const IPC::Message& msg);
 
-  // Like PostToBrowser/Renderer but expects a response.
+  // Like PostToBrowser/Renderer but expects a response. |callback| is
+  // a |base::Callback| that will be run when a reply message with a sequence
+  // number matching that of the call is received. |ReplyMsgClass| is the type
+  // of the reply message that is expected. An example of usage:
+  //
+  // CallBrowser<PpapiPluginMsg_MyResourceType_MyReplyMessage>(
+  //     PpapiHostMsg_MyResourceType_MyRequestMessage(),
+  //     base::Bind(&MyPluginResource::ReplyHandler, this));
+  //
+  // If a reply message to this call is received whose type does not match
+  // |ReplyMsgClass| (for example, in the case of an error), the callback will
+  // still be invoked but with the default values of the message parameters.
   //
   // Returns the new request's sequence number which can be used to identify
-  // the callback. The host will reply and ppapi::Resource::OnReplyReceived
-  // will be called.
+  // the callback.
   //
   // Note that all integers (including 0 and -1) are valid request IDs.
-  int32_t CallBrowser(const IPC::Message& msg);
-  int32_t CallRenderer(const IPC::Message& msg);
+  template<typename ReplyMsgClass, typename CallbackType>
+  int32_t CallBrowser(const IPC::Message& msg, const CallbackType& callback);
+  template<typename ReplyMsgClass, typename CallbackType>
+  int32_t CallRenderer(const IPC::Message& msg, const CallbackType& callback);
 
  private:
+  // Helper function to send a |PpapiHostMsg_ResourceCall| to the given sender
+  // with |nested_msg| and |call_params|.
+  bool SendResourceCall(IPC::Sender* sender,
+                        const ResourceMessageCallParams& call_params,
+                        const IPC::Message& nested_msg);
+
+  // Helper function to make a Resource Call to a host with a callback.
+  template<typename ReplyMsgClass, typename CallbackType>
+  int32_t CallHost(IPC::Sender* sender,
+                   const IPC::Message& msg,
+                   const CallbackType& callback);
+
   Connection connection_;
 
   int32_t next_sequence_number_;
@@ -59,9 +91,43 @@ class PPAPI_PROXY_EXPORT PluginResource : public Resource {
   bool sent_create_to_browser_;
   bool sent_create_to_renderer_;
 
+  typedef std::map<int32_t, scoped_refptr<PluginResourceCallbackBase> >
+      CallbackMap;
+  CallbackMap callbacks_;
+
   DISALLOW_COPY_AND_ASSIGN(PluginResource);
 };
 
+template<typename ReplyMsgClass, typename CallbackType>
+int32_t PluginResource::CallBrowser(const IPC::Message& msg,
+                                    const CallbackType& callback) {
+  return CallHost<ReplyMsgClass, CallbackType>(
+      connection_.browser_sender, msg, callback);
+}
+
+template<typename ReplyMsgClass, typename CallbackType>
+int32_t PluginResource::CallRenderer(const IPC::Message& msg,
+                                    const CallbackType& callback) {
+  return CallHost<ReplyMsgClass, CallbackType>(
+      connection_.renderer_sender, msg, callback);
+}
+
+template<typename ReplyMsgClass, typename CallbackType>
+int32_t PluginResource::CallHost(IPC::Sender* sender,
+                                 const IPC::Message& msg,
+                                 const CallbackType& callback) {
+  ResourceMessageCallParams params(pp_resource(),
+                                   next_sequence_number_++);
+  // Stash the |callback| in |callbacks_| identified by the sequence number of
+  // the call.
+  scoped_refptr<PluginResourceCallbackBase> plugin_callback(
+      new PluginResourceCallback<ReplyMsgClass, CallbackType>(callback));
+  callbacks_.insert(std::make_pair(params.sequence(), plugin_callback));
+  params.set_has_callback();
+  SendResourceCall(sender, params, msg);
+  return params.sequence();
+}
+
 }  // namespace proxy
 }  // namespace ppapi

[commit-bot: I haz the power, 2012-10-03 21:58:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11022005/21005

[commit-bot: I haz the power, 2012-10-04 00:07:45]

Change committed as 160015