The platform should be able to send start-of-interruption events to MediaSessions so we can invoke the media session interruption algorithm (4.5.2).
<rdar://problem/21768356>
Created attachment 256588 [details] Patch
Created attachment 256589 [details] Patch
Comment on attachment 256589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256589&action=review > Source/WebCore/Modules/mediasession/MediaSessionEvents.h:33 > +enum MediaEventType { This should be "enum class". > Source/WebCore/Modules/mediasession/MediaSessionEvents.h:39 > +enum MediaSessionInterruptingCategory { Ditto. > Source/WebCore/page/Page.cpp:1230 > +void Page::handleStartOfInterruptionEvent(MediaSessionInterruptingCategory category) > +{ > +} Please add notImplemented();
(In reply to comment #4) > Comment on attachment 256589 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256589&action=review > > > Source/WebCore/Modules/mediasession/MediaSessionEvents.h:33 > > +enum MediaEventType { > > This should be "enum class". These are "enums" so we can cast to them from WKMediaEventTypes. We can't do that with a strongly-typed "enum class". > > > Source/WebCore/Modules/mediasession/MediaSessionEvents.h:39 > > +enum MediaSessionInterruptingCategory { > > Ditto. > > > Source/WebCore/page/Page.cpp:1230 > > +void Page::handleStartOfInterruptionEvent(MediaSessionInterruptingCategory category) > > +{ > > +} > > Please add notImplemented(); Added.
Created attachment 256610 [details] Patch
Comment on attachment 256610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256610&action=review > Source/WebKit2/UIProcess/API/C/WKPage.cpp:2197 > + switch (wkCategory) { I realize there's only one user, but the enum conversion functions are usually separate static methods. Maybe just keep that in mind if we end up needing this code again? > Source/WebKit2/WebProcess/WebPage/WebPage.h:746 > + void handleStartOfInterruptionEvent(uint32_t /* WebCore::MediaSessionInterruptingCategory */); Since this is on WebPage, it should be more clear that it's about media. Also, maybe just "handleMediaInterruptionEvent", with a state/phase to indicate whether it's starting or ending? Won't you eventually need the end case too?
Comment on attachment 256610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256610&action=review > Source/WebKit2/UIProcess/API/C/WKPage.cpp:2192 > +void WKPageHandleStartOfInterruptionEvent(WKPageRef page, WKMediaSessionInterruptingCategory wkCategory) You should COMPILE_ASSERT([value in MediaEventType] == [value in WKMediaEventTypes] for each value to make sure the enums stay in sync. See HTMLTrackElement.cpp for an example.
Comment on attachment 256610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256610&action=review >> Source/WebKit2/UIProcess/API/C/WKPage.cpp:2192 >> +void WKPageHandleStartOfInterruptionEvent(WKPageRef page, WKMediaSessionInterruptingCategory wkCategory) > > You should COMPILE_ASSERT([value in MediaEventType] == [value in WKMediaEventTypes] for each value to make sure the enums stay in sync. See HTMLTrackElement.cpp for an example. Or rather, if you do that then you can use a static_cast to convert values.
Created attachment 256706 [details] Patch
(In reply to comment #7) > Comment on attachment 256610 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256610&action=review > > > Source/WebKit2/UIProcess/API/C/WKPage.cpp:2197 > > + switch (wkCategory) { > > I realize there's only one user, but the enum conversion functions are > usually separate static methods. Maybe just keep that in mind if we end up > needing this code again? Will do. > > > Source/WebKit2/WebProcess/WebPage/WebPage.h:746 > > + void handleStartOfInterruptionEvent(uint32_t /* WebCore::MediaSessionInterruptingCategory */); > > Since this is on WebPage, it should be more clear that it's about media. > Also, maybe just "handleMediaInterruptionEvent", with a state/phase to > indicate whether it's starting or ending? Won't you eventually need the end > case too? I renamed it to 'handleMediaSessionInterruptionEvent' and added an enum for the event type (currently either start-of-interruption and end-of-interruption).
(In reply to comment #9) > Comment on attachment 256610 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256610&action=review > > >> Source/WebKit2/UIProcess/API/C/WKPage.cpp:2192 > >> +void WKPageHandleStartOfInterruptionEvent(WKPageRef page, WKMediaSessionInterruptingCategory wkCategory) > > > > You should COMPILE_ASSERT([value in MediaEventType] == [value in WKMediaEventTypes] for each value to make sure the enums stay in sync. See HTMLTrackElement.cpp for an example. > > Or rather, if you do that then you can use a static_cast to convert values. Added the static assertions.
Comment on attachment 256706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256706&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:191 > +#include "WKPagePrivate.h" Put the enums in a new header (probably in Shared/API/C) that you include in both places (and install as a private header), do not import UIProcess API headers here!
Created attachment 256708 [details] Patch
(In reply to comment #13) > Comment on attachment 256706 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256706&action=review > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:191 > > +#include "WKPagePrivate.h" > > Put the enums in a new header (probably in Shared/API/C) that you include in > both places (and install as a private header), do not import UIProcess API > headers here! That header import was left over from a previous revision of the patch. Removed.
Comment on attachment 256708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256708&action=review > Source/WebKit2/UIProcess/API/C/WKPage.cpp:2193 > +// NOTE: The values in the WKMediaSessionInterruptingCategory enum must stay in sync with those in WebCore::MediaSessionInterruptingCategory. Why must they? You do a proper conversion below.
(In reply to comment #16) > Comment on attachment 256708 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256708&action=review > > > Source/WebKit2/UIProcess/API/C/WKPage.cpp:2193 > > +// NOTE: The values in the WKMediaSessionInterruptingCategory enum must stay in sync with those in WebCore::MediaSessionInterruptingCategory. > > Why must they? You do a proper conversion below. We cast from a WKMediaSessionInterruptionEvent to a MediaSessionInterruptionEvent in WebPage.cpp. We need to ensure the enums' values are the same, otherwise we'll get them mixed up after casting.
Committed r186791: <http://trac.webkit.org/changeset/186791>
(In reply to comment #17) > (In reply to comment #16) > > Comment on attachment 256708 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=256708&action=review > > > > > Source/WebKit2/UIProcess/API/C/WKPage.cpp:2193 > > > +// NOTE: The values in the WKMediaSessionInterruptingCategory enum must stay in sync with those in WebCore::MediaSessionInterruptingCategory. > > > > Why must they? You do a proper conversion below. > > We cast from a WKMediaSessionInterruptionEvent to a > MediaSessionInterruptionEvent in WebPage.cpp. We need to ensure the enums' > values are the same, otherwise we'll get them mixed up after casting. I'm not sure why we have a proper conversion function in one direction and just cast in the other direction?