Bug 147512 – [iOS] Make the AllowsInlineMediaPlayback preference work in WebKit / WebKit2.

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

[Jer Noble]

[iOS] Make the AllowsInlineMediaPlayback preference work in WebKit / WebKit2.

[Jer Noble]

Created attachment 257976 [details]
WIP

[Jer Noble]

Created attachment 257993 [details]
WIP

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Jer Noble]

Created attachment 258066 [details]
WIP

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Jer Noble]

Created attachment 259767 [details]
Patch

[Eric Carlson]

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.

[Jer Noble]

Created attachment 259849 [details]
Patch

[Chris Dumez]

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.

[Eric Carlson]

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.

[Jer Noble]

Created attachment 259854 [details]
Patch

Re-based; should apply now

[Jer Noble]

Created attachment 259855 [details]
Patch

Address (pre-)review feedback.

[Jer Noble]

Created attachment 259858 [details]
Patch

Once more with the rebasing.

[Jer Noble]

Created attachment 259863 [details]
Patch

Once more with the rebasing.

[Jer Noble]

Created attachment 259864 [details]
Patch

Once more (3x) with the rebasing.

[Jer Noble]

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

[Chris Dumez]

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

[Jer Noble]

Created attachment 260081 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 260081 [details]
Patch

Clearing flags on attachment: 260081

Committed r189112: <http://trac.webkit.org/changeset/189112>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

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.

[Jer Noble]

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

[Jer Noble]

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