Make mediaPlayerRequestFullscreen() user gesture bypass explicit

Make mediaPlayerRequestFullscreen() user gesture bypass explicit

mediaPlayerRequestFullscreen() is only ever called from Chromium's
BrowserMediaPlayerManager::OnProtectedSurfaceRequested() when the
--disable-gesture-requirement-for-media-fullscreen flag is used.

Make this behavior explicit, limiting it to just this case instead of
disabling it even for script-initiated fullscreen requests.

This will allow removing the mediaFullscreenRequiresUserGesture setting
once Chromium has been updated.

BUG=355439
TEST=LayoutTests/media/media-player-request-fullscreen.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170080

[qinmin, 2014-03-24 06:32:53]

https://codereview.chromium.org/209693003/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/209693003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2855: UserGestureIndicator
gestureIndicator(DefinitelyProcessingNewUserGesture);
will the gesture be consumed by FullscreenElementStack? Otherwise, it will
invoke some other functions that require user gesture.

[philipj_slow, 2014-03-24 15:18:23]

https://codereview.chromium.org/209693003/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/209693003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2855: UserGestureIndicator
gestureIndicator(DefinitelyProcessingNewUserGesture);
On 2014/03/24 06:32:54, qinmin wrote:
> will the gesture be consumed by FullscreenElementStack? Otherwise, it will
> invoke some other functions that require user gesture.

The UserGestureIndicator is only active while it's on the stack, see its
constructor and destructor. No events are dispatches synchronously in
FullscreenElementStack::requestFullScreenForElement so AFAICT there's no way for
scripts to run and abuse the user gesture for something else.

As for consuming gestures, it seems like that's only done from Chromium and
internally in UserGestureIndicator, so I don't think there's anything I should
do with that.

[qinmin, 2014-03-24 16:03:32]

lgtm

[acolwell GONE FROM CHROMIUM, 2014-03-25 22:14:42]

https://codereview.chromium.org/209693003/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/209693003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2855: UserGestureIndicator
gestureIndicator(DefinitelyProcessingNewUserGesture);
On 2014/03/24 15:18:23, philipj wrote:
> On 2014/03/24 06:32:54, qinmin wrote:
> > will the gesture be consumed by FullscreenElementStack? Otherwise, it will
> > invoke some other functions that require user gesture.
> 
> The UserGestureIndicator is only active while it's on the stack, see its
> constructor and destructor. No events are dispatches synchronously in
> FullscreenElementStack::requestFullScreenForElement so AFAICT there's no way
for
> scripts to run and abuse the user gesture for something else.
> 
> As for consuming gestures, it seems like that's only done from Chromium and
> internally in UserGestureIndicator, so I don't think there's anything I should
> do with that.

How about we just move the settings code from FullscreenElementStack.cpp to here
so the code looks something like?

if (!document()->settings()->mediaFullscreenRequiresUserGesture()) {
  UserGestureIndicator gestureIndicator(DefinitelyProcessingNewUserGesture);
  enterFullscreen();
  return;
}

enterFullscreen();

It is a slight duplication in code, but it puts all the logic in one place.

[philipj_slow, 2014-03-26 07:48:17]

https://codereview.chromium.org/209693003/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/209693003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2855: UserGestureIndicator
gestureIndicator(DefinitelyProcessingNewUserGesture);
On 2014/03/25 22:14:43, acolwell wrote:
> On 2014/03/24 15:18:23, philipj wrote:
> > On 2014/03/24 06:32:54, qinmin wrote:
> > > will the gesture be consumed by FullscreenElementStack? Otherwise, it will
> > > invoke some other functions that require user gesture.
> > 
> > The UserGestureIndicator is only active while it's on the stack, see its
> > constructor and destructor. No events are dispatches synchronously in
> > FullscreenElementStack::requestFullScreenForElement so AFAICT there's no way
> for
> > scripts to run and abuse the user gesture for something else.
> > 
> > As for consuming gestures, it seems like that's only done from Chromium and
> > internally in UserGestureIndicator, so I don't think there's anything I
should
> > do with that.
> 
> How about we just move the settings code from FullscreenElementStack.cpp to
here
> so the code looks something like?
> 
> if (!document()->settings()->mediaFullscreenRequiresUserGesture()) {
>   UserGestureIndicator gestureIndicator(DefinitelyProcessingNewUserGesture);
>   enterFullscreen();
>   return;
> }
> 
> enterFullscreen();
> 
> It is a slight duplication in code, but it puts all the logic in one place.

Sorry, I didn't communicate my intentions very clearly. I mean to remove the
setting entirely so that the code in
FullscreenElementStack::requestFullScreenForElement becomes just:

if (!UserGestureIndicator::processingUserGesture())
    break;

The reason I didn't do it in this CL is because there's a tests for the setting
that would start failing, and I'd rather fix that when removing the setting
entirely. Here are the two CLs that would finish the job:

https://codereview.chromium.org/212133003/
https://codereview.chromium.org/212323003/

However, if you want to keep the setting, I agree that moving it as suggested
would be an improvement, since it limits the exemption to just
mediaPlayerRequestFullscreen(). Let me know what you prefer, even if it's
leaving things as they are.

[acolwell GONE FROM CHROMIUM, 2014-03-26 17:39:42]

lgtm

https://codereview.chromium.org/209693003/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/209693003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2855: UserGestureIndicator
gestureIndicator(DefinitelyProcessingNewUserGesture);
On 2014/03/26 07:48:17, philipj wrote:
> On 2014/03/25 22:14:43, acolwell wrote:
> > On 2014/03/24 15:18:23, philipj wrote:
> > > On 2014/03/24 06:32:54, qinmin wrote:
> > > > will the gesture be consumed by FullscreenElementStack? Otherwise, it
will
> > > > invoke some other functions that require user gesture.
> > > 
> > > The UserGestureIndicator is only active while it's on the stack, see its
> > > constructor and destructor. No events are dispatches synchronously in
> > > FullscreenElementStack::requestFullScreenForElement so AFAICT there's no
way
> > for
> > > scripts to run and abuse the user gesture for something else.
> > > 
> > > As for consuming gestures, it seems like that's only done from Chromium
and
> > > internally in UserGestureIndicator, so I don't think there's anything I
> should
> > > do with that.
> > 
> > How about we just move the settings code from FullscreenElementStack.cpp to
> here
> > so the code looks something like?
> > 
> > if (!document()->settings()->mediaFullscreenRequiresUserGesture()) {
> >   UserGestureIndicator gestureIndicator(DefinitelyProcessingNewUserGesture);
> >   enterFullscreen();
> >   return;
> > }
> > 
> > enterFullscreen();
> > 
> > It is a slight duplication in code, but it puts all the logic in one place.
> 
> Sorry, I didn't communicate my intentions very clearly. I mean to remove the
> setting entirely so that the code in
> FullscreenElementStack::requestFullScreenForElement becomes just:
> 
> if (!UserGestureIndicator::processingUserGesture())
>     break;
> 
> The reason I didn't do it in this CL is because there's a tests for the
setting
> that would start failing, and I'd rather fix that when removing the setting
> entirely. Here are the two CLs that would finish the job:
> 
> https://codereview.chromium.org/212133003/
> https://codereview.chromium.org/212323003/
> 
> However, if you want to keep the setting, I agree that moving it as suggested
> would be an improvement, since it limits the exemption to just
> mediaPlayerRequestFullscreen(). Let me know what you prefer, even if it's
> leaving things as they are.

Ok. Given the other 2 CLs, I'm fine with doing deferring the
FullscreenElementStack change until https://codereview.chromium.org/212323003/

[philipj_slow, 2014-03-26 17:41:41]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-03-26 17:41:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/209693003/1

[commit-bot: I haz the power, 2014-03-26 18:04:17]

Change committed as 170080