Implement flash menu and net connector resources using the API/thunk model.

Implement flash menu and net connector resources using the API/thunk model.
Write thunk for image data trusted.

BUG=none
TEST=ppapi ui tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89204

[yzshen1, 2011-06-15 01:12:44]

Hi, Brett.

Only a few nits. Thanks!

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_menu_pro...
File ppapi/proxy/ppb_flash_menu_proxy.cc (right):

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_menu_pro...
ppapi/proxy/ppb_flash_menu_proxy.cc:32: virtual int32_t Show(const struct
PP_Point* location,
You probably don't need the "struct" keyword.

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_menu_pro...
ppapi/proxy/ppb_flash_menu_proxy.cc:149: if (enter.succeeded()) {
Please set |result| properly if enter fails.

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_net_conn...
File ppapi/proxy/ppb_flash_net_connector_proxy.cc (right):

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_net_conn...
ppapi/proxy/ppb_flash_net_connector_proxy.cc:11: #include
"ppapi/thunk/ppb_flash_net_connector_api.h"
Please arrange them alphabetically.

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_net_conn...
ppapi/proxy/ppb_flash_net_connector_proxy.cc:54: FlashNetConnector(const
HostResource& resource);
Please add "explicit".

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_net_conn...
ppapi/proxy/ppb_flash_net_connector_proxy.cc:83: struct PP_Flash_NetAddress*
local_addr_out,
You could probably remove "struct".

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_net_conn...
ppapi/proxy/ppb_flash_net_connector_proxy.cc:260: }
Please set |result| properly if enter fails.

http://codereview.chromium.org/7149026/diff/34/ppapi/thunk/ppb_flash_menu_api.h
File ppapi/thunk/ppb_flash_menu_api.h (right):

http://codereview.chromium.org/7149026/diff/34/ppapi/thunk/ppb_flash_menu_api...
ppapi/thunk/ppb_flash_menu_api.h:15: virtual int32_t Show(const struct PP_Point*
location,
Please define a virtual destructor for this class (and other API classes).

[optional] I think "struct" on line 15 & 17 are not necessary.

http://codereview.chromium.org/7149026/diff/34/ppapi/thunk/ppb_flash_menu_thu...
File ppapi/thunk/ppb_flash_menu_thunk.cc (right):

http://codereview.chromium.org/7149026/diff/34/ppapi/thunk/ppb_flash_menu_thu...
ppapi/thunk/ppb_flash_menu_thunk.cc:19: EnterFunction<ResourceCreationAPI>
enter(instance, true);
BTW, I took a look at the implementation of EnterFunction and found that
instance_ was not set anywhere.

[brettw, 2011-06-15 02:29:51]

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_menu_pro...
File ppapi/proxy/ppb_flash_menu_proxy.cc (right):

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_menu_pro...
ppapi/proxy/ppb_flash_menu_proxy.cc:149: if (enter.succeeded()) {
This is automatically done by the HostResource constructor.

http://codereview.chromium.org/7149026/diff/34/ppapi/thunk/ppb_flash_menu_api.h
File ppapi/thunk/ppb_flash_menu_api.h (right):

http://codereview.chromium.org/7149026/diff/34/ppapi/thunk/ppb_flash_menu_api...
ppapi/thunk/ppb_flash_menu_api.h:15: virtual int32_t Show(const struct PP_Point*
location,
On 2011/06/15 01:12:44, yzshen1 wrote:
> Please define a virtual destructor for this class (and other API classes).

Good point. If it's OK I'll file a bug and do this in a followup patch or this
will get out of control.

> [optional] I think "struct" on line 15 & 17 are not necessary.

Yeah, thanks.

http://codereview.chromium.org/7149026/diff/34/ppapi/thunk/ppb_flash_menu_thu...
File ppapi/thunk/ppb_flash_menu_thunk.cc (right):

http://codereview.chromium.org/7149026/diff/34/ppapi/thunk/ppb_flash_menu_thu...
ppapi/thunk/ppb_flash_menu_thunk.cc:19: EnterFunction<ResourceCreationAPI>
enter(instance, true);
Good catch! I removed the instance_ member and accessor.

[yzshen1, 2011-06-15 06:49:51]

Thanks Brett!

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_menu_pro...
File ppapi/proxy/ppb_flash_menu_proxy.cc (right):

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_menu_pro...
ppapi/proxy/ppb_flash_menu_proxy.cc:149: if (enter.succeeded()) {
On 2011/06/15 02:29:51, brettw wrote:
> This is automatically done by the HostResource constructor.
[out of curiosity]
Does it rely on the initial value of |result| in CreateProxyResource()?

In other words, I wonder what will happen if in CreateProxyResource() we call
result.SetHostResource(some_invalid_instance, some_invalid_resource) before
sending the IPC message. (I know it is illegal. :) )
Will |result| at the plugin side get reset to NULL properly, if |enter| in this
method fails?

http://codereview.chromium.org/7149026/diff/34/ppapi/thunk/ppb_flash_menu_api.h
File ppapi/thunk/ppb_flash_menu_api.h (right):

http://codereview.chromium.org/7149026/diff/34/ppapi/thunk/ppb_flash_menu_api...
ppapi/thunk/ppb_flash_menu_api.h:15: virtual int32_t Show(const struct PP_Point*
location,

On 2011/06/15 02:29:51, brettw wrote:
> On 2011/06/15 01:12:44, yzshen1 wrote:
> > Please define a virtual destructor for this class (and other API classes).
> 
> Good point. If it's OK I'll file a bug and do this in a followup patch or this
> will get out of control.

Sounds good.

> 
> > [optional] I think "struct" on line 15 & 17 are not necessary.
> 
> Yeah, thanks.

http://codereview.chromium.org/7149026/diff/34/ppapi/thunk/ppb_flash_menu_thu...
File ppapi/thunk/ppb_flash_menu_thunk.cc (right):

http://codereview.chromium.org/7149026/diff/34/ppapi/thunk/ppb_flash_menu_thu...
ppapi/thunk/ppb_flash_menu_thunk.cc:19: EnterFunction<ResourceCreationAPI>
enter(instance, true);
On 2011/06/15 02:29:51, brettw wrote:
> Good catch! I removed the instance_ member and accessor.

Thanks! Please note that you haven't included enter.h in this CL.

[brettw, 2011-06-15 15:37:03]

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_menu_pro...
File ppapi/proxy/ppb_flash_menu_proxy.cc (right):

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_menu_pro...
ppapi/proxy/ppb_flash_menu_proxy.cc:149: if (enter.succeeded()) {
That shouldn't happen. The host side will constuct a default-constructed
HostResource as part of the message processing. This is the version we're
getting at this point, and it's getting copied back to the caller. For "return"
params, the IPC system doesn't copy the source from the caller at all.

http://codereview.chromium.org/7149026/diff/34/ppapi/thunk/ppb_flash_menu_thu...
File ppapi/thunk/ppb_flash_menu_thunk.cc (right):

http://codereview.chromium.org/7149026/diff/34/ppapi/thunk/ppb_flash_menu_thu...
ppapi/thunk/ppb_flash_menu_thunk.cc:19: EnterFunction<ResourceCreationAPI>
enter(instance, true);
Another good catch! :)

[yzshen1, 2011-06-15 16:40:30]

LGTM

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_menu_pro...
File ppapi/proxy/ppb_flash_menu_proxy.cc (right):

http://codereview.chromium.org/7149026/diff/34/ppapi/proxy/ppb_flash_menu_pro...
ppapi/proxy/ppb_flash_menu_proxy.cc:149: if (enter.succeeded()) {
Thanks for explaining!

On 2011/06/15 15:37:03, brettw wrote:
> That shouldn't happen. The host side will constuct a default-constructed
> HostResource as part of the message processing. This is the version we're
> getting at this point, and it's getting copied back to the caller. For
"return"
> params, the IPC system doesn't copy the source from the caller at all.