PPB_HostResolver_Private is switched to the new Pepper proxy.

PPB_HostResolver_Private is switched to the new Pepper proxy.

Blocked on https://chromiumcodereview.appspot.com/11434042.

BUG=163861
TEST=browser_tests:*HostResolverPrivate*

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

[brettw, 2012-12-06 00:05:37]

Yuzhu, Raymes, can one of you take this review? Please don't both do it at the
same time :)

[yzshen1, 2012-12-06 00:13:38]

I could do it.

On Wed, Dec 5, 2012 at 4:05 PM, <brettw@chromium.org> wrote:

> Yuzhu, Raymes, can one of you take this review? Please don't both do it at
> the
> same time :)
>
>
https://chromiumcodereview.**appspot.com/11411357/<https://chromiumcodereview...
>

[yzshen1, 2012-12-06 21:45:08]

A high-level questions: why do you need to keep the old implementation for
in-process mode? If you have a reason not that delete that part, I would suggest
to postpone switching it to the new host/proxy architecture. Keeping two
implementations is even more confusing?

[brettw, 2012-12-06 22:20:44]

I think when we first discussed this, the proxies hadn't really been switched
and we needed the in-process one to keep nacl working.

Since we keep having freak-outs about proxy switches and this is shaping up to
be a rather complex release, I'm now preferring to keep anything big and new
from landing until after the branch point (Dec 17th) unless there's a reason we
need it for M25.

Once we hit that (and are still using the IPC proxy) then indeed we can just
remove the in-process one altogether.



One thing I noticed: I think this is necessary for the SSH app. We don't give
nacl apps the "Private" permission bit. For the things we needed for the SSH
app, we set up this crazy whitelist of APIs. For this one, there's a comment in
the code about it being public since it's not dangerous to expose to malicious
plugins, we just don't want people depending on it for now. So I'd probably move
it in the _factory.cc file to be public.

[ygorshenin1, 2012-12-20 08:55:49]

friendly ping

[yzshen1, 2012-12-20 20:00:20]

The first round of review comments. Haven't reviewed
browser_ppapi_host/*_resource/*_host in details. I will do that after a few
high-level review comments are addressed.

Thanks!

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:49:
if (host_->GetPpapiHost()->permissions().HasPermission(
Now that there is a GetPermissions() method, please use it.

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:51:
message.type() == PpapiHostMsg_HostResolverPrivate_Create::ID) {
Adding a comment about why we need an '||' here would be good. Or we could
separate privates interfaces and those don't require PERMISSION_PRIVATE.

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_host_resolver_private_shared.h
(right):

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_shared.h:29:
class PepperHostResolverPrivateShared {
What not merging this into the host?

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_message_filter.cc:41: #include
"ppapi/c/private/ppb_host_resolver_private.h"
Do we still need this?

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_message_filter.cc:46: #include
"ppapi/shared_impl/private/ppb_host_resolver_shared.h"
Do we still need this?

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_message_filter.h (left):

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_message_filter.h:31: struct
PP_NetAddress_Private;
nit: it seems okay to keep this forward declaration, according to "include what
you use"?

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_message_filter.h (right):

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_message_filter.h:20: #include
"net/base/host_resolver.h"
nit: you could use a forward declaration, because only pointer/reference is used
in the .h file.

https://codereview.chromium.org/11411357/diff/29001/content/public/browser/br...
File content/public/browser/browser_ppapi_host.h (right):

https://codereview.chromium.org/11411357/diff/29001/content/public/browser/br...
content/public/browser/browser_ppapi_host.h:105: bool
PostOnUIThreadWithRenderProcessAndReply(
Is it possible to be done using ResourceMessageFilter?

https://codereview.chromium.org/11411357/diff/29001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/11411357/diff/29001/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:89: return
kInvalidResource;
I personally don't think this is necessary, because we use 0 almost everywhere.
If you feel like to have a local constant name for it, please also change line
58.

https://codereview.chromium.org/11411357/diff/29001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/11411357/diff/29001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1181: // HostResolverPrivate.
There is a section called "Resource call/reply messages." at the bottom.
All messages for new design should be moved there.

https://codereview.chromium.org/11411357/diff/29001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1187: bool /* succeeded */,
You could use the 'result' field in ResourceMessageReplyParams instead.

This ResourceMessageReplyParams will be sent back along with your ResolveReply.

https://codereview.chromium.org/11411357/diff/29001/ppapi/shared_impl/private...
File ppapi/shared_impl/private/ppb_host_resolver_shared.h (right):

https://codereview.chromium.org/11411357/diff/29001/ppapi/shared_impl/private...
ppapi/shared_impl/private/ppb_host_resolver_shared.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
I think this file and the corresponding .cc file are not needed any more, right?

https://codereview.chromium.org/11411357/diff/29001/webkit/plugins/ppapi/ppb_...
File webkit/plugins/ppapi/ppb_host_resolver_private_impl.h (left):

https://codereview.chromium.org/11411357/diff/29001/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_host_resolver_private_impl.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
Weird that this and the .cc file are showed as 'M' (modified) instead of 'D'
(deleted).

[ygorshenin1, 2012-12-21 12:53:26]

PTAL

Added HostResolverPrivate permission check on host side and corresponding
browser test. Still thinking how to use ResourceMessageFilter.

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:49:
if (host_->GetPpapiHost()->permissions().HasPermission(
On 2012/12/20 20:00:20, yzshen1 wrote:
> Now that there is a GetPermissions() method, please use it.

Done.

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:51:
message.type() == PpapiHostMsg_HostResolverPrivate_Create::ID) {
Done.

I added code that checks for a plugin that it can use socket API.

On 2012/12/20 20:00:20, yzshen1 wrote:
> Adding a comment about why we need an '||' here would be good. Or we could
> separate privates interfaces and those don't require PERMISSION_PRIVATE.

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_host_resolver_private_shared.h
(right):

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_shared.h:29:
class PepperHostResolverPrivateShared {
On 2012/12/20 20:00:20, yzshen1 wrote:
> What not merging this into the host?

Done.

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_message_filter.cc:41: #include
"ppapi/c/private/ppb_host_resolver_private.h"
On 2012/12/20 20:00:20, yzshen1 wrote:
> Do we still need this?

Done.

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_message_filter.cc:46: #include
"ppapi/shared_impl/private/ppb_host_resolver_shared.h"
On 2012/12/20 20:00:20, yzshen1 wrote:
> Do we still need this?

Done.

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_message_filter.h (left):

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_message_filter.h:31: struct
PP_NetAddress_Private;
On 2012/12/20 20:00:20, yzshen1 wrote:
> nit: it seems okay to keep this forward declaration, according to "include
what
> you use"?

Done.

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_message_filter.h (right):

https://codereview.chromium.org/11411357/diff/29001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_message_filter.h:20: #include
"net/base/host_resolver.h"
On 2012/12/20 20:00:20, yzshen1 wrote:
> nit: you could use a forward declaration, because only pointer/reference is
used
> in the .h file.

Done.

https://codereview.chromium.org/11411357/diff/29001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/11411357/diff/29001/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:89: return
kInvalidResource;
Done.

I prefer to use symbolic names instead of numeric constants, even if these
constants are well known.

On 2012/12/20 20:00:20, yzshen1 wrote:
> I personally don't think this is necessary, because we use 0 almost
everywhere.
> If you feel like to have a local constant name for it, please also change line
> 58.

https://codereview.chromium.org/11411357/diff/29001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/11411357/diff/29001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1181: // HostResolverPrivate.
On 2012/12/20 20:00:20, yzshen1 wrote:
> There is a section called "Resource call/reply messages." at the bottom.
> All messages for new design should be moved there.

Done.

https://codereview.chromium.org/11411357/diff/29001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1187: bool /* succeeded */,
On 2012/12/20 20:00:20, yzshen1 wrote:
> You could use the 'result' field in ResourceMessageReplyParams instead.
> 
> This ResourceMessageReplyParams will be sent back along with your
ResolveReply.

Done.

https://codereview.chromium.org/11411357/diff/29001/ppapi/shared_impl/private...
File ppapi/shared_impl/private/ppb_host_resolver_shared.h (right):

https://codereview.chromium.org/11411357/diff/29001/ppapi/shared_impl/private...
ppapi/shared_impl/private/ppb_host_resolver_shared.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/12/20 20:00:20, yzshen1 wrote:
> I think this file and the corresponding .cc file are not needed any more,
right?

Done.

https://codereview.chromium.org/11411357/diff/29001/webkit/plugins/ppapi/ppb_...
File webkit/plugins/ppapi/ppb_host_resolver_private_impl.h (left):

https://codereview.chromium.org/11411357/diff/29001/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_host_resolver_private_impl.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
It's really weird because I deleted these files in local branch.

On 2012/12/20 20:00:20, yzshen1 wrote:
> Weird that this and the .cc file are showed as 'M' (modified) instead of 'D'
> (deleted).

[ygorshenin1, 2012-12-24 14:41:06]

PTAL.

I merged PPB_UDPSocket_Private to UDPSocketPrivateResource and
PepperUDPSocketPrivateShared to PepperUDPSocketPrivateResource (to be consistent
with https://codereview.chromium.org/11411357/).

[ygorshenin1, 2012-12-24 14:42:19]

Sorry for this irrelevant comment.

On 2012/12/24 14:41:06, ygorshenin1 wrote:
> PTAL.
> 
> I merged PPB_UDPSocket_Private to UDPSocketPrivateResource and
> PepperUDPSocketPrivateShared to PepperUDPSocketPrivateResource (to be
consistent
> with https://codereview.chromium.org/11411357/).

[ygorshenin1, 2013-01-21 17:12:12]

PTAL

[ygorshenin1, 2013-01-22 10:41:12]

friendly ping

[yzshen1, 2013-01-24 18:45:03]

Haven't reviewed pepper_socket_utils.h.
I will wait till you have considered the suggestion ofusing
ResourceMessageFilter. (Please see below.)

Thanks!

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_host_resolver_private_host.cc
(right):

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_host.cc:130:
host_resolver_is_ready_ = true;
nit: |host_resolver_is_ready_| is not very accurate. It sounds like
|host_resolver_| is guaranteed to be valid (which is not true).

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_host.cc:135:
int32_t PepperHostResolverPrivateHost::OnMsgResolve(
Now that we have to jump to the UI thread to:
- get the ResourceContext;
- check permission.

How about using a ResourceMessageFilter to handle Resolve messages directly on
the UI thread, on which we will retrive resource context and check permission,
and then we jump to the IO thread to do the actual host resolution?
That will have less jumps, and we don't need things like
|host_resolver_is_ready|.

What do you think?

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_host_resolver_private_host.h
(right):

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_host.h:51:
private:
nit: wrong indent.

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_host.h:52:
typedef ppapi::host::ReplyMessageContext ReplyMessageContext;
You could use a 'using' in the .cc file instead of typedef like this.

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_host.h:56:
void SetHostResolver(ResourceContext* resource_context);
It would be good to comment on which thread each method runs on and which thread
each data member is accessed on.

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_host.h:61:
void OnResolve(const ReplyMessageContext& context,
Can you please rename OnResolve() and DoResolve()? They look too similar to
OnMsgResolve().
And please add comments about which thread they are run on, and what they do.

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_host.h:80:
net::HostResolver* host_resolver() const { return host_resolver_; }
I don't see why we need these two private accessors. You could directly use the
data members, right?

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_socket_utils.h (right):

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_socket_utils.h:122: bool
PostOnUIThreadWithRenderProcessHostAndReply(
Is this going to be used by other callers other than host resolver?
If not, we could keep it in the host resolver cc file.

https://codereview.chromium.org/11411357/diff/66002/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/11411357/diff/66002/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:90: PP_Instance
instance) {
nit: This file is for those resources that we actually provide in-process
support. Please move this back to resource_creation_impl.

(It is good to also move CreateUDPSocketPrivate.)

https://codereview.chromium.org/11411357/diff/66002/ppapi/proxy/host_resolver...
File ppapi/proxy/host_resolver_private_resource.cc (right):

https://codereview.chromium.org/11411357/diff/66002/ppapi/proxy/host_resolver...
ppapi/proxy/host_resolver_private_resource.cc:72: if (succeeded) {
Please directly check whether params.result() == PP_OK.

(Don't use 0 to represent PP_OK.)

https://codereview.chromium.org/11411357/diff/66002/ppapi/proxy/host_resolver...
ppapi/proxy/host_resolver_private_resource.cc:79:
resolve_callback_->Run(succeeded ? PP_OK : PP_ERROR_FAILED);
You could directly return params.result().

https://codereview.chromium.org/11411357/diff/66002/ppapi/tests/test_host_res...
File ppapi/tests/test_host_resolver_private_disallowed.cc (right):

https://codereview.chromium.org/11411357/diff/66002/ppapi/tests/test_host_res...
ppapi/tests/test_host_resolver_private_disallowed.cc:32: CheckTestingInterface()
&&
It seems we don't need the testing interface?

[ygorshenin1, 2013-02-05 12:45:31]

Yuzhu, sorry for the delay in response. PTAL.

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_host_resolver_private_host.cc
(right):

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_host.cc:130:
host_resolver_is_ready_ = true;
On 2013/01/24 18:45:03, yzshen1 wrote:
> nit: |host_resolver_is_ready_| is not very accurate. It sounds like
> |host_resolver_| is guaranteed to be valid (which is not true).

Done.

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_host.cc:135:
int32_t PepperHostResolverPrivateHost::OnMsgResolve(
Great idea. Code is much cleaner now. Thanks!

On 2013/01/24 18:45:03, yzshen1 wrote:
> Now that we have to jump to the UI thread to:
> - get the ResourceContext;
> - check permission.
> 
> How about using a ResourceMessageFilter to handle Resolve messages directly on
> the UI thread, on which we will retrive resource context and check permission,
> and then we jump to the IO thread to do the actual host resolution?
> That will have less jumps, and we don't need things like
> |host_resolver_is_ready|.
> 
> What do you think?

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_host_resolver_private_host.h
(right):

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_host.h:51:
private:
On 2013/01/24 18:45:03, yzshen1 wrote:
> nit: wrong indent.

Done.

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_host.h:52:
typedef ppapi::host::ReplyMessageContext ReplyMessageContext;
On 2013/01/24 18:45:03, yzshen1 wrote:
> You could use a 'using' in the .cc file instead of typedef like this.

Done.

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_host.h:56:
void SetHostResolver(ResourceContext* resource_context);
On 2013/01/24 18:45:03, yzshen1 wrote:
> It would be good to comment on which thread each method runs on and which
thread
> each data member is accessed on.

Done.

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_host.h:61:
void OnResolve(const ReplyMessageContext& context,
On 2013/01/24 18:45:03, yzshen1 wrote:
> Can you please rename OnResolve() and DoResolve()? They look too similar to
> OnMsgResolve().
> And please add comments about which thread they are run on, and what they do.

Done.

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_host.h:80:
net::HostResolver* host_resolver() const { return host_resolver_; }
On 2013/01/24 18:45:03, yzshen1 wrote:
> I don't see why we need these two private accessors. You could directly use
the
> data members, right?

Done.

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_socket_utils.h (right):

https://codereview.chromium.org/11411357/diff/66002/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_socket_utils.h:122: bool
PostOnUIThreadWithRenderProcessHostAndReply(
PostOnUIThreadWithRenderProcessHostAndReply is needed to get HostResolver from
RenderProcessHost -> BrowserContext -> ResourceContext.

It'll be needed when we switch TCPSocketPrivate to the new proxy, because
TCPSocketPrivate uses HostResolver
(https://cs.corp.google.com/#chrome/src/content/browser/renderer_host/pepper/p...).


On 2013/01/24 18:45:03, yzshen1 wrote:
> Is this going to be used by other callers other than host resolver?
> If not, we could keep it in the host resolver cc file.

https://codereview.chromium.org/11411357/diff/66002/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/11411357/diff/66002/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:90: PP_Instance
instance) {
On 2013/01/24 18:45:03, yzshen1 wrote:
> nit: This file is for those resources that we actually provide in-process
> support. Please move this back to resource_creation_impl.
> 
> (It is good to also move CreateUDPSocketPrivate.)

Done.

https://codereview.chromium.org/11411357/diff/66002/ppapi/proxy/host_resolver...
File ppapi/proxy/host_resolver_private_resource.cc (right):

https://codereview.chromium.org/11411357/diff/66002/ppapi/proxy/host_resolver...
ppapi/proxy/host_resolver_private_resource.cc:72: if (succeeded) {
On 2013/01/24 18:45:03, yzshen1 wrote:
> Please directly check whether params.result() == PP_OK.
> 
> (Don't use 0 to represent PP_OK.)

Done.

https://codereview.chromium.org/11411357/diff/66002/ppapi/proxy/host_resolver...
ppapi/proxy/host_resolver_private_resource.cc:79:
resolve_callback_->Run(succeeded ? PP_OK : PP_ERROR_FAILED);
On 2013/01/24 18:45:03, yzshen1 wrote:
> You could directly return params.result().

Done.

https://codereview.chromium.org/11411357/diff/66002/ppapi/tests/test_host_res...
File ppapi/tests/test_host_resolver_private_disallowed.cc (right):

https://codereview.chromium.org/11411357/diff/66002/ppapi/tests/test_host_res...
ppapi/tests/test_host_resolver_private_disallowed.cc:32: CheckTestingInterface()
&&
On 2013/01/24 18:45:03, yzshen1 wrote:
> It seems we don't need the testing interface?

Done.

[yzshen1, 2013-02-07 22:28:21]

only a few more nits. Thanks for the message filter change.

https://codereview.chromium.org/11411357/diff/85001/content/browser/renderer_...
File
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.cc
(right):

https://codereview.chromium.org/11411357/diff/85001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.cc:82:
: host_(host),
You don't have to store |host_|, right?

https://codereview.chromium.org/11411357/diff/85001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.cc:83:
instance_(instance),
You don't have to store |instance_|, right?

https://codereview.chromium.org/11411357/diff/85001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.cc:148:
base::Bind(&PepperHostResolverPrivateMessageFilter::DoResolve, this,
Is it intentional that you use "this" here while  weak_factory_.GetWeakPtr() at
line 182?

https://codereview.chromium.org/11411357/diff/85001/content/browser/renderer_...
File
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.h
(right):

https://codereview.chromium.org/11411357/diff/85001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.h:51:
// ppapi::host::ResourceMessageFilter overrides.
wrong indent.

[ygorshenin1, 2013-02-08 08:56:40]

PTAL

https://codereview.chromium.org/11411357/diff/85001/content/browser/renderer_...
File
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.cc
(right):

https://codereview.chromium.org/11411357/diff/85001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.cc:82:
: host_(host),
On 2013/02/07 22:28:21, yzshen1 wrote:
> You don't have to store |host_|, right?

Done.

https://codereview.chromium.org/11411357/diff/85001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.cc:83:
instance_(instance),
On 2013/02/07 22:28:21, yzshen1 wrote:
> You don't have to store |instance_|, right?

Done.

https://codereview.chromium.org/11411357/diff/85001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.cc:148:
base::Bind(&PepperHostResolverPrivateMessageFilter::DoResolve, this,
On 2013/02/07 22:28:21, yzshen1 wrote:
> Is it intentional that you use "this" here while  weak_factory_.GetWeakPtr()
at
> line 182?

Done.

https://codereview.chromium.org/11411357/diff/85001/content/browser/renderer_...
File
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.h
(right):

https://codereview.chromium.org/11411357/diff/85001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.h:51:
// ppapi::host::ResourceMessageFilter overrides.
On 2013/02/07 22:28:21, yzshen1 wrote:
> wrong indent.

Done.

[yzshen1, 2013-02-08 18:41:30]

https://codereview.chromium.org/11411357/diff/77079/content/browser/renderer_...
File
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.h
(right):

https://codereview.chromium.org/11411357/diff/77079/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.h:86:
base::WeakPtr<PepperHostResolverPrivateMessageFilter> weak_ref_;
Why don't you get a weak ptr from |weak_factory_| every time?
What you want to store one like this?

Is it because |weak_factory_| is not thread safe? Then what is the reason to use
|weak_factory_|? Is there any reason that you cannot use |this| in base::Bind()?
(Please note that this class is thread-safe ref-counted.)

[ygorshenin1, 2013-02-11 05:53:48]

PTAL

https://codereview.chromium.org/11411357/diff/77079/content/browser/renderer_...
File
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.h
(right):

https://codereview.chromium.org/11411357/diff/77079/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.h:86:
base::WeakPtr<PepperHostResolverPrivateMessageFilter> weak_ref_;
I're right: as this class is thread-safe and ref-counted, weak_factory isn't
needed there. Thanks!

On 2013/02/08 18:41:30, yzshen1 wrote:
> Why don't you get a weak ptr from |weak_factory_| every time?
> What you want to store one like this?
> 
> Is it because |weak_factory_| is not thread safe? Then what is the reason to
use
> |weak_factory_|? Is there any reason that you cannot use |this| in
base::Bind()?
> (Please note that this class is thread-safe ref-counted.)

[yzshen1, 2013-02-11 19:10:45]

LGTM

Thanks!

[ygorshenin1, 2013-02-11 19:29:42]

谢谢 Yuzhu!

[ygorshenin1, 2013-02-11 19:32:44]

+ Justin

Justin, could you please take a look at ppapi/proxy/ppapi_messages.h
Brett, could you please give an OWNER's lgtm for content/* and webkit/*?

[brettw, 2013-02-11 20:46:58]

LGTM rubberstamp, just noted a few minor nits.

https://codereview.chromium.org/11411357/diff/83013/content/browser/renderer_...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/11411357/diff/83013/content/browser/renderer_...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:83:
if (message.type() == PpapiHostMsg_HostResolverPrivate_Create::ID) {
Can you just group this with the UDPSocket one below and only have the comment
about permissions once?

https://codereview.chromium.org/11411357/diff/83013/content/browser/renderer_...
File
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.cc
(right):

https://codereview.chromium.org/11411357/diff/83013/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.cc:214:
SendResolveReply(PP_ERROR_FAILED, "", NetAddressList(), context);
Can you use std::string() instead of ""?

[ygorshenin1, 2013-02-11 21:28:53]

PTAL

https://codereview.chromium.org/11411357/diff/83013/content/browser/renderer_...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/11411357/diff/83013/content/browser/renderer_...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:83:
if (message.type() == PpapiHostMsg_HostResolverPrivate_Create::ID) {
On 2013/02/11 20:46:58, brettw wrote:
> Can you just group this with the UDPSocket one below and only have the comment
> about permissions once?

Done.

https://codereview.chromium.org/11411357/diff/83013/content/browser/renderer_...
File
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.cc
(right):

https://codereview.chromium.org/11411357/diff/83013/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_host_resolver_private_message_filter.cc:214:
SendResolveReply(PP_ERROR_FAILED, "", NetAddressList(), context);
On 2013/02/11 20:46:58, brettw wrote:
> Can you use std::string() instead of ""?

Done.

[jschuh, 2013-02-12 01:26:06]

This looks like it's just refactoring to use the proxy without changing the
attack surface or other aspects of the implementation, so lgtm (assuming that's
accurate).

[ygorshenin1, 2013-02-12 08:01:42]

Many thanks!

[commit-bot: I haz the power, 2013-02-12 08:02:54]

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

[commit-bot: I haz the power, 2013-02-12 09:01:05]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-12 10:12:09]

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

[commit-bot: I haz the power, 2013-02-12 11:12:26]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-12 11:43:18]

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

[commit-bot: I haz the power, 2013-02-12 12:50:50]

Change committed as 181908