Introduce PPB_NetAddress_Dev.

Introduce PPB_NetAddress_Dev.

BUG=247225
TEST=Newly added tests.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205176

[yzshen1, 2013-06-06 06:02:37]

Hi, Bill.

Would you please take a look? Thanks!

[bbudge, 2013-06-06 19:00:59]

Some nits and one big question: I know it's been discussed, but is 'port'
returned in network byte order? If so, this seems like a pain for developers.
Could we just always have that in host order? (The address is different since
it's not really a well defined numeric type, so it makes sense to leave it in
net order.)

https://codereview.chromium.org/16331007/diff/1/ppapi/api/dev/ppb_net_address...
File ppapi/api/dev/ppb_net_address_dev.idl (right):

https://codereview.chromium.org/16331007/diff/1/ppapi/api/dev/ppb_net_address...
ppapi/api/dev/ppb_net_address_dev.idl:31: * The contents are expressed in
network byte order.
I think it would be clearer if you say "port and addr" instead of contents.
Ditto below.

https://codereview.chromium.org/16331007/diff/1/ppapi/api/dev/ppb_net_address...
ppapi/api/dev/ppb_net_address_dev.idl:44: * IP address.
IP -> IPv4 for consistency with the following enum.

https://codereview.chromium.org/16331007/diff/1/ppapi/api/dev/ppb_net_address...
ppapi/api/dev/ppb_net_address_dev.idl:110: */
Return value comment, as below.

https://codereview.chromium.org/16331007/diff/1/ppapi/api/dev/ppb_net_address...
ppapi/api/dev/ppb_net_address_dev.idl:119: * map it to an IPv6 address.
Does the above method map IPv6 to IPv4? If not, perhaps the comment could be
better stated:
Returns PP_FALSE if the network address is not of PP_NETADDRESS_FAMILY_IPV6...  
and the return comment for the previous function could be similar.

https://codereview.chromium.org/16331007/diff/1/ppapi/shared_impl/private/net...
File ppapi/shared_impl/private/net_address_private_impl.cc (right):

https://codereview.chromium.org/16331007/diff/1/ppapi/shared_impl/private/net...
ppapi/shared_impl/private/net_address_private_impl.cc:65: uint16
ConvertToNetEndian16(uint16 x) {
Since the functions are identical, why create this one? If it's for clarity
perhaps a macro?

https://codereview.chromium.org/16331007/diff/1/ppapi/shared_impl/private/net...
File ppapi/shared_impl/private/net_address_private_impl.h (right):

https://codereview.chromium.org/16331007/diff/1/ppapi/shared_impl/private/net...
ppapi/shared_impl/private/net_address_private_impl.h:44: static void
CreateNetAddressPrivateFromIPv4Address(
Could we use method overloading to avoid the "FromIPv*Address" and
"AsIPv*Address" suffixes here too?

https://codereview.chromium.org/16331007/diff/1/ppapi/tests/test_net_address.cc
File ppapi/tests/test_net_address.cc (right):

https://codereview.chromium.org/16331007/diff/1/ppapi/tests/test_net_address....
ppapi/tests/test_net_address.cc:51: return NetAddress_Dev::IsAvailable();
Nice! We should probably start adding these IsAvailable methods to other
wrappers.

https://codereview.chromium.org/16331007/diff/1/ppapi/thunk/resource_creation...
File ppapi/thunk/resource_creation_api.h (right):

https://codereview.chromium.org/16331007/diff/1/ppapi/thunk/resource_creation...
ppapi/thunk/resource_creation_api.h:148: virtual PP_Resource
CreateNetAddressFromIPv4Address(
Could we use method overloading to shorten these names? ie CreateNetAddress,
with two overloads taking the different types. CreateFileRef does this.

[yzshen1, 2013-06-06 20:57:15]

Thanks, Bill!

> Some nits and one big question: I know it's been discussed, but is 'port'
> returned in network byte order? If so, this seems like a pain for developers.
> Could we just always have that in host order?

I personally think that it is fine to use network byte order for 'port'.
Developers must be familiar with sockaddr_in and sockaddr_in6, in which the port
is in network byte order.

I had the same concern as you previously. But after a second thought, I think
network byte order seems better.

What do you think?

https://codereview.chromium.org/16331007/diff/1/ppapi/api/dev/ppb_net_address...
File ppapi/api/dev/ppb_net_address_dev.idl (right):

https://codereview.chromium.org/16331007/diff/1/ppapi/api/dev/ppb_net_address...
ppapi/api/dev/ppb_net_address_dev.idl:31: * The contents are expressed in
network byte order.
I changed it to "All members" so readers won't need to double-check whether
there is any member other than "port and addr" in the structure.

What do you think?

On 2013/06/06 19:01:10, bbudge1 wrote:
> I think it would be clearer if you say "port and addr" instead of contents.
> Ditto below.

https://codereview.chromium.org/16331007/diff/1/ppapi/api/dev/ppb_net_address...
ppapi/api/dev/ppb_net_address_dev.idl:44: * IP address.
On 2013/06/06 19:01:10, bbudge1 wrote:
> IP -> IPv4 for consistency with the following enum.

Done.

https://codereview.chromium.org/16331007/diff/1/ppapi/api/dev/ppb_net_address...
ppapi/api/dev/ppb_net_address_dev.idl:110: */
On 2013/06/06 19:01:10, bbudge1 wrote:
> Return value comment, as below.

Done.

https://codereview.chromium.org/16331007/diff/1/ppapi/api/dev/ppb_net_address...
ppapi/api/dev/ppb_net_address_dev.idl:119: * map it to an IPv6 address.
On 2013/06/06 19:01:10, bbudge1 wrote:
> Does the above method map IPv6 to IPv4? If not, perhaps the comment could be
> better stated:
> Returns PP_FALSE if the network address is not of PP_NETADDRESS_FAMILY_IPV6...
 
> and the return comment for the previous function could be similar.

Done.

https://codereview.chromium.org/16331007/diff/1/ppapi/shared_impl/private/net...
File ppapi/shared_impl/private/net_address_private_impl.cc (right):

https://codereview.chromium.org/16331007/diff/1/ppapi/shared_impl/private/net...
ppapi/shared_impl/private/net_address_private_impl.cc:65: uint16
ConvertToNetEndian16(uint16 x) {
I think it is clearer to have two different names.

By using macro, do you mean the following? (Try to make sure I understand the
suggestion before making change.)

uint16 ConvertNetEndian16(uint16 x) {...}

#define ConvertToNetEndian16(x) ConvertNetEndian16(x)
#define ConvertFromNetEndian16(x) ConvertNetEndian16(x)


On 2013/06/06 19:01:10, bbudge1 wrote:
> Since the functions are identical, why create this one? If it's for clarity
> perhaps a macro?

https://codereview.chromium.org/16331007/diff/1/ppapi/shared_impl/private/net...
File ppapi/shared_impl/private/net_address_private_impl.h (right):

https://codereview.chromium.org/16331007/diff/1/ppapi/shared_impl/private/net...
ppapi/shared_impl/private/net_address_private_impl.h:44: static void
CreateNetAddressPrivateFromIPv4Address(
I think the style guide prefers not to use overloading (although it doesn't
forbid that entirely).

"""
If you want to overload a function, consider qualifying the name with some
information about the arguments, e.g., AppendString(), AppendInt() rather than
just Append().
"""

What do you think?

On 2013/06/06 19:01:10, bbudge1 wrote:
> Could we use method overloading to avoid the "FromIPv*Address" and
> "AsIPv*Address" suffixes here too?

https://codereview.chromium.org/16331007/diff/1/ppapi/thunk/resource_creation...
File ppapi/thunk/resource_creation_api.h (right):

https://codereview.chromium.org/16331007/diff/1/ppapi/thunk/resource_creation...
ppapi/thunk/resource_creation_api.h:148: virtual PP_Resource
CreateNetAddressFromIPv4Address(
I think our style guide prefer this way.

"""
If you want to overload a function, consider qualifying the name with some
information about the arguments, e.g., AppendString(), AppendInt() rather than
just Append().
"""

On 2013/06/06 19:01:10, bbudge1 wrote:
> Could we use method overloading to shorten these names? ie CreateNetAddress,
> with two overloads taking the different types. CreateFileRef does this.

[bbudge, 2013-06-06 22:31:34]

I looked at the .idl CL you posted and saw there has been some discussion about
the the byte order for port. I went through a similar discussion with David
Michael when I first landed the shared NetAddress code. This is just a
suggestion.

If you want to keep 'port' in net order, I would consider changing the type to
an array of 2 bytes. Then you won't need so much 'endian' flipping code.

See this article by Rob Pike:
http://commandcenter.blogspot.com/2012/04/byte-order-fallacy.html

As for overloading, you're right, the style guide recommends against it. it just
seems to be verbose to me and overloading seems safe here. OK.

https://codereview.chromium.org/16331007/diff/14001/ppapi/api/dev/ppb_net_add...
File ppapi/api/dev/ppb_net_address_dev.idl (right):

https://codereview.chromium.org/16331007/diff/14001/ppapi/api/dev/ppb_net_add...
ppapi/api/dev/ppb_net_address_dev.idl:112: * is an IPv4-mapped IPv6 address.
Sorry I wasn't clearer before. I was thinking that since it should return
PP_FALSE for any invalid resource type, the comment should read something like
this:

Fills a <code>PP_NetAddress_IPv4_Dev</code> structure if the network
address is of <code>PP_NETADDRESS_FAMILY_IPV4</code> address family.
Returns PP_FALSE on failure. Note that passing a net address of
<code>PP_NETADDRESS_FAMILY_IPV6</code> address family will fail even
if the address is an IPv4-mapped IPv6 address.

https://codereview.chromium.org/16331007/diff/14001/ppapi/api/dev/ppb_net_add...
ppapi/api/dev/ppb_net_address_dev.idl:122: * map it to an IPv6 address.
Similarly here:

Fills a <code>PP_NetAddress_IPv6_Dev</code> structure if the network
address is of <code>PP_NETADDRESS_FAMILY_IPV6</code> address family.
Returns PP_FALSE on failure. Note that passing a net address of
<code>PP_NETADDRESS_FAMILY_IPV4</code> address family will fail - this
method will not map it to an IPv6 address.

[yzshen1, 2013-06-06 23:13:55]

> If you want to keep 'port' in net order, I would consider changing the type to
> an array of 2 bytes. Then you won't need so much 'endian' flipping code.

> See this article by Rob Pike:
> http://commandcenter.blogspot.com/2012/04/byte-order-fallacy.html

Thanks for the suggestion! I am a little bit unsure. Using an array of 2 bytes
allows us to do: host_order_port = port[1] | (port[0]<<8) regardless of system
endianness, which is nice.

However, one good thing of having uint16_t as port is that it is the same as the
port in sockaddr_in/6. On most of platforms, people can use ntohs() to do the
conversion directly.

+CC noelallen who discussed with me on this field in the previous code review.

Thanks!

https://codereview.chromium.org/16331007/diff/14001/ppapi/api/dev/ppb_net_add...
File ppapi/api/dev/ppb_net_address_dev.idl (right):

https://codereview.chromium.org/16331007/diff/14001/ppapi/api/dev/ppb_net_add...
ppapi/api/dev/ppb_net_address_dev.idl:112: * is an IPv4-mapped IPv6 address.
Your comment sounds much better. Thanks!

On 2013/06/06 22:31:34, bbudge1 wrote:
> Sorry I wasn't clearer before. I was thinking that since it should return
> PP_FALSE for any invalid resource type, the comment should read something like
> this:
> 
> Fills a <code>PP_NetAddress_IPv4_Dev</code> structure if the network
> address is of <code>PP_NETADDRESS_FAMILY_IPV4</code> address family.
> Returns PP_FALSE on failure. Note that passing a net address of
> <code>PP_NETADDRESS_FAMILY_IPV6</code> address family will fail even
> if the address is an IPv4-mapped IPv6 address.

https://codereview.chromium.org/16331007/diff/14001/ppapi/api/dev/ppb_net_add...
ppapi/api/dev/ppb_net_address_dev.idl:122: * map it to an IPv6 address.
On 2013/06/06 22:31:34, bbudge1 wrote:
> Similarly here:
> 
> Fills a <code>PP_NetAddress_IPv6_Dev</code> structure if the network
> address is of <code>PP_NETADDRESS_FAMILY_IPV6</code> address family.
> Returns PP_FALSE on failure. Note that passing a net address of
> <code>PP_NETADDRESS_FAMILY_IPV4</code> address family will fail - this
> method will not map it to an IPv6 address.

Done.

[bbudge, 2013-06-07 00:31:55]

LGTM

[noelallen_use_chromium, 2013-06-07 00:34:20]

The most important part to me is that both port and addr are in the same
byte order.  Next, people typically expect network byte order.  Finally I
think people are more used to uint16_t port, uint32_t addr than having addr
as 4 byte.   Unfortunately thanks to IPv6 there is no uint128_t and instead
addr is expressed as bytes. That being said, I can live with everything as
bytes as well.   Someone can cast to uint16_t if they want.




On Thu, Jun 6, 2013 at 4:13 PM, <yzshen@chromium.org> wrote:

> If you want to keep 'port' in net order, I would consider changing the
>> type to
>> an array of 2 bytes. Then you won't need so much 'endian' flipping code.
>>
>
>  See this article by Rob Pike:
>>
http://commandcenter.blogspot.**com/2012/04/byte-order-**fallacy.html<http://...
>>
>
> Thanks for the suggestion! I am a little bit unsure. Using an array of 2
> bytes
> allows us to do: host_order_port = port[1] | (port[0]<<8) regardless of
> system
> endianness, which is nice.
>
> However, one good thing of having uint16_t as port is that it is the same
> as the
> port in sockaddr_in/6. On most of platforms, people can use ntohs() to do
> the
> conversion directly.
>
> +CC noelallen who discussed with me on this field in the previous code
> review.
>
> Thanks!
>
>
>
> https://codereview.chromium.**org/16331007/diff/14001/ppapi/**
>
api/dev/ppb_net_address_dev.**idl<https://codereview.chromium.org/16331007/diff/14001/ppapi/api/dev/ppb_net_address_dev.idl>
> File ppapi/api/dev/ppb_net_address_**dev.idl (right):
>
> https://codereview.chromium.**org/16331007/diff/14001/ppapi/**
>
api/dev/ppb_net_address_dev.**idl#newcode112<https://codereview.chromium.org/16331007/diff/14001/ppapi/api/dev/ppb_net_address_dev.idl#newcode112>
> ppapi/api/dev/ppb_net_address_**dev.idl:112: * is an IPv4-mapped IPv6
> address.
> Your comment sounds much better. Thanks!
>
> On 2013/06/06 22:31:34, bbudge1 wrote:
>
>> Sorry I wasn't clearer before. I was thinking that since it should
>>
> return
>
>> PP_FALSE for any invalid resource type, the comment should read
>>
> something like
>
>> this:
>>
>
>  Fills a <code>PP_NetAddress_IPv4_Dev</**code> structure if the network
>> address is of <code>PP_NETADDRESS_FAMILY_**IPV4</code> address family.
>> Returns PP_FALSE on failure. Note that passing a net address of
>> <code>PP_NETADDRESS_FAMILY_**IPV6</code> address family will fail even
>> if the address is an IPv4-mapped IPv6 address.
>>
>
> https://codereview.chromium.**org/16331007/diff/14001/ppapi/**
>
api/dev/ppb_net_address_dev.**idl#newcode122<https://codereview.chromium.org/16331007/diff/14001/ppapi/api/dev/ppb_net_address_dev.idl#newcode122>
> ppapi/api/dev/ppb_net_address_**dev.idl:122: * map it to an IPv6 address.
> On 2013/06/06 22:31:34, bbudge1 wrote:
>
>> Similarly here:
>>
>
>  Fills a <code>PP_NetAddress_IPv6_Dev</**code> structure if the network
>> address is of <code>PP_NETADDRESS_FAMILY_**IPV6</code> address family.
>> Returns PP_FALSE on failure. Note that passing a net address of
>> <code>PP_NETADDRESS_FAMILY_**IPV4</code> address family will fail - this
>> method will not map it to an IPv6 address.
>>
>
> Done.
>
>
https://codereview.chromium.**org/16331007/<https://codereview.chromium.org/1...
>

[yzshen1, 2013-06-07 21:04:10]

Hi,

Would you please do an OWNER review?

Ben: native_client_sdk/src/build_tools/sdk_files.list

Darin: webkit/common/plugins/ppapi/ppapi_utils.cc

Thanks!

[yzshen1, 2013-06-07 21:09:23]

On 2013/06/07 21:04:10, yzshen1 wrote:
> Hi,
> 
> Would you please do an OWNER review?
> 
> Ben: native_client_sdk/src/build_tools/sdk_files.list
> 
> Darin: webkit/common/plugins/ppapi/ppapi_utils.cc
> 
> Thanks!

Hi, Darin.

I also copied the owners from webkit/plugins/ppapi/OWNERS into
webkit/common/plugins/ppapi/OWNERS.
Please also take a look. Thanks!

[binji, 2013-06-07 21:15:47]

native_client_sdk lgtm

[kinuko, 2013-06-10 03:54:07]

webkit/common/plugins/ppapi lgtm

[kinuko, 2013-06-10 04:27:16]

https://codereview.chromium.org/16331007/diff/31001/webkit/common/plugins/ppa...
File webkit/common/plugins/ppapi/OWNERS (left):

https://codereview.chromium.org/16331007/diff/31001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/OWNERS:1: brettw@chromium.org
Just as a general comment, can we add this in a separate CL?  (While adding this
file looks perfectly reasonable to me)

[yzshen1, 2013-06-10 05:25:30]

https://codereview.chromium.org/16331007/diff/31001/webkit/common/plugins/ppa...
File webkit/common/plugins/ppapi/OWNERS (left):

https://codereview.chromium.org/16331007/diff/31001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/OWNERS:1: brettw@chromium.org
Sure. Thanks for the suggestion.
On 2013/06/10 04:27:16, kinuko wrote:
> Just as a general comment, can we add this in a separate CL?  (While adding
this
> file looks perfectly reasonable to me)

[commit-bot: I haz the power, 2013-06-10 05:25:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/16331007/35001

[commit-bot: I haz the power, 2013-06-10 09:19:23]

Change committed as 205176