Pepper: Implement PPB_NetAddress_Private Describe() for IPv6 addresses on Windows.

Pepper: Implement PPB_NetAddress_Private Describe() for IPv6 addresses on Windows.

See RFC 5952. I might have even implemented it mostly correctly.

BUG=103969, 103968
TEST=ui_tests {PPAPITest,OutOfProcessPPAPITest}.NetAddressPrivate (DescribeIPv6)
TBR=dmichael@chromium.org

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

[viettrungluu, 2011-11-16 22:43:15]

(yzshen for real review, dmichael for OWNERS review)

[dmichael (off chromium), 2011-11-17 04:23:03]

http://codereview.chromium.org/8590006/diff/1/ppapi/tests/test_net_address_pr...
File ppapi/tests/test_net_address_private.h (right):

http://codereview.chromium.org/8590006/diff/1/ppapi/tests/test_net_address_pr...
ppapi/tests/test_net_address_private.h:18: virtual void RunTest();
You need to merge to the latest test stuff

[dmichael (off chromium), 2011-11-17 04:41:46]

http://codereview.chromium.org/8590006/diff/1/ppapi/tests/test_net_address_pr...
File ppapi/tests/test_net_address_private.cc (right):

http://codereview.chromium.org/8590006/diff/1/ppapi/tests/test_net_address_pr...
ppapi/tests/test_net_address_private.cc:56: a->sin6_flowinfo = 0;
Would it maybe be safer to use a sockaddr_in6 on the stack and then memcpy it to
data? That would avoid any possible alignment problems.

[yzshen1, 2011-11-17 18:17:56]

http://codereview.chromium.org/8590006/diff/1/ppapi/shared_impl/private/net_a...
File ppapi/shared_impl/private/net_address_private_impl.cc (right):

http://codereview.chromium.org/8590006/diff/1/ppapi/shared_impl/private/net_a...
ppapi/shared_impl/private/net_address_private_impl.cc:109: unsigned scope =
a->sin6_scope_id;
It is also in network-byte-order, isn't it?

[viettrungluu, 2011-11-17 20:20:24]

On 2011/11/17 18:17:56, yzshen1 wrote:
>
http://codereview.chromium.org/8590006/diff/1/ppapi/shared_impl/private/net_a...
> File ppapi/shared_impl/private/net_address_private_impl.cc (right):
> 
>
http://codereview.chromium.org/8590006/diff/1/ppapi/shared_impl/private/net_a...
> ppapi/shared_impl/private/net_address_private_impl.cc:109: unsigned scope =
> a->sin6_scope_id;
> It is also in network-byte-order, isn't it?

You'd think so (I did), but my experiments indicate no (at least on Linux, at
least if its getnameinfo() works properly). I have no idea why.

[viettrungluu, 2011-11-17 20:22:56]

http://codereview.chromium.org/8590006/diff/1/ppapi/tests/test_net_address_pr...
File ppapi/tests/test_net_address_private.cc (right):

http://codereview.chromium.org/8590006/diff/1/ppapi/tests/test_net_address_pr...
ppapi/tests/test_net_address_private.cc:56: a->sin6_flowinfo = 0;
On 2011/11/17 04:41:46, dmichael wrote:
> Would it maybe be safer to use a sockaddr_in6 on the stack and then memcpy it
to
> data? That would avoid any possible alignment problems.

That's a good point, though in that case I should change various code elsewhere
too. Maybe it's better/easier/less error-prone overall if I add appropriate
alignment assertions about PP_NetAddress_Private? WDYT?

[dmichael (off chromium), 2011-11-17 20:43:57]

On 2011/11/17 20:22:56, viettrungluu wrote:
>
http://codereview.chromium.org/8590006/diff/1/ppapi/tests/test_net_address_pr...
> File ppapi/tests/test_net_address_private.cc (right):
> 
>
http://codereview.chromium.org/8590006/diff/1/ppapi/tests/test_net_address_pr...
> ppapi/tests/test_net_address_private.cc:56: a->sin6_flowinfo = 0;
> On 2011/11/17 04:41:46, dmichael wrote:
> > Would it maybe be safer to use a sockaddr_in6 on the stack and then memcpy
it
> to
> > data? That would avoid any possible alignment problems.
> 
> That's a good point, though in that case I should change various code
elsewhere
> too. Maybe it's better/easier/less error-prone overall if I add appropriate
> alignment assertions about PP_NetAddress_Private? WDYT?
Either way. I don't think it will *really* happen, but it would be nice to
protect it with an assertion or by avoiding the problem with memcpy. Up to you.

[yzshen1, 2011-11-17 23:08:23]

LGTM

Thanks for the testing with scoped ID!

[viettrungluu, 2011-11-17 23:56:06]

On 2011/11/17 20:43:57, dmichael wrote:
> On 2011/11/17 20:22:56, viettrungluu wrote:
> >
>
http://codereview.chromium.org/8590006/diff/1/ppapi/tests/test_net_address_pr...
> > File ppapi/tests/test_net_address_private.cc (right):
> > 
> >
>
http://codereview.chromium.org/8590006/diff/1/ppapi/tests/test_net_address_pr...
> > ppapi/tests/test_net_address_private.cc:56: a->sin6_flowinfo = 0;
> > On 2011/11/17 04:41:46, dmichael wrote:
> > > Would it maybe be safer to use a sockaddr_in6 on the stack and then memcpy
> it
> > to
> > > data? That would avoid any possible alignment problems.
> > 
> > That's a good point, though in that case I should change various code
> elsewhere
> > too. Maybe it's better/easier/less error-prone overall if I add appropriate
> > alignment assertions about PP_NetAddress_Private? WDYT?
> Either way. I don't think it will *really* happen, but it would be nice to
> protect it with an assertion or by avoiding the problem with memcpy. Up to
you.

Okay, I'll add an assertion in a separate change.

I converted ppapi_uitest.cc. Please take a look. It might even compile.... :)

[viettrungluu, 2011-11-18 06:26:00]

On 2011/11/17 23:56:06, viettrungluu wrote:
> On 2011/11/17 20:43:57, dmichael wrote:
> > On 2011/11/17 20:22:56, viettrungluu wrote:
> > >
> >
>
http://codereview.chromium.org/8590006/diff/1/ppapi/tests/test_net_address_pr...
> > > File ppapi/tests/test_net_address_private.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8590006/diff/1/ppapi/tests/test_net_address_pr...
> > > ppapi/tests/test_net_address_private.cc:56: a->sin6_flowinfo = 0;
> > > On 2011/11/17 04:41:46, dmichael wrote:
> > > > Would it maybe be safer to use a sockaddr_in6 on the stack and then
memcpy
> > it
> > > to
> > > > data? That would avoid any possible alignment problems.
> > > 
> > > That's a good point, though in that case I should change various code
> > elsewhere
> > > too. Maybe it's better/easier/less error-prone overall if I add
appropriate
> > > alignment assertions about PP_NetAddress_Private? WDYT?
> > Either way. I don't think it will *really* happen, but it would be nice to
> > protect it with an assertion or by avoiding the problem with memcpy. Up to
> you.
> 
> Okay, I'll add an assertion in a separate change.
> 
> I converted ppapi_uitest.cc. Please take a look. It might even compile.... :)

Ha, it didn't. My incompetence impresses even me.

But I'll TBR it and let the commit queue work its magic overnight.

[commit-bot: I haz the power, 2011-11-18 06:26:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/viettrungluu@chromium.org/8590006/11001

[commit-bot: I haz the power, 2011-11-18 06:41:17]

Try job failure for 8590006-11001 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-18 21:15:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/viettrungluu@chromium.org/8590006/7007

[commit-bot: I haz the power, 2011-11-18 22:18:30]

Try job failure for 8590006-7007 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-11-18 22:19:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/viettrungluu@chromium.org/8590006/8008

[commit-bot: I haz the power, 2011-11-18 23:46:47]

Try job failure for 8590006-8008 (retry) on mac_rel for step "ui_tests".
It's a second try, previously, steps "check_deps, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[viettrungluu, 2011-11-21 20:21:57]

PTAL.

This CL turned out to be a bit of a fiasco.... In addition to the various build
issues:

- I discovered that |getnameinfo()| doesn't behave the same way on Mac (it's
doesn't follow RFC 5952 in formatting IPv6 addresses) ... so I switched to our
implementation on Mac.
- I realized that I should handle IPv4 addresses embedded in IPv6 differently
(format them, e.g., as "::ffff:192.168.0.1"). So I implemented that.
- I made the test nicer to loop over an array of test cases (mostly since I was
tired of searching and replacing variable names).

On 2011/11/17 23:08:23, yzshen1 wrote:
> LGTM
> 
> Thanks for the testing with scoped ID!

[dmichael (off chromium), 2011-11-21 21:20:46]

http://codereview.chromium.org/8590006/diff/16001/ppapi/shared_impl/private/n...
File ppapi/shared_impl/private/net_address_private_impl.cc (right):

http://codereview.chromium.org/8590006/diff/16001/ppapi/shared_impl/private/n...
ppapi/shared_impl/private/net_address_private_impl.cc:151: continue;
optional nit: I think the algorithm here would be easier to follow if you put
the rest in an else instead of using continue. But maybe it's just me that
doesn't like continue.

http://codereview.chromium.org/8590006/diff/16001/ppapi/tests/test_net_addres...
File ppapi/tests/test_net_address_private.cc (right):

http://codereview.chromium.org/8590006/diff/16001/ppapi/tests/test_net_addres...
ppapi/tests/test_net_address_private.cc:243: NetAddressPrivate::Describe(addr,
true));
The downside with this approach is if one of these fails, you won't know which
one :-(  If you still want the array approach, you could change the asserts to
ifs that return a more useful error string on failure...

[viettrungluu, 2011-11-21 21:33:48]

Thanks.

http://codereview.chromium.org/8590006/diff/16001/ppapi/shared_impl/private/n...
File ppapi/shared_impl/private/net_address_private_impl.cc (right):

http://codereview.chromium.org/8590006/diff/16001/ppapi/shared_impl/private/n...
ppapi/shared_impl/private/net_address_private_impl.cc:151: continue;
On 2011/11/21 21:20:46, dmichael wrote:
> optional nit: I think the algorithm here would be easier to follow if you put
> the rest in an else instead of using continue. But maybe it's just me that
> doesn't like continue.

Done.

http://codereview.chromium.org/8590006/diff/16001/ppapi/tests/test_net_addres...
File ppapi/tests/test_net_address_private.cc (right):

http://codereview.chromium.org/8590006/diff/16001/ppapi/tests/test_net_addres...
ppapi/tests/test_net_address_private.cc:243: NetAddressPrivate::Describe(addr,
true));
On 2011/11/21 21:20:46, dmichael wrote:
> The downside with this approach is if one of these fails, you won't know which
> one :-(  If you still want the array approach, you could change the asserts to
> ifs that return a more useful error string on failure...

I'll defer this, since what I'd really like is a more useful ASSERT_EQ() that
logs expected/actual values (like gtest's macros). I find that in the case of
failure, I end up needing to know that anyway and knowing which case failed
isn't quite sufficient.

[dmichael (off chromium), 2011-11-21 21:55:33]

lgtm

I'll let yzshen1 check the network stuff, which I'm not very familiar with.

[yzshen1, 2011-11-21 23:55:03]

lgtm

http://codereview.chromium.org/8590006/diff/21001/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/8590006/diff/21001/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:12: #include "chrome/common/chrome_paths.h"
This is probably not your change, but it would be nice if you sort them. :)

http://codereview.chromium.org/8590006/diff/21001/ppapi/shared_impl/private/n...
File ppapi/shared_impl/private/net_address_private_impl.cc (right):

http://codereview.chromium.org/8590006/diff/21001/ppapi/shared_impl/private/n...
ppapi/shared_impl/private/net_address_private_impl.cc:98: #if defined(OS_WIN) ||
defined(OS_MACOSX)
It may be better to use this implementation for all platforms.

http://codereview.chromium.org/8590006/diff/21001/ppapi/shared_impl/private/n...
ppapi/shared_impl/private/net_address_private_impl.cc:129: if
(a->sin6_addr.s6_addr16[0] == 0 && a->sin6_addr.s6_addr16[1] == 0 &&
[optional]
It might be good to define a reference/pointer to a->sin6_addr.s6_addr16, using
a shorter name.

[dhollowa, 2011-11-22 02:33:27]

Some Valgrind test failure fallout from this:

  http://codereview.chromium.org/8635001/