Support mouse lock in Flash fullscreen mode.

Support mouse lock in Flash fullscreen mode.

In this mode, we won't show a bubble to ask for user permission. Mouse lock content settings are not considered, either.

BUG=129241
TEST=1) run ppapi/examples/mouse_lock;
     2) when one of the plugins is focused, press 'f' to enter Flash fullscreen;
     3) left click to enter mouse lock.


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

[yzshen1, 2012-05-29 08:06:05]

Hi, Brett and Vincent.

Would you please take a look? Thanks!

[yzshen1, 2012-05-29 08:06:06]

Hi, Brett and Vincent.

Would you please take a look? Thanks!

[brettw, 2012-05-29 20:20:09]

http://codereview.chromium.org/10458008/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/10458008/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host_impl.cc:1500: if (privileged) {
Are there security considerations here? I'm not sure if we consider mouse lock
privileged or whether this capability already exists. If this is changing the
capabilities an owned renderer has, you may want to run this by Justin.

[yzshen1, 2012-05-29 20:45:19]

On 2012/05/29 20:20:09, brettw wrote:
>
http://codereview.chromium.org/10458008/diff/1/content/browser/renderer_host/...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
http://codereview.chromium.org/10458008/diff/1/content/browser/renderer_host/...
> content/browser/renderer_host/render_widget_host_impl.cc:1500: if (privileged)
{
> Are there security considerations here? I'm not sure if we consider mouse lock
> privileged or whether this capability already exists. If this is changing the
> capabilities an owned renderer has, you may want to run this by Justin.

[+CC Justin]

Thanks Brett!

If the renderer is owned, then it is capable of starting mouse lock without
user's permission. Mouse lock won't succeed if the renderer is not focused. And
mouse lock will be revoked when the renderer loses focus.

I am not sure whether this significantly changes the capabilities of an owned
renderer. Currently, we already have ViewHostMsg_SetCursor, so an owned renderer
can hide/change cursor, but IIRC it only affects the cursor when it hovers over
the owned renderer.

Justin: Would you please take a look? Thanks!

[brettw, 2012-05-29 21:27:10]

lgtm except for this code move.

http://codereview.chromium.org/10458008/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10458008/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:452: class RenderViewMouseLockDispatcher :
public MouseLockDispatcher,
Could you move this to a separate file for just this class? I'm loath to make
render_view_impl.cc any longer...

[scheib, 2012-05-29 21:47:14]

Will any other content be able to request Mouse Lock while flash is fullscreen?
I'm concerned about bypassing the logic in the fullscreen_controller, and not
even informing it. Note, we have a change in flight
http://codereview.chromium.org/10443045/ that will allow entry to mouse lock
with no notification to the user.

http://codereview.chromium.org/10458008/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10458008/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:450: 
Consider moving to a new implementation file? render_view_impl.cc is already
huge, and the initial goal of creating the mouse lock dispatcher object was to
move details out.

http://codereview.chromium.org/10458008/diff/1/ppapi/examples/mouse_lock/mous...
File ppapi/examples/mouse_lock/mouse_lock.cc (right):

http://codereview.chromium.org/10458008/diff/1/ppapi/examples/mouse_lock/mous...
ppapi/examples/mouse_lock/mouse_lock.cc:78: } else if (key_event.GetKeyCode() ==
70) {
Document in HTML how and why to enter this mode.

[yzshen1, 2012-05-30 06:41:30]

> Will any other content be able to request Mouse Lock while flash is
fullscreen?
Please note that flash fullscreen is actually a different window, and it won't
be closed when losing focus.

So yes, it is possible for other contents on the web page (using Pepper API or
JS) to request mouse lock while flash is fullscreen. It will succeed if the web
page has focus (and fullscreen_controller is happy).

However, mouse lock will be revoked when losing focus. This rule holds true for
both normal mouse lock and flash fullscreen mouse lock. Therefore, it will never
happen that mouse lock is granted to two or more targets at the same time. We
should be safe.

> I'm concerned about bypassing the logic in the fullscreen_controller, and not
> even informing it.
I also considered that. However, it should be noted that flash fullscreen has
nothing to do with fullscreen_controller either. IMHO, mouse lock in flash
fullscreen mode should be treated in the same way as flash fullscreen. Getting
fullscreen_controller involved seems more confusing.

But I understand your concern. Maybe we could have a face-to-face discussion
about this.
 
> Note, we have a change in flight 
> http://codereview.chromium.org/10443045/ that will allow entry to mouse lock
> with no notification to the user.

http://codereview.chromium.org/10458008/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10458008/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:450: 
Good point. Thanks!

On 2012/05/29 21:47:14, scheib wrote:
> Consider moving to a new implementation file? render_view_impl.cc is already
> huge, and the initial goal of creating the mouse lock dispatcher object was to
> move details out.

http://codereview.chromium.org/10458008/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:452: class RenderViewMouseLockDispatcher :
public MouseLockDispatcher,
Done. Thanks!

On 2012/05/29 21:27:10, brettw wrote:
> Could you move this to a separate file for just this class? I'm loath to make
> render_view_impl.cc any longer...

http://codereview.chromium.org/10458008/diff/1/ppapi/examples/mouse_lock/mous...
File ppapi/examples/mouse_lock/mouse_lock.cc (right):

http://codereview.chromium.org/10458008/diff/1/ppapi/examples/mouse_lock/mous...
ppapi/examples/mouse_lock/mouse_lock.cc:78: } else if (key_event.GetKeyCode() ==
70) {
I have revised the HTML page.
I didn't do it because I was thinking that we might not want to explicitly talk
about Flash-private feature in the example page. But with a second thought, I
think it is okay to do so. (Of course, we should not do this in the NaCl
example.)

On 2012/05/29 21:47:14, scheib wrote:
> Document in HTML how and why to enter this mode.

[brettw, 2012-05-30 14:32:46]

lgtm

[scheib, 2012-05-30 14:56:50]

lgtm

[commit-bot: I haz the power, 2012-05-31 20:59:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/10458008/12021

[yzshen1, 2012-05-31 21:00:55]

[in order to make this thread complete]
Justin and I have talked and we agreed that we could limit the ability of
privileged mouse lock to fullscreen render widgets. And this check should be
done at the browser side.


On 2012/05/29 20:45:19, yzshen1 wrote:
> On 2012/05/29 20:20:09, brettw wrote:
> >
>
http://codereview.chromium.org/10458008/diff/1/content/browser/renderer_host/...
> > File content/browser/renderer_host/render_widget_host_impl.cc (right):
> > 
> >
>
http://codereview.chromium.org/10458008/diff/1/content/browser/renderer_host/...
> > content/browser/renderer_host/render_widget_host_impl.cc:1500: if
(privileged)
> {
> > Are there security considerations here? I'm not sure if we consider mouse
lock
> > privileged or whether this capability already exists. If this is changing
the
> > capabilities an owned renderer has, you may want to run this by Justin.
> 
> [+CC Justin]
> 
> Thanks Brett!
> 
> If the renderer is owned, then it is capable of starting mouse lock without
> user's permission. Mouse lock won't succeed if the renderer is not focused.
And
> mouse lock will be revoked when the renderer loses focus.
> 
> I am not sure whether this significantly changes the capabilities of an owned
> renderer. Currently, we already have ViewHostMsg_SetCursor, so an owned
renderer
> can hide/change cursor, but IIRC it only affects the cursor when it hovers
over
> the owned renderer.
> 
> Justin: Would you please take a look? Thanks!

[commit-bot: I haz the power, 2012-05-31 21:24:18]

Try job failure for 10458008-12021 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-06-03 07:15:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/10458008/15001

[commit-bot: I haz the power, 2012-06-03 07:15:37]

Failed to apply patch for
content/browser/renderer_host/render_widget_host_impl.cc:
While running patch -p1 --forward --force;
patching file content/browser/renderer_host/render_widget_host_impl.cc
Hunk #3 FAILED at 1488.
Hunk #4 FAILED at 1498.
2 out of 4 hunks FAILED -- saving rejects to file
content/browser/renderer_host/render_widget_host_impl.cc.rej

[commit-bot: I haz the power, 2012-06-03 08:48:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/10458008/19002

[commit-bot: I haz the power, 2012-06-03 10:29:27]

Change committed as 140231