Add SetProperty() in the PPB_Transport_Dev interface.

Add SetProperti() in the PPB_Transport_Dev interface.

BUG=41776
TEST=Unittests

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

[Sergey Ulanov, 2011-08-22 23:25:10]

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

http://codereview.chromium.org/7713021/diff/1/ppapi/tests/test_transport.cc#n...
ppapi/tests/test_transport.cc:252:
pp::Module::Get()->core()->CallOnMainThread(kUdpWaitTimeMs, timeout_cb);
I changed this code and removed timeout in TestSendDataTcp() because otherwise
the callback passed to CallOnMainThread() may be called later when
TestCompletionCallback object is destroyed, which sometimes causes crashes or
often quits renderer main loop.

[brettw, 2011-08-22 23:59:59]

http://codereview.chromium.org/7713021/diff/1/ppapi/c/dev/ppb_transport_dev.h
File ppapi/c/dev/ppb_transport_dev.h (right):

http://codereview.chromium.org/7713021/diff/1/ppapi/c/dev/ppb_transport_dev.h...
ppapi/c/dev/ppb_transport_dev.h:16: #define PPB_TRANSPORT_DEV_INTERFACE_0_5
"PPB_Transport;0.5"
Be sure to rev the interface string.

http://codereview.chromium.org/7713021/diff/1/ppapi/c/dev/ppb_transport_dev.h...
ppapi/c/dev/ppb_transport_dev.h:38: // configuration parameters.
This documentation gives me no idea what I can pass in config. Can this be
significantly expanded or can you give a pointer to where I can figure this out?
(Maybe a simple example would also be nice.)

Actually, I think I'd prefer something more like the URLRequestInfo where you
have an enum of all the supported options, and then set them individually. I
think this makes the calling code more clear, and it also makes it easier to
tell what options we support. The parameter can be a var so you can pass
integers for things like the receive window and not have to do parsing.

[Sergey Ulanov, 2011-08-23 19:02:04]

http://codereview.chromium.org/7713021/diff/1/ppapi/c/dev/ppb_transport_dev.h
File ppapi/c/dev/ppb_transport_dev.h (right):

http://codereview.chromium.org/7713021/diff/1/ppapi/c/dev/ppb_transport_dev.h...
ppapi/c/dev/ppb_transport_dev.h:38: // configuration parameters.
On 2011/08/22 23:59:59, brettw wrote:
> This documentation gives me no idea what I can pass in config. Can this be
> significantly expanded or can you give a pointer to where I can figure this
out?
> (Maybe a simple example would also be nice.)
> 
> Actually, I think I'd prefer something more like the URLRequestInfo where you
> have an enum of all the supported options, and then set them individually. I
> think this makes the calling code more clear, and it also makes it easier to
> tell what options we support. The parameter can be a var so you can pass
> integers for things like the receive window and not have to do parsing.

I like the idea of SetProperty() and enum with all properties. One problem I see
with this approach is that array support is not yet implemented in PP_Var, and
it would be useful here for specifying stun and relay server addresses. For now
I think I can use comma-separated lists for these parameters, and switch to
arrays later when they are implemented. What do you think about it?

How hard would it be to implement array vars?

[brettw, 2011-08-23 19:39:43]

On Tue, Aug 23, 2011 at 12:02 PM,  <sergeyu@chromium.org> wrote:
>
> http://codereview.chromium.org/7713021/diff/1/ppapi/c/dev/ppb_transport_dev.h
> File ppapi/c/dev/ppb_transport_dev.h (right):
>
>
http://codereview.chromium.org/7713021/diff/1/ppapi/c/dev/ppb_transport_dev.h...
> ppapi/c/dev/ppb_transport_dev.h:38: // configuration parameters.
> On 2011/08/22 23:59:59, brettw wrote:
>>
>> This documentation gives me no idea what I can pass in config. Can
>
> this be
>>
>> significantly expanded or can you give a pointer to where I can figure
>
> this out?
>>
>> (Maybe a simple example would also be nice.)
>
>> Actually, I think I'd prefer something more like the URLRequestInfo
>
> where you
>>
>> have an enum of all the supported options, and then set them
>
> individually. I
>>
>> think this makes the calling code more clear, and it also makes it
>
> easier to
>>
>> tell what options we support. The parameter can be a var so you can
>
> pass
>>
>> integers for things like the receive window and not have to do
>
> parsing.
>
> I like the idea of SetProperty() and enum with all properties. One
> problem I see with this approach is that array support is not yet
> implemented in PP_Var, and it would be useful here for specifying stun
> and relay server addresses. For now I think I can use comma-separated
> lists for these parameters, and switch to arrays later when they are
> implemented. What do you think about it?
>
> How hard would it be to implement array vars?

I'm not sure it's super hard, but I'd rather put this off to a time
when we can plan out the whole future of Vars. I think a comma (or
even space) separated list sounds fine when you need to specify a list
of servers.

Brett

[Sergey Ulanov, 2011-08-23 21:37:20]

PTAL, I've replaced SetConfig() with SetProperty().

[Sergey Ulanov, 2011-08-24 17:22:16]

Also simplified properties: now only one server can be specified for stun and
relay server - it's not necessary now, can be added later if we need it.

http://codereview.chromium.org/7713021/diff/1/ppapi/c/dev/ppb_transport_dev.h
File ppapi/c/dev/ppb_transport_dev.h (right):

http://codereview.chromium.org/7713021/diff/1/ppapi/c/dev/ppb_transport_dev.h...
ppapi/c/dev/ppb_transport_dev.h:16: #define PPB_TRANSPORT_DEV_INTERFACE_0_5
"PPB_Transport;0.5"
On 2011/08/22 23:59:59, brettw wrote:
> Be sure to rev the interface string.

Done.

[brettw, 2011-08-25 16:48:43]

http://codereview.chromium.org/7713021/diff/8005/ppapi/c/dev/ppb_transport_dev.h
File ppapi/c/dev/ppb_transport_dev.h (right):

http://codereview.chromium.org/7713021/diff/8005/ppapi/c/dev/ppb_transport_de...
ppapi/c/dev/ppb_transport_dev.h:16: #define PPB_TRANSPORT_DEV_INTERFACE_0_5
"PPB_Transport;0.5"
Since we're not keeping backwards compat, you can remove this old line.

http://codereview.chromium.org/7713021/diff/8005/ppapi/c/dev/ppb_transport_de...
ppapi/c/dev/ppb_transport_dev.h:21: // STUN server address and port, e.g 
"stun.l.google.com:19302".
Why don't you use "...example.com" here and below for the examples.

http://codereview.chromium.org/7713021/diff/8005/ppapi/c/dev/ppb_transport_de...
ppapi/c/dev/ppb_transport_dev.h:22: PP_TRANSPORTPROPERTY_STUN_SERVER,
These should have explicit values, so can you say "= 0", etc for them?

Wasn't at least one of these a list of servers? It seems like the comments
should say the format for that one.

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

http://codereview.chromium.org/7713021/diff/8005/ppapi/tests/test_transport.c...
ppapi/tests/test_transport.cc:177:
pp::Var("stun1.a.google.com:31323,stun2.a.google.com:54324")), PP_OK);
Does example.com also make sense for these tests?

http://codereview.chromium.org/7713021/diff/8005/webkit/glue/p2p_transport.cc
File webkit/glue/p2p_transport.cc (right):

http://codereview.chromium.org/7713021/diff/8005/webkit/glue/p2p_transport.cc...
webkit/glue/p2p_transport.cc:14: P2PTransport::Config::~Config() { }
Style guide says no space in {} (in this case I'd actually put the } on the next
line, personally, since in the .cc file it's no less clear if it's one line
longer.

http://codereview.chromium.org/7713021/diff/8005/webkit/plugins/ppapi/ppb_tra...
File webkit/plugins/ppapi/ppb_transport_impl.cc (right):

http://codereview.chromium.org/7713021/diff/8005/webkit/plugins/ppapi/ppb_tra...
webkit/plugins/ppapi/ppb_transport_impl.cc:106: const char kStringListSeparator
= ',';
It doesn't look like you use this.

http://codereview.chromium.org/7713021/diff/8005/webkit/plugins/ppapi/ppb_tra...
webkit/plugins/ppapi/ppb_transport_impl.cc:167: };
Don't think you need the ;

[Sergey Ulanov, 2011-08-25 17:36:53]

http://codereview.chromium.org/7713021/diff/8005/ppapi/c/dev/ppb_transport_dev.h
File ppapi/c/dev/ppb_transport_dev.h (right):

http://codereview.chromium.org/7713021/diff/8005/ppapi/c/dev/ppb_transport_de...
ppapi/c/dev/ppb_transport_dev.h:16: #define PPB_TRANSPORT_DEV_INTERFACE_0_5
"PPB_Transport;0.5"
On 2011/08/25 16:48:43, brettw wrote:
> Since we're not keeping backwards compat, you can remove this old line.

Done.

http://codereview.chromium.org/7713021/diff/8005/ppapi/c/dev/ppb_transport_de...
ppapi/c/dev/ppb_transport_dev.h:21: // STUN server address and port, e.g 
"stun.l.google.com:19302".
On 2011/08/25 16:48:43, brettw wrote:
> Why don't you use "...example.com" here and below for the examples.

Done.

http://codereview.chromium.org/7713021/diff/8005/ppapi/c/dev/ppb_transport_de...
ppapi/c/dev/ppb_transport_dev.h:22: PP_TRANSPORTPROPERTY_STUN_SERVER,
On 2011/08/25 16:48:43, brettw wrote:
> These should have explicit values, so can you say "= 0", etc for them?
> 
> Wasn't at least one of these a list of servers? It seems like the comments
> should say the format for that one.

Done.

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

http://codereview.chromium.org/7713021/diff/8005/ppapi/tests/test_transport.c...
ppapi/tests/test_transport.cc:177:
pp::Var("stun1.a.google.com:31323,stun2.a.google.com:54324")), PP_OK);
On 2011/08/25 16:48:43, brettw wrote:
> Does http://example.com also make sense for these tests?

Done.

http://codereview.chromium.org/7713021/diff/8005/webkit/glue/p2p_transport.cc
File webkit/glue/p2p_transport.cc (right):

http://codereview.chromium.org/7713021/diff/8005/webkit/glue/p2p_transport.cc...
webkit/glue/p2p_transport.cc:14: P2PTransport::Config::~Config() { }
On 2011/08/25 16:48:43, brettw wrote:
> Style guide says no space in {} (in this case I'd actually put the } on the
next
> line, personally, since in the .cc file it's no less clear if it's one line
> longer.
Done.
Personally I prefer to have space here, and there is a lot of code that does
have space in such cases, e.g.
  ~/c/src/base$ grep \{\ \} * |wc -l
  49
I didn't know that the style guide says that there should be no space. Just out
of curiosity, can you point out at the specific rule that mentions it. Thanks

http://codereview.chromium.org/7713021/diff/8005/webkit/plugins/ppapi/ppb_tra...
File webkit/plugins/ppapi/ppb_transport_impl.cc (right):

http://codereview.chromium.org/7713021/diff/8005/webkit/plugins/ppapi/ppb_tra...
webkit/plugins/ppapi/ppb_transport_impl.cc:106: const char kStringListSeparator
= ',';
On 2011/08/25 16:48:43, brettw wrote:
> It doesn't look like you use this.

Done.

http://codereview.chromium.org/7713021/diff/8005/webkit/plugins/ppapi/ppb_tra...
webkit/plugins/ppapi/ppb_transport_impl.cc:167: };
On 2011/08/25 16:48:43, brettw wrote:
> Don't think you need the ;

Done.

[brettw, 2011-08-26 20:24:06]

http://codereview.chromium.org/7713021/diff/8005/webkit/glue/p2p_transport.cc
File webkit/glue/p2p_transport.cc (right):

http://codereview.chromium.org/7713021/diff/8005/webkit/glue/p2p_transport.cc...
webkit/glue/p2p_transport.cc:14: P2PTransport::Config::~Config() { }
On 2011/08/25 17:36:53, sergeyu wrote:
> On 2011/08/25 16:48:43, brettw wrote:
> > Style guide says no space in {} (in this case I'd actually put the } on the
> next
> > line, personally, since in the .cc file it's no less clear if it's one line
> > longer.
> Done.
> Personally I prefer to have space here, and there is a lot of code that does
> have space in such cases, e.g.
>   ~/c/src/base$ grep \{\ \} * |wc -l
>   49
> I didn't know that the style guide says that there should be no space. Just
out
> of curiosity, can you point out at the specific rule that mentions it. Thanks

http://www.corp.google.com/eng/doc/cppguide.xml?showone=Horizontal_Whitespace...
"no spaces inside empty braces"

[brettw, 2011-08-26 20:28:16]

LGTM with previous style comment fixed.

[Sergey Ulanov, 2011-08-26 20:34:35]

http://codereview.chromium.org/7713021/diff/8005/webkit/glue/p2p_transport.cc
File webkit/glue/p2p_transport.cc (right):

http://codereview.chromium.org/7713021/diff/8005/webkit/glue/p2p_transport.cc...
webkit/glue/p2p_transport.cc:14: P2PTransport::Config::~Config() { }
On 2011/08/26 20:24:06, brettw wrote:
> On 2011/08/25 17:36:53, sergeyu wrote:
> > On 2011/08/25 16:48:43, brettw wrote:
> > > Style guide says no space in {} (in this case I'd actually put the } on
the
> > next
> > > line, personally, since in the .cc file it's no less clear if it's one
line
> > > longer.
> > Done.
> > Personally I prefer to have space here, and there is a lot of code that does
> > have space in such cases, e.g.
> >   ~/c/src/base$ grep \{\ \} * |wc -l
> >   49
> > I didn't know that the style guide says that there should be no space. Just
> out
> > of curiosity, can you point out at the specific rule that mentions it.
Thanks
> 
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Horizontal_Whitespace...
> "no spaces inside empty braces"
Thanks!