Basic implementation of Pepper Transport API.

Basic implementation of Pepper Transport API.

BUG=None
TEST=Unittests

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

[Sergey Ulanov, 2011-02-10 02:34:57]

This is the same changes as in http://codereview.chromium.org/6042003/
integrated with the latest Pepper code.

[brettw, 2011-02-10 07:19:16]

http://codereview.chromium.org/6478018/diff/1/ppapi/cpp/dev/transport_dev.cc
File ppapi/cpp/dev/transport_dev.cc (right):

http://codereview.chromium.org/6478018/diff/1/ppapi/cpp/dev/transport_dev.cc#...
ppapi/cpp/dev/transport_dev.cc:49: return
get_interface<PPB_Transport_Dev>()->GetNextAddress(pp_resource(), address,
80 cols.

http://codereview.chromium.org/6478018/diff/1/ppapi/cpp/dev/transport_dev.cc#...
ppapi/cpp/dev/transport_dev.cc:65: cc.pp_completion_callback());
Don't indent this stuff randomly. Do 4 spaces if it doesn't fit. I usually try
to align the args if this happens (moving the previous ones down so some aren't
indented to the end of Recv( while some are indented 4 spaces).

http://codereview.chromium.org/6478018/diff/1/ppapi/tests/test_transport.cc
File ppapi/tests/test_transport.cc (right):

http://codereview.chromium.org/6478018/diff/1/ppapi/tests/test_transport.cc#n...
ppapi/tests/test_transport.cc:34: pp::Transport_Dev t1(instance_, "test", ""),
t2(instance_, "test", "");
I think Google style discourages multiple declarations on the same line like
this. I'd use a separate line and "pp::Transport_Dev" type.

http://codereview.chromium.org/6478018/diff/1/ppapi/tests/test_transport.cc#n...
ppapi/tests/test_transport.cc:40: if (rv != PP_ERROR_WOULDBLOCK) {
test_case.h defines ASSERT_TRUE which makes this pattern more readable and makes
nicer output:
ASSERT_TRUE(rv == PP_ERROR_WOULDBLOCK).
I realize maybe you did it this way because you wanted the names in the errors
(the ASSERT_TRUE just includes the line #). Either way is fine, I just wanted to
make sure you knew about the macro.

http://codereview.chromium.org/6478018/diff/1/webkit/plugins/ppapi/ppb_transp...
File webkit/plugins/ppapi/ppb_transport_impl.cc (right):

http://codereview.chromium.org/6478018/diff/1/webkit/plugins/ppapi/ppb_transp...
webkit/plugins/ppapi/ppb_transport_impl.cc:7: #include
"ppapi/c/pp_completion_callback.h"
Be sure the file list it alphabetized.

http://codereview.chromium.org/6478018/diff/1/webkit/plugins/ppapi/ppb_transp...
webkit/plugins/ppapi/ppb_transport_impl.cc:51: int32_t
GetNextAddress(PP_Resource resource, PP_Var* address,
I was a little unclear about how this is supposed to work from just looking at
the interface declaration. It seems the address is an out param and that it must
remain valid I take it address is an out param and that the caller must ensure
it remains valid?

I think using this from the caller correctly will be difficult. Often you would
pass it the address of some class member and use the callback factory to make
sure your object doesn't get callbacks after it's destroyed.

I wonder if we can change the interface to make the completion callback just
indicates when you can re-call this function to get a synchronous result? I
think this would be much easier to code to.

http://codereview.chromium.org/6478018/diff/1/webkit/plugins/ppapi/ppb_transp...
webkit/plugins/ppapi/ppb_transport_impl.cc:191: recv_buffer_ = data;
This has the same problem as GetNextAddress with respect to lifetimes. In this
case, it's really being like the URLLoader and we probably don't want to make a
copy. For the URLLoader, we added an explicit Close() function that you can call
to cancel all the pending callbacks. This way, when your object goes out of
scope, you can explicitly ensure that your buffer isn't used regardless of who
else may hold a ref to the Transport object.

[Sergey Ulanov, 2011-02-10 21:14:54]

addressed all comments

http://codereview.chromium.org/6478018/diff/1/ppapi/cpp/dev/transport_dev.cc
File ppapi/cpp/dev/transport_dev.cc (right):

http://codereview.chromium.org/6478018/diff/1/ppapi/cpp/dev/transport_dev.cc#...
ppapi/cpp/dev/transport_dev.cc:49: return
get_interface<PPB_Transport_Dev>()->GetNextAddress(pp_resource(), address,
On 2011/02/10 07:19:16, brettw wrote:
> 80 cols.

Done.

http://codereview.chromium.org/6478018/diff/1/ppapi/cpp/dev/transport_dev.cc#...
ppapi/cpp/dev/transport_dev.cc:65: cc.pp_completion_callback());
On 2011/02/10 07:19:16, brettw wrote:
> Don't indent this stuff randomly. Do 4 spaces if it doesn't fit. I usually try
> to align the args if this happens (moving the previous ones down so some
aren't
> indented to the end of Recv( while some are indented 4 spaces).
Yeah, I just forgot to indent this line after changing the previous one.

http://codereview.chromium.org/6478018/diff/1/ppapi/tests/test_transport.cc
File ppapi/tests/test_transport.cc (right):

http://codereview.chromium.org/6478018/diff/1/ppapi/tests/test_transport.cc#n...
ppapi/tests/test_transport.cc:34: pp::Transport_Dev t1(instance_, "test", ""),
t2(instance_, "test", "");
On 2011/02/10 07:19:16, brettw wrote:
> I think Google style discourages multiple declarations on the same line like
> this. I'd use a separate line and "pp::Transport_Dev" type.

Done. Also renamed all variables.

http://codereview.chromium.org/6478018/diff/1/ppapi/tests/test_transport.cc#n...
ppapi/tests/test_transport.cc:40: if (rv != PP_ERROR_WOULDBLOCK) {
On 2011/02/10 07:19:16, brettw wrote:
> test_case.h defines ASSERT_TRUE which makes this pattern more readable and
makes
> nicer output:
> ASSERT_TRUE(rv == PP_ERROR_WOULDBLOCK).
> I realize maybe you did it this way because you wanted the names in the errors
> (the ASSERT_TRUE just includes the line #). Either way is fine, I just wanted
to
> make sure you knew about the macro.
Actullay ASSERT_EQ is more appropriate here.

http://codereview.chromium.org/6478018/diff/1/webkit/plugins/ppapi/ppb_transp...
File webkit/plugins/ppapi/ppb_transport_impl.cc (right):

http://codereview.chromium.org/6478018/diff/1/webkit/plugins/ppapi/ppb_transp...
webkit/plugins/ppapi/ppb_transport_impl.cc:7: #include
"ppapi/c/pp_completion_callback.h"
On 2011/02/10 07:19:16, brettw wrote:
> Be sure the file list it alphabetized.

Done.

http://codereview.chromium.org/6478018/diff/1/webkit/plugins/ppapi/ppb_transp...
webkit/plugins/ppapi/ppb_transport_impl.cc:51: int32_t
GetNextAddress(PP_Resource resource, PP_Var* address,
On 2011/02/10 07:19:16, brettw wrote:
> I was a little unclear about how this is supposed to work from just looking at
> the interface declaration. It seems the address is an out param and that it
must
> remain valid I take it address is an out param and that the caller must ensure
> it remains valid?
> 
> I think using this from the caller correctly will be difficult. Often you
would
> pass it the address of some class member and use the callback factory to make
> sure your object doesn't get callbacks after it's destroyed.
> 
> I wonder if we can change the interface to make the completion callback just
> indicates when you can re-call this function to get a synchronous result? I
> think this would be much easier to code to.

FileIO API uses exactly the same approach (see PPB_FileIO_Impl::ReadCallback()
and PPB_FileIO_Impl::Read()). Sockets interface in chrome does it too (see
src/net/socket/socket.h). I don't think it makes it much more difficult to use
the interface. In the same time, this approach helps to reduce number of memcpy
for read/write operations.

http://codereview.chromium.org/6478018/diff/1/webkit/plugins/ppapi/ppb_transp...
webkit/plugins/ppapi/ppb_transport_impl.cc:191: recv_buffer_ = data;
On 2011/02/10 07:19:16, brettw wrote:
> This has the same problem as GetNextAddress with respect to lifetimes. In this
> case, it's really being like the URLLoader and we probably don't want to make
a
> copy. For the URLLoader, we added an explicit Close() function that you can
call
> to cancel all the pending callbacks. This way, when your object goes out of
> scope, you can explicitly ensure that your buffer isn't used regardless of who
> else may hold a ref to the Transport object.

We do have Close() in this interface. I've added line to abort all pending
callbacks when transport is closed.

[Sergey Ulanov, 2011-02-11 21:00:17]

ping

[brettw, 2011-02-14 17:13:58]

http://codereview.chromium.org/6478018/diff/1/webkit/plugins/ppapi/ppb_transp...
File webkit/plugins/ppapi/ppb_transport_impl.cc (right):

http://codereview.chromium.org/6478018/diff/1/webkit/plugins/ppapi/ppb_transp...
webkit/plugins/ppapi/ppb_transport_impl.cc:51: int32_t
GetNextAddress(PP_Resource resource, PP_Var* address,
Yes, I think this makes sense for reading since we want to avoid having to copy
the buffer. The same is true for your case which is why I suggested you needed a
Close function rather than changing that case as well (sorry, guess I missed
yours).

In this case, there's no issue with copying, so I'd prefer the version with
slightly easier memory management.

A more compelling reason for changing is that the current API is almost
impossible to write a C++ wrapper around. We'll need to convert a PP_Var* to a
pp::Var*. This may involve cleaning up the ownership of the "old" value of the
existing pp::Var, and this requires us to do some checking at the time of
setting the value. When the value is set at a random time, it's not possible for
us to do this without writing some kind of complicated proxy for the callback
function.

[Sergey Ulanov, 2011-02-14 22:22:55]

http://codereview.chromium.org/6478018/diff/1/webkit/plugins/ppapi/ppb_transp...
File webkit/plugins/ppapi/ppb_transport_impl.cc (right):

http://codereview.chromium.org/6478018/diff/1/webkit/plugins/ppapi/ppb_transp...
webkit/plugins/ppapi/ppb_transport_impl.cc:51: int32_t
GetNextAddress(PP_Resource resource, PP_Var* address,
On 2011/02/14 17:13:58, brettw wrote:
> Yes, I think this makes sense for reading since we want to avoid having to
copy
> the buffer. The same is true for your case which is why I suggested you needed
a
> Close function rather than changing that case as well (sorry, guess I missed
> yours).
> 
> In this case, there's no issue with copying, so I'd prefer the version with
> slightly easier memory management.
> 
> A more compelling reason for changing is that the current API is almost
> impossible to write a C++ wrapper around. We'll need to convert a PP_Var* to a
> pp::Var*. This may involve cleaning up the ownership of the "old" value of the
> existing pp::Var, and this requires us to do some checking at the time of
> setting the value. When the value is set at a random time, it's not possible
for
> us to do this without writing some kind of complicated proxy for the callback
> function.

Yes, I seen now. Thanks for explaining it. I've also update comments in
ppb_transport_dev.h to reflect this behavior.

[brettw, 2011-02-14 23:16:39]

http://codereview.chromium.org/6478018/diff/20001/ppapi/cpp/dev/transport_dev.cc
File ppapi/cpp/dev/transport_dev.cc (right):

http://codereview.chromium.org/6478018/diff/20001/ppapi/cpp/dev/transport_dev...
ppapi/cpp/dev/transport_dev.cc:45: int32_t Transport_Dev::GetNextAddress(PP_Var*
address,
This should be a pp::Var*

To implement this, I'd do this:

PP_Var temp_address = PP_MakeUndefined();
int32_t ret_val = GetNextAddress(&temp_adderss);
*out = Var(Var::PassRef, temp_address);
return ret_val;

http://codereview.chromium.org/6478018/diff/20001/webkit/plugins/ppapi/ppb_tr...
File webkit/plugins/ppapi/ppb_transport_impl.cc (right):

http://codereview.chromium.org/6478018/diff/20001/webkit/plugins/ppapi/ppb_tr...
webkit/plugins/ppapi/ppb_transport_impl.cc:253: // TODO(juberti): Come up with a
real wire format!
We should release any existing value in the var here or it will leak. I think
Var::PluginReleasePPVar is what we want (it shouldn't be necessary to do
checking on the type before doing this call, it will be a nop if it's not a
string or an object).

[Sergey Ulanov, 2011-02-15 00:26:25]

http://codereview.chromium.org/6478018/diff/20001/ppapi/cpp/dev/transport_dev.cc
File ppapi/cpp/dev/transport_dev.cc (right):

http://codereview.chromium.org/6478018/diff/20001/ppapi/cpp/dev/transport_dev...
ppapi/cpp/dev/transport_dev.cc:45: int32_t Transport_Dev::GetNextAddress(PP_Var*
address,
On 2011/02/14 23:16:39, brettw wrote:
> This should be a pp::Var*
> 
> To implement this, I'd do this:
> 
> PP_Var temp_address = PP_MakeUndefined();
> int32_t ret_val = GetNextAddress(&temp_adderss);
> *out = Var(Var::PassRef, temp_address);
> return ret_val;

Done.

http://codereview.chromium.org/6478018/diff/20001/webkit/plugins/ppapi/ppb_tr...
File webkit/plugins/ppapi/ppb_transport_impl.cc (right):

http://codereview.chromium.org/6478018/diff/20001/webkit/plugins/ppapi/ppb_tr...
webkit/plugins/ppapi/ppb_transport_impl.cc:253: // TODO(juberti): Come up with a
real wire format!
On 2011/02/14 23:16:39, brettw wrote:
> We should release any existing value in the var here or it will leak. I think
> Var::PluginReleasePPVar is what we want (it shouldn't be necessary to do
> checking on the type before doing this call, it will be a nop if it's not a
> string or an object).

Done.

[brettw, 2011-02-15 00:46:27]

LGTM