Expose a setting to exempt media playback from user gesture requirement after a user gesture is initiated on loading/playing a media

• Component: New Bugs
• Status: RESOLVED FIXED

[Min Qin]

Expose a setting to exempt media playback from user gesture requirement after a user gesture is initiated on loading/playing a media

[Min Qin]

Created attachment 128092 [details]
Patch

[Adam Barth]

Comment on attachment 128092 [details]
Patch

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
> [phone]  Min Qin  <[e-mail]>

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

?

[Dean Jackson]

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?

[Min Qin]

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?

[Eric Carlson]

(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.

[Jer Noble]

> (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.

[Jer Noble]

(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?

[Eric Carlson]

(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().

[Jer Noble]

(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?

[Eric Carlson]

(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.

[Jer Noble]

(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?

[Min Qin]

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?

[Min Qin]

Created attachment 128371 [details]
Patch

[Min Qin]

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.

[Jer Noble]

This change definitely needs a layout test.

[Min Qin]

Created attachment 128583 [details]
Patch

[Min Qin]

Ok, layout test added.
Since we don't have any layout test for userGestureRequiredForRateChange before, this one should change that situation :-)

[Adam Barth]

Comment on attachment 128583 [details]
Patch

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.

[Adam Barth]

Comment on attachment 128583 [details]
Patch

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.

[Min Qin]

Created attachment 128745 [details]
Patch

[Min Qin]

(In reply to comment #18)
> (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.

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.

[Adam Barth]

Comment on attachment 128745 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 128745 [details]
Patch

Clearing flags on attachment: 128745

Committed r108831: <http://trac.webkit.org/changeset/108831>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

(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?

[Min Qin]

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.

[Min Qin]

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

[Philippe Normand]

(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.

[Min Qin]

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