Simplify PPB_NetworkMonitor proxy.

Simplify PPB_NetworkMonitor proxy.

The new proxy is based on ppapi::proxy::PluginResource and
ppapi::host::ResourceHost which simplifies code significantly. Also
the permission check is consistent with socket APIs now.

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

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

R=brettw@chromium.org, yzshen@chromium.org

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

[Sergey Ulanov, 2013-09-11 18:43:20]

yzshen: ping!

[yzshen1, 2013-09-11 22:51:26]

https://codereview.chromium.org/23819033/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (left):

https://codereview.chromium.org/23819033/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_message_filter.cc:130:
ppapi::API_ID_PPB_NETWORKMANAGER_PRIVATE, *it, *list_copy));
Please also remove the definition of the ID from api_id.h

https://codereview.chromium.org/23819033/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

https://codereview.chromium.org/23819033/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_message_filter.cc:7: #include
"base/bind.h"
Please remove those .h files that are not needed.

https://codereview.chromium.org/23819033/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_monitor_host.cc
(right):

https://codereview.chromium.org/23819033/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_monitor_host.cc:27:
external_plugin, true /* private_api */,
wrong indent.

https://codereview.chromium.org/23819033/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_monitor_host.cc:45:
reply_context_(context.MakeReplyMessageContext()),
This makes a "reply" for the Create message, which is incorrect. Because the
network list change notification could happen multiple times.
Please remove |reply_context_| and use PpapiHost::SendUnsolicitedReply().

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
File ppapi/proxy/network_monitor_resource.cc (right):

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.cc:19: forbidden_(false) {
Please also init the PP_Resource* member, too.

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.cc:44: PP_Resource* network_list,
wrong indent.

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.cc:55: thunk::EnterResourceCreationNoLock
enter(pp_instance());
You don't need this line, right? |enter| is never used.

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.cc:56: *network_list =
PPB_NetworkList_Private_Shared::Create(
You could also make changes to PPB_NetworkList_Private_Shared. It is not shared
by in-process impl anymore.

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.cc:74: thunk::EnterResourceCreationNoLock
enter(pp_instance());
This line is not needed.

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.cc:83: void
NetworkMonitorResource::OnPluginMsgForbidden(
This message is not needed, |params| has a result field. You could reuse
PpapiMsg_NetworkMonitor_NetworkList and set it to PP_ERROR_NOACCESS.

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
File ppapi/proxy/network_monitor_resource.h (right):

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.h:35: PP_Resource* network_list,
wrong indent.

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

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1082:
IPC_MESSAGE_CONTROL0(PpapiHostMsg_NetworkMonitor_Create)
Messages for the host/resource design are at the bottom. Please search "Resource
call/reply messages." in this file.

[Sergey Ulanov, 2013-09-12 07:01:50]

https://codereview.chromium.org/23819033/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (left):

https://codereview.chromium.org/23819033/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_message_filter.cc:130:
ppapi::API_ID_PPB_NETWORKMANAGER_PRIVATE, *it, *list_copy));
On 2013/09/11 22:51:26, yzshen1 wrote:
> Please also remove the definition of the ID from api_id.h

Done.

https://codereview.chromium.org/23819033/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

https://codereview.chromium.org/23819033/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_message_filter.cc:7: #include
"base/bind.h"
On 2013/09/11 22:51:26, yzshen1 wrote:
> Please remove those .h files that are not needed.

Done.

https://codereview.chromium.org/23819033/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_monitor_host.cc
(right):

https://codereview.chromium.org/23819033/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_monitor_host.cc:27:
external_plugin, true /* private_api */,
On 2013/09/11 22:51:26, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/23819033/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_monitor_host.cc:45:
reply_context_(context.MakeReplyMessageContext()),
On 2013/09/11 22:51:26, yzshen1 wrote:
> This makes a "reply" for the Create message, which is incorrect. Because the
> network list change notification could happen multiple times.
> Please remove |reply_context_| and use PpapiHost::SendUnsolicitedReply(). 

Done.

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
File ppapi/proxy/network_monitor_resource.cc (right):

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.cc:19: forbidden_(false) {
On 2013/09/11 22:51:26, yzshen1 wrote:
> Please also init the PP_Resource* member, too.

Done.

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.cc:44: PP_Resource* network_list,
On 2013/09/11 22:51:26, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.cc:55: thunk::EnterResourceCreationNoLock
enter(pp_instance());
On 2013/09/11 22:51:26, yzshen1 wrote:
> You don't need this line, right? |enter| is never used.

Done.

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.cc:56: *network_list =
PPB_NetworkList_Private_Shared::Create(
On 2013/09/11 22:51:26, yzshen1 wrote:
> You could also make changes to PPB_NetworkList_Private_Shared. It is not
shared
> by in-process impl anymore.

Yes. Good point. I moved it to this directory, and simplified it further.

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.cc:74: thunk::EnterResourceCreationNoLock
enter(pp_instance());
On 2013/09/11 22:51:26, yzshen1 wrote:
> This line is not needed.

Done.

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.cc:83: void
NetworkMonitorResource::OnPluginMsgForbidden(
On 2013/09/11 22:51:26, yzshen1 wrote:
> This message is not needed, |params| has a result field. You could reuse
> PpapiMsg_NetworkMonitor_NetworkList and set it to PP_ERROR_NOACCESS.

I don't see way to set the result filed when using SendUnsolicitedReply(). Do
you think I should add result parameter in that method? (it seems wrong to abuse
NetworkList message this way - that message is supposed to deliver notifications
about network list changes, not results of UpdateNetworkList() call).

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
File ppapi/proxy/network_monitor_resource.h (right):

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.h:35: PP_Resource* network_list,
On 2013/09/11 22:51:26, yzshen1 wrote:
> wrong indent.

Done.

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

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1082:
IPC_MESSAGE_CONTROL0(PpapiHostMsg_NetworkMonitor_Create)
On 2013/09/11 22:51:26, yzshen1 wrote:
> Messages for the host/resource design are at the bottom. Please search
"Resource
> call/reply messages." in this file.

Done.

[Sergey Ulanov, 2013-09-13 17:25:55]

ping

[yzshen1, 2013-09-13 17:47:10]

LGTM with a few  nits.

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
File ppapi/proxy/network_monitor_resource.cc (right):

https://codereview.chromium.org/23819033/diff/4001/ppapi/proxy/network_monito...
ppapi/proxy/network_monitor_resource.cc:83: void
NetworkMonitorResource::OnPluginMsgForbidden(
Right. SendUnsolicitedReply() doesn't have params. 
Your current approach is fine.

It makes sense to add a |result| parameter for SendUnsolicitedReply(), but I
don't think it is good to touch a lot of irrelevant files in this CL.


On 2013/09/12 07:01:51, Sergey Ulanov wrote:
> On 2013/09/11 22:51:26, yzshen1 wrote:
> > This message is not needed, |params| has a result field. You could reuse
> > PpapiMsg_NetworkMonitor_NetworkList and set it to PP_ERROR_NOACCESS.
> 
> I don't see way to set the result filed when using SendUnsolicitedReply(). Do
> you think I should add result parameter in that method? (it seems wrong to
abuse
> NetworkList message this way - that message is supposed to deliver
notifications
> about network list changes, not results of UpdateNetworkList() call).

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/network_list_...
File ppapi/proxy/network_list_resource.cc (right):

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/network_list_...
ppapi/proxy/network_list_resource.cc:35: ::ppapi::thunk::PPB_NetworkList_API*
maybe we don't need ::ppapi?

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/network_list_...
File ppapi/proxy/network_list_resource.h (right):

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/network_list_...
ppapi/proxy/network_list_resource.h:34: : public Resource,
You probably want to still include resource.h on line 14.

It is okay to inherit Resource instead of PluginResource, because this class
doesn't need the extra capability that PluginResource provides
(sending/receiving IPCs).

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/network_list_...
ppapi/proxy/network_list_resource.h:37: NetworkListResource(ResourceObjectType
type,
|type| is not needed, we only use it as PROXY.

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/network_monit...
File ppapi/proxy/network_monitor_resource.cc (right):

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/network_monit...
ppapi/proxy/network_monitor_resource.cc:80: if
(TrackedCallback::IsPending(update_callback_)) {
nit: no need to have {}

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

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1436: 
Please also move the other two messages here.

[Sergey Ulanov, 2013-09-13 18:35:00]

+brettw@ - please approve changes in content/public and
content/content_browser.gypi
+jschuh@ - please review ppapi_messages.h

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/network_list_...
File ppapi/proxy/network_list_resource.cc (right):

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/network_list_...
ppapi/proxy/network_list_resource.cc:35: ::ppapi::thunk::PPB_NetworkList_API*
On 2013/09/13 17:47:10, yzshen1 wrote:
> maybe we don't need ::ppapi?

Done.

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/network_list_...
File ppapi/proxy/network_list_resource.h (right):

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/network_list_...
ppapi/proxy/network_list_resource.h:34: : public Resource,
On 2013/09/13 17:47:10, yzshen1 wrote:
> You probably want to still include resource.h on line 14.
> 
> It is okay to inherit Resource instead of PluginResource, because this class
> doesn't need the extra capability that PluginResource provides
> (sending/receiving IPCs).

Done.

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/network_list_...
ppapi/proxy/network_list_resource.h:37: NetworkListResource(ResourceObjectType
type,
On 2013/09/13 17:47:10, yzshen1 wrote:
> |type| is not needed, we only use it as PROXY.

Done.

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/network_monit...
File ppapi/proxy/network_monitor_resource.cc (right):

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/network_monit...
ppapi/proxy/network_monitor_resource.cc:80: if
(TrackedCallback::IsPending(update_callback_)) {
On 2013/09/13 17:47:10, yzshen1 wrote:
> nit: no need to have {}

Done.

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

https://codereview.chromium.org/23819033/diff/20001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1436: 
On 2013/09/13 17:47:10, yzshen1 wrote:
> Please also move the other two messages here.

Done. Also renamed them for consistency.

[Sergey Ulanov, 2013-09-16 16:33:52]

brettw,jschuh: ping

[brettw, 2013-09-16 17:42:15]

LGTM for aforementioned stuff.

[jschuh, 2013-09-16 17:53:28]

ipc security lgtm. all params are passed high->low.

[commit-bot: I haz the power, 2013-09-16 17:56:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/23819033/50001

[commit-bot: I haz the power, 2013-09-16 18:03:38]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-16 19:07:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/23819033/70001

[Sergey Ulanov, 2013-09-16 20:50:22]

To fix compilation on windows moved ppapi::proxy::NetworkInfo to
serialized_structs.h and renamed it to SerializedNetworkInfo.

[commit-bot: I haz the power, 2013-09-16 20:51:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/23819033/86001

[commit-bot: I haz the power, 2013-09-17 00:03:49]

Change committed as 223482

[commit-bot: I haz the power, 2013-09-17 00:53:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/23819033/86001

[commit-bot: I haz the power, 2013-09-17 00:59:16]

Change committed as 223494

[commit-bot: I haz the power, 2013-09-17 01:20:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/23819033/95001

[Sergey Ulanov, 2013-09-17 04:15:18]

Committed patchset #11 manually as r223535 (presubmit successful).