Implemented PPB_Broker_Proxy.

Implemented PPB_Broker_Proxy.


BUG=none
TEST=none

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

[brettw, 2011-04-13 17:53:40]

http://codereview.chromium.org/6833002/diff/1001/ppapi/proxy/ppb_broker_proxy.cc
File ppapi/proxy/ppb_broker_proxy.cc (right):

http://codereview.chromium.org/6833002/diff/1001/ppapi/proxy/ppb_broker_proxy...
ppapi/proxy/ppb_broker_proxy.cc:53: // it may make sense to cache the handle
here.
I think we should have the Broker object manage the handle lifetime in the
plugin process.

We'd like to avoid sync messages for this stuff as well. I'd copy what's done
for ppb_file_system_proxy where the renderer sends an async message back with
the handle, and then the proxy issues the callback.

Unfortunately, this is more complicated. I've been meaning to do some work to
make doing this cleaner since we've had to use this pattern in a bunch of
places, but I haven't had the time. Hopefully it's at least easy for you to copy
(you wouldn't send the serialized callback, but instead would have a callback
factory in the host to grab the completion callback and turn that into an async
message back to the plugin containing the result code and the duplicated handle.

You can see some other examples of this by grepping for CallbackFactory in the
proxy directory.

http://codereview.chromium.org/6833002/diff/1001/ppapi/proxy/ppb_broker_proxy...
ppapi/proxy/ppb_broker_proxy.cc:152: false,
This should be true for is_trusted.

[ddorwin, 2011-04-14 18:08:38]

Addressed feedback and added comments about handle ownership and broker
lifetime. There are some new files in this patch.

http://codereview.chromium.org/6833002/diff/1001/ppapi/proxy/ppb_broker_proxy.cc
File ppapi/proxy/ppb_broker_proxy.cc (right):

http://codereview.chromium.org/6833002/diff/1001/ppapi/proxy/ppb_broker_proxy...
ppapi/proxy/ppb_broker_proxy.cc:53: // it may make sense to cache the handle
here.
On 2011/04/13 17:53:40, brettw wrote:
> I think we should have the Broker object manage the handle lifetime in the
> plugin process.
> 
> We'd like to avoid sync messages for this stuff as well. I'd copy what's done
> for ppb_file_system_proxy where the renderer sends an async message back with
> the handle, and then the proxy issues the callback.
> 
> Unfortunately, this is more complicated. I've been meaning to do some work to
> make doing this cleaner since we've had to use this pattern in a bunch of
> places, but I haven't had the time. Hopefully it's at least easy for you to
copy
> (you wouldn't send the serialized callback, but instead would have a callback
> factory in the host to grab the completion callback and turn that into an
async
> message back to the plugin containing the result code and the duplicated
handle.
> 
> You can see some other examples of this by grepping for CallbackFactory in the
> proxy directory.

Done.

I also updated several comments about handle ownership.

http://codereview.chromium.org/6833002/diff/1001/ppapi/proxy/ppb_broker_proxy...
ppapi/proxy/ppb_broker_proxy.cc:152: false,
On 2011/04/13 17:53:40, brettw wrote:
> This should be true for is_trusted.

Done.

[brettw, 2011-04-15 16:09:25]

http://codereview.chromium.org/6833002/diff/12001/ppapi/c/trusted/ppb_broker_...
File ppapi/c/trusted/ppb_broker_trusted.h (right):

http://codereview.chromium.org/6833002/diff/12001/ppapi/c/trusted/ppb_broker_...
ppapi/c/trusted/ppb_broker_trusted.h:13: // DO NOT USE THIS INTERFACE
I'd probably just delete this. It's a trusted one so only internal users can use
it anyway.

http://codereview.chromium.org/6833002/diff/12001/ppapi/proxy/ppb_broker_prox...
File ppapi/proxy/ppb_broker_proxy.cc (right):

http://codereview.chromium.org/6833002/diff/12001/ppapi/proxy/ppb_broker_prox...
ppapi/proxy/ppb_broker_proxy.cc:39: socket_handle_ =
base::kInvalidPlatformFileValue;
You set this above. I don't see where this handle is closed. You say here the
plugin owns it, but this code is in the plugin and there's no close call.

Maybe you want this to be a scoped_refptr<base::SyncSocket> so that the object
will deal with the lifetime for you?

http://codereview.chromium.org/6833002/diff/12001/ppapi/proxy/ppb_broker_prox...
ppapi/proxy/ppb_broker_proxy.cc:101: return success ? 
PP_ERROR_WOULDBLOCK/*after merge: PP_OK_COMPLETIONPENDING*/ : PP_ERROR_FAILED;
Did you mean to update this?

http://codereview.chromium.org/6833002/diff/12001/ppapi/proxy/ppb_broker_prox...
ppapi/proxy/ppb_broker_proxy.cc:245: true);
I'm unclear on the ownership of this handle also. You set it to close source,
which will close the handle given to you from the broker_target() interface. It
doesn't look like this interface expects you to take ownership of its handle.

[ddorwin, 2011-04-15 18:41:31]

http://codereview.chromium.org/6833002/diff/12001/ppapi/c/trusted/ppb_broker_...
File ppapi/c/trusted/ppb_broker_trusted.h (right):

http://codereview.chromium.org/6833002/diff/12001/ppapi/c/trusted/ppb_broker_...
ppapi/c/trusted/ppb_broker_trusted.h:13: // DO NOT USE THIS INTERFACE
On 2011/04/15 16:09:25, brettw wrote:
> I'd probably just delete this. It's a trusted one so only internal users can
use
> it anyway.

Done.

http://codereview.chromium.org/6833002/diff/12001/ppapi/proxy/ppb_broker_prox...
File ppapi/proxy/ppb_broker_proxy.cc (right):

http://codereview.chromium.org/6833002/diff/12001/ppapi/proxy/ppb_broker_prox...
ppapi/proxy/ppb_broker_proxy.cc:39: socket_handle_ =
base::kInvalidPlatformFileValue;
On 2011/04/15 16:09:25, brettw wrote:
> You set this above. I don't see where this handle is closed. You say here the
> plugin owns it, but this code is in the plugin and there's no close call.
> 
> Maybe you want this to be a scoped_refptr<base::SyncSocket> so that the object
> will deal with the lifetime for you?

This was supposed to be in the destructor. Moved.

I meant the plugin-specific code (.so/.dll) owns it. The broker and plugin, not
the infrastructure in ppapi/, own the handles. This allows them to close them
early if appropriate. I considered using a SyncSocket in this class but removed
it. If we managed the lifetime in the plugin, we should probably also manage it
in the broker, which would complicate ppp_broker.h.

http://codereview.chromium.org/6833002/diff/12001/ppapi/proxy/ppb_broker_prox...
ppapi/proxy/ppb_broker_proxy.cc:101: return success ? 
PP_ERROR_WOULDBLOCK/*after merge: PP_OK_COMPLETIONPENDING*/ : PP_ERROR_FAILED;
On 2011/04/15 16:09:25, brettw wrote:
> Did you mean to update this?

Yes. Once I get an LGTM, I'll sync and change this. Thanks for reminding me.

http://codereview.chromium.org/6833002/diff/12001/ppapi/proxy/ppb_broker_prox...
ppapi/proxy/ppb_broker_proxy.cc:245: true);
On 2011/04/15 16:09:25, brettw wrote:
> I'm unclear on the ownership of this handle also. You set it to close source,
> which will close the handle given to you from the broker_target() interface.
It
> doesn't look like this interface expects you to take ownership of its handle.

As I mentioned above, PPB_Broker_Impl never owns the handle and expects the
plugin to own it. The proxy is the "plugin" to PPB_Broker_Impl, so I think the
proxy owns it and should close it when it duplicates it for the plugin side of
the proxy.

The intent to close is documented in a function comment above. Also a function
comment for PPB_Broker_Impl::BrokerConnected, which is what calls this callback,
says "Transfers ownership of the handle to the plugin."

[brettw, 2011-04-15 19:28:05]

LGTM

http://codereview.chromium.org/6833002/diff/15001/ppapi/proxy/ppb_broker_prox...
File ppapi/proxy/ppb_broker_proxy.cc (right):

http://codereview.chromium.org/6833002/diff/15001/ppapi/proxy/ppb_broker_prox...
ppapi/proxy/ppb_broker_proxy.cc:26: base::SyncSocket::Handle socket_handle_;
How about moving the comment here about the plugin owning the handle. Can you
also mention that the actual class that owns this in the plugin is the
PPB_Broker_Proxy object.

[ddorwin, 2011-04-15 21:51:11]

http://codereview.chromium.org/6833002/diff/15001/ppapi/proxy/ppb_broker_prox...
File ppapi/proxy/ppb_broker_proxy.cc (right):

http://codereview.chromium.org/6833002/diff/15001/ppapi/proxy/ppb_broker_prox...
ppapi/proxy/ppb_broker_proxy.cc:26: base::SyncSocket::Handle socket_handle_;
On 2011/04/15 19:28:05, brettw wrote:
> How about moving the comment here about the plugin owning the handle. Can you
> also mention that the actual class that owns this in the plugin is the
> PPB_Broker_Proxy object.

Done. The PPB_Broker_Proxy object doesn't own it, but I tried to clarify.