Bug 79167

Summary: Expose a setting to exempt media playback from user gesture requirement after a user gesture is initiated on loading/playing a media
Product: WebKit Reporter: Min Qin <qinmin@chromium.org>
Component: New BugsAssignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth@webkit.org, dino@apple.com, eric.carlson@apple.com, jer.noble@apple.com, pnormand@igalia.com, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 66687    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description From 2012-02-21 17:10:15 PST
Expose a setting to exempt media playback from user gesture requirement after a user gesture is initiated on loading/playing a media
------- Comment #1 From 2012-02-21 17:26:51 PST -------
Created an attachment (id=128092) [details]
Patch
------- Comment #2 From 2012-02-21 19:44:43 PST -------
(From update of attachment 128092 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=128092&action=review

You seem to be missing the changes to Source/WebKit/chromium/public, which is presumably useful to you.

> ChangeLog:1
> +2012-02-21  Min Qin  <qinmin@google.com>

This isn't the right ChangeLog file.  These comments should be in Source/WebCore/ChangeLog.  I wonder how you came to edit this file...  ./Tools/Scripts/prepare-ChangeLog should have edited the right file.

> Source/WebCore/html/HTMLMediaElement.cpp:261
> +    if (document->settings() && document->settings()->exemptUserGestureForPlaybackAfterInitiated())
> +        m_userGestureExemptedForPlaybackAfterInitiated = true;
> +    else
> +        m_userGestureExemptedForPlaybackAfterInitiated = false;
> +        

Why not just:

m_userGestureExemptedForPlaybackAfterInitiated = document->settings() && document->settings()->exemptUserGestureForPlaybackAfterInitiated();

?

> Source/WebCore/html/HTMLMediaElement.cpp:2077
> -    if (userGestureRequiredForRateChange() && !ScriptController::processingUserGesture())
> +    if (userGestureRequiredForRateChange() && !ScriptController::processingUserGesture() && !m_userGestureExempted)

Should userGestureRequiredForRateChange() read m_userGestureExempted rather than use reading it directly?

> Source/WebCore/html/HTMLMediaElement.h:596
> +    bool m_userGestureExemptedForPlaybackAfterInitiated: 1;
> +    bool m_userGestureExempted: 1;

m_userGestureExemptedForPlaybackAfterInitiated doesn't seem to be initialized in the constructor.

I'm not in love with these variable names.  Maybe....

m_userGestureExemptedForPlaybackAfterInitiated -> m_shouldSkipUserGestureChecksAfterFirstUserGesture
m_userGestureExempted -> m_skipUserGestureChecks

?
------- Comment #3 From 2012-02-21 20:23:24 PST -------
What's the use case here? I thought we lifted the restriction once we got a user gesture anyway. Or is this for loading subsequent media?
------- Comment #4 From 2012-02-21 20:38:51 PST -------
No, we do not lift the restriction in HTMLMediaElement. 
So if I build chrome with requireUserGestureForMediaPlayback settings on, I have to manually click on the play and pause button to play and pause the media, even if the media is loaded through user gesture.
There is no place we called removeBehaviorRestriction() in chromium.

(In reply to comment #3)
> What's the use case here? I thought we lifted the restriction once we got a user gesture anyway. Or is this for loading subsequent media?
------- Comment #5 From 2012-02-21 21:58:27 PST -------
(In reply to comment #4)
> No, we do not lift the restriction in HTMLMediaElement. 
> So if I build chrome with requireUserGestureForMediaPlayback settings on, I have to manually click on the play and pause button to play and pause the media, even if the media is loaded through user gesture.
> There is no place we called removeBehaviorRestriction() in chromium.
> 
I think we should just clear m_restrictions if loading or playback is initiated by a user gesture. I would suggest adding a function like this:

    void HTMLMediaElement::userRequestsMediaLoading()
    {
        // The user is requesting data loading and/or playback, so remove the "only change playback in response
        // to a user gesture" restriction on this movie.
        m_restrictions = NoRestrictions;
    }

 and then this:

    if (ScriptController::processingUserGesture())
        userRequestsMediaLoading();

to play() and load(), and you won't need any of the rest of your patch.
------- Comment #6 From 2012-02-21 22:02:21 PST -------
> (In reply to comment #4)
> to play() and load(), and you won't need any of the rest of your patch.

You will, however, still need to write a layout test.
------- Comment #7 From 2012-02-21 22:04:04 PST -------
(In reply to comment #5)
> I think we should just clear m_restrictions if loading or playback is initiated by a user gesture.

What about RequirePageConsentToLoadMediaRestriction?  Should be more discriminating about which restrictions we clear?
------- Comment #8 From 2012-02-21 22:06:47 PST -------
(In reply to comment #7)
> (In reply to comment #5)
> > I think we should just clear m_restrictions if loading or playback is initiated by a user gesture.
> 
> What about RequirePageConsentToLoadMediaRestriction?  Should be more discriminating about which restrictions we clear?

I don't think so, because calling play() when nothing is loaded implicitly calls load().
------- Comment #9 From 2012-02-21 22:14:37 PST -------
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > I think we should just clear m_restrictions if loading or playback is initiated by a user gesture.
> > 
> > What about RequirePageConsentToLoadMediaRestriction?  Should be more discriminating about which restrictions we clear?
> 
> I don't think so, because calling play() when nothing is loaded implicitly calls load().

And RequireUserGestureForFullscreenRestriction?
------- Comment #10 From 2012-02-21 22:23:18 PST -------
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #5)
> > > > I think we should just clear m_restrictions if loading or playback is initiated by a user gesture.
> > > 
> > > What about RequirePageConsentToLoadMediaRestriction?  Should be more discriminating about which restrictions we clear?
> > 
> > I don't think so, because calling play() when nothing is loaded implicitly calls load().
> 
> And RequireUserGestureForFullscreenRestriction?

Yes. The user is explicitly requesting playback and loading, so I think all restriction should be relaxed. This what we do on iOS FWIW.
------- Comment #11 From 2012-02-21 23:25:59 PST -------
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > (In reply to comment #5)
> > > > > I think we should just clear m_restrictions if loading or playback is initiated by a user gesture.
> > > > 
> > > > What about RequirePageConsentToLoadMediaRestriction?  Should be more discriminating about which restrictions we clear?
> > > 
> > > I don't think so, because calling play() when nothing is loaded implicitly calls load().
> > 
> > And RequireUserGestureForFullscreenRestriction?
> 
> Yes. The user is explicitly requesting playback and loading, so I think all restriction should be relaxed. This what we do on iOS FWIW.

Wouldn't this break the intended behavior for ports who set only RequireUserGestureForFullscreenRestriction?  (Not that there necessarily are any, but...)  I'm just saying that this change could introduce all kinds of unintended side-effects, not just for the current set of restrictions, but especially if any are added in the future.

Plus, isn't it a bit weird that, with this change, causing a load with a user gesture will clear the full screen restriction, but actually clicking a full screen button won't?
------- Comment #12 From 2012-02-22 06:36:05 PST -------
Maybe we should single out fullscreen restriction?
Another question is that autoplay seems to be an exception against the requireUserGestureForMediaPlayerback. Should we also put a check into the autoplay() function to to return false when the restriction is set?
------- Comment #13 From 2012-02-22 19:30:09 PST -------
Created an attachment (id=128371) [details]
Patch
------- Comment #14 From 2012-02-22 19:36:11 PST -------
Uploaded a new patch according to Eric's suggestion. 
When user gesture is involved in loading/playing a video, I think remove RequireUserGestureForFullscreenRestriction is reasonable. 
There are some sites that will call enterfullscreen when I hit the play button, (Youtube and vimeo mobile sites, for example). However, when the media finishes, if they call exitfullscreen then it never works.
------- Comment #15 From 2012-02-23 08:28:39 PST -------
This change definitely needs a layout test.
------- Comment #16 From 2012-02-23 16:00:51 PST -------
Created an attachment (id=128583) [details]
Patch
------- Comment #17 From 2012-02-23 16:05:14 PST -------
Ok, layout test added.
Since we don't have any layout test for userGestureRequiredForRateChange before, this one should change that situation :-)
------- Comment #18 From 2012-02-23 22:59:55 PST -------
(From update of attachment 128583 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=128583&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:4057
> +void HTMLMediaElement::removeBehaviorsRestrictionsAfterFirstUserGesture()
> +{
> +    m_restrictions = NoRestrictions;
> +}

I'm trying to understand whether we're removing too many restrictions here.  I need to re-read the bug thread.

> Source/WebCore/html/HTMLMediaElement.h:479
> +    // Remove behavior restrictions after user requested load/play

I would skip this comment.  It's pretty redundant with the name of the method.

> LayoutTests/media/video-play-require-user-gesture.html:10
> +            if (window.internals) 
> +	        window.internals.setMediaPlaybackRequiresUserGesture(document, true);

Looks like there's some problem with you indent here.
------- Comment #19 From 2012-02-23 23:01:59 PST -------
(From update of attachment 128583 [details])
Ok, this feels like we're being slightly aggressive in removing these restrictions, but it sounds like folks on the thread are in agreement that this is the right thing to do.
------- Comment #20 From 2012-02-24 09:17:30 PST -------
Created an attachment (id=128745) [details]
Patch
------- Comment #21 From 2012-02-24 09:18:46 PST -------
(In reply to comment #18)
> (From update of attachment 128583 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128583&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4057
> > +void HTMLMediaElement::removeBehaviorsRestrictionsAfterFirstUserGesture()
> > +{
> > +    m_restrictions = NoRestrictions;
> > +}
> 
> I'm trying to understand whether we're removing too many restrictions here.  I need to re-read the bug thread.
> 
> > Source/WebCore/html/HTMLMediaElement.h:479
> > +    // Remove behavior restrictions after user requested load/play
> 
> I would skip this comment.  It's pretty redundant with the name of the method.

Done, comments removed.

> 
> > LayoutTests/media/video-play-require-user-gesture.html:10
> > +            if (window.internals) 
> > +	        window.internals.setMediaPlaybackRequiresUserGesture(document, true);
> 
> Looks like there's some problem with you indent here.

Removed the offending tab and fixed the indent.
------- Comment #22 From 2012-02-24 09:57:52 PST -------
(From update of attachment 128745 [details])
Thanks!
------- Comment #23 From 2012-02-24 11:35:43 PST -------
(From update of attachment 128745 [details])
Clearing flags on attachment: 128745

Committed r108831: <http://trac.webkit.org/changeset/108831>
------- Comment #24 From 2012-02-24 11:35:49 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #25 From 2012-02-27 10:24:22 PST -------
(In reply to comment #24)
> All reviewed patches have been landed.  Closing bug.

It seems this patch made the media tests on GTK (at least) very flaky. They seem to timeout on first pass and pass fine on second pass. Any hint?
------- Comment #26 From 2012-02-27 10:41:43 PST -------
Which test is timing out? Is that the test in this change or tests already exists in WebKit?
It looks to me there were no media tests that used eventSender to emulate user gesture before this change is submitted. All the other media tests should support video.load(), and video.play() without any user interaction. So I doubt whether this change will have any impact on other media tests.
------- Comment #27 From 2012-02-27 11:20:47 PST -------
Sorry, I made a mistake in my previous comment.
There are some tests that are using eventSender. But this change should not affect them.
Here is a list of media tests that rely on eventSender:

These tests do not click the play button, so nothing should happen after user gesture:
audio-delete-while-slider-thumb-clicked.html.
controls-drag-timebar.html
controls-right-click-on-timebar.html
video-volume-slider.html
video-mouse-focus.html
crash-closing-page-with-media-as-plugin-fallback.html
context-menu-actions.html
audio-delete-while-step-button-clicked.html
media-fullscreen-inline.html
media-fullscreen-not-in-document.html

These tests clicks on the play button, however, they just check whether the media is not paused afterwards:
video-controls-transformed.html
video-controls-visible-audio-only.html
video-controls-zoomed.html
------- Comment #28 From 2012-02-27 11:37:02 PST -------
(In reply to comment #27)
> Sorry, I made a mistake in my previous comment.
> There are some tests that are using eventSender. But this change should not affect them.
> Here is a list of media tests that rely on eventSender:
> 
> These tests do not click the play button, so nothing should happen after user gesture:
> audio-delete-while-slider-thumb-clicked.html.
> controls-drag-timebar.html
> controls-right-click-on-timebar.html
> video-volume-slider.html
> video-mouse-focus.html
> crash-closing-page-with-media-as-plugin-fallback.html
> context-menu-actions.html
> audio-delete-while-step-button-clicked.html
> media-fullscreen-inline.html
> media-fullscreen-not-in-document.html
> 
> These tests clicks on the play button, however, they just check whether the media is not paused afterwards:
> video-controls-transformed.html
> video-controls-visible-audio-only.html
> video-controls-zoomed.html

You can see the list of flaky tests there:
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r109009%20(18855)/results.html

I wonder, though. Could the new test affect the test environment so that the tests executed after it run differently? That would explain the flakiness.
For instance I see the new test does:

window.internals.setMediaPlaybackRequiresUserGesture(document, true);

but doesn't seem to revert that before ending.
------- Comment #29 From 2012-02-27 11:59:26 PST -------
Good catch, could be possibly related if the tests are within the same window.

created a fix here: https://bugs.webkit.org/show_bug.cgi?id=79690