Finish basic implementation of Pepper Transport API and...

Finish basic implementation of Pepper Transport API and
wire it up to libjingle's P2P transport.
TEST=ppapi_tests
BUG=none

[juberti, 2010-12-21 18:38:31]

Wiring up the guts of the Pepper Transport

[brettw, 2010-12-21 21:06:06]

Now that our test framework is getting better, I want to be sure to be better at
getting tests with the code. Have you thought about how unit tests for this type
of thing could work or what level of testing would be practical?

[juberti, 2010-12-21 21:15:25]

Agree. I added an initial test in ppapi_tests. Is this the right place? I
saw your email recently about adding tests to a test shell, but I'm not sure
how that relates to the ppapi_tests.

On Tue, Dec 21, 2010 at 1:06 PM, <brettw@chromium.org> wrote:

> Now that our test framework is getting better, I want to be sure to be
> better at
> getting tests with the code. Have you thought about how unit tests for this
> type
> of thing could work or what level of testing would be practical?
>
>
> http://codereview.chromium.org/6042003/
>

[brettw, 2010-12-21 21:55:50]

Those tests were fast. They look reasonable. The unit tests I mentioned are for
when you want to test some internal implementation stuff. You get access to the
implementation object directly, but then don't have a "real" web page or
context. I think most of your tests can use PPAPI, so the way you did it is
best.

Unfortunately, a some stuff has changed since you checked out (sorry, but it's
improved) this so you will have to move to the new location (should be easy
since the contents of the files haven't changed), and also use the new callback
system.

The substance of the patch looks good, I just have some superficial comments.

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

http://codereview.chromium.org/6042003/diff/17001/ppapi/tests/test_transport....
ppapi/tests/test_transport.cc:85: /*rv = t1.Send(kTestData, sizeof(kTestData),
IsWritable()) {
This should be either uncommented or have a comment about why it doesn't work
right now.

http://codereview.chromium.org/6042003/diff/17001/webkit/glue/plugins/pepper_...
File webkit/glue/plugins/pepper_transport.cc (right):

http://codereview.chromium.org/6042003/diff/17001/webkit/glue/plugins/pepper_...
webkit/glue/plugins/pepper_transport.cc:105: // for now, always create
http://www.google.com/transport/p2p
Be sure comments are complete sentences with capitals and periods.

http://codereview.chromium.org/6042003/diff/17001/webkit/glue/plugins/pepper_...
File webkit/glue/plugins/pepper_transport.h (right):

http://codereview.chromium.org/6042003/diff/17001/webkit/glue/plugins/pepper_...
webkit/glue/plugins/pepper_transport.h:27: namespace pepper {
Sorry, you'll have to rebase this patch on the latest Chrome tree. The new
location is webkit/plugins/ppapi/ppb_transport_impl.*

http://codereview.chromium.org/6042003/diff/17001/webkit/glue/plugins/pepper_...
webkit/glue/plugins/pepper_transport.h:53: explicit
CompletionContext(PP_CompletionCallback cb) {
Can you unify this with the stuff in webkit/plugins/ppapi/callbacks.h? Trung
just landed this callback tracking system a day or two ago (there are lots of
comments there, and most of the other code should be changed over to using it).
It handles properly ensuring that the callbacks are fired when the resource is
deleted or the plugin is destroyed (which are bugs your current code, as well
and most of our old code, had).

http://codereview.chromium.org/6042003/diff/17001/webkit/glue/plugins/pepper_...
webkit/glue/plugins/pepper_transport.h:76: void OnRequestSignaling();
Are these functions implementation of some interface? Usually we would comment
that and use "virtual":
  // Foo implementation.
  virtual void OnCandidateReady(...

http://codereview.chromium.org/6042003/diff/17001/webkit/glue/plugins/pepper_...
webkit/glue/plugins/pepper_transport.h:77: void
OnCandidateReady(cricket::TransportChannelImpl*,
Normally we would name all arguments here.

http://codereview.chromium.org/6042003/diff/17001/webkit/glue/plugins/pepper_...
webkit/glue/plugins/pepper_transport.h:88: std::list<cricket::Candidate>
local_candidates_;
Can you provide documentation for this member?

The other ones are a little more self-explanatory, but can you make sure
anything the programmer should know (especially lifetime) is documented here?

[nfullagar, 2011-01-04 22:07:16]

small comment nit, same concerns as brett regarding completion callback tracking

http://codereview.chromium.org/6042003/diff/17001/webkit/glue/plugins/pepper_...
File webkit/glue/plugins/pepper_transport.cc (right):

http://codereview.chromium.org/6042003/diff/17001/webkit/glue/plugins/pepper_...
webkit/glue/plugins/pepper_transport.cc:226: // TODO(juberti): come up with a a
real wire format!
comment above "...with a real..."