| Summary: | [iOS] Make the AllowsInlineMediaPlayback preference work in WebKit / WebKit2. | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Jer Noble <jer.noble> | ||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | aestes, buildbot, cdumez, commit-queue, dbates, eric.carlson, rniwa | ||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||||||
| Bug Blocks: | 148579 | ||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||
|
Description
Jer Noble
2015-07-31 15:09:15 PDT
Created attachment 257976 [details]
WIP
Created attachment 257993 [details]
WIP
Comment on attachment 257993 [details] WIP Attachment 257993 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5000 New failing tests: media/video-frame-accurate-seek.html http/tests/css/vertical-align-baseline-after-image-load-2.html media/video-aspect-ratio.html compositing/video/video-object-fit.html media/video-transformed.html media/video-canvas-alpha.html media/video-fullscreeen-only-controls.html media/track/track-cue-rendering-horizontal.html media/video-controls-visible-audio-only.html media/video-remove-insert-repaints.html http/tests/css/vertical-align-baseline-after-image-load-3.html compositing/overflow/overflow-compositing-descendant.html media/video-controls-visible-exiting-fullscreen.html compositing/layers-inside-overflow-scroll.html media/video-zoom.html compositing/self-painting-layers.html compositing/overflow/scroll-ancestor-update.html compositing/geometry/video-opacity-overlay.html compositing/geometry/video-fixed-scrolling.html fast/css/object-fit/object-fit-video-poster.html media/media-captions-no-controls.html compositing/video/video-poster.html http/tests/css/vertical-align-baseline-after-image-load.html Created attachment 257995 [details]
Archive of layout-test-results from ews102 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 257993 [details] WIP Attachment 257993 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5038 New failing tests: media/video-frame-accurate-seek.html http/tests/css/vertical-align-baseline-after-image-load-2.html media/video-aspect-ratio.html compositing/video/video-object-fit.html media/video-transformed.html media/video-canvas-alpha.html media/video-fullscreeen-only-controls.html media/track/track-cue-rendering-horizontal.html media/video-controls-visible-audio-only.html media/video-remove-insert-repaints.html http/tests/contentextensions/text-track-blocked.html http/tests/css/vertical-align-baseline-after-image-load-3.html compositing/overflow/overflow-compositing-descendant.html media/video-controls-visible-exiting-fullscreen.html compositing/layers-inside-overflow-scroll.html media/video-zoom.html compositing/self-painting-layers.html compositing/overflow/scroll-ancestor-update.html compositing/geometry/video-opacity-overlay.html http/tests/contentextensions/media-filtered.html compositing/geometry/video-fixed-scrolling.html fast/css/object-fit/object-fit-video-poster.html media/media-captions-no-controls.html compositing/video/video-poster.html http/tests/css/vertical-align-baseline-after-image-load.html Created attachment 257997 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 258066 [details]
WIP
Comment on attachment 258066 [details] WIP Attachment 258066 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/12595 New failing tests: media/video-frame-accurate-seek.html http/tests/css/vertical-align-baseline-after-image-load-2.html media/video-aspect-ratio.html compositing/video/video-object-fit.html media/video-transformed.html media/video-canvas-alpha.html media/video-fullscreeen-only-controls.html media/track/track-cue-rendering-horizontal.html media/video-controls-visible-audio-only.html media/video-remove-insert-repaints.html http/tests/css/vertical-align-baseline-after-image-load-3.html compositing/overflow/overflow-compositing-descendant.html media/video-controls-visible-exiting-fullscreen.html compositing/layers-inside-overflow-scroll.html media/video-zoom.html compositing/self-painting-layers.html compositing/overflow/scroll-ancestor-update.html compositing/geometry/video-opacity-overlay.html compositing/geometry/video-fixed-scrolling.html fast/css/object-fit/object-fit-video-poster.html media/media-captions-no-controls.html compositing/video/video-poster.html http/tests/css/vertical-align-baseline-after-image-load.html Created attachment 258115 [details]
Archive of layout-test-results from ews100 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 258066 [details] WIP Attachment 258066 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/12613 New failing tests: media/video-frame-accurate-seek.html http/tests/css/vertical-align-baseline-after-image-load-2.html media/video-aspect-ratio.html compositing/video/video-object-fit.html media/video-transformed.html media/video-canvas-alpha.html media/video-fullscreeen-only-controls.html media/track/track-cue-rendering-horizontal.html media/video-controls-visible-audio-only.html media/video-remove-insert-repaints.html http/tests/contentextensions/text-track-blocked.html http/tests/css/vertical-align-baseline-after-image-load-3.html compositing/overflow/overflow-compositing-descendant.html media/video-controls-visible-exiting-fullscreen.html compositing/layers-inside-overflow-scroll.html media/video-zoom.html compositing/self-painting-layers.html compositing/overflow/scroll-ancestor-update.html compositing/geometry/video-opacity-overlay.html http/tests/contentextensions/media-filtered.html compositing/geometry/video-fixed-scrolling.html fast/css/object-fit/object-fit-video-poster.html media/media-captions-no-controls.html compositing/video/video-poster.html http/tests/css/vertical-align-baseline-after-image-load.html Created attachment 258118 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 259767 [details]
Patch
Comment on attachment 259767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259767&action=review > Source/WebCore/html/MediaElementSession.cpp:369 > + if (!element.fastHasAttribute(HTMLNames::webkit_playsinlineAttr)) > + return true; This isn't right, we don't require the attribute on all devices. Created attachment 259849 [details]
Patch
Comment on attachment 259849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259849&action=review > Source/WebCore/html/MediaElementSession.cpp:367 > + if (settings->inlineMediaPlaybackRequiresPlaysInlineAttribute() && !element.fastHasAttribute(HTMLNames::webkit_playsinlineAttr)) nit: return settings->inlineMediaPlaybackRequiresPlaysInlineAttribute() && !element.fastHasAttribute(HTMLNames::webkit_playsinlineAttr); > Source/WebCore/testing/InternalSettings.cpp:531 > +void InternalSettings::setAllowsInlineMediaPlayback(bool allows, ExceptionCode& ec) ec is unused. > Source/WebCore/testing/InternalSettings.cpp:537 > +void InternalSettings::setInlineMediaPlaybackRequiresPlaysInlineAttribute(bool requires, ExceptionCode& ec) ec is unused. > Source/WebCore/testing/InternalSettings.idl:77 > + [RaisesException] void setAllowsInlineMediaPlayback(boolean allows); You can drop the [RaisesException] > Source/WebCore/testing/InternalSettings.idl:78 > + [RaisesException] void setInlineMediaPlaybackRequiresPlaysInlineAttribute(boolean requires); ditto. Comment on attachment 259849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259849&action=review Looks OK modulo the nits, but I am not a WK2 reviewer so someone else will have to give the official r+ > Source/WebKit/mac/WebView/WebPreferences.mm:525 > + [NSNumber numberWithBool:YES], WebKitInlineMediaPlaybackRequiresPlaysInlineAttributeKey, Nit: doesn't align like the surrounding code. Created attachment 259854 [details]
Patch
Re-based; should apply now
Created attachment 259855 [details]
Patch
Address (pre-)review feedback.
Created attachment 259858 [details]
Patch
Once more with the rebasing.
Created attachment 259863 [details]
Patch
Once more with the rebasing.
Created attachment 259864 [details]
Patch
Once more (3x) with the rebasing.
(In reply to comment #15) > Comment on attachment 259849 [details] > > Source/WebCore/testing/InternalSettings.cpp:531 > > +void InternalSettings::setAllowsInlineMediaPlayback(bool allows, ExceptionCode& ec) > > ec is unused. > > > Source/WebCore/testing/InternalSettings.cpp:537 > > +void InternalSettings::setInlineMediaPlaybackRequiresPlaysInlineAttribute(bool requires, ExceptionCode& ec) > > ec is unused. > > > Source/WebCore/testing/InternalSettings.idl:77 > > + [RaisesException] void setAllowsInlineMediaPlayback(boolean allows); > > You can drop the [RaisesException] > > > Source/WebCore/testing/InternalSettings.idl:78 > > + [RaisesException] void setInlineMediaPlaybackRequiresPlaysInlineAttribute(boolean requires); > > ditto. Turns out, ec is very much used. `InternalSettingsGuardForSettings()` is a macro which sets `ec` if settings() is NULL. I'll upload a new patch which puts these back. (In reply to comment #22) > (In reply to comment #15) > > Comment on attachment 259849 [details] > > > Source/WebCore/testing/InternalSettings.cpp:531 > > > +void InternalSettings::setAllowsInlineMediaPlayback(bool allows, ExceptionCode& ec) > > > > ec is unused. > > > > > Source/WebCore/testing/InternalSettings.cpp:537 > > > +void InternalSettings::setInlineMediaPlaybackRequiresPlaysInlineAttribute(bool requires, ExceptionCode& ec) > > > > ec is unused. > > > > > Source/WebCore/testing/InternalSettings.idl:77 > > > + [RaisesException] void setAllowsInlineMediaPlayback(boolean allows); > > > > You can drop the [RaisesException] > > > > > Source/WebCore/testing/InternalSettings.idl:78 > > > + [RaisesException] void setInlineMediaPlaybackRequiresPlaysInlineAttribute(boolean requires); > > > > ditto. > > Turns out, ec is very much used. `InternalSettingsGuardForSettings()` is a > macro which sets `ec` if settings() is NULL. I'll upload a new patch which > puts these back. Ahhh, sorry about that :( I failed to notice this. Created attachment 260081 [details]
Patch
Comment on attachment 260081 [details] Patch Clearing flags on attachment: 260081 Committed r189112: <http://trac.webkit.org/changeset/189112> All reviewed patches have been landed. Closing bug. This caused media/video-fullscreeen-only-playback.html to start failing. See <https://build.webkit.org/results/Apple%20Mavericks%20Release%20WK2%20(Tests)/r189112%20(16761)/results.html>. It looks like you removed the RUN() around internals.settings.setAllowsInlineMediaPlayback(false) in the expected result, but the actual result still includes that text. (In reply to comment #27) > This caused media/video-fullscreeen-only-playback.html to start failing. See > <https://build.webkit.org/results/ > Apple%20Mavericks%20Release%20WK2%20(Tests)/r189112%20(16761)/results.html>. > > It looks like you removed the RUN() around > internals.settings.setAllowsInlineMediaPlayback(false) in the expected > result, but the actual result still includes that text. Weird; since I generated those new results with run-webkit-tests --reset-results. Will fix. (In reply to comment #28) > (In reply to comment #27) > > This caused media/video-fullscreeen-only-playback.html to start failing. See > > <https://build.webkit.org/results/ > > Apple%20Mavericks%20Release%20WK2%20(Tests)/r189112%20(16761)/results.html>. > > > > It looks like you removed the RUN() around > > internals.settings.setAllowsInlineMediaPlayback(false) in the expected > > result, but the actual result still includes that text. > > Weird; since I generated those new results with run-webkit-tests > --reset-results. Will fix. Landed follow up test expectations fix in <http://trac.webkit.org/r189121>. |