Add fullscreen support to Pepper.

Add fullscreen support to Pepper.

This needs http://codereview.chromium.org/3320019/show first for the Pepper interfaces to be there.

BUG=none
TEST=run a pepper test application, trigger fullscreen on and off.

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

[darin (slow to review), 2010-09-10 23:51:47]

Can you explain why we need yet another RenderWidgetFullscreenPepper when we
already have RenderWidgetFullscreen.  Can't we reuse the existing
infrastructure?

[darin (slow to review), 2010-09-10 23:54:59]

http://codereview.chromium.org/3352019/diff/2001/3006
File chrome/renderer/render_widget_fullscreen_pepper.cc (right):

http://codereview.chromium.org/3352019/diff/2001/3006#newcode27
chrome/renderer/render_widget_fullscreen_pepper.cc:27: // WebWidget that simply
wraps the pepper plugin.
please use an anonymous namespace for the local classes.

http://codereview.chromium.org/3352019/diff/2001/3006#newcode34
chrome/renderer/render_widget_fullscreen_pepper.cc:34:
cursor_(WebKit::WebCursorInfo::TypePointer) {
no need for the WebKit:: prefix since you have the using declaration above

http://codereview.chromium.org/3352019/diff/2001/3007
File chrome/renderer/render_widget_fullscreen_pepper.h (right):

http://codereview.chromium.org/3352019/diff/2001/3007#newcode19
chrome/renderer/render_widget_fullscreen_pepper.h:19: class
RenderWidgetFullscreenPepper : public RenderWidget {
As I read further, I think I understand better.  Maybe you should consider
subclassing from RenderWidgetFullscreen?

[piman, 2010-09-11 01:00:28]

http://codereview.chromium.org/3352019/diff/2001/3006
File chrome/renderer/render_widget_fullscreen_pepper.cc (right):

http://codereview.chromium.org/3352019/diff/2001/3006#newcode27
chrome/renderer/render_widget_fullscreen_pepper.cc:27: // WebWidget that simply
wraps the pepper plugin.
On 2010/09/10 23:55:00, darin wrote:
> please use an anonymous namespace for the local classes.

Done.

http://codereview.chromium.org/3352019/diff/2001/3006#newcode34
chrome/renderer/render_widget_fullscreen_pepper.cc:34:
cursor_(WebKit::WebCursorInfo::TypePointer) {
On 2010/09/10 23:55:00, darin wrote:
> no need for the WebKit:: prefix since you have the using declaration above

Done.

http://codereview.chromium.org/3352019/diff/2001/3007
File chrome/renderer/render_widget_fullscreen_pepper.h (right):

http://codereview.chromium.org/3352019/diff/2001/3007#newcode19
chrome/renderer/render_widget_fullscreen_pepper.h:19: class
RenderWidgetFullscreenPepper : public RenderWidget {
On 2010/09/10 23:55:00, darin wrote:
> As I read further, I think I understand better.  Maybe you should consider
> subclassing from RenderWidgetFullscreen?

When I first looked at it I didn't see that much possibility of re-use (just one
function really). Reworking a bit RenderWidgetFullscreen it's a little better,
so I did that, PTAL and see if you think it's worth it.

[piman, 2010-09-11 01:01:42]

Adding boliu in case he has comments about the modification/reuse of
RenderWidgetFullscreen

[darin (slow to review), 2010-09-11 06:42:20]

On Fri, Sep 10, 2010 at 6:00 PM, <piman@chromium.org> wrote:

>
> http://codereview.chromium.org/3352019/diff/2001/3006
> File chrome/renderer/render_widget_fullscreen_pepper.cc (right):
>
> http://codereview.chromium.org/3352019/diff/2001/3006#newcode27
> chrome/renderer/render_widget_fullscreen_pepper.cc:27: // WebWidget that
> simply wraps the pepper plugin.
> On 2010/09/10 23:55:00, darin wrote:
>
>> please use an anonymous namespace for the local classes.
>>
>
> Done.
>
>
> http://codereview.chromium.org/3352019/diff/2001/3006#newcode34
> chrome/renderer/render_widget_fullscreen_pepper.cc:34:
> cursor_(WebKit::WebCursorInfo::TypePointer) {
> On 2010/09/10 23:55:00, darin wrote:
>
>> no need for the WebKit:: prefix since you have the using declaration
>>
> above
>
> Done.
>
>
> http://codereview.chromium.org/3352019/diff/2001/3007
> File chrome/renderer/render_widget_fullscreen_pepper.h (right):
>
> http://codereview.chromium.org/3352019/diff/2001/3007#newcode19
> chrome/renderer/render_widget_fullscreen_pepper.h:19: class
> RenderWidgetFullscreenPepper : public RenderWidget {
> On 2010/09/10 23:55:00, darin wrote:
>
>> As I read further, I think I understand better.  Maybe you should
>>
> consider
>
>> subclassing from RenderWidgetFullscreen?
>>
>
> When I first looked at it I didn't see that much possibility of re-use
> (just one function really). Reworking a bit RenderWidgetFullscreen it's
> a little better, so I did that, PTAL and see if you think it's worth it.


Yes, looks better to me.
-Darin


>
>
> http://codereview.chromium.org/3352019/show
>

[brettw, 2010-09-12 16:02:49]

LGTM, not sure if Darin has more comments.

http://codereview.chromium.org/3352019/diff/8001/9002
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/3352019/diff/8001/9002#newcode645
chrome/renderer/pepper_plugin_delegate_impl.cc:645: pepper::FullscreenContainer*
PepperPluginDelegateImpl::CreateFullscreenContainer(
nit: 80 cols

http://codereview.chromium.org/3352019/diff/8001/9009
File chrome/renderer/render_widget_fullscreen_pepper.h (right):

http://codereview.chromium.org/3352019/diff/8001/9009#newcode21
chrome/renderer/render_widget_fullscreen_pepper.h:21: static
RenderWidgetFullscreenPepper* Create(int32 opener_id,
Style nit: since the args don'e fit, indent them all to 4 spaces (incl
opener_id, on the next line).

http://codereview.chromium.org/3352019/diff/8001/9009#newcode48
chrome/renderer/render_widget_fullscreen_pepper.h:48:
scoped_ptr<pepper::FullscreenContainer> container_;
Does DISALLOW_COPY_AND_ASSIGN make sense here?

http://codereview.chromium.org/3352019/diff/8001/9013
File webkit/glue/plugins/pepper_plugin_instance.h (right):

http://codereview.chromium.org/3352019/diff/8001/9013#newcode224
webkit/glue/plugins/pepper_plugin_instance.h:224: FullscreenContainer*
fullscreen_container_;
Can you comment about the cases that this will be NULL? Or maybe you don't even
need fullscreen_ and just key of this being non-null? Then you don't have 2
things to keep in sync.

[piman, 2010-09-13 21:09:47]

Addressed comments. I'll need a LGTM on
http://codereview.chromium.org/3320019/show before this can go in

http://codereview.chromium.org/3352019/diff/8001/9002
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/3352019/diff/8001/9002#newcode645
chrome/renderer/pepper_plugin_delegate_impl.cc:645: pepper::FullscreenContainer*
PepperPluginDelegateImpl::CreateFullscreenContainer(
On 2010/09/12 16:02:49, brettw wrote:
> nit: 80 cols

Done.

http://codereview.chromium.org/3352019/diff/8001/9009
File chrome/renderer/render_widget_fullscreen_pepper.h (right):

http://codereview.chromium.org/3352019/diff/8001/9009#newcode21
chrome/renderer/render_widget_fullscreen_pepper.h:21: static
RenderWidgetFullscreenPepper* Create(int32 opener_id,
On 2010/09/12 16:02:49, brettw wrote:
> Style nit: since the args don'e fit, indent them all to 4 spaces (incl
> opener_id, on the next line).

Done.

http://codereview.chromium.org/3352019/diff/8001/9009#newcode48
chrome/renderer/render_widget_fullscreen_pepper.h:48:
scoped_ptr<pepper::FullscreenContainer> container_;
On 2010/09/12 16:02:49, brettw wrote:
> Does DISALLOW_COPY_AND_ASSIGN make sense here?

Done.

http://codereview.chromium.org/3352019/diff/8001/9013
File webkit/glue/plugins/pepper_plugin_instance.h (right):

http://codereview.chromium.org/3352019/diff/8001/9013#newcode224
webkit/glue/plugins/pepper_plugin_instance.h:224: FullscreenContainer*
fullscreen_container_;
On 2010/09/12 16:02:49, brettw wrote:
> Can you comment about the cases that this will be NULL? Or maybe you don't
even
> need fullscreen_ and just key of this being non-null? Then you don't have 2
> things to keep in sync.

Good point, I've done the latter.

[darin (slow to review), 2010-09-13 22:16:33]

no further comments from me.
-darin

On Sun, Sep 12, 2010 at 9:02 AM, <brettw@chromium.org> wrote:

> LGTM, not sure if Darin has more comments.
>
>
> http://codereview.chromium.org/3352019/diff/8001/9002
> File chrome/renderer/pepper_plugin_delegate_impl.cc (right):
>
> http://codereview.chromium.org/3352019/diff/8001/9002#newcode645
> chrome/renderer/pepper_plugin_delegate_impl.cc:645:
> pepper::FullscreenContainer*
> PepperPluginDelegateImpl::CreateFullscreenContainer(
> nit: 80 cols
>
> http://codereview.chromium.org/3352019/diff/8001/9009
>
> File chrome/renderer/render_widget_fullscreen_pepper.h (right):
>
> http://codereview.chromium.org/3352019/diff/8001/9009#newcode21
> chrome/renderer/render_widget_fullscreen_pepper.h:21: static
> RenderWidgetFullscreenPepper* Create(int32 opener_id,
> Style nit: since the args don'e fit, indent them all to 4 spaces (incl
> opener_id, on the next line).
>
> http://codereview.chromium.org/3352019/diff/8001/9009#newcode48
> chrome/renderer/render_widget_fullscreen_pepper.h:48:
> scoped_ptr<pepper::FullscreenContainer> container_;
> Does DISALLOW_COPY_AND_ASSIGN make sense here?
>
> http://codereview.chromium.org/3352019/diff/8001/9013
> File webkit/glue/plugins/pepper_plugin_instance.h (right):
>
> http://codereview.chromium.org/3352019/diff/8001/9013#newcode224
> webkit/glue/plugins/pepper_plugin_instance.h:224: FullscreenContainer*
> fullscreen_container_;
> Can you comment about the cases that this will be NULL? Or maybe you
> don't even need fullscreen_ and just key of this being non-null? Then
> you don't have 2 things to keep in sync.
>
>
> http://codereview.chromium.org/3352019/show
>

[piman, 2010-09-14 02:52:05]

PTAL: I rebased on t-o-t and it wasn't a trivial merge...

[brettw, 2010-09-14 04:27:21]

This looks nice, LGTM

http://codereview.chromium.org/3352019/diff/22001/23009
File chrome/renderer/render_widget_fullscreen_pepper.h (right):

http://codereview.chromium.org/3352019/diff/22001/23009#newcode49
chrome/renderer/render_widget_fullscreen_pepper.h:49: pepper::PluginInstance*
plugin_;
Google style says only private data, which seems like a reasonable thing to do
here. You just need to add a plugin() getter (you can make it protected if you
want). Don't forget to move container_ too.

[darin (slow to review), 2010-09-14 04:34:50]

http://codereview.chromium.org/3352019/diff/22001/23008
File chrome/renderer/render_widget_fullscreen_pepper.cc (right):

http://codereview.chromium.org/3352019/diff/22001/23008#newcode209
chrome/renderer/render_widget_fullscreen_pepper.cc:209:
didInvalidateRect(gfx::Rect(size_.width(), size_.height()));
i don't think this is the right thing to do for composited pages given nduca's
recent changes.

[Antoine Labour, 2010-09-14 05:09:11]

http://codereview.chromium.org/3352019/diff/22001/23008
File chrome/renderer/render_widget_fullscreen_pepper.cc (right):

http://codereview.chromium.org/3352019/diff/22001/23008#newcode209
chrome/renderer/render_widget_fullscreen_pepper.cc:209:
didInvalidateRect(gfx::Rect(size_.width(), size_.height()));
On 2010/09/14 04:34:50, darin wrote:
> i don't think this is the right thing to do for composited pages given nduca's
> recent changes.

Currently, RenderWidgetFullsceenPepper is forced to be non-composited (see above
in PepperWidget::isAcceleratedCompositingActive). It has its own window, so I
think this should be ok. I'll double-check.

[darin (slow to review), 2010-09-14 05:23:18]

On Mon, Sep 13, 2010 at 10:09 PM, <piman@google.com> wrote:

>
> http://codereview.chromium.org/3352019/diff/22001/23008
> File chrome/renderer/render_widget_fullscreen_pepper.cc (right):
>
> http://codereview.chromium.org/3352019/diff/22001/23008#newcode209
> chrome/renderer/render_widget_fullscreen_pepper.cc:209:
> didInvalidateRect(gfx::Rect(size_.width(), size_.height()));
> On 2010/09/14 04:34:50, darin wrote:
>
>> i don't think this is the right thing to do for composited pages given
>>
> nduca's
>
>> recent changes.
>>
>
> Currently, RenderWidgetFullsceenPepper is forced to be non-composited
> (see above in PepperWidget::isAcceleratedCompositingActive). It has its
> own window, so I think this should be ok. I'll double-check.


Oh, right.  How about adding a TODO for supporting compositing?

-darin




>
>
> http://codereview.chromium.org/3352019/show
>

[piman, 2010-09-14 17:35:05]

http://codereview.chromium.org/3352019/diff/22001/23008
File chrome/renderer/render_widget_fullscreen_pepper.cc (right):

http://codereview.chromium.org/3352019/diff/22001/23008#newcode209
chrome/renderer/render_widget_fullscreen_pepper.cc:209:
didInvalidateRect(gfx::Rect(size_.width(), size_.height()));
On 2010/09/14 05:09:11, Antoine Labour wrote:
> On 2010/09/14 04:34:50, darin wrote:
> > i don't think this is the right thing to do for composited pages given
nduca's
> > recent changes.
> 
> Currently, RenderWidgetFullsceenPepper is forced to be non-composited (see
above
> in PepperWidget::isAcceleratedCompositingActive). It has its own window, so I
> think this should be ok. I'll double-check.
> 
> 

Done. Even if I enable accelerated compositing at the browser level, this still
works with it disabled.
The reason it's not clear it's useful to support it directly, is that this
window only has 1 fullscreen widget that isn't composited at all, so all we need
is a bitmap update path.
That being said, once we switch to scaling the plugin instead of resizing it, it
may make sense to use the accelerated compositing path.

http://codereview.chromium.org/3352019/diff/22001/23009
File chrome/renderer/render_widget_fullscreen_pepper.h (right):

http://codereview.chromium.org/3352019/diff/22001/23009#newcode49
chrome/renderer/render_widget_fullscreen_pepper.h:49: pepper::PluginInstance*
plugin_;
On 2010/09/14 04:27:22, brettw wrote:
> Google style says only private data, which seems like a reasonable thing to do
> here. You just need to add a plugin() getter (you can make it protected if you
> want). Don't forget to move container_ too.

Done.

[brettw, 2010-09-14 17:36:59]

On Tue, Sep 14, 2010 at 10:35 AM,  <piman@chromium.org> wrote:
>
> http://codereview.chromium.org/3352019/diff/22001/23008
> File chrome/renderer/render_widget_fullscreen_pepper.cc (right):
>
> http://codereview.chromium.org/3352019/diff/22001/23008#newcode209
> chrome/renderer/render_widget_fullscreen_pepper.cc:209:
> didInvalidateRect(gfx::Rect(size_.width(), size_.height()));
> On 2010/09/14 05:09:11, Antoine Labour wrote:
>>
>> On 2010/09/14 04:34:50, darin wrote:
>> > i don't think this is the right thing to do for composited pages
>
> given nduca's
>>
>> > recent changes.
>
>> Currently, RenderWidgetFullsceenPepper is forced to be non-composited
>
> (see above
>>
>> in PepperWidget::isAcceleratedCompositingActive). It has its own
>
> window, so I
>>
>> think this should be ok. I'll double-check.
>
>
>
> Done. Even if I enable accelerated compositing at the browser level,
> this still works with it disabled.
> The reason it's not clear it's useful to support it directly, is that
> this window only has 1 fullscreen widget that isn't composited at all,
> so all we need is a bitmap update path.
> That being said, once we switch to scaling the plugin instead of
> resizing it, it may make sense to use the accelerated compositing path.

We'll also want to support the 3D API (eventually) in full screen
mode. I'm not sure what this means, but probably it will mean running
the compositor. Perhaps we can optimize this case since there's not
actually anything to composite (just the one context).

Brett

[darin (slow to review), 2010-09-14 17:37:14]

On Tue, Sep 14, 2010 at 10:35 AM, <piman@chromium.org> wrote:

>
> http://codereview.chromium.org/3352019/diff/22001/23008
> File chrome/renderer/render_widget_fullscreen_pepper.cc (right):
>
> http://codereview.chromium.org/3352019/diff/22001/23008#newcode209
> chrome/renderer/render_widget_fullscreen_pepper.cc:209:
> didInvalidateRect(gfx::Rect(size_.width(), size_.height()));
> On 2010/09/14 05:09:11, Antoine Labour wrote:
>
>> On 2010/09/14 04:34:50, darin wrote:
>> > i don't think this is the right thing to do for composited pages
>>
> given nduca's
>
>> > recent changes.
>>
>
>  Currently, RenderWidgetFullsceenPepper is forced to be non-composited
>>
> (see above
>
>> in PepperWidget::isAcceleratedCompositingActive). It has its own
>>
> window, so I
>
>> think this should be ok. I'll double-check.
>>
>
>
>
> Done. Even if I enable accelerated compositing at the browser level,
> this still works with it disabled.
> The reason it's not clear it's useful to support it directly, is that
> this window only has 1 fullscreen widget that isn't composited at all,
> so all we need is a bitmap update path.
> That being said, once we switch to scaling the plugin instead of
> resizing it, it may make sense to use the accelerated compositing path.


I was thinking about the case where the plugin uses Graphics3D as its
output.
-Darin



>
>
> http://codereview.chromium.org/3352019/diff/22001/23009
> File chrome/renderer/render_widget_fullscreen_pepper.h (right):
>
> http://codereview.chromium.org/3352019/diff/22001/23009#newcode49
> chrome/renderer/render_widget_fullscreen_pepper.h:49:
> pepper::PluginInstance* plugin_;
> On 2010/09/14 04:27:22, brettw wrote:
>
>> Google style says only private data, which seems like a reasonable
>>
> thing to do
>
>> here. You just need to add a plugin() getter (you can make it
>>
> protected if you
>
>> want). Don't forget to move container_ too.
>>
>
> Done.
>
>
> http://codereview.chromium.org/3352019/show
>

[brettw, 2010-09-14 17:49:07]

On Tue, Sep 14, 2010 at 10:37 AM, Darin Fisher <darin@chromium.org> wrote:
> On Tue, Sep 14, 2010 at 10:35 AM, <piman@chromium.org> wrote:
>>
>> http://codereview.chromium.org/3352019/diff/22001/23008
>> File chrome/renderer/render_widget_fullscreen_pepper.cc (right):
>>
>> http://codereview.chromium.org/3352019/diff/22001/23008#newcode209
>> chrome/renderer/render_widget_fullscreen_pepper.cc:209:
>> didInvalidateRect(gfx::Rect(size_.width(), size_.height()));
>> On 2010/09/14 05:09:11, Antoine Labour wrote:
>>>
>>> On 2010/09/14 04:34:50, darin wrote:
>>> > i don't think this is the right thing to do for composited pages
>>
>> given nduca's
>>>
>>> > recent changes.
>>
>>> Currently, RenderWidgetFullsceenPepper is forced to be non-composited
>>
>> (see above
>>>
>>> in PepperWidget::isAcceleratedCompositingActive). It has its own
>>
>> window, so I
>>>
>>> think this should be ok. I'll double-check.
>>
>>
>>
>> Done. Even if I enable accelerated compositing at the browser level,
>> this still works with it disabled.
>> The reason it's not clear it's useful to support it directly, is that
>> this window only has 1 fullscreen widget that isn't composited at all,
>> so all we need is a bitmap update path.
>> That being said, once we switch to scaling the plugin instead of
>> resizing it, it may make sense to use the accelerated compositing path.
>
> I was thinking about the case where the plugin uses Graphics3D as its
> output.

The most important use case is 2D for Flash right now, so I don't want
to hold up that for the 3D case. But clearly the 3D case will be the
long-term major use-case for full screen and we should design it so
that it's easy to add soonish.

Brett

[darin (slow to review), 2010-09-14 18:00:24]

On Tue, Sep 14, 2010 at 10:48 AM, Brett Wilson <brettw@chromium.org> wrote:

> On Tue, Sep 14, 2010 at 10:37 AM, Darin Fisher <darin@chromium.org> wrote:
> > On Tue, Sep 14, 2010 at 10:35 AM, <piman@chromium.org> wrote:
> >>
> >> http://codereview.chromium.org/3352019/diff/22001/23008
> >> File chrome/renderer/render_widget_fullscreen_pepper.cc (right):
> >>
> >> http://codereview.chromium.org/3352019/diff/22001/23008#newcode209
> >> chrome/renderer/render_widget_fullscreen_pepper.cc:209:
> >> didInvalidateRect(gfx::Rect(size_.width(), size_.height()));
> >> On 2010/09/14 05:09:11, Antoine Labour wrote:
> >>>
> >>> On 2010/09/14 04:34:50, darin wrote:
> >>> > i don't think this is the right thing to do for composited pages
> >>
> >> given nduca's
> >>>
> >>> > recent changes.
> >>
> >>> Currently, RenderWidgetFullsceenPepper is forced to be non-composited
> >>
> >> (see above
> >>>
> >>> in PepperWidget::isAcceleratedCompositingActive). It has its own
> >>
> >> window, so I
> >>>
> >>> think this should be ok. I'll double-check.
> >>
> >>
> >>
> >> Done. Even if I enable accelerated compositing at the browser level,
> >> this still works with it disabled.
> >> The reason it's not clear it's useful to support it directly, is that
> >> this window only has 1 fullscreen widget that isn't composited at all,
> >> so all we need is a bitmap update path.
> >> That being said, once we switch to scaling the plugin instead of
> >> resizing it, it may make sense to use the accelerated compositing path.
> >
> > I was thinking about the case where the plugin uses Graphics3D as its
> > output.
>
> The most important use case is 2D for Flash right now, so I don't want
> to hold up that for the 3D case. But clearly the 3D case will be the
> long-term major use-case for full screen and we should design it so
> that it's easy to add soonish.
>
> Brett
>


Totally agree!