Bug 169145

<rdar://problem/17048858>

Created attachment 303339 [details] Patch

Comment on attachment 303339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303339&action=review > Source/WebCore/ChangeLog:33 > + As a result of the new styles applied by the modern-media-controls module, media documents have a > + similar behavior on macOS and iPad, where we only enforce a min-width for video allowing them > + to play at their natural size otherwise, and enforcing a fixed width for audio. On iPhone however, > + we want to always play the media at full width, with some padding in the case of audio. > + Did you check all this against the spreadsheet in Quip? > Source/WebCore/Modules/modern-media-controls/controls/media-document.css:45 > +:host(.media-document.audio.ipad) { > + width: 650px !important; > +} What about iPads with different widths? Should it be the padding that is constant, or the overall width? > Source/WebCore/Modules/modern-media-controls/media/media-controller.js:73 > + const isLiveBroadcast = this.media.duration === Number.POSITIVE_INFINITY; > + const hasVideoTracks = this.media.videoWidth != 0; > + return !isLiveBroadcast && !hasVideoTracks; I don't like that all live broadcasts get a video player. I can imagine live "radio" being fairly popular - bringing radio to the internet. Could we swap dynamically? > Source/WebCore/Modules/modern-media-controls/media/media-document-controller.js:60 > + media.classList.add(window.navigator.platform === "MacIntel" ? "mac" : window.navigator.platform); Ugggh. I don't like this. Could we get it from the mediaController instead? > Source/WebCore/html/MediaDocument.cpp:-113 > - if (RuntimeEnabledFeatures::sharedFeatures().modernMediaControlsEnabled()) { > - StringBuilder bodyStyle; > - bodyStyle.appendLiteral("margin: 0; padding: 0;"); > - bodyStyle.appendLiteral("background-color: rgb(38, 38, 38);"); > - bodyStyle.appendLiteral("display: flex; justify-content: center; align-items: center;"); > - body->setAttribute(styleAttr, bodyStyle.toString()); > - } Goodbye Dean code. > Source/WebCore/html/MediaDocument.cpp:113 > + videoElement->setAttributeWithoutSynchronization(playsinlineAttr, emptyAtom); Do we want this on iPhone?

(In reply to comment #3) > Comment on attachment 303339 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=303339&action=review > > > Source/WebCore/Modules/modern-media-controls/controls/media-document.css:45 > > +:host(.media-document.audio.ipad) { > > + width: 650px !important; > > +} > > What about iPads with different widths? Should it be the padding that is > constant, or the overall width? Always the same width on all iPads, with a max-width of 100%. > > Source/WebCore/Modules/modern-media-controls/media/media-controller.js:73 > > + const isLiveBroadcast = this.media.duration === Number.POSITIVE_INFINITY; > > + const hasVideoTracks = this.media.videoWidth != 0; > > + return !isLiveBroadcast && !hasVideoTracks; > > I don't like that all live broadcasts get a video player. I can imagine live > "radio" being fairly popular - bringing radio to the internet. > > Could we swap dynamically? There is no way we can determine that HLS is ever audio-only. Video tracks could appear at any moment I'm afraid. > > Source/WebCore/Modules/modern-media-controls/media/media-document-controller.js:60 > > + media.classList.add(window.navigator.platform === "MacIntel" ? "mac" : window.navigator.platform); > > Ugggh. I don't like this. Could we get it from the mediaController instead? We could add those as layout traits, would that be good? > > Source/WebCore/html/MediaDocument.cpp:113 > > + videoElement->setAttributeWithoutSynchronization(playsinlineAttr, emptyAtom); > > Do we want this on iPhone? I think so.

Comment on attachment 303339 [details] Patch Attachment 303339 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3231744 New failing tests: media/modern-media-controls/media-documents/media-document-audio-ios-sizing.html

Created attachment 303348 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Created attachment 303354 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 303354 [details]: media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html bug 169158 (authors: graouts@apple.com and ryanhaddad@apple.com) media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html bug 169159 (authors: graouts@apple.com and ryanhaddad@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 303354 [details] Patch for landing Clearing flags on attachment: 303354 Committed r213400: <http://trac.webkit.org/changeset/213400>

All reviewed patches have been landed. Closing bug.

This caused very frequent assertion failures on several media tests (media/modern-media-controls/media-documents/click-on-video-should-not-pause.html, http/tests/media/media-document.html, possibly more). https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fmedia%2Fmedia-document.html https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=media%2Fmodern-media-controls%2Fmedia-documents%2Fclick-on-video-should-not-pause.html https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/r213410%20(3609)/fast/events/media-focus-in-standalone-media-document-crash-log.txt Rolling out.

Re-opened since this is blocked by bug 169163

It seems the line of code that triggers `ASSERT(!needsLayout())` in FrameView::paintContents is this: media.classList.add(window.navigator.platform === "MacIntel" ? "mac" : window.navigator.platform) I'm not sure why that is, but maybe having this class set in MediaDocumentParser::createDocumentStructure() where we already have some platform-specific code.

Created attachment 305457 [details] Patch for landing

Raised https://bugs.webkit.org/show_bug.cgi?id=170113 to figure out why we were hitting this ASSERT in the first place. The patch is working around it by using a requestAnimationFrame call before setting the offending CSS classes.

Comment on attachment 305457 [details] Patch for landing Clearing flags on attachment: 305457 Committed r214411: <http://trac.webkit.org/changeset/214411>

All reviewed patches have been landed. Closing bug.

Comment on attachment 305457 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=305457&action=review > Source/WebCore/Modules/modern-media-controls/media/media-controller.js:72 > + const isLiveBroadcast = this.media.duration === Number.POSITIVE_INFINITY; Why test this here, a live stream can be audio-only?

Reverted r214411 for reason: Two of the LayoutTests for this change time out on ios-simulator. Committed r214429: <http://trac.webkit.org/changeset/214429>

*** Bug 170123 has been marked as a duplicate of this bug. ***

Committed r214468: <http://trac.webkit.org/changeset/214468>