Bug 155141 – Add separate WK and WK2 preferences for requiring user gestures for video media, distinct from user gestures for media generally

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Add separate WK and WK2 preferences for requiring user gestures for video media, distinct from user gestures for media generally

[Jer Noble]

Created attachment 273227 [details]
Patch

[Eric Carlson]

Comment on attachment 273227 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3766
> +//    ASSERT(obscuredInsets.top >= 0);
> +//    ASSERT(obscuredInsets.left >= 0);
> +//    ASSERT(obscuredInsets.bottom >= 0);
> +//    ASSERT(obscuredInsets.right >= 0);

Oops!

[Jer Noble]

Created attachment 273305 [details]
Patch

[Jer Noble]

Created attachment 273397 [details]
Patch

Fixed some broken layout tests, and added a TestWebKitAPI test to ensure these new preferences don't break clients of our existing APIs.

[Jer Noble]

Created attachment 273398 [details]
Patch

[Anders Carlsson]

Comment on attachment 273398 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:65
> +@property (nonatomic) BOOL allowsInlineMediaPlayback;
> +@property (nonatomic) BOOL requiresUserActionForMediaPlayback;

If this is SPI it needs to be prefixed with _.

[Eric Carlson]

Comment on attachment 273398 [details]
Patch

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

> Tools/ChangeLog:10
> +        * TestWebKitAPI/Tests/WebKit/ios/video-with-audio.html:
> +        * TestWebKitAPI/Tests/WebKit/ios/video-without-audio.html:

Should these still be in the ios directory?

> Tools/ChangeLog:32
> [phone]  Jer Noble  <[e-mail]>
> +
> +        Add separate WK and WK2 preferences for requiring user gestures for video media, distinct from user gestures for media generally
> +        https://bugs.webkit.org/show_bug.cgi?id=155141
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Set the default values for media preferences unconditionally, not just on PLATFORM(IOS). Set "video requires user gesture"
> +        to sane defaults in addition to "audio requires user gesture" and (in the case of DumpREnderTree) instead of "media requires user gesture".
> +
> +        * DumpRenderTree/mac/DumpRenderTree.mm:
> +        (resetWebPreferencesToConsistentValues):
> +        * TestWebKitAPI/Tests/WebKit2Cocoa/Coding.mm:
> +        (TEST):

Double entry.

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:331
> +		CD47E7971C8F6FF6004BCFDC /* RequireUserGestureForPlayback.mm in Sources */ = {isa = PBXBuildFile; fileRef = CD47E7961C8F6FF6004BCFDC /* RequireUserGestureForPlayback.mm */; };

It looks like you forgot to add this new test to the repository.

[Jer Noble]

Comment on attachment 273398 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:65
>> +@property (nonatomic) BOOL requiresUserActionForMediaPlayback;
> 
> If this is SPI it needs to be prefixed with _.

They're not really SPI, in that they exist as API for TARGET_OS_IPHONE. I was trying to avoid modifying the WKWebViewConfiguration API (to remove the TARGET_OS_IPHONE conditional), which is why they're here.  Should I add a note saying so, just remove the conditional in the public header, or prefix the !TARGET_OS_IPHONE properties and add a bunch of #if TARGET_OS_IPHOSE conditionals where they're accessed?

[Jer Noble]

Created attachment 273432 [details]
Patch for EWS

[Jer Noble]

Created attachment 273478 [details]
Patch

Removed the non-underscore-prefixed SPI properties from WKWebViewConfigurationPrivate.h and replaced them with nothing.

[Jer Noble]

Created attachment 273484 [details]
Patch

[Jer Noble]

Created attachment 273600 [details]
Patch

[Eric Carlson]

Comment on attachment 273600 [details]
Patch

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

> Tools/ChangeLog:9
> +        to sane defaults in addition to "audio requires user gesture" and (in the case of DumpREnderTree) instead of "media requires user gesture".

Nit: DumpREnderTree

[Eric Carlson]

Comment on attachment 273600 [details]
Patch

These changes look good to  me, but a WK2 reviewer should give it the final nod.

[Jer Noble]

The gtk-wk2 failure is unrelated; the mac build failures are due to changing Settings.in and not having the derived InternalSettingsGenerated.idl get updated by DerivedSources.make. A clean build will solve that build issue.

[Beth Dakin]

Comment on attachment 273600 [details]
Patch

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

r+ for WK2

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:404
> +#endif

You should add a comment after the #endif to say what the #if was. That will make this more readable.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:-110
> -#endif

This might also benefit from a comment for what the #if was.

[Jer Noble]

Committed r197953: <http://trac.webkit.org/changeset/197953>

[Simon Fraser (smfr)]

This broke windows:
https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/93666/steps/compile-webkit/logs/stdio
C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebKit\win\WebView.cpp(5308): error C2039: 'setRequiresUserGestureForVideoPlayback': is not a member of 'WebCore::Settings' [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebKit\WebKit.vcxproj]

[Simon Fraser (smfr)]

The test is also crashing in debug:
https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r197955%20(10663)/results.html

[Jer Noble]

On it.

[Jer Noble]

Committed build-fix, r197988: <http://trac.webkit.org/changeset/197988>

[Jer Noble]

And the crash is the same as <https://bugs.webkit.org/show_bug.cgi?id=155209>, which I'm about to land a fix for.