Pepper: Add tests for the PPB_NetAddress_Private interface.

Pepper: Add tests for the PPB_NetAddress_Private interface.

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

[viettrungluu, 2011-11-11 03:17:44]

Obviously, I forgot to actually upload (well, git add) the new files. D'oh. I'll
re-upload tomorrow....

[viettrungluu, 2011-11-11 18:11:17]

Okay, new patch set uploaded, with all the files this time.

[yzshen1, 2011-11-11 18:55:27]

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
File ppapi/tests/test_net_address_private.cc (right):

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
ppapi/tests/test_net_address_private.cc:11: // |PP_NetAddress_Private| structs
from just this interface. We'll cheat and
[not a review comment]
Are we going to provide API to convert between PP_NetAddress_Private and
sockaddr etc.?

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
ppapi/tests/test_net_address_private.cc:29: #include <arpa/inet.h>
Sort it, please.

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
ppapi/tests/test_net_address_private.cc:57: if
(!NetAddressPrivate::IsAvailable())
Maybe directly return NetAddressPrivate::IsAvailable().

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
ppapi/tests/test_net_address_private.cc:64: instance_->LogTest("AreEqual",
TestAreEqual());
Please consider using RUN_TEST

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
File ppapi/tests/test_net_address_private.h (right):

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
ppapi/tests/test_net_address_private.h:10: #include
"ppapi/c/private/ppb_net_address_private.h"
I think this include can be moved into .cc file.

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
ppapi/tests/test_net_address_private.h:13: class TestNetAddressPrivate : public
TestCase {
Please also add the test to ppapi_uitest.cc, so that it can be run
automatically.

[viettrungluu, 2011-11-11 19:07:40]

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
File ppapi/tests/test_net_address_private.cc (right):

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
ppapi/tests/test_net_address_private.cc:11: // |PP_NetAddress_Private| structs
from just this interface. We'll cheat and
On 2011/11/11 18:55:28, yzshen1 wrote:
> [not a review comment]
> Are we going to provide API to convert between PP_NetAddress_Private and
> sockaddr etc.?

It's not my immediate intention to, though I guess it's something to consider.
It might be better to add "constructor" functions to the API though.

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
ppapi/tests/test_net_address_private.cc:29: #include <arpa/inet.h>
On 2011/11/11 18:55:28, yzshen1 wrote:
> Sort it, please.

Done.

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
ppapi/tests/test_net_address_private.cc:57: if
(!NetAddressPrivate::IsAvailable())
On 2011/11/11 18:55:28, yzshen1 wrote:
> Maybe directly return NetAddressPrivate::IsAvailable().

Done.

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
ppapi/tests/test_net_address_private.cc:64: instance_->LogTest("AreEqual",
TestAreEqual());
On 2011/11/11 18:55:28, yzshen1 wrote:
> Please consider using RUN_TEST

Done.

I also converted test_uma.cc (where I copied stuff from) as a drive-by.

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
File ppapi/tests/test_net_address_private.h (right):

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
ppapi/tests/test_net_address_private.h:10: #include
"ppapi/c/private/ppb_net_address_private.h"
On 2011/11/11 18:55:28, yzshen1 wrote:
> I think this include can be moved into .cc file.

Done.

http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
ppapi/tests/test_net_address_private.h:13: class TestNetAddressPrivate : public
TestCase {
On 2011/11/11 18:55:28, yzshen1 wrote:
> Please also add the test to ppapi_uitest.cc, so that it can be run
> automatically.

Done.

[yzshen1, 2011-11-11 19:10:00]

LGTM

Thanks!
On 2011/11/11 19:07:40, viettrungluu wrote:
>
http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
> File ppapi/tests/test_net_address_private.cc (right):
> 
>
http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
> ppapi/tests/test_net_address_private.cc:11: // |PP_NetAddress_Private| structs
> from just this interface. We'll cheat and
> On 2011/11/11 18:55:28, yzshen1 wrote:
> > [not a review comment]
> > Are we going to provide API to convert between PP_NetAddress_Private and
> > sockaddr etc.?
> 
> It's not my immediate intention to, though I guess it's something to consider.
> It might be better to add "constructor" functions to the API though.
Yes, that sounds good, too.

> 
>
http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
> ppapi/tests/test_net_address_private.cc:29: #include <arpa/inet.h>
> On 2011/11/11 18:55:28, yzshen1 wrote:
> > Sort it, please.
> 
> Done.
> 
>
http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
> ppapi/tests/test_net_address_private.cc:57: if
> (!NetAddressPrivate::IsAvailable())
> On 2011/11/11 18:55:28, yzshen1 wrote:
> > Maybe directly return NetAddressPrivate::IsAvailable().
> 
> Done.
> 
>
http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
> ppapi/tests/test_net_address_private.cc:64: instance_->LogTest("AreEqual",
> TestAreEqual());
> On 2011/11/11 18:55:28, yzshen1 wrote:
> > Please consider using RUN_TEST
> 
> Done.
> 
> I also converted test_uma.cc (where I copied stuff from) as a drive-by.
> 
>
http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
> File ppapi/tests/test_net_address_private.h (right):
> 
>
http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
> ppapi/tests/test_net_address_private.h:10: #include
> "ppapi/c/private/ppb_net_address_private.h"
> On 2011/11/11 18:55:28, yzshen1 wrote:
> > I think this include can be moved into .cc file.
> 
> Done.
> 
>
http://codereview.chromium.org/8527017/diff/4001/ppapi/tests/test_net_address...
> ppapi/tests/test_net_address_private.h:13: class TestNetAddressPrivate :
public
> TestCase {
> On 2011/11/11 18:55:28, yzshen1 wrote:
> > Please also add the test to ppapi_uitest.cc, so that it can be run
> > automatically.
> 
> Done.

[dmichael (off chromium), 2011-11-11 19:47:41]

LGTM too, thanks!