Don't require a recreation of the context when fullscreening

Don't require a recreation of the context when fullscreening

Instead of requiring flash to destroy and recreate its context, we can reparent its context onto the new view context (and later back again).

BUG=147746
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157641

[piman, 2012-09-13 00:04:33]

LGTM overall, it's great that you're fixing this.
I think we can make the transition faster by not requiring the command buffer
Finish. That code predates the Flush semantics, but I don't think it's needed
anymore.

https://chromiumcodereview.appspot.com/10918182/diff/18/content/renderer/pepp...
File content/renderer/pepper/pepper_platform_context_3d_impl.cc (right):

https://chromiumcodereview.appspot.com/10918182/diff/18/content/renderer/pepp...
content/renderer/pepper/pepper_platform_context_3d_impl.cc:157:
parent_gles2->helper()->CommandBufferHelper::Finish();
BTW, this has been carried over for some time, but with the Flush semantics, I'm
pretty sure we don't need a Finish, just a Flush.

https://chromiumcodereview.appspot.com/10918182/diff/18/content/renderer/pepp...
content/renderer/pepper/pepper_platform_context_3d_impl.cc:173:
parent_gles2->helper()->CommandBufferHelper::Finish();
same here.

https://chromiumcodereview.appspot.com/10918182/diff/18/webkit/plugins/ppapi/...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://chromiumcodereview.appspot.com/10918182/diff/18/webkit/plugins/ppapi/...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1655: }
I think you can write the 2 blocks above in a slightly simpler way. We returned
earlier if flash_fullscreen == flash_fullscreen_ so you know you have a
transition. So you can write this as something like:
PPB_Graphics3D_Impl* graphics_3d  = GetBoundGraphics3D();
if (graphics_3d) {
  if (flash_fullscreen) {
    fullscreen_container_->ReparentContext(graphics_3d->platform_context());
  } else {
    delegate_->ReparentContext(graphics_3d->platform_context());
    setBackingTextureId(graphics_3d->GetBackingTextureId(),
                        graphics_3d->IsOpaque());
  }
}

[jbauman, 2012-09-13 18:11:32]

Ok, fixed these things.

On 2012/09/13 00:04:33, piman wrote:
> LGTM overall, it's great that you're fixing this.
> I think we can make the transition faster by not requiring the command buffer
> Finish. That code predates the Flush semantics, but I don't think it's needed
> anymore.
> 
>
https://chromiumcodereview.appspot.com/10918182/diff/18/content/renderer/pepp...
> File content/renderer/pepper/pepper_platform_context_3d_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10918182/diff/18/content/renderer/pepp...
> content/renderer/pepper/pepper_platform_context_3d_impl.cc:157:
> parent_gles2->helper()->CommandBufferHelper::Finish();
> BTW, this has been carried over for some time, but with the Flush semantics,
I'm
> pretty sure we don't need a Finish, just a Flush.
> 
>
https://chromiumcodereview.appspot.com/10918182/diff/18/content/renderer/pepp...
> content/renderer/pepper/pepper_platform_context_3d_impl.cc:173:
> parent_gles2->helper()->CommandBufferHelper::Finish();
> same here.
> 
>
https://chromiumcodereview.appspot.com/10918182/diff/18/webkit/plugins/ppapi/...
> File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10918182/diff/18/webkit/plugins/ppapi/...
> webkit/plugins/ppapi/ppapi_plugin_instance.cc:1655: }
> I think you can write the 2 blocks above in a slightly simpler way. We
returned
> earlier if flash_fullscreen == flash_fullscreen_ so you know you have a
> transition. So you can write this as something like:
> PPB_Graphics3D_Impl* graphics_3d  = GetBoundGraphics3D();
> if (graphics_3d) {
>   if (flash_fullscreen) {
>     fullscreen_container_->ReparentContext(graphics_3d->platform_context());
>   } else {
>     delegate_->ReparentContext(graphics_3d->platform_context());
>     setBackingTextureId(graphics_3d->GetBackingTextureId(),
>                         graphics_3d->IsOpaque());
>   }
> }

[piman, 2012-09-13 18:57:20]

LGTM+nit

http://codereview.chromium.org/10918182/diff/10001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10918182/diff/10001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1642: if (flash_fullscreen !=
flash_fullscreen_) {
That test is always true (see early return above). Just remove?

[jbauman, 2012-09-18 17:34:32]

brettw: ping

On 2012/09/13 18:57:20, piman wrote:
> LGTM+nit
> 
>
http://codereview.chromium.org/10918182/diff/10001/webkit/plugins/ppapi/ppapi...
> File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):
> 
>
http://codereview.chromium.org/10918182/diff/10001/webkit/plugins/ppapi/ppapi...
> webkit/plugins/ppapi/ppapi_plugin_instance.cc:1642: if (flash_fullscreen !=
> flash_fullscreen_) {
> That test is always true (see early return above). Just remove?

Ok, fixed.

[brettw, 2012-09-18 17:56:46]

lgtm

http://codereview.chromium.org/10918182/diff/12001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10918182/diff/12001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1644: if (flash_fullscreen)
Use {} for this since the lower arm has it.

[commit-bot: I haz the power, 2012-09-19 17:04:22]

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

[commit-bot: I haz the power, 2012-09-19 18:48:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
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-09-19 21:07:35]

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

[commit-bot: I haz the power, 2012-09-19 22:58:54]

Change committed as 157641

[viettrungluu, 2012-09-20 21:13:50]

https://chromiumcodereview.appspot.com/10918182/diff/15002/ppapi/cpp/private/...
File ppapi/cpp/private/flash_fullscreen.cc (right):

https://chromiumcodereview.appspot.com/10918182/diff/15002/ppapi/cpp/private/...
ppapi/cpp/private/flash_fullscreen.cc:35: return
has_interface<PPB_FlashFullscreen_0_1>() &&
You should really do:

if (has_interface<PPB_FlashFullscreen_1_0>())
  return get_interface<..._1_0>...;
if (has_interface<PPB_FlashFullscreen_0_1>())
  return get_interface<..._0_1>...;
return false;

This makes it possible to actually remove support for 0.1 (in Chrome) at some
point. It also makes it easy to remove the corresponding support in the wrapper.

https://chromiumcodereview.appspot.com/10918182/diff/15002/ppapi/cpp/private/...
ppapi/cpp/private/flash_fullscreen.cc:41: if
(!has_interface<PPB_FlashFullscreen_0_1>())
"

https://chromiumcodereview.appspot.com/10918182/diff/15002/ppapi/cpp/private/...
ppapi/cpp/private/flash_fullscreen.cc:48: if
(!has_interface<PPB_FlashFullscreen_0_1>())
"