| Summary: | Teach MediaSessionManager to manage interruptions | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||||||
| Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | bunhere, cdumez, commit-queue, eflews.bot, esprehn+autocc, glenn, gyuyoung.kim, gyuyoung.kim, jer.noble, ossy, rakuco | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Eric Carlson
2014-01-06 11:19:34 PST
Created attachment 220451 [details]
Patch for the bots to chew on
Attachment 220451 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/video-interruption-active-when-element-created-expected.txt', u'LayoutTests/media/video-interruption-active-when-element-created.html', u'LayoutTests/media/video-interruption-with-resume-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-allowing-play.html', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/html/HTMLVideoElement.cpp', u'Source/WebCore/platform/audio/MediaSession.cpp', u'Source/WebCore/platform/audio/MediaSession.h', u'Source/WebCore/platform/audio/MediaSessionManager.cpp', u'Source/WebCore/platform/audio/MediaSessionManager.h', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.h', u'Source/WebCore/platform/audio/mac/MediaSessionManagerMac.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/html/HTMLMediaElement.cpp:62: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/WebCore/html/HTMLMediaElement.cpp:4744: More than one command on the same line in if [whitespace/parens] [4]
Total errors found: 2 in 27 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 220452 [details]
Fix style errors
Comment on attachment 220452 [details] Fix style errors Attachment 220452 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/5040858160168960 (In reply to comment #4) > (From update of attachment 220452 [details]) > Attachment 220452 [details] did not pass efl-ews (efl): > Output: http://webkit-queues.appspot.com/results/5040858160168960 It looks you need to add a include path to efl-wk1 first. Please add "${WEBCORE_DIR}/platform/audio" to below line. http://trac.webkit.org/browser/trunk/Source/WebKit/CMakeLists.txt#L40 If there is a similar build error on efl-wk2 as well, please add it("${WEBCORE_DIR}/platform/audio") to below as well. http://trac.webkit.org/browser/trunk/Source/WebKit2/CMakeLists.txt#L96 Now I'm verifying it locally. Comment on attachment 220452 [details] Fix style errors Attachment 220452 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/4755002585251840 Created attachment 220459 [details]
Updated patch
Patch updated with Gyuyoung's suggested fixes.
Comment on attachment 220459 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=220459&action=review > Source/WebCore/platform/audio/MediaSession.h:73 > + bool playbackPermitted(const HTMLMediaElement&) const; > + bool dataLoadingPermitted(const HTMLMediaElement&) const; > + bool fullscreenPermitted(const HTMLMediaElement&) const; > + bool pageAllowsDataLoading(const HTMLMediaElement&) const; > + bool pageAllowsPlaybackAfterResuming(const HTMLMediaElement&) const; > +#if ENABLE(IOS_AIRPLAY) > + bool showingPlaybackTargetPickerPermitted(const HTMLMediaElement&) const; > +#endif The platform layer is not allowed to know about HTMLMediaElements. Is this really a platform layer concept? Is it wrapping an existing platform API? (In reply to comment #8) > (From update of attachment 220459 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=220459&action=review > > > Source/WebCore/platform/audio/MediaSession.h:73 > > + bool playbackPermitted(const HTMLMediaElement&) const; > > + bool dataLoadingPermitted(const HTMLMediaElement&) const; > > + bool fullscreenPermitted(const HTMLMediaElement&) const; > > + bool pageAllowsDataLoading(const HTMLMediaElement&) const; > > + bool pageAllowsPlaybackAfterResuming(const HTMLMediaElement&) const; > > +#if ENABLE(IOS_AIRPLAY) > > + bool showingPlaybackTargetPickerPermitted(const HTMLMediaElement&) const; > > +#endif > > The platform layer is not allowed to know about HTMLMediaElements. Is this really a platform layer concept? Is it wrapping an existing platform API? Oops, good point. The session only uses an HTMLMediaElement to manage restrictions, which is tangential to the real purpose of this patch. I will move that logic back to HTMLMediaElement. Created attachment 220537 [details]
Updated patch
Attachment 220537 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/video-interruption-active-when-element-created-expected.txt', u'LayoutTests/media/video-interruption-active-when-element-created.html', u'LayoutTests/media/video-interruption-with-resume-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-allowing-play.html', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/platform/audio/MediaSession.cpp', u'Source/WebCore/platform/audio/MediaSession.h', u'Source/WebCore/platform/audio/MediaSessionManager.cpp', u'Source/WebCore/platform/audio/MediaSessionManager.h', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.h', u'Source/WebCore/platform/audio/mac/MediaSessionManagerMac.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl', u'Source/WebKit/CMakeLists.txt', u'Source/WebKit/ChangeLog', u'Source/WebKit2/CMakeLists.txt', u'Source/WebKit2/ChangeLog', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/audio/MediaSession.h:27: MediaSession_h is incorrect. #defined constants should use all uppercase names with words separated by underscores. [readability/naming/define/constants] [4]
Total errors found: 1 in 30 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 220537 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=220537&action=review > Source/WebCore/platform/audio/MediaSessionManager.cpp:52 > + for (auto it = m_sessions.begin(), end = m_sessions.end(); it != end; ++it) { > if ((*it)->mediaType() == type) > return true; > } for (auto* session : m_sessions) { if (session->mediaType() == type) return true; } > Source/WebCore/platform/audio/MediaSessionManager.cpp:65 > + for (auto it = m_sessions.begin(), end = m_sessions.end(); it != end; ++it) { > if ((*it)->mediaType() == type) > ++count; > } Same here. > Source/WebCore/platform/audio/MediaSessionManager.cpp:76 > + for (auto it = m_sessions.begin(), end = m_sessions.end(); it != end; ++it) > + (*it)->beginInterruption(); And here. > Source/WebCore/platform/audio/MediaSessionManager.cpp:86 > + for (auto it = m_sessions.begin(), end = m_sessions.end(); it != end; ++it) > + (*it)->endInterruption(flags); And here. (In reply to comment #11) > Attachment 220537 [details] did not pass style-queue: ... > ERROR: Source/WebCore/platform/audio/MediaSession.h:27: MediaSession_h is incorrect. #defined constants should use all uppercase names with words separated by underscores. [readability/naming/define/constants] [4] > Huh? Created attachment 220548 [details]
Updated to address Sam's comments
Attachment 220548 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/video-interruption-active-when-element-created-expected.txt', u'LayoutTests/media/video-interruption-active-when-element-created.html', u'LayoutTests/media/video-interruption-with-resume-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-allowing-play.html', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/platform/audio/MediaSession.cpp', u'Source/WebCore/platform/audio/MediaSession.h', u'Source/WebCore/platform/audio/MediaSessionManager.cpp', u'Source/WebCore/platform/audio/MediaSessionManager.h', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.h', u'Source/WebCore/platform/audio/mac/MediaSessionManagerMac.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl', u'Source/WebKit/CMakeLists.txt', u'Source/WebKit/ChangeLog', u'Source/WebKit2/CMakeLists.txt', u'Source/WebKit2/ChangeLog', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/audio/MediaSession.h:27: MediaSession_h is incorrect. #defined constants should use all uppercase names with words separated by underscores. [readability/naming/define/constants] [4]
Total errors found: 1 in 30 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 220548 [details] Updated to address Sam's comments View in context: https://bugs.webkit.org/attachment.cgi?id=220548&action=review Even though this patch touches WebKit2 in only the most cursory way, you should probably get Sam or another OWNER to check off on it. > Source/WebCore/html/HTMLMediaElement.cpp:4116 > bool pausedToBuffer = m_readyStateMaximum >= HAVE_FUTURE_DATA && m_readyState < HAVE_FUTURE_DATA; > - return (pausedToBuffer || m_readyState >= HAVE_FUTURE_DATA) && couldPlayIfEnoughData() && !isBlockedOnMediaController(); > + > + if (!(pausedToBuffer || m_readyState >= HAVE_FUTURE_DATA)) > + return false; > + > + return couldPlayIfEnoughData() && !isBlockedOnMediaController(); If you're cleaning this up to be more understandable, the if-clause could be further cleaned up: if (!pausedToBuffer && m_readyState < HAVE_FUTURE_DATA) return false; > Source/WebCore/platform/audio/MediaSessionManager.cpp:29 > +#include "HTMLMediaElement.h" You should remove this #include now that we don't pass in a HTMLMediaElement. > LayoutTests/media/video-interruption-active-when-element-created.html:34 > + setTimeout(checkState, 250); 250ms? Can we bring this down to something slightly smaller? > LayoutTests/media/video-interruption-active-when-element-created.html:39 > + if (window.internals) We should just fail the test immediately if window.internals isn't present. > LayoutTests/media/video-interruption-active-when-element-created.html:40 > + run("window.internals.beginMediaSessionInterruption()");; The "window." is unnecessary. > LayoutTests/media/video-interruption-with-resume-allowing-play.html:10 > + consoleWrite("250ms timer fired..."); Ditto about 250ms. > LayoutTests/media/video-interruption-with-resume-allowing-play.html:34 > + if (window.internals) Ditto about failing early. > LayoutTests/media/video-interruption-with-resume-not-allowing-play.html:23 > + setTimeout(checkState, 250); Again with the 250ms. > LayoutTests/media/video-interruption-with-resume-not-allowing-play.html:38 > + if (window.internals) Again with the failing early. Created attachment 220567 [details]
Another update.
Attachment 220567 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/video-interruption-active-when-element-created-expected.txt', u'LayoutTests/media/video-interruption-active-when-element-created.html', u'LayoutTests/media/video-interruption-with-resume-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-allowing-play.html', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/platform/audio/MediaSession.cpp', u'Source/WebCore/platform/audio/MediaSession.h', u'Source/WebCore/platform/audio/MediaSessionManager.cpp', u'Source/WebCore/platform/audio/MediaSessionManager.h', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.h', u'Source/WebCore/platform/audio/mac/MediaSessionManagerMac.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl', u'Source/WebKit/CMakeLists.txt', u'Source/WebKit/ChangeLog', u'Source/WebKit2/CMakeLists.txt', u'Source/WebKit2/ChangeLog', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/audio/MediaSession.h:27: MediaSession_h is incorrect. #defined constants should use all uppercase names with words separated by underscores. [readability/naming/define/constants] [4]
Total errors found: 1 in 30 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 220567 [details] Another update. Clearing flags on attachment: 220567 Committed r161481: <http://trac.webkit.org/changeset/161481> All reviewed patches have been landed. Closing bug. (In reply to comment #19) > (From update of attachment 220567 [details]) > Clearing flags on attachment: 220567 > > Committed r161481: <http://trac.webkit.org/changeset/161481> It broke the Windows build: 1>C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Include\WebCore/HTMLMediaElement.h(36): fatal error C1083: Cannot open include file: 'MediaSession.h': No such file or directory (..\..\win\FullscreenVideoController.cpp) 1>C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Include\WebCore/HTMLMediaElement.h(36): fatal error C1083: Cannot open include file: 'MediaSession.h': No such file or directory (..\..\win\WebView.cpp) 1>c:\cygwin\home\buildbot\slave\win-release\build\webkitbuild\release\include\webcore\HTMLMediaElement.h(36): fatal error C1083: Cannot open include file: 'MediaSession.h': No such file or directory (..\..\win\WebCoreSupport\WebChromeClient.cpp) |