HostResolver is exposed to plugin.

HostResolver is exposed to plugin.


BUG=114225
TEST=UI test TestHostResolverPrivate


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

[jam, 2012-02-27 21:47:40]

you listed 4 reviewers. can you specify which parts they should be looking at?

[ygorshenin1, 2012-02-28 08:14:35]

On 2012/02/27 21:47:40, John Abd-El-Malek wrote:
> you listed 4 reviewers. can you specify which parts they should be looking at?

sky: chrome/test/*
brettw, yzshen: content/*, ppapi/*
jam: webkit/plugins/*

[yzshen1, 2012-02-28 08:28:59]

First part of comments...

http://codereview.chromium.org/9455092/diff/2075/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9455092/diff/2075/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:60: // #include
"ppapi/c/private/ppb_host_resolver_private.h"
Remove it instead of commenting it out.

http://codereview.chromium.org/9455092/diff/2075/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1348: if (hint == NULL) {
nit: if (!hint) is more common.

http://codereview.chromium.org/9455092/diff/2075/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1616:
IPC_MESSAGE_HANDLER(PpapiMsg_PPBHostResolver_ResolveACK,
You might want to keep the list in the same order as the handler definitions.

http://codereview.chromium.org/9455092/diff/2075/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/9455092/diff/2075/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:359: uint32 /* host_resolver_id */,
You probably need to add plugin_dispatcher_id as well when you implement the
out-of-process support. (But that depends on how you implement it.)

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/ne...
File ppapi/shared_impl/private/network_list.cc (right):

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/ne...
ppapi/shared_impl/private/network_list.cc:69: memmove(address,
&address_list_[index], sizeof(*address));
You could assign directly.

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/ne...
File ppapi/shared_impl/private/network_list.h (right):

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/ne...
ppapi/shared_impl/private/network_list.h:23: class NetworkList {
Comparing to NetworkList, I prefer std::vector<SimpleStructOfNameAndAddress>.
And put the struct as well as things like CreateFromAddrInfo in the
ppb_host_resolver_shared files.

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/ne...
ppapi/shared_impl/private/network_list.h:26: virtual ~NetworkList();
It doesn't need to be virtual.

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/pp...
File ppapi/shared_impl/private/ppb_host_resolver_shared.cc (right):

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_host_resolver_shared.cc:65: if
(resolve_in_progress_)
If GetItem() is not allowed while resolving, shall we also disallow GetSize() to
return the previous size?

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_host_resolver_shared.cc:75: if (canonical_name !=
NULL)
nit: please be consistent. You could use if (!canonical_name).

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/pp...
File ppapi/shared_impl/private/ppb_host_resolver_shared.h (right):

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_host_resolver_shared.h:61: bool
resolve_in_progress_;
It seems you don't need resolve_in_progress_:
TrackedCallback::IsPending(resolve_callback_) <==> resolve_in_progress_.

http://codereview.chromium.org/9455092/diff/2075/webkit/plugins/ppapi/plugin_...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/9455092/diff/2075/webkit/plugins/ppapi/plugin_...
webkit/plugins/ppapi/plugin_delegate.h:488: virtual void HostResolverResolve(
Please use VerbNoun, say, ResolveHost.

http://codereview.chromium.org/9455092/diff/2075/webkit/plugins/ppapi/resourc...
File webkit/plugins/ppapi/resource_creation_impl.h (right):

http://codereview.chromium.org/9455092/diff/2075/webkit/plugins/ppapi/resourc...
webkit/plugins/ppapi/resource_creation_impl.h:75: virtual PP_Resource
CreateHostResolverPrivate(PP_Instance instance) OVERRIDE;
Sort alphabetically, please. (And also the corresponding definitions.)

[ygorshenin1, 2012-02-28 12:09:46]

PTAL

http://codereview.chromium.org/9455092/diff/2075/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9455092/diff/2075/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:60: // #include
"ppapi/c/private/ppb_host_resolver_private.h"
On 2012/02/28 08:29:00, yzshen1 wrote:
> Remove it instead of commenting it out.

Done.

http://codereview.chromium.org/9455092/diff/2075/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1348: if (hint == NULL) {
On 2012/02/28 08:29:00, yzshen1 wrote:
> nit: if (!hint) is more common.

Done.

http://codereview.chromium.org/9455092/diff/2075/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1616:
IPC_MESSAGE_HANDLER(PpapiMsg_PPBHostResolver_ResolveACK,
On 2012/02/28 08:29:00, yzshen1 wrote:
> You might want to keep the list in the same order as the handler definitions.

Done.

http://codereview.chromium.org/9455092/diff/2075/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/9455092/diff/2075/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:359: uint32 /* host_resolver_id */,
Yes, but I think it's better to do this in CL for out-of-process support.

On 2012/02/28 08:29:00, yzshen1 wrote:
> You probably need to add plugin_dispatcher_id as well when you implement the
> out-of-process support. (But that depends on how you implement it.)

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/ne...
File ppapi/shared_impl/private/network_list.cc (right):

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/ne...
ppapi/shared_impl/private/network_list.cc:69: memmove(address,
&address_list_[index], sizeof(*address));
On 2012/02/28 08:29:00, yzshen1 wrote:
> You could assign directly.

Done.

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/ne...
File ppapi/shared_impl/private/network_list.h (right):

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/ne...
ppapi/shared_impl/private/network_list.h:23: class NetworkList {
Thanks! I replaced the hand-written NetworkList to
std::vector<NameAndAddressPair>.

On 2012/02/28 08:29:00, yzshen1 wrote:
> Comparing to NetworkList, I prefer std::vector<SimpleStructOfNameAndAddress>.
> And put the struct as well as things like CreateFromAddrInfo in the
> ppb_host_resolver_shared files.

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/ne...
ppapi/shared_impl/private/network_list.h:26: virtual ~NetworkList();
NetworkList is removed.

On 2012/02/28 08:29:00, yzshen1 wrote:
> It doesn't need to be virtual.

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/pp...
File ppapi/shared_impl/private/ppb_host_resolver_shared.cc (right):

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_host_resolver_shared.cc:65: if
(resolve_in_progress_)
On 2012/02/28 08:29:00, yzshen1 wrote:
> If GetItem() is not allowed while resolving, shall we also disallow GetSize()
to
> return the previous size?

Done.

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_host_resolver_shared.cc:75: if (canonical_name !=
NULL)
On 2012/02/28 08:29:00, yzshen1 wrote:
> nit: please be consistent. You could use if (!canonical_name).

Done.

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/pp...
File ppapi/shared_impl/private/ppb_host_resolver_shared.h (right):

http://codereview.chromium.org/9455092/diff/2075/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_host_resolver_shared.h:61: bool
resolve_in_progress_;
On 2012/02/28 08:29:00, yzshen1 wrote:
> It seems you don't need resolve_in_progress_:
> TrackedCallback::IsPending(resolve_callback_) <==> resolve_in_progress_.

Done.

http://codereview.chromium.org/9455092/diff/2075/webkit/plugins/ppapi/plugin_...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/9455092/diff/2075/webkit/plugins/ppapi/plugin_...
webkit/plugins/ppapi/plugin_delegate.h:488: virtual void HostResolverResolve(
Why? As you can see below all names has the form InterfaceNameMethod
(UDPSocketSendTo, TCPSocketSSLHandshake etc.). I decided to name this method so
only for consistency.

On 2012/02/28 08:29:00, yzshen1 wrote:
> Please use VerbNoun, say, ResolveHost.

http://codereview.chromium.org/9455092/diff/2075/webkit/plugins/ppapi/resourc...
File webkit/plugins/ppapi/resource_creation_impl.h (right):

http://codereview.chromium.org/9455092/diff/2075/webkit/plugins/ppapi/resourc...
webkit/plugins/ppapi/resource_creation_impl.h:75: virtual PP_Resource
CreateHostResolverPrivate(PP_Instance instance) OVERRIDE;
On 2012/02/28 08:29:00, yzshen1 wrote:
> Sort alphabetically, please. (And also the corresponding definitions.)

Done.

[jam, 2012-02-28 16:07:13]

On 2012/02/28 08:14:35, ygorshenin1 wrote:
> On 2012/02/27 21:47:40, John Abd-El-Malek wrote:
> > you listed 4 reviewers. can you specify which parts they should be looking
at?
> 
> sky: chrome/test/*
> brettw, yzshen: content/*, ppapi/*
> jam: webkit/plugins/*

the changes in webkit/plugins are in the ppapi directory, which brettw is an
owner of, so I'll let him review it. Note that it's generally better to pick
reviewers that are closest to the directory that you're modifying.

[brettw, 2012-02-28 17:02:50]

You didn't register your interface for the proxy so it won't appear as a
supported interface to a proxied plugin. If you add the stuff to
interfaces_ppb_private like I describe below, this will happen automatically.

However, this does indicate that you haven't tested the proxy support. We should
be sure to get some automated test for this interface running in and out of
process.

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:341: if (strcmp(name,
PPB_HOSTRESOLVER_PRIVATE_INTERFACE_0_1) == 0)
Instead of hardcoding this, you should add a mapping in
ppapi/thunk/interfaces_ppb_private.h (registed your API and IFACE there like the
TCP/UDP stuff).

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/ppb_ho...
File webkit/plugins/ppapi/ppb_host_resolver_private_impl.cc (right):

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/ppb_ho...
webkit/plugins/ppapi/ppb_host_resolver_private_impl.cc:15: PP_Instance instance)
: ::ppapi::PPB_HostResolver_Shared(instance) {
Only put initializers at the end of the line if everything fits on one line
(including all the class names & stuff that you have on the previous one). So
here, put the initializer on the next line.

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/ppb_ho...
File webkit/plugins/ppapi/ppb_host_resolver_private_impl.h (right):

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/ppb_ho...
webkit/plugins/ppapi/ppb_host_resolver_private_impl.h:20: const
PP_HostResolver_Private_Hint* hint);
Can you append OVERRIDE here?

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/resour...
File webkit/plugins/ppapi/resource_creation_impl.cc (right):

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/resour...
webkit/plugins/ppapi/resource_creation_impl.cc:185: return
PPB_HostResolver_Private_Impl::CreateResource(instance);
Can you do one of the following things here?

- If I wrote this, I would have used the form like CreateFlashNetConnector above
and skipped the extra static function. Since your resource can't fail creating,
this saves some code. The places that use Create here can all fail in various
ways and return 0.

- If you want to keep the static factory function, can you call it just "Create"
like the other classes here?

[sky, 2012-02-28 20:39:29]

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

http://codereview.chromium.org/9455092/diff/19047/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:416:
TEST_PPAPI_IN_PROCESS_VIA_HTTP(MAYBE_HostResolverPrivate_ResolveIPV6)
How come you're checking in a disabled test?

[ygorshenin1, 2012-02-29 14:02:36]

Thanks, PTAL.

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

http://codereview.chromium.org/9455092/diff/19047/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:416:
TEST_PPAPI_IN_PROCESS_VIA_HTTP(MAYBE_HostResolverPrivate_ResolveIPV6)
On 2012/02/28 20:39:29, sky wrote:
> How come you're checking in a disabled test?

By unknown reason IPV6 tests fail on trybots. OK, I'll delete this tests.

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:341: if (strcmp(name,
PPB_HOSTRESOLVER_PRIVATE_INTERFACE_0_1) == 0)
On 2012/02/28 17:02:50, brettw wrote:
> Instead of hardcoding this, you should add a mapping in
> ppapi/thunk/interfaces_ppb_private.h (registed your API and IFACE there like
the
> TCP/UDP stuff).

Done.

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/ppb_ho...
File webkit/plugins/ppapi/ppb_host_resolver_private_impl.cc (right):

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/ppb_ho...
webkit/plugins/ppapi/ppb_host_resolver_private_impl.cc:15: PP_Instance instance)
: ::ppapi::PPB_HostResolver_Shared(instance) {
On 2012/02/28 17:02:50, brettw wrote:
> Only put initializers at the end of the line if everything fits on one line
> (including all the class names & stuff that you have on the previous one). So
> here, put the initializer on the next line.

Done.

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/ppb_ho...
File webkit/plugins/ppapi/ppb_host_resolver_private_impl.h (right):

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/ppb_ho...
webkit/plugins/ppapi/ppb_host_resolver_private_impl.h:20: const
PP_HostResolver_Private_Hint* hint);
On 2012/02/28 17:02:50, brettw wrote:
> Can you append OVERRIDE here?

Done.

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/resour...
File webkit/plugins/ppapi/resource_creation_impl.cc (right):

http://codereview.chromium.org/9455092/diff/19047/webkit/plugins/ppapi/resour...
webkit/plugins/ppapi/resource_creation_impl.cc:185: return
PPB_HostResolver_Private_Impl::CreateResource(instance);
Thanks, first way is better.

On 2012/02/28 17:02:50, brettw wrote:
> Can you do one of the following things here?
> 
> - If I wrote this, I would have used the form like CreateFlashNetConnector
above
> and skipped the extra static function. Since your resource can't fail
creating,
> this saves some code. The places that use Create here can all fail in various
> ways and return 0.
> 
> - If you want to keep the static factory function, can you call it just
"Create"
> like the other classes here?

[ygorshenin1, 2012-02-29 14:06:18]

http://codereview.chromium.org/9455092/diff/25001/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_host_resolver_shared.h (right):

http://codereview.chromium.org/9455092/diff/25001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_host_resolver_shared.h:27: struct HostPortPair {
I added this class to combine host and port into single entity, because I don't
want to deal with implementation of IPC_MESSAGE_ROUTED6.

[sky, 2012-02-29 16:08:42]

chrome/test LGTM

[yzshen1, 2012-02-29 19:11:49]

http://codereview.chromium.org/9455092/diff/32045/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9455092/diff/32045/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:139:
IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBHostResolver_Resolve,
wrong indent.

http://codereview.chromium.org/9455092/diff/32045/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:281:
OnConnectTcpBoundInfo* bound_info) {
nit: it is better to pass const OnConnectTcpBoundInfo&.

http://codereview.chromium.org/9455092/diff/32045/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:633:
CanUseSocketAPIs(routing_id),
wrong indent.

http://codereview.chromium.org/9455092/diff/32045/ppapi/proxy/ppapi_param_tra...
File ppapi/proxy/ppapi_param_traits.cc (right):

http://codereview.chromium.org/9455092/diff/32045/ppapi/proxy/ppapi_param_tra...
ppapi/proxy/ppapi_param_traits.cc:466: // NameAndAddressPair
----------------------------------------------------------
Is it possible to use IPC_STRUCT_TRAITS_*? And I thought we won't need to handle
NetworkList explicitly, since it is just std::vector<NameAndAddressPair>?

http://codereview.chromium.org/9455092/diff/32045/ppapi/proxy/ppb_host_resolv...
File ppapi/proxy/ppb_host_resolver_private_proxy.cc (right):

http://codereview.chromium.org/9455092/diff/32045/ppapi/proxy/ppb_host_resolv...
ppapi/proxy/ppb_host_resolver_private_proxy.cc:50:
(*g_id_to_host_resolver)[host_resolver_id_] = this;
Should you remove entries somewhere?

[ygorshenin1, 2012-03-01 09:37:34]

PTAL

http://codereview.chromium.org/9455092/diff/32045/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9455092/diff/32045/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:139:
IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBHostResolver_Resolve,
On 2012/02/29 19:11:49, yzshen1 wrote:
> wrong indent.

Done.

http://codereview.chromium.org/9455092/diff/32045/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:281:
OnConnectTcpBoundInfo* bound_info) {
On 2012/02/29 19:11:49, yzshen1 wrote:
> nit: it is better to pass const OnConnectTcpBoundInfo&.

Done.

http://codereview.chromium.org/9455092/diff/32045/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:633:
CanUseSocketAPIs(routing_id),
On 2012/02/29 19:11:49, yzshen1 wrote:
> wrong indent.

Done.

http://codereview.chromium.org/9455092/diff/32045/ppapi/proxy/ppapi_param_tra...
File ppapi/proxy/ppapi_param_traits.cc (right):

http://codereview.chromium.org/9455092/diff/32045/ppapi/proxy/ppapi_param_tra...
ppapi/proxy/ppapi_param_traits.cc:466: // NameAndAddressPair
----------------------------------------------------------
Thanks! This is greatly simplified the code.

On 2012/02/29 19:11:49, yzshen1 wrote:
> Is it possible to use IPC_STRUCT_TRAITS_*? And I thought we won't need to
handle
> NetworkList explicitly, since it is just std::vector<NameAndAddressPair>?

http://codereview.chromium.org/9455092/diff/32045/ppapi/proxy/ppb_host_resolv...
File ppapi/proxy/ppb_host_resolver_private_proxy.cc (right):

http://codereview.chromium.org/9455092/diff/32045/ppapi/proxy/ppb_host_resolv...
ppapi/proxy/ppb_host_resolver_private_proxy.cc:50:
(*g_id_to_host_resolver)[host_resolver_id_] = this;
Thanks for pointing this out. I added cleanup code to destructor.

On 2012/02/29 19:11:49, yzshen1 wrote:
> Should you remove entries somewhere?

[ygorshenin1, 2012-03-01 10:58:33]

I propose to add "GetCanonicalName" method and simplify "GetItem" method in
PPB_HostResolver_Private interface. New "Getitem" method is called
"GetNetAddress" and stores only i-th network address.

The reason is that getaddrinfo stores canonical name only in the head of
returned addrinfo list, in the other nodes ai_canonname fields are NULLs. So,
it's misleading users that canonical names can be different.

[yzshen1, 2012-03-06 08:07:46]

lgtm

http://codereview.chromium.org/9455092/diff/45001/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_host_resolver_shared.cc (right):

http://codereview.chromium.org/9455092/diff/45001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_host_resolver_shared.cc:24: ai->ai_addr,
nit: when calling a method/function, the style guide doesn't require to put each
argument on a separate line. You could arrange them to be more compact (and
still clear to read).
For example:
if (!NetAddressPrivateImpl::SockaddrToNetAddress(
        ai->ai_addr, ai->ai_addrlen, &address)) {

http://codereview.chromium.org/9455092/diff/45001/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_host_resolver_shared.h (right):

http://codereview.chromium.org/9455092/diff/45001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_host_resolver_shared.h:27: typedef
std::vector<PP_NetAddress_Private> NetworkList;
Now that it is simply a list of PP_NetAddress_Private, NetAddressList may be
more clear. (Or you could directly use std::vector<blah> instead of doing the
typedef.)

[ygorshenin1, 2012-03-06 10:38:17]

Many thanks, PTAL.

https://chromiumcodereview.appspot.com/9455092/diff/45001/ppapi/shared_impl/p...
File ppapi/shared_impl/private/ppb_host_resolver_shared.cc (right):

https://chromiumcodereview.appspot.com/9455092/diff/45001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_host_resolver_shared.cc:24: ai->ai_addr,
On 2012/03/06 08:07:46, yzshen1 wrote:
> nit: when calling a method/function, the style guide doesn't require to put
each
> argument on a separate line. You could arrange them to be more compact (and
> still clear to read).
> For example:
> if (!NetAddressPrivateImpl::SockaddrToNetAddress(
>         ai->ai_addr, ai->ai_addrlen, &address)) {

Done.

https://chromiumcodereview.appspot.com/9455092/diff/45001/ppapi/shared_impl/p...
File ppapi/shared_impl/private/ppb_host_resolver_shared.h (right):

https://chromiumcodereview.appspot.com/9455092/diff/45001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_host_resolver_shared.h:27: typedef
std::vector<PP_NetAddress_Private> NetworkList;
On 2012/03/06 08:07:46, yzshen1 wrote:
> Now that it is simply a list of PP_NetAddress_Private, NetAddressList may be
> more clear. (Or you could directly use std::vector<blah> instead of doing the
> typedef.)

Done.

[brettw, 2012-03-06 17:16:07]

Thanks for the tests! We should hook them up to run automatically on the
buildbots.

One problem with the test is that we've had bad luck accessing the internet from
the tests. It runs so often that random internet problems cause tests to go red,
so we try hard to avoid it.

Fortunately, there's an easy way around this for you. You can copy what the
test_url_loader.cc does, which runs our local test server. You can see in
GetReachableAbsoluteURL it grabs the document URL which will contain the host
and port you can use to access the test server. Unfortunately, there isn't an
easy way to parse URLs in the test environment, so you may have to parse it
yourself (sorry, just from the first double slash to the colon should be good
for the host and from the first colon to the following slash should be good for
the port).

On
http://dev.chromium.org/developers/design-documents/pepper-plugin-implementation
at the bottom there are instructions for manually running tests over the test
HTTP server. I'd try to get that running with url_loader to validate your setup
first.

Then to hook them up for the buildbots, just add this to
chrome/test/ui/ppapi_uitest.cc
TEST_PPAPI_IN_PROCESS_VIA_HTTP(HostResolverProvate)
TEST_PPAPI_OUT_OF_PROCESS_VIA_HTTP(HostResolverProvate)
to run the tests, I just do:
ui_tests --gtest_filter="*PPAPI*"

[Dmitry Polukhin, 2012-03-07 06:56:56]

Eh :( we know that test shouldn't relay on internet access from bots. But
it is very hard to test DNS resolution without internet. I think using
use test_url_loader.cc
approach it will be possible to test only very limited use-case (localhost
resolution) because HTTP server is used only to verify that HostResolver
gave something that can be used with TCPSockets. To really test
HostResolver without internet we need to setup fake DSN server on localhost
and temporary replace /etc/resolv.conf to redirect system to this fake DNS
server. I think we can mark this test as flaky from the beginning to don't
make tree red if these test fails due to temporary internet problems on
bots.

On Tue, Mar 6, 2012 at 9:16 PM, <brettw@chromium.org> wrote:

> Thanks for the tests! We should hook them up to run automatically on the
> buildbots.
>
> One problem with the test is that we've had bad luck accessing the
> internet from
> the tests. It runs so often that random internet problems cause tests to
> go red,
> so we try hard to avoid it.
>
> Fortunately, there's an easy way around this for you. You can copy what the
> test_url_loader.cc does, which runs our local test server. You can see in
> GetReachableAbsoluteURL it grabs the document URL which will contain the
> host
> and port you can use to access the test server. Unfortunately, there isn't
> an
> easy way to parse URLs in the test environment, so you may have to parse it
> yourself (sorry, just from the first double slash to the colon should be
> good
> for the host and from the first colon to the following slash should be
> good for
> the port).
>
> On
> http://dev.chromium.org/**developers/design-documents/**
>
pepper-plugin-implementation<http://dev.chromium.org/developers/design-documents/pepper-plugin-implementation>
> at the bottom there are instructions for manually running tests over the
> test
> HTTP server. I'd try to get that running with url_loader to validate your
> setup
> first.
>
> Then to hook them up for the buildbots, just add this to
> chrome/test/ui/ppapi_uitest.cc
> TEST_PPAPI_IN_PROCESS_VIA_**HTTP(HostResolverProvate)
> TEST_PPAPI_OUT_OF_PROCESS_VIA_**HTTP(HostResolverProvate)
> to run the tests, I just do:
> ui_tests --gtest_filter="*PPAPI*"
>
>
https://chromiumcodereview.**appspot.com/9455092/<https://chromiumcodereview....
>

[ygorshenin1, 2012-03-07 11:52:55]

It's not difficult to change tests to use local host and port instead of
www.google.com (GetLocalHostPort method is very helpful), but in this case
Resolve method returns only single PP_NetAddress_Private. If this is acceptable,
I'll change the tests in this way.

On 2012/03/07 06:56:56, Dmitry Polukhin wrote:
> Eh :( we know that test shouldn't relay on internet access from bots. But
> it is very hard to test DNS resolution without internet. I think using
> use test_url_loader.cc
> approach it will be possible to test only very limited use-case (localhost
> resolution) because HTTP server is used only to verify that HostResolver
> gave something that can be used with TCPSockets. To really test
> HostResolver without internet we need to setup fake DSN server on localhost
> and temporary replace /etc/resolv.conf to redirect system to this fake DNS
> server. I think we can mark this test as flaky from the beginning to don't
> make tree red if these test fails due to temporary internet problems on
> bots.
> 
> On Tue, Mar 6, 2012 at 9:16 PM, <mailto:brettw@chromium.org> wrote:
> 
> > Thanks for the tests! We should hook them up to run automatically on the
> > buildbots.
> >
> > One problem with the test is that we've had bad luck accessing the
> > internet from
> > the tests. It runs so often that random internet problems cause tests to
> > go red,
> > so we try hard to avoid it.
> >
> > Fortunately, there's an easy way around this for you. You can copy what the
> > test_url_loader.cc does, which runs our local test server. You can see in
> > GetReachableAbsoluteURL it grabs the document URL which will contain the
> > host
> > and port you can use to access the test server. Unfortunately, there isn't
> > an
> > easy way to parse URLs in the test environment, so you may have to parse it
> > yourself (sorry, just from the first double slash to the colon should be
> > good
> > for the host and from the first colon to the following slash should be
> > good for
> > the port).
> >
> > On
> > http://dev.chromium.org/**developers/design-documents/**
> >
>
pepper-plugin-implementation<http://dev.chromium.org/developers/design-documents/pepper-plugin-implementation>
> > at the bottom there are instructions for manually running tests over the
> > test
> > HTTP server. I'd try to get that running with url_loader to validate your
> > setup
> > first.
> >
> > Then to hook them up for the buildbots, just add this to
> > chrome/test/ui/ppapi_uitest.cc
> > TEST_PPAPI_IN_PROCESS_VIA_**HTTP(HostResolverProvate)
> > TEST_PPAPI_OUT_OF_PROCESS_VIA_**HTTP(HostResolverProvate)
> > to run the tests, I just do:
> > ui_tests --gtest_filter="*PPAPI*"
> >
> >
>
https://chromiumcodereview.**appspot.com/9455092/%3Chttps://chromiumcoderevie...>
> >

[brettw, 2012-03-08 05:43:31]

Yeah, I think local is good enough. Otherwise this test will just get disabled
and we won't have any coverage.

[ygorshenin1, 2012-03-11 08:13:33]

Test for HostResolver now sends requests for local host and port. PTAL.

[brettw, 2012-03-11 18:16:12]

thanks, LGTM

[ygorshenin1, 2012-03-12 08:05:06]

+ darin

Need an OWNER's LGTM for webkit/*.

[ygorshenin1, 2012-03-14 09:08:20]

+ tony

Need an OWNER's LGTM for webkit/.

[tony, 2012-03-14 19:12:15]

LGTM

http://codereview.chromium.org/9455092/diff/75001/webkit/plugins/ppapi/ppb_ho...
File webkit/plugins/ppapi/ppb_host_resolver_private_impl.h (right):

http://codereview.chromium.org/9455092/diff/75001/webkit/plugins/ppapi/ppb_ho...
webkit/plugins/ppapi/ppb_host_resolver_private_impl.h:16: explicit
PPB_HostResolver_Private_Impl(PP_Instance instance);
Nit: Can we explicitly include the header that defines PP_Instance (I didn't see
it in ppb_host_resolver_shared.h)?

[ygorshenin1, 2012-03-14 20:15:06]

Many thanks!

http://codereview.chromium.org/9455092/diff/75001/webkit/plugins/ppapi/ppb_ho...
File webkit/plugins/ppapi/ppb_host_resolver_private_impl.h (right):

http://codereview.chromium.org/9455092/diff/75001/webkit/plugins/ppapi/ppb_ho...
webkit/plugins/ppapi/ppb_host_resolver_private_impl.h:16: explicit
PPB_HostResolver_Private_Impl(PP_Instance instance);
For why? PPB_HostResolver_Shared implements PPB_HostResolver_Private_API, which
includes "ppapi/c/private/ppb_host_resolver_private.h". And this one includes
all necessary headers.

On 2012/03/14 19:12:15, tony wrote:
> Nit: Can we explicitly include the header that defines PP_Instance (I didn't
see
> it in ppb_host_resolver_shared.h)?

[commit-bot: I haz the power, 2012-03-14 20:19:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/9455092/75001

[tony, 2012-03-14 20:52:15]

On 2012/03/14 20:15:06, ygorshenin1 wrote:
>
http://codereview.chromium.org/9455092/diff/75001/webkit/plugins/ppapi/ppb_ho...
> File webkit/plugins/ppapi/ppb_host_resolver_private_impl.h (right):
> 
>
http://codereview.chromium.org/9455092/diff/75001/webkit/plugins/ppapi/ppb_ho...
> webkit/plugins/ppapi/ppb_host_resolver_private_impl.h:16: explicit
> PPB_HostResolver_Private_Impl(PP_Instance instance);
> For why? PPB_HostResolver_Shared implements PPB_HostResolver_Private_API,
which
> includes "ppapi/c/private/ppb_host_resolver_private.h". And this one includes
> all necessary headers.

It makes it easier to refactor ppapi/c/private/ppb_host_resolver_private.h. 
Anyway, not a big deal either way.

[commit-bot: I haz the power, 2012-03-15 01:21:29]

Change committed as 126813