Add separate WK and WK2 preferences for requiring user gestures for video media, distinct from user gestures for media generally
Created attachment 273227 [details] Patch
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!
Created attachment 273305 [details] Patch
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.
Created attachment 273398 [details] Patch
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 _.
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 > +2016-03-07 Jer Noble <jer.noble@apple.com> > + > + 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.
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?
Created attachment 273432 [details] Patch for EWS
Created attachment 273478 [details] Patch Removed the non-underscore-prefixed SPI properties from WKWebViewConfigurationPrivate.h and replaced them with nothing.
Created attachment 273484 [details] Patch
Created attachment 273600 [details] Patch
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
Comment on attachment 273600 [details] Patch These changes look good to me, but a WK2 reviewer should give it the final nod.
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.
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.
Committed r197953: <http://trac.webkit.org/changeset/197953>
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]
The test is also crashing in debug: https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r197955%20(10663)/results.html
On it.
Committed build-fix, r197988: <http://trac.webkit.org/changeset/197988>
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.