Refactor 3 PPB_Flash functions to the new resource model.

Refactor 3 PPB_Flash functions to the new resource model.

This refactors the following flash functions to the new resource model.
-GetProxyForURL
-UpdateActivity
-SetCrashData

BUG=


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

[victorhsieh, 2012-11-28 07:09:50]

https://codereview.chromium.org/11415140/diff/3030/content/browser/renderer_h...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/11415140/diff/3030/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:75: 
nit: remove 1 blank line

https://codereview.chromium.org/11415140/diff/3030/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_flash_browser_host.h (right):

https://codereview.chromium.org/11415140/diff/3030/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_flash_browser_host.h:23: virtual
int32_t OnResourceMessageReceived(
// ppapi::host::ResourceHost  override.

https://codereview.chromium.org/11415140/diff/3030/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_renderer_host.cc (right):

https://codereview.chromium.org/11415140/diff/3030/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_renderer_host.cc:51:
host_context->reply_msg = PpapiPluginMsg_Flash_GetProxyForURLReply(proxy);
you don't need to host()->SendReply?

https://codereview.chromium.org/11415140/diff/3030/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_renderer_host.h (right):

https://codereview.chromium.org/11415140/diff/3030/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_renderer_host.h:23: virtual int32_t
OnResourceMessageReceived(
ppapi::host::ResourceHost override.

https://codereview.chromium.org/11415140/diff/3030/ppapi/proxy/flash_resource.h
File ppapi/proxy/flash_resource.h (right):

https://codereview.chromium.org/11415140/diff/3030/ppapi/proxy/flash_resource...
ppapi/proxy/flash_resource.h:8: #include "ppapi/proxy/connection.h"
I don't see every header includes those common pp-types.  But maybe you should
include them?
#include "ppapi/c/pp_var.h"
#include "ppapi/c/pp_instance.h"
#include "ppapi/c/private/ppb_flash.h"

https://codereview.chromium.org/11415140/diff/3030/ppapi/thunk/ppb_flash_thun...
File ppapi/thunk/ppb_flash_thunk.cc (right):

https://codereview.chromium.org/11415140/diff/3030/ppapi/thunk/ppb_flash_thun...
ppapi/thunk/ppb_flash_thunk.cc:51: EnterInstanceAPI<PPB_Flash_Functions_API>
enter(instance);
Where is EnterInstanceAPI defined?

[raymes, 2012-11-29 00:08:07]

https://codereview.chromium.org/11415140/diff/3030/content/browser/renderer_h...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/11415140/diff/3030/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:75: 
On 2012/11/28 07:09:50, Victor Hsieh wrote:
> nit: remove 1 blank line

Done.

https://codereview.chromium.org/11415140/diff/3030/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_flash_browser_host.h (right):

https://codereview.chromium.org/11415140/diff/3030/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_flash_browser_host.h:23: virtual
int32_t OnResourceMessageReceived(
On 2012/11/28 07:09:50, Victor Hsieh wrote:
> // ppapi::host::ResourceHost  override.

Done.

https://codereview.chromium.org/11415140/diff/3030/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_renderer_host.cc (right):

https://codereview.chromium.org/11415140/diff/3030/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_renderer_host.cc:51:
host_context->reply_msg = PpapiPluginMsg_Flash_GetProxyForURLReply(proxy);
Nope. If you handle the message synchronously then the reply will get sent
automatically.

https://codereview.chromium.org/11415140/diff/3030/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_renderer_host.h (right):

https://codereview.chromium.org/11415140/diff/3030/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_renderer_host.h:23: virtual int32_t
OnResourceMessageReceived(
On 2012/11/28 07:09:50, Victor Hsieh wrote:
> ppapi::host::ResourceHost override.

Done.

https://codereview.chromium.org/11415140/diff/3030/ppapi/proxy/flash_resource.h
File ppapi/proxy/flash_resource.h (right):

https://codereview.chromium.org/11415140/diff/3030/ppapi/proxy/flash_resource...
ppapi/proxy/flash_resource.h:8: #include "ppapi/proxy/connection.h"
Thanks. Yeh we do a bad job of including all thsee in all the right places.

https://codereview.chromium.org/11415140/diff/3030/ppapi/thunk/ppb_flash_thun...
File ppapi/thunk/ppb_flash_thunk.cc (right):

https://codereview.chromium.org/11415140/diff/3030/ppapi/thunk/ppb_flash_thun...
ppapi/thunk/ppb_flash_thunk.cc:51: EnterInstanceAPI<PPB_Flash_Functions_API>
enter(instance);
In enter.h. This is based on another change which is still in the commit queue
so it doesn't show up in the checked in code yet.

[victorhsieh, 2012-11-29 06:17:23]

lgtm

https://codereview.chromium.org/11415140/diff/3030/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_renderer_host.cc (right):

https://codereview.chromium.org/11415140/diff/3030/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_renderer_host.cc:51:
host_context->reply_msg = PpapiPluginMsg_Flash_GetProxyForURLReply(proxy);
On 2012/11/29 00:08:08, raymes wrote:
> Nope. If you handle the message synchronously then the reply will get sent
> automatically.

Hmm.. we could have a "WillSyncReply" or something later.  It's not easy to know
at a glance.

[raymes, 2012-11-30 21:45:12]

+brettw

[brettw, 2012-11-30 23:29:24]

lgtm

https://codereview.chromium.org/11415140/diff/9006/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_renderer_host.cc (right):

https://codereview.chromium.org/11415140/diff/9006/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_renderer_host.cc:42: bool result;
Random nit: I'd put these two out params right above the Send() call where
they're used.

https://codereview.chromium.org/11415140/diff/9006/ppapi/proxy/flash_resource.cc
File ppapi/proxy/flash_resource.cc (right):

https://codereview.chromium.org/11415140/diff/9006/ppapi/proxy/flash_resource...
ppapi/proxy/flash_resource.cc:48: case PP_FLASHCRASHKEY_URL:
Need {} since you declare local vars.

https://codereview.chromium.org/11415140/diff/9006/ppapi/proxy/flash_resource...
ppapi/proxy/flash_resource.cc:49: StringVar
*url_string_var(StringVar::FromPPVar(value));
* next to the type instead.

https://codereview.chromium.org/11415140/diff/9006/ppapi/proxy/flash_resource...
ppapi/proxy/flash_resource.cc:52: std::string
url_string(url_string_var->value());
Can you just pass url_string_var->value() into SetActiveURL()? I think your code
makes an extra copy and seems unnecessary anyway.

[raymes, 2012-12-03 17:30:31]

https://codereview.chromium.org/11415140/diff/9006/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_renderer_host.cc (right):

https://codereview.chromium.org/11415140/diff/9006/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_renderer_host.cc:42: bool result;
On 2012/11/30 23:29:25, brettw wrote:
> Random nit: I'd put these two out params right above the Send() call where
> they're used.

Done.

https://codereview.chromium.org/11415140/diff/9006/ppapi/proxy/flash_resource.cc
File ppapi/proxy/flash_resource.cc (right):

https://codereview.chromium.org/11415140/diff/9006/ppapi/proxy/flash_resource...
ppapi/proxy/flash_resource.cc:48: case PP_FLASHCRASHKEY_URL:
On 2012/11/30 23:29:25, brettw wrote:
> Need {} since you declare local vars.

Done.

https://codereview.chromium.org/11415140/diff/9006/ppapi/proxy/flash_resource...
ppapi/proxy/flash_resource.cc:49: StringVar
*url_string_var(StringVar::FromPPVar(value));
On 2012/11/30 23:29:25, brettw wrote:
> * next to the type instead.

Done.

https://codereview.chromium.org/11415140/diff/9006/ppapi/proxy/flash_resource...
ppapi/proxy/flash_resource.cc:52: std::string
url_string(url_string_var->value());
On 2012/11/30 23:29:25, brettw wrote:
> Can you just pass url_string_var->value() into SetActiveURL()? I think your
code
> makes an extra copy and seems unnecessary anyway.

Done.

[commit-bot: I haz the power, 2012-12-03 17:31:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11415140/3035

[commit-bot: I haz the power, 2012-12-03 17:32:08]

Presubmit check for 11415140-3035 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ppapi/proxy/ppapi_messages.h

Presubmit checks took 2.5s to calculate.

[raymes, 2012-12-04 00:33:33]

+tsepez for ppapi_messages.h. Note that the attack surface is unchanged.

[Tom Sepez, 2012-12-04 00:43:56]

Rubberstamp LGTM on ppapi_messages.h

[commit-bot: I haz the power, 2012-12-04 00:45:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11415140/3035

[commit-bot: I haz the power, 2012-12-04 04:22:10]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-04 04:43:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11415140/3035

[commit-bot: I haz the power, 2012-12-04 08:47:36]

Change committed as 170907