Refactor 4 PPB_Flash functions to the new PPAPI resource model.

Refactor 4 PPB_Flash functions to the new PPAPI resource model.

The functions being refactored are:
-SetInstanceAlwaysOnTop
-DrawGlyphs
-GetProxyForURL
-Navigate

Each of these functions was manually tested with a Flash movie that uses it.

Navigate() was tested on:
-http://www.tjgames.com.br/jogos-online/169-Yan_Loong.htm 
-http://www.flonga.com/play/zombotron2.htm
which were consistently causing instance deletion during a Navigate() call (causing https://code.google.com/p/chromium/issues/detail?id=165978).

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

[yzshen1, 2012-12-12 19:09:31]

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

https://codereview.chromium.org/11510008/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_renderer_host.cc:23: #include
"ppapi/thunk/ppb_url_request_info_api.h"
You don't need this.

https://codereview.chromium.org/11510008/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_renderer_host.cc:37: using
ppapi::thunk::PPB_URLRequestInfo_API;
you don't need this.

https://codereview.chromium.org/11510008/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_renderer_host.cc:91:
plugin_instance->set_always_on_top(on_top);
|plugin_instance| may be NULL. (I don't remember whether it is possible to have
resource hosts alive after the instance is gone. I need to read to be sure.)

(There are some other places in this file has the same problem.)

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

https://codereview.chromium.org/11510008/diff/2001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1602: // parmaters for drawing (including the
resource to draw to) are contianed in
parmaters -> parameters

https://codereview.chromium.org/11510008/diff/2001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1618: // sent in a reply message indicating whether
or not the rect is topmost.
It would be good to comment on what error_codes indicates true / false. It is
not as obvious as succeeded/failed.
I think PP_OK -> true, but its value is 0 so it might cause bugs if we are not
careful enough.

https://codereview.chromium.org/11510008/diff/2001/ppapi/thunk/ppb_flash_api.h
File ppapi/thunk/ppb_flash_api.h (right):

https://codereview.chromium.org/11510008/diff/2001/ppapi/thunk/ppb_flash_api....
ppapi/thunk/ppb_flash_api.h:9: #include "ppapi/c/private/ppb_flash_file.h"
nit: I think this is not needed.

https://codereview.chromium.org/11510008/diff/2001/webkit/plugins/ppapi/ppb_f...
File webkit/plugins/ppapi/ppb_flash_impl.cc (right):

https://codereview.chromium.org/11510008/diff/2001/webkit/plugins/ppapi/ppb_f...
webkit/plugins/ppapi/ppb_flash_impl.cc:11: #include
"ppapi/c/trusted/ppb_browser_font_trusted.h"
I notice that some includes/using's/forward declarations are not needed anymore
in some files, such as impl and proxy. (But it is probably fine now that you are
going to remove those files entirely soon.)

[raymes, 2012-12-12 22:01:21]

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

https://codereview.chromium.org/11510008/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_renderer_host.cc:23: #include
"ppapi/thunk/ppb_url_request_info_api.h"
On 2012/12/12 19:09:31, yzshen1 wrote:
> You don't need this.

Done.

https://codereview.chromium.org/11510008/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_renderer_host.cc:23: #include
"ppapi/thunk/ppb_url_request_info_api.h"
On 2012/12/12 19:09:31, yzshen1 wrote:
> You don't need this.

Done.

https://codereview.chromium.org/11510008/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_renderer_host.cc:37: using
ppapi::thunk::PPB_URLRequestInfo_API;
On 2012/12/12 19:09:31, yzshen1 wrote:
> you don't need this.

Done.

https://codereview.chromium.org/11510008/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_renderer_host.cc:91:
plugin_instance->set_always_on_top(on_top);
On 2012/12/12 19:09:31, yzshen1 wrote:
> |plugin_instance| may be NULL. (I don't remember whether it is possible to
have
> resource hosts alive after the instance is gone. I need to read to be sure.)
> 
> (There are some other places in this file has the same problem.)

Done.

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

https://codereview.chromium.org/11510008/diff/2001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1602: // parmaters for drawing (including the
resource to draw to) are contianed in
On 2012/12/12 19:09:31, yzshen1 wrote:
> parmaters -> parameters

Done.

https://codereview.chromium.org/11510008/diff/2001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1618: // sent in a reply message indicating whether
or not the rect is topmost.
On 2012/12/12 19:09:31, yzshen1 wrote:
> It would be good to comment on what error_codes indicates true / false. It is
> not as obvious as succeeded/failed.
> I think PP_OK -> true, but its value is 0 so it might cause bugs if we are not
> careful enough.

Done.

https://codereview.chromium.org/11510008/diff/2001/ppapi/thunk/ppb_flash_api.h
File ppapi/thunk/ppb_flash_api.h (right):

https://codereview.chromium.org/11510008/diff/2001/ppapi/thunk/ppb_flash_api....
ppapi/thunk/ppb_flash_api.h:9: #include "ppapi/c/private/ppb_flash_file.h"
On 2012/12/12 19:09:31, yzshen1 wrote:
> nit: I think this is not needed.

Done.

https://codereview.chromium.org/11510008/diff/2001/webkit/plugins/ppapi/ppb_f...
File webkit/plugins/ppapi/ppb_flash_impl.cc (right):

https://codereview.chromium.org/11510008/diff/2001/webkit/plugins/ppapi/ppb_f...
webkit/plugins/ppapi/ppb_flash_impl.cc:11: #include
"ppapi/c/trusted/ppb_browser_font_trusted.h"
Thanks - yeh I wasn't very careful here since it will be deleted very soon.

[yzshen1, 2012-12-12 22:10:44]

lgtm

[commit-bot: I haz the power, 2012-12-12 22:27:16]

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

[commit-bot: I haz the power, 2012-12-12 22:27:21]

Presubmit check for 11510008-21002 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 1.5s to calculate.

[raymes, 2012-12-12 22:35:51]

+tsepez for security review

[Tom Sepez, 2012-12-12 23:43:22]

messages LGTM.

[commit-bot: I haz the power, 2012-12-13 00:00:52]

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

[commit-bot: I haz the power, 2012-12-13 03:35:37]

Change committed as 172806

[raymes, 2012-12-14 02:03:38]

Please diff to patch set 4 to see the updates to fix Navigate. Since I don't
expect this case to arise very often, I kept the fix local to the ResourceHost.
This avoids having to increase the complexity of the host side framework. But if
this pattern arises more often, we may want to.

[brettw, 2012-12-14 19:12:30]

Ugh, LGTM

[yzshen1, 2012-12-14 19:13:14]

https://codereview.chromium.org/11510008/diff/42002/content/renderer/pepper/p...
File content/renderer/pepper/pepper_flash_renderer_host.cc (right):

https://codereview.chromium.org/11510008/diff/42002/content/renderer/pepper/p...
content/renderer/pepper/pepper_flash_renderer_host.cc:49: // This object may be
destroyed in the middle of a sync message. If that is
I guess we will need this for some other classes. But I am fine to do that in a
later CL (maybe by me).
:)

https://codereview.chromium.org/11510008/diff/42002/content/renderer/pepper/p...
content/renderer/pepper/pepper_flash_renderer_host.cc:51: for (size_t i = 0; i <
navigate_replies_.size(); ++i)
maybe sending in reverse order is more consistent?

https://codereview.chromium.org/11510008/diff/42002/ppapi/host/resource_messa...
File ppapi/host/resource_message_handler.cc (right):

https://codereview.chromium.org/11510008/diff/42002/ppapi/host/resource_messa...
ppapi/host/resource_message_handler.cc:26: // PP_OK_COMPLETIONPENDING is
returned, we should assume that we might have
Why PP_OK_COMPLETIONPENDING is the only possible outcome that this object may
have been destructed?
(If not, line 53 cannot be called.)

[raymes, 2012-12-17 04:07:57]

https://codereview.chromium.org/11510008/diff/42002/content/renderer/pepper/p...
File content/renderer/pepper/pepper_flash_renderer_host.cc (right):

https://codereview.chromium.org/11510008/diff/42002/content/renderer/pepper/p...
content/renderer/pepper/pepper_flash_renderer_host.cc:51: for (size_t i = 0; i <
navigate_replies_.size(); ++i)
On 2012/12/14 19:13:14, yzshen1 wrote:
> maybe sending in reverse order is more consistent?

Done.

https://codereview.chromium.org/11510008/diff/42002/ppapi/host/resource_messa...
File ppapi/host/resource_message_handler.cc (right):

https://codereview.chromium.org/11510008/diff/42002/ppapi/host/resource_messa...
ppapi/host/resource_message_handler.cc:26: // PP_OK_COMPLETIONPENDING is
returned, we should assume that we might have
Right now I'm making the assumption that if the message handler might cause the
object to be destroyed, it MUST return PP_OK_COMPLETIONPENDING (otherwise line
53 will get called).

[yzshen1, 2012-12-17 05:02:23]

LGTM

Thanks!

https://codereview.chromium.org/11510008/diff/42002/ppapi/host/resource_messa...
File ppapi/host/resource_message_handler.cc (right):

https://codereview.chromium.org/11510008/diff/42002/ppapi/host/resource_messa...
ppapi/host/resource_message_handler.cc:26: // PP_OK_COMPLETIONPENDING is
returned, we should assume that we might have
This assumption doesn't look very intuitive. Can it be said here or elsewhere
explicitly?

On 2012/12/17 04:07:57, raymes wrote:
> Right now I'm making the assumption that if the message handler might cause
the
> object to be destroyed, it MUST return PP_OK_COMPLETIONPENDING (otherwise line
> 53 will get called).

[raymes, 2012-12-17 05:11:00]

https://codereview.chromium.org/11510008/diff/42002/ppapi/host/resource_messa...
File ppapi/host/resource_message_handler.cc (right):

https://codereview.chromium.org/11510008/diff/42002/ppapi/host/resource_messa...
ppapi/host/resource_message_handler.cc:26: // PP_OK_COMPLETIONPENDING is
returned, we should assume that we might have
I tried to make it clear in the comment above but I probably did a bad job :P
Do you have suggestions on how it might be clearer?

On 2012/12/17 05:02:23, yzshen1 wrote:
> This assumption doesn't look very intuitive. Can it be said here or elsewhere
> explicitly?
> 
> On 2012/12/17 04:07:57, raymes wrote:
> > Right now I'm making the assumption that if the message handler might cause
> the
> > object to be destroyed, it MUST return PP_OK_COMPLETIONPENDING (otherwise
line
> > 53 will get called).
>

[yzshen1, 2012-12-17 05:19:37]

Still LGTM

https://codereview.chromium.org/11510008/diff/42002/ppapi/host/resource_messa...
File ppapi/host/resource_message_handler.cc (right):

https://codereview.chromium.org/11510008/diff/42002/ppapi/host/resource_messa...
ppapi/host/resource_message_handler.cc:26: // PP_OK_COMPLETIONPENDING is
returned, we should assume that we might have
If my understanding is correct, it is not that it won't happen with other return
values; instead, you are making a rule and asking all resource hosts to follow
it. 

The comment sounds like "it won't happen with other return values".

On 2012/12/17 05:11:00, raymes wrote:
> I tried to make it clear in the comment above but I probably did a bad job :P
> Do you have suggestions on how it might be clearer?
> 
> On 2012/12/17 05:02:23, yzshen1 wrote:
> > This assumption doesn't look very intuitive. Can it be said here or
elsewhere
> > explicitly?
> > 
> > On 2012/12/17 04:07:57, raymes wrote:
> > > Right now I'm making the assumption that if the message handler might
cause
> > the
> > > object to be destroyed, it MUST return PP_OK_COMPLETIONPENDING (otherwise
> line
> > > 53 will get called).
> > 
>