Bug 167132 – Pass down website autoplay policies to media elements

• Component: Media
• Status: RESOLVED DUPLICATE

[Matt Rajca]

Website autoplay policies passed to the web frame loader should be respected by media elements.

[Matt Rajca]

Created attachment 299054 [details]
Patch

[Build Bot]

Comment on attachment 299054 [details]
Patch

Attachment 299054 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2905646

New failing tests:
imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html

[Build Bot]

Created attachment 299059 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Matt Rajca]

Created attachment 299073 [details]
Patch

[Matt Rajca]

(In reply to comment #4)
> Created attachment 299073 [details]
> Patch

I don't think that bot failure is related. That test was added just a few hours ago.

[Build Bot]

Comment on attachment 299073 [details]
Patch

Attachment 299073 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2906401

New failing tests:
imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html

[Build Bot]

Created attachment 299087 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Matt Rajca]

Working on a new patch with tests that also run on iOS.

[Alex Christensen]

Comment on attachment 299073 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299073&action=review

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:821
> +    if (coreFrame && !websitePolicies.autoplayEnabled) {

I don't think we need to check coreFrame.  It was dereferenced on line 804, so if it were null we would have already crashed.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:824
> +        settings.setVideoPlaybackRequiresUserGesture(true);
> +        settings.setAudioPlaybackRequiresUserGesture(true);

Are these values set back to their default value for the next navigation?  Add a test that sets autoplayEnabled back to true and does another navigation to make sure that it behaves as expected.

> Tools/TestWebKitAPI/Tests/WebKit2/autoplayCheck.html:12
> +                }, 100)

Is 100ms enough time for the video to have always started playing when it is allowed to?

[Matt Rajca]

(In reply to comment #9)
> Comment on attachment 299073 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299073&action=review
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:821
> > +    if (coreFrame && !websitePolicies.autoplayEnabled) {
> 
> I don't think we need to check coreFrame.  It was dereferenced on line 804,
> so if it were null we would have already crashed.

Removed.

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:824
> > +        settings.setVideoPlaybackRequiresUserGesture(true);
> > +        settings.setAudioPlaybackRequiresUserGesture(true);
> 
> Are these values set back to their default value for the next navigation? 
> Add a test that sets autoplayEnabled back to true and does another
> navigation to make sure that it behaves as expected.

Good catch. I polished this up and added a test.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2/autoplayCheck.html:12
> > +                }, 100)
> 
> Is 100ms enough time for the video to have always started playing when it is
> allowed to?

It should be, given the video file is around 100 KB and loaded locally.

[Matt Rajca]

Created attachment 299267 [details]
Patch

[Build Bot]

Comment on attachment 299267 [details]
Patch

Attachment 299267 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2916574

New failing tests:
media/ios/autoplay-only-in-main-document.html

[Build Bot]

Created attachment 299272 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Matt Rajca]

Created attachment 299275 [details]
Patch

[Matt Rajca]

Created attachment 299278 [details]
Patch

[Alex Christensen]

Comment on attachment 299278 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299278&action=review

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:821
> +    coreFrame->settings().setAudioPlaybackRequiresUserGesture(!websitePolicies.autoplayEnabled);

This overrides the existing settings.setAudioPlaybackRequiresUserGesture.  We should remove that if it no longer works.

[WebKit Commit Bot]

Comment on attachment 299278 [details]
Patch

Rejecting attachment 299278 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
TestWebKitAPI/Tests/WebKit2/ForceRepaint.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPILibrary.build/Objects-normal/x86_64/ForceRepaint.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPILibrary.build/Objects-normal/x86_64/WebsitePolicies.o Tests/WebKit2Cocoa/WebsitePolicies.mm normal x86_64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/2936483

[Matt Rajca]

Created attachment 299532 [details]
Patch

[Matt Rajca]

Created attachment 299537 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 299537 [details]
Patch

Clearing flags on attachment: 299537

Committed r211062: <http://trac.webkit.org/changeset/211062>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This change appears to have caused 4 API test failures:

Tests that failed:
  RequiresUserActionForPlaybackTest.DeprecatedRequiresUserActionForAudioAndVideoPlayback
  RequiresUserActionForPlaybackTest.DeprecatedRequiresUserActionForAudioButNotVideoPlayback
  RequiresUserActionForPlaybackTest.RequiresUserActionForAudioAndVideoPlayback
  RequiresUserActionForPlaybackTest.RequiresUserActionForAudioButNotVideoPlayback

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20(Tests)/builds/3030

[Matt Rajca]

(In reply to comment #22)
> This change appears to have caused 4 API test failures:
> 
> Tests that failed:
>  
> RequiresUserActionForPlaybackTest.
> DeprecatedRequiresUserActionForAudioAndVideoPlayback
>  
> RequiresUserActionForPlaybackTest.
> DeprecatedRequiresUserActionForAudioButNotVideoPlayback
>  
> RequiresUserActionForPlaybackTest.RequiresUserActionForAudioAndVideoPlayback
>  
> RequiresUserActionForPlaybackTest.
> RequiresUserActionForAudioButNotVideoPlayback
> 
> https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20(Tests)/
> builds/3030

A follow-up patch that addresses these is almost ready.

[Matt Rajca]

https://bugs.webkit.org/show_bug.cgi?id=167346 is tracking a fix. Just waiting for one other patch to land on the commit queue. Thanks for the heads up!

[WebKit Commit Bot]

Re-opened since this is blocked by bug 167354

[Matt Rajca]

Take 2 of this work has landed with regressions fixed: https://bugs.webkit.org/show_bug.cgi?id=167355

*** This bug has been marked as a duplicate of bug 167355 ***