Refactor PPB_Flash_Fullscreen to the new resource model.

Refactor PPB_Flash_Fullscreen to the new resource model.

This moves PPB_Flash_Fullscreen to the new ppapi resource model and removes the in-process implementation.

BUG=


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

[raymes, 2012-11-27 18:06:36]

Hey David,
yzshen is out this week so sending to you. If you're busy I can find someone
else.
Thanks

[dmichael (off chromium), 2012-11-27 20:58:01]

https://codereview.chromium.org/11421066/diff/9070/chrome/test/ppapi/ppapi_br...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/11421066/diff/9070/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:911: // async flakiness: 
http://crbug.com/108471
It looks like this bug is fixed; can you remove the comments about it?

https://codereview.chromium.org/11421066/diff/9070/ppapi/proxy/flash_fullscre...
File ppapi/proxy/flash_fullscreen_resource.cc (right):

https://codereview.chromium.org/11421066/diff/9070/ppapi/proxy/flash_fullscre...
ppapi/proxy/flash_fullscreen_resource.cc:8: #include
"ppapi/proxy/plugin_dispatcher.h"
^^^ Do you need this?

https://codereview.chromium.org/11421066/diff/9070/ppapi/proxy/flash_fullscre...
ppapi/proxy/flash_fullscreen_resource.cc:41: void
FlashFullscreenResource::SetFullscreenInternal(PP_Instance instance,
Maybe SetLocalIsFullscreen, or something? It's not very descriptive;
SetFullscreenInternal sounds almost like it's going to actually change the
fullscreen state.

https://codereview.chromium.org/11421066/diff/9070/ppapi/shared_impl/api_id.h
File ppapi/shared_impl/api_id.h (right):

https://codereview.chromium.org/11421066/diff/9070/ppapi/shared_impl/api_id.h...
ppapi/shared_impl/api_id.h:83: FLASH_FULLSCREEN_SINGLETON_ID,
I thought you were going to put these in a separate file per brettw's request?

[raymes, 2012-11-27 21:24:28]

https://codereview.chromium.org/11421066/diff/9070/chrome/test/ppapi/ppapi_br...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/11421066/diff/9070/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:911: // async flakiness: 
http://crbug.com/108471
On 2012/11/27 20:58:01, dmichael wrote:
> It looks like this bug is fixed; can you remove the comments about it?

Done.

https://codereview.chromium.org/11421066/diff/9070/ppapi/proxy/flash_fullscre...
File ppapi/proxy/flash_fullscreen_resource.cc (right):

https://codereview.chromium.org/11421066/diff/9070/ppapi/proxy/flash_fullscre...
ppapi/proxy/flash_fullscreen_resource.cc:8: #include
"ppapi/proxy/plugin_dispatcher.h"
On 2012/11/27 20:58:01, dmichael wrote:
> ^^^ Do you need this?

Done.

https://codereview.chromium.org/11421066/diff/9070/ppapi/proxy/flash_fullscre...
ppapi/proxy/flash_fullscreen_resource.cc:41: void
FlashFullscreenResource::SetFullscreenInternal(PP_Instance instance,
You're right. I prefer that. I also improved the comment in the interface
definition.

https://codereview.chromium.org/11421066/diff/9070/ppapi/shared_impl/api_id.h
File ppapi/shared_impl/api_id.h (right):

https://codereview.chromium.org/11421066/diff/9070/ppapi/shared_impl/api_id.h...
ppapi/shared_impl/api_id.h:83: FLASH_FULLSCREEN_SINGLETON_ID,
Yep I have in the other CL, I just hadn't rebased this yet. It's updated in the
latest patch set.

[dmichael (off chromium), 2012-11-27 21:28:08]

lgtm

https://codereview.chromium.org/11421066/diff/4045/ppapi/shared_impl/singleto...
File ppapi/shared_impl/singleton_resource_id.h (right):

https://codereview.chromium.org/11421066/diff/4045/ppapi/shared_impl/singleto...
ppapi/shared_impl/singleton_resource_id.h:13: FLASH_SINGLETON_ID,
Should this maybe be alphabetized?

[raymes, 2012-11-27 21:32:30]

On 2012/11/27 21:28:08, dmichael wrote:
> lgtm
> 
>
https://codereview.chromium.org/11421066/diff/4045/ppapi/shared_impl/singleto...
> File ppapi/shared_impl/singleton_resource_id.h (right):
> 
>
https://codereview.chromium.org/11421066/diff/4045/ppapi/shared_impl/singleto...
> ppapi/shared_impl/singleton_resource_id.h:13: FLASH_SINGLETON_ID,
> Should this maybe be alphabetized?

Done and the order fixed in all the switches.

Thanks for the really fast review.

[raymes, 2012-11-27 21:57:25]

+tsepez for ppapi_messages.h changes.

Note that the attack surface has not changed (in fact it is reduced because I
have eliminated the GetScreenSize IPC).

[Tom Sepez, 2012-11-27 22:20:15]

LGTM for message file changes.

[commit-bot: I haz the power, 2012-11-29 18:51:42]

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

[commit-bot: I haz the power, 2012-11-29 18:51:57]

Presubmit check for 11421066-9074 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:
    chrome

Presubmit checks took 2.4s to calculate.

[raymes, 2012-11-29 18:54:26]

+brettw for chrome/ OWNERS

[brettw, 2012-11-29 19:10:45]

lgtm

https://codereview.chromium.org/11421066/diff/89/chrome/renderer/pepper/peppe...
File chrome/renderer/pepper/pepper_flash_fullscreen_host.h (right):

https://codereview.chromium.org/11421066/diff/89/chrome/renderer/pepper/peppe...
chrome/renderer/pepper/pepper_flash_fullscreen_host.h:22:
content::RendererPpapiHost* host,
These should fit after the (

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

https://codereview.chromium.org/11421066/diff/89/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:1633: PP_Bool /* fullscreen */)
I'd make this a "bool" instead. In the old proxy, I was preferring PP_Bool since
that gets re-forwarded to Pepper and you can avoid conversions. But with the new
one, I think of the host side as "chrome" so we should just use the more regular
Chrome types when possible and do the conversion in the proxy.

[raymes, 2012-11-29 20:07:36]

https://codereview.chromium.org/11421066/diff/89/chrome/renderer/pepper/peppe...
File chrome/renderer/pepper/pepper_flash_fullscreen_host.h (right):

https://codereview.chromium.org/11421066/diff/89/chrome/renderer/pepper/peppe...
chrome/renderer/pepper/pepper_flash_fullscreen_host.h:22:
content::RendererPpapiHost* host,
On 2012/11/29 19:10:46, brettw wrote:
> These should fit after the (

Done.

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

https://codereview.chromium.org/11421066/diff/89/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:1633: PP_Bool /* fullscreen */)
On 2012/11/29 19:10:46, brettw wrote:
> I'd make this a "bool" instead. In the old proxy, I was preferring PP_Bool
since
> that gets re-forwarded to Pepper and you can avoid conversions. But with the
new
> one, I think of the host side as "chrome" so we should just use the more
regular
> Chrome types when possible and do the conversion in the proxy.

Done.

[commit-bot: I haz the power, 2012-11-29 20:07:49]

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

[commit-bot: I haz the power, 2012-11-29 20:51:10]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_aura&n...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-29 23:05:05]

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

[commit-bot: I haz the power, 2012-11-30 01:55:46]

Change committed as 170346