Bug 79167

Created attachment 128092 [details] Patch

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

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?

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?

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

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

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

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

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

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

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

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?

Created attachment 128371 [details] Patch

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.

This change definitely needs a layout test.

Created attachment 128583 [details] Patch

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

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.

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.

Created attachment 128745 [details] Patch

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

Comment on attachment 128745 [details] Patch Thanks!

Comment on attachment 128745 [details] Patch Clearing flags on attachment: 128745 Committed r108831: <http://trac.webkit.org/changeset/108831>

All reviewed patches have been landed. Closing bug.

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

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.

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

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

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