Add private Pepper API for dealing with PP_Flash_NetAddress.

Add private Pepper API for dealing with PP_Flash_NetAddress.

BUG=none
TEST=none

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

[viettrungluu, 2011-11-07 20:43:04]

ping

[yzshen1, 2011-11-07 21:57:44]

Sorry for the delay!

http://codereview.chromium.org/8357030/diff/6001/ppapi/cpp/private/flash_net_...
File ppapi/cpp/private/flash_net_address.cc (right):

http://codereview.chromium.org/8357030/diff/6001/ppapi/cpp/private/flash_net_...
ppapi/cpp/private/flash_net_address.cc:13: #include
"ppapi/c/private/ppb_flash_net_address.h"
nit: please sort the include list.

http://codereview.chromium.org/8357030/diff/6001/ppapi/cpp/private/flash_net_...
ppapi/cpp/private/flash_net_address.cc:37: const PP_Flash_NetAddress& addr2) {
Wrong indent.

http://codereview.chromium.org/8357030/diff/6001/ppapi/cpp/private/flash_net_...
File ppapi/cpp/private/flash_net_address.h (right):

http://codereview.chromium.org/8357030/diff/6001/ppapi/cpp/private/flash_net_...
ppapi/cpp/private/flash_net_address.h:32: static void GetAnyAddress(bool
is_ipv6, struct PP_Flash_NetAddress* addr);
nit: we could omit the struct keyword here.

http://codereview.chromium.org/8357030/diff/6001/ppapi/shared_impl/private/fl...
File ppapi/shared_impl/private/flash_net_address_impl.cc (right):

http://codereview.chromium.org/8357030/diff/6001/ppapi/shared_impl/private/fl...
ppapi/shared_impl/private/flash_net_address_impl.cc:34: inline sa_family_t
GetFamily(const PP_Flash_NetAddress& addr) {
It might be better to:
(1) put all code manipulating PP_Flash_NetAddress into a single namespace/class,
including those functions in pepper_message_filter now.
(2) put the thunk code in the thunk/ folder, and simply call functions mentioned
in (1).

http://codereview.chromium.org/8357030/diff/6001/ppapi/shared_impl/private/fl...
ppapi/shared_impl/private/flash_net_address_impl.cc:74:
memcmp(a1->sin6_addr.s6_addr, a2->sin6_addr.s6_addr,
I think on Windows the sin6_addr is of the follow type:
typedef struct in6_addr {
  union {
    UCHAR  Byte[16];
    USHORT Word[8];
  } u;
} IN6_ADDR, *PIN6_ADDR, *LPIN6_ADDR;

[viettrungluu, 2011-11-07 23:33:37]

Thanks, new patchset uploaded.

http://codereview.chromium.org/8357030/diff/6001/ppapi/cpp/private/flash_net_...
File ppapi/cpp/private/flash_net_address.cc (right):

http://codereview.chromium.org/8357030/diff/6001/ppapi/cpp/private/flash_net_...
ppapi/cpp/private/flash_net_address.cc:13: #include
"ppapi/c/private/ppb_flash_net_address.h"
On 2011/11/07 21:57:44, yzshen1 wrote:
> nit: please sort the include list.

Ugly/counterintuitive as it is, the canonical sort order according to the style
guide is that of (the UNIX command) "sort", which apparently just ignores
characters like '/' and '_'. This order is correct.

http://codereview.chromium.org/8357030/diff/6001/ppapi/cpp/private/flash_net_...
ppapi/cpp/private/flash_net_address.cc:37: const PP_Flash_NetAddress& addr2) {
On 2011/11/07 21:57:44, yzshen1 wrote:
> Wrong indent.

Done.

http://codereview.chromium.org/8357030/diff/6001/ppapi/cpp/private/flash_net_...
File ppapi/cpp/private/flash_net_address.h (right):

http://codereview.chromium.org/8357030/diff/6001/ppapi/cpp/private/flash_net_...
ppapi/cpp/private/flash_net_address.h:32: static void GetAnyAddress(bool
is_ipv6, struct PP_Flash_NetAddress* addr);
On 2011/11/07 21:57:44, yzshen1 wrote:
> nit: we could omit the struct keyword here.

Done.

http://codereview.chromium.org/8357030/diff/6001/ppapi/shared_impl/private/fl...
File ppapi/shared_impl/private/flash_net_address_impl.cc (right):

http://codereview.chromium.org/8357030/diff/6001/ppapi/shared_impl/private/fl...
ppapi/shared_impl/private/flash_net_address_impl.cc:34: inline sa_family_t
GetFamily(const PP_Flash_NetAddress& addr) {
On 2011/11/07 21:57:44, yzshen1 wrote:
> It might be better to:
> (1) put all code manipulating PP_Flash_NetAddress into a single
namespace/class,
> including those functions in pepper_message_filter now.
> (2) put the thunk code in the thunk/ folder, and simply call functions
mentioned
> in (1).

I've added a ppapi/shared_impl/private/flash_net_address_impl.h and moved
ValidateNetAddress there. I added a TODO to move other stuff there, to be done
after I delete the old "Flapper hacks" TCP code.

http://codereview.chromium.org/8357030/diff/6001/ppapi/shared_impl/private/fl...
ppapi/shared_impl/private/flash_net_address_impl.cc:74:
memcmp(a1->sin6_addr.s6_addr, a2->sin6_addr.s6_addr,
Fixed.

(I agree that that's what the documentation says. On the other hand, somehow it
managed to compile this file on Windows....)

On 2011/11/07 21:57:44, yzshen1 wrote:
> I think on Windows the sin6_addr is of the follow type:
> typedef struct in6_addr {
>   union {
>     UCHAR  Byte[16];
>     USHORT Word[8];
>   } u;
> } IN6_ADDR, *PIN6_ADDR, *LPIN6_ADDR;

[yzshen1, 2011-11-07 23:58:35]

Thanks!

http://codereview.chromium.org/8357030/diff/6001/ppapi/cpp/private/flash_net_...
File ppapi/cpp/private/flash_net_address.cc (right):

http://codereview.chromium.org/8357030/diff/6001/ppapi/cpp/private/flash_net_...
ppapi/cpp/private/flash_net_address.cc:13: #include
"ppapi/c/private/ppb_flash_net_address.h"
Thanks. I didn't know that.

On 2011/11/07 23:33:37, viettrungluu wrote:
> On 2011/11/07 21:57:44, yzshen1 wrote:
> > nit: please sort the include list.
> 
> Ugly/counterintuitive as it is, the canonical sort order according to the
style
> guide is that of (the UNIX command) "sort", which apparently just ignores
> characters like '/' and '_'. This order is correct.

http://codereview.chromium.org/8357030/diff/2002/ppapi/cpp/private/flash_net_...
File ppapi/cpp/private/flash_net_address.cc (right):

http://codereview.chromium.org/8357030/diff/2002/ppapi/cpp/private/flash_net_...
ppapi/cpp/private/flash_net_address.cc:73: void NetAddress::GetAnyAddress(bool
is_ipv6, struct PP_Flash_NetAddress* addr) {
nit: you don't need to have 'struct'.

http://codereview.chromium.org/8357030/diff/2002/ppapi/cpp/private/flash_net_...
ppapi/cpp/private/flash_net_address.cc:76: return
!!get_interface<PPB_Flash_NetAddress>()->GetAnyAddress(
The return type is 'void'.

http://codereview.chromium.org/8357030/diff/2002/ppapi/shared_impl/private/fl...
File ppapi/shared_impl/private/flash_net_address_impl.cc (right):

http://codereview.chromium.org/8357030/diff/2002/ppapi/shared_impl/private/fl...
ppapi/shared_impl/private/flash_net_address_impl.cc:151: PPAPI_THUNK_EXPORT
const PPB_Flash_NetAddress* GetPPB_Flash_NetAddress_Thunk() {
I personally think that it may be better to have thunk-related code in the
thunk/ folder:
- either having FlashNetAddressImpl expose AreEqual etc. and calling them there;
- or moving all code above to thunk/.

What do you think?

[yzshen1, 2011-11-08 00:08:41]

LGTM

Thanks!
On 2011/11/07 23:58:35, yzshen1 wrote:
> Thanks!
> 
>
http://codereview.chromium.org/8357030/diff/6001/ppapi/cpp/private/flash_net_...
> File ppapi/cpp/private/flash_net_address.cc (right):
> 
>
http://codereview.chromium.org/8357030/diff/6001/ppapi/cpp/private/flash_net_...
> ppapi/cpp/private/flash_net_address.cc:13: #include
> "ppapi/c/private/ppb_flash_net_address.h"
> Thanks. I didn't know that.
> 
> On 2011/11/07 23:33:37, viettrungluu wrote:
> > On 2011/11/07 21:57:44, yzshen1 wrote:
> > > nit: please sort the include list.
> > 
> > Ugly/counterintuitive as it is, the canonical sort order according to the
> style
> > guide is that of (the UNIX command) "sort", which apparently just ignores
> > characters like '/' and '_'. This order is correct.
> 
>
http://codereview.chromium.org/8357030/diff/2002/ppapi/cpp/private/flash_net_...
> File ppapi/cpp/private/flash_net_address.cc (right):
> 
>
http://codereview.chromium.org/8357030/diff/2002/ppapi/cpp/private/flash_net_...
> ppapi/cpp/private/flash_net_address.cc:73: void NetAddress::GetAnyAddress(bool
> is_ipv6, struct PP_Flash_NetAddress* addr) {
> nit: you don't need to have 'struct'.
> 
>
http://codereview.chromium.org/8357030/diff/2002/ppapi/cpp/private/flash_net_...
> ppapi/cpp/private/flash_net_address.cc:76: return
> !!get_interface<PPB_Flash_NetAddress>()->GetAnyAddress(
> The return type is 'void'.
> 
>
http://codereview.chromium.org/8357030/diff/2002/ppapi/shared_impl/private/fl...
> File ppapi/shared_impl/private/flash_net_address_impl.cc (right):
> 
>
http://codereview.chromium.org/8357030/diff/2002/ppapi/shared_impl/private/fl...
> ppapi/shared_impl/private/flash_net_address_impl.cc:151: PPAPI_THUNK_EXPORT
> const PPB_Flash_NetAddress* GetPPB_Flash_NetAddress_Thunk() {
> I personally think that it may be better to have thunk-related code in the
> thunk/ folder:
> - either having FlashNetAddressImpl expose AreEqual etc. and calling them
there;
> - or moving all code above to thunk/.
> 
> What do you think?

[viettrungluu, 2011-11-08 00:12:42]

Thanks.

http://codereview.chromium.org/8357030/diff/2002/ppapi/cpp/private/flash_net_...
File ppapi/cpp/private/flash_net_address.cc (right):

http://codereview.chromium.org/8357030/diff/2002/ppapi/cpp/private/flash_net_...
ppapi/cpp/private/flash_net_address.cc:73: void NetAddress::GetAnyAddress(bool
is_ipv6, struct PP_Flash_NetAddress* addr) {
On 2011/11/07 23:58:36, yzshen1 wrote:
> nit: you don't need to have 'struct'.

Done.

http://codereview.chromium.org/8357030/diff/2002/ppapi/cpp/private/flash_net_...
ppapi/cpp/private/flash_net_address.cc:76: return
!!get_interface<PPB_Flash_NetAddress>()->GetAnyAddress(
On 2011/11/07 23:58:36, yzshen1 wrote:
> The return type is 'void'.

Done.

http://codereview.chromium.org/8357030/diff/2002/ppapi/shared_impl/private/fl...
File ppapi/shared_impl/private/flash_net_address_impl.cc (right):

http://codereview.chromium.org/8357030/diff/2002/ppapi/shared_impl/private/fl...
ppapi/shared_impl/private/flash_net_address_impl.cc:151: PPAPI_THUNK_EXPORT
const PPB_Flash_NetAddress* GetPPB_Flash_NetAddress_Thunk() {
On 2011/11/07 23:58:36, yzshen1 wrote:
> I personally think that it may be better to have thunk-related code in the
> thunk/ folder:
> - either having FlashNetAddressImpl expose AreEqual etc. and calling them
there;
> - or moving all code above to thunk/.
> 
> What do you think?
>  

There's no real thunking involved; see the comment at the top of the file.
Having this in this file follows the pattern of crypto_impl.cc and
memory_impl.cc (and prevents us from having to export symbols for the impls).

[viettrungluu, 2011-11-08 18:02:48]

Adding piman/dmichael/brettw for OWNERS approval.

[dmichael (off chromium), 2011-11-08 18:06:48]

I can haz tests?

[viettrungluu, 2011-11-08 18:15:47]

On 2011/11/08 18:06:48, dmichael wrote:
> I can haz tests?

"Traditionally", we haven't had any testing for the Flash-specific interfaces,
though we should change that and I'd be happy to do so. Whether I should start
with this new, as-yet unused interface or other interfaces is up to you....

[dmichael (off chromium), 2011-11-08 18:22:49]

If you really need to get this in now, I could give up on the request for tests.
But... we definitely should have tests for Flash interfaces if possible (it
ships on ChromeOS, after all), and we have to start somewhere :-)

[viettrungluu, 2011-11-08 18:35:23]

Can I do the test as a separate change?

I also want to do some related gyp file reorganization (e.g., we need to start
compiling the private cpp files in Chrome to go with the tests).

On 2011/11/08 18:22:49, dmichael wrote:
> If you really need to get this in now, I could give up on the request for
tests.
> But... we definitely should have tests for Flash interfaces if possible (it
> ships on ChromeOS, after all), and we have to start somewhere :-)

[dmichael (off chromium), 2011-11-08 18:46:16]

On 2011/11/08 18:35:23, viettrungluu wrote:
> Can I do the test as a separate change?
> 
> I also want to do some related gyp file reorganization (e.g., we need to start
> compiling the private cpp files in Chrome to go with the tests).
Sure.

LGTM

[viettrungluu, 2011-11-08 18:58:53]

+jam for yet more OWNERS approval. Sigh.

[viettrungluu, 2011-11-08 19:07:43]

-piman, +avi (whose OWNERS stamp would also suffice)

[Avi (use Gerrit), 2011-11-08 20:20:32]

content owner rubberstamp lgtm