Allow WebVideoFullscreenManager and Proxy to be used by audio elements.
Created attachment 276421 [details] Patch
Attachment 276421 [details] did not pass style-queue: ERROR: Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.mm:245: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.mm:274: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.mm:101: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.mm:219: *SoftLink.h header should be included after all other headers. [build/include_order] [4] Total errors found: 4 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 276431 [details] Patch
Attachment 276431 [details] did not pass style-queue: ERROR: Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.mm:245: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.mm:274: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.mm:101: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.mm:219: *SoftLink.h header should be included after all other headers. [build/include_order] [4] Total errors found: 4 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 276434 [details] Patch
Attachment 276434 [details] did not pass style-queue: ERROR: Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.mm:245: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.mm:274: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.mm:101: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.mm:219: *SoftLink.h header should be included after all other headers. [build/include_order] [4] Total errors found: 4 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 276436 [details] Patch Once more to un-break the EFL and GTK EWS bots.
Attachment 276436 [details] did not pass style-queue: ERROR: Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.mm:245: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.mm:274: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.mm:101: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.mm:219: *SoftLink.h header should be included after all other headers. [build/include_order] [4] Total errors found: 4 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 276436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276436&action=review Overall, this looks good. You should get all the bots green, and it would be good if Eric took a look too. He's more familiar than I am with a lot of this code. The WK2 changes look good. > Source/WebCore/ChangeLog:24 Through composition? This sentence does not parse for me.
(In reply to comment #9) > Comment on attachment 276436 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=276436&action=review > > Overall, this looks good. You should get all the bots green, The EFL And GTK bits should be ok. They're backed against a general build failure in ToT. BCD that clears up, I'll resubmit to give them a chance to chew on the patch. > and it would be > good if Eric took a look too. He's more familiar than I am with a lot of > this code. The WK2 changes look good. > > > Source/WebCore/ChangeLog:24 > > > Through composition? This sentence does not parse for me. Composition rather than inheritance. Each WebVideoFullscreen object owns a WebPlaybackSession object that it uses to fulfil its interface requirements (rather than inheriting from those objects' classes).
(In reply to comment #10) > bits … backed … BCD Bots … blocked … once. (Typing on a bus)
Comment on attachment 276436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276436&action=review Nice cleanup! Looks good to me modulo the nits noted, but a WK2 reviewer will have to give it the official review. > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.h:43 > +class WebPlaybackSessionModelMediaElement : public WebPlaybackSessionModel, public EventListener { Can this class be marked final? > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.h:69 > + WEBCORE_EXPORT void play() override; > + WEBCORE_EXPORT void pause() override; > + WEBCORE_EXPORT void togglePlayState() override; > + WEBCORE_EXPORT void beginScrubbing() override; > + WEBCORE_EXPORT void endScrubbing() override; > + WEBCORE_EXPORT void seekToTime(double time) override; > + WEBCORE_EXPORT void fastSeek(double time) override; > + WEBCORE_EXPORT void beginScanningForward() override; > + WEBCORE_EXPORT void beginScanningBackward() override; > + WEBCORE_EXPORT void endScanning() override; > + WEBCORE_EXPORT void selectAudioMediaOption(uint64_t index) override; > + WEBCORE_EXPORT void selectLegibleMediaOption(uint64_t index) override; All of these "override"s can be "final" > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:227 > +void WebPlaybackSessionModelMediaElement::selectAudioMediaOption(uint64_t selectedAudioIndex) > +{ > + AudioTrack* selectedAudioTrack = nullptr; nit: this isn't new, but you could assert that selectedAudioIndex fits in a size_t > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:240 > +{ Ditto. > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:259 > + WTF::AtomicString displayMode = m_mediaElement->mediaControlsHost()->captionDisplayMode(); "WTF::" isn't necessary. > Source/WebCore/platform/cocoa/WebVideoFullscreenModelVideoElement.mm:233 > if (!sEventNames.get().size()) { Nit: it doesn't look like you need the braces. > Source/WebCore/platform/ios/WebPlaybackSessionInterfaceAVKit.h:64 > +class WEBCORE_EXPORT WebPlaybackSessionInterfaceAVKit final > Source/WebCore/platform/ios/WebPlaybackSessionInterfaceAVKit.h:87 > + WEBCORE_EXPORT void resetMediaState() override; > + WEBCORE_EXPORT void setDuration(double) override; > + WEBCORE_EXPORT void setCurrentTime(double currentTime, double anchorTime) override; > + WEBCORE_EXPORT void setBufferedTime(double) override; > + WEBCORE_EXPORT void setRate(bool isPlaying, float playbackRate) override; > + WEBCORE_EXPORT void setSeekableRanges(const TimeRanges&) override; > + WEBCORE_EXPORT void setCanPlayFastReverse(bool) override; > + WEBCORE_EXPORT void setAudioMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override; > + WEBCORE_EXPORT void setLegibleMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override; > + WEBCORE_EXPORT void setExternalPlayback(bool enabled, ExternalPlaybackTargetType, WTF::String localizedDeviceName) override; > + WEBCORE_EXPORT void setWirelessVideoPlaybackDisabled(bool) override; "override" -> "final" > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:560 > + auto playbackSessionInterface = WebPlaybackSessionInterfaceAVKit::create(); > + m_interface = WebVideoFullscreenInterfaceAVKit::create(playbackSessionInterface.get()); Nit: the local variable isn't necessary. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:568 > + auto playbackSessionModel = WebPlaybackSessionModelMediaElement::create(); > + m_model = WebVideoFullscreenModelVideoElement::create(playbackSessionModel.get()); Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:60 > +class WebPlaybackSessionInterfaceAVKit; Is this necessary, you include "WebPlaybackSessionInterfaceAVKit.h" above. > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:130 > + void externalPlaybackEnabledChanged(bool) override; final > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:297 > + _fullscreenInterface->model()->setVideoLayerFrame(self.modelVideoLayerFrame); Nit: it might be worth asserting "fullscreenInterface->model()" > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:329 > + _fullscreenInterface->model()->setVideoLayerFrame(self.modelVideoLayerFrame); Ditto > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:352 > + _fullscreenInterface->model()->setVideoLayerGravity(gravity); Ditto > Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.h:43 > +class WEBCORE_EXPORT WebPlaybackSessionInterfaceMac final > Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.h:65 > + WEBCORE_EXPORT void resetMediaState() override { } > + WEBCORE_EXPORT void setDuration(double) override; > + WEBCORE_EXPORT void setCurrentTime(double /*currentTime*/, double /*anchorTime*/) override; > + WEBCORE_EXPORT void setBufferedTime(double) override { } > + WEBCORE_EXPORT void setRate(bool /*isPlaying*/, float /*playbackRate*/) override; > + WEBCORE_EXPORT void setSeekableRanges(const TimeRanges&) override; > + WEBCORE_EXPORT void setCanPlayFastReverse(bool) override { } > + WEBCORE_EXPORT void setAudioMediaSelectionOptions(const Vector<String>& /*options*/, uint64_t /*selectedIndex*/) override; > + WEBCORE_EXPORT void setLegibleMediaSelectionOptions(const Vector<String>& /*options*/, uint64_t /*selectedIndex*/) override; > + WEBCORE_EXPORT void setExternalPlayback(bool, ExternalPlaybackTargetType, String) override { } > + WEBCORE_EXPORT void setWirelessVideoPlaybackDisabled(bool) override { } "override" -> "final" > Source/WebKit2/UIProcess/Cocoa/WebPlaybackSessionManagerProxy.h:87 > + void play() override; > + void pause() override; > + void togglePlayState() override; > + void beginScrubbing() override; > + void endScrubbing() override; > + void seekToTime(double) override; > + void fastSeek(double time) override; > + void beginScanningForward() override; > + void beginScanningBackward() override; > + void endScanning() override; > + void selectAudioMediaOption(uint64_t) override; > + void selectLegibleMediaOption(uint64_t) override; "override" -> "final" > Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.h:57 > +class WebPlaybackSessionInterfaceContext : public RefCounted<WebPlaybackSessionInterfaceContext>, public WebCore::WebPlaybackSessionInterface { final > Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.h:81 > + void resetMediaState() override; > + void setDuration(double) override; > + void setCurrentTime(double currentTime, double anchorTime) override; > + void setBufferedTime(double) override; > + void setRate(bool isPlaying, float playbackRate) override; > + void setSeekableRanges(const WebCore::TimeRanges&) override; > + void setCanPlayFastReverse(bool value) override; > + void setAudioMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override; > + void setLegibleMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override; > + void setExternalPlayback(bool enabled, ExternalPlaybackTargetType, WTF::String localizedDeviceName) override; > + void setWirelessVideoPlaybackDisabled(bool) override; "override" -> "final"
(In reply to comment #12) > Comment on attachment 276436 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=276436&action=review > > Nice cleanup! Looks good to me modulo the nits noted, but a WK2 reviewer > will have to give it the official review. > > > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.h:43 > > +class WebPlaybackSessionModelMediaElement : public WebPlaybackSessionModel, public EventListener { > > Can this class be marked final? Sure. > > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.h:69 > > + WEBCORE_EXPORT void play() override; > > + WEBCORE_EXPORT void pause() override; > > + WEBCORE_EXPORT void togglePlayState() override; > > + WEBCORE_EXPORT void beginScrubbing() override; > > + WEBCORE_EXPORT void endScrubbing() override; > > + WEBCORE_EXPORT void seekToTime(double time) override; > > + WEBCORE_EXPORT void fastSeek(double time) override; > > + WEBCORE_EXPORT void beginScanningForward() override; > > + WEBCORE_EXPORT void beginScanningBackward() override; > > + WEBCORE_EXPORT void endScanning() override; > > + WEBCORE_EXPORT void selectAudioMediaOption(uint64_t index) override; > > + WEBCORE_EXPORT void selectLegibleMediaOption(uint64_t index) override; > > All of these "override"s can be "final" Sure. > > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:227 > > +void WebPlaybackSessionModelMediaElement::selectAudioMediaOption(uint64_t selectedAudioIndex) > > +{ > > + AudioTrack* selectedAudioTrack = nullptr; > > nit: this isn't new, but you could assert that selectedAudioIndex fits in a > size_t Sure, but wouldn't the comparison "index == static_cast<size_t>(selectedAudioIndex)" just fail? > > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:240 > > +{ > > Ditto. Ok. > > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:259 > > + WTF::AtomicString displayMode = m_mediaElement->mediaControlsHost()->captionDisplayMode(); > > "WTF::" isn't necessary. Ok. > > Source/WebCore/platform/cocoa/WebVideoFullscreenModelVideoElement.mm:233 > > if (!sEventNames.get().size()) { > > Nit: it doesn't look like you need the braces. Ok. > > Source/WebCore/platform/ios/WebPlaybackSessionInterfaceAVKit.h:64 > > +class WEBCORE_EXPORT WebPlaybackSessionInterfaceAVKit > > final Ok. > > Source/WebCore/platform/ios/WebPlaybackSessionInterfaceAVKit.h:87 > > + WEBCORE_EXPORT void resetMediaState() override; > > + WEBCORE_EXPORT void setDuration(double) override; > > + WEBCORE_EXPORT void setCurrentTime(double currentTime, double anchorTime) override; > > + WEBCORE_EXPORT void setBufferedTime(double) override; > > + WEBCORE_EXPORT void setRate(bool isPlaying, float playbackRate) override; > > + WEBCORE_EXPORT void setSeekableRanges(const TimeRanges&) override; > > + WEBCORE_EXPORT void setCanPlayFastReverse(bool) override; > > + WEBCORE_EXPORT void setAudioMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override; > > + WEBCORE_EXPORT void setLegibleMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override; > > + WEBCORE_EXPORT void setExternalPlayback(bool enabled, ExternalPlaybackTargetType, WTF::String localizedDeviceName) override; > > + WEBCORE_EXPORT void setWirelessVideoPlaybackDisabled(bool) override; > > "override" -> "final" Ok. > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:560 > > + auto playbackSessionInterface = WebPlaybackSessionInterfaceAVKit::create(); > > + m_interface = WebVideoFullscreenInterfaceAVKit::create(playbackSessionInterface.get()); > > Nit: the local variable isn't necessary. Ok. > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:568 > > + auto playbackSessionModel = WebPlaybackSessionModelMediaElement::create(); > > + m_model = WebVideoFullscreenModelVideoElement::create(playbackSessionModel.get()); > > Ditto. Ok. > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:60 > > +class WebPlaybackSessionInterfaceAVKit; > > Is this necessary, you include "WebPlaybackSessionInterfaceAVKit.h" above. Nope! > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:130 > > + void externalPlaybackEnabledChanged(bool) override; > > final Ok. > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:297 > > + _fullscreenInterface->model()->setVideoLayerFrame(self.modelVideoLayerFrame); > > Nit: it might be worth asserting "fullscreenInterface->model()" Ok. > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:329 > > + _fullscreenInterface->model()->setVideoLayerFrame(self.modelVideoLayerFrame); > > Ditto Ok. > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:352 > > + _fullscreenInterface->model()->setVideoLayerGravity(gravity); > > Ditto Ok. > > Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.h:43 > > +class WEBCORE_EXPORT WebPlaybackSessionInterfaceMac > > final Ok. > > Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.h:65 > > + WEBCORE_EXPORT void resetMediaState() override { } > > + WEBCORE_EXPORT void setDuration(double) override; > > + WEBCORE_EXPORT void setCurrentTime(double /*currentTime*/, double /*anchorTime*/) override; > > + WEBCORE_EXPORT void setBufferedTime(double) override { } > > + WEBCORE_EXPORT void setRate(bool /*isPlaying*/, float /*playbackRate*/) override; > > + WEBCORE_EXPORT void setSeekableRanges(const TimeRanges&) override; > > + WEBCORE_EXPORT void setCanPlayFastReverse(bool) override { } > > + WEBCORE_EXPORT void setAudioMediaSelectionOptions(const Vector<String>& /*options*/, uint64_t /*selectedIndex*/) override; > > + WEBCORE_EXPORT void setLegibleMediaSelectionOptions(const Vector<String>& /*options*/, uint64_t /*selectedIndex*/) override; > > + WEBCORE_EXPORT void setExternalPlayback(bool, ExternalPlaybackTargetType, String) override { } > > + WEBCORE_EXPORT void setWirelessVideoPlaybackDisabled(bool) override { } > > "override" -> "final" Ok. > > Source/WebKit2/UIProcess/Cocoa/WebPlaybackSessionManagerProxy.h:87 > > + void play() override; > > + void pause() override; > > + void togglePlayState() override; > > + void beginScrubbing() override; > > + void endScrubbing() override; > > + void seekToTime(double) override; > > + void fastSeek(double time) override; > > + void beginScanningForward() override; > > + void beginScanningBackward() override; > > + void endScanning() override; > > + void selectAudioMediaOption(uint64_t) override; > > + void selectLegibleMediaOption(uint64_t) override; > > "override" -> "final" Ok. > > Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.h:57 > > +class WebPlaybackSessionInterfaceContext : public RefCounted<WebPlaybackSessionInterfaceContext>, public WebCore::WebPlaybackSessionInterface { > > final Ok. > > Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.h:81 > > + void resetMediaState() override; > > + void setDuration(double) override; > > + void setCurrentTime(double currentTime, double anchorTime) override; > > + void setBufferedTime(double) override; > > + void setRate(bool isPlaying, float playbackRate) override; > > + void setSeekableRanges(const WebCore::TimeRanges&) override; > > + void setCanPlayFastReverse(bool value) override; > > + void setAudioMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override; > > + void setLegibleMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override; > > + void setExternalPlayback(bool enabled, ExternalPlaybackTargetType, WTF::String localizedDeviceName) override; > > + void setWirelessVideoPlaybackDisabled(bool) override; > > "override" -> "final" Ok.
Created attachment 276482 [details] Patch
Attachment 276482 [details] did not pass style-queue: ERROR: Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.mm:245: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.mm:274: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.mm:101: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.mm:219: *SoftLink.h header should be included after all other headers. [build/include_order] [4] Total errors found: 4 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r199593: <http://trac.webkit.org/changeset/199593>