Bug 156564 – Allow WebVideoFullscreenManager and Proxy to be used by audio elements.

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

[Jer Noble]

Allow WebVideoFullscreenManager and Proxy to be used by audio elements.

[Jer Noble]

Created attachment 276421 [details]
Patch

[WebKit Commit Bot]

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.

[Jer Noble]

Created attachment 276431 [details]
Patch

[WebKit Commit Bot]

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.

[Jer Noble]

Created attachment 276434 [details]
Patch

[WebKit Commit Bot]

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.

[Jer Noble]

Created attachment 276436 [details]
Patch

Once more to un-break the EFL and GTK EWS bots.

[WebKit Commit Bot]

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.

[Beth Dakin]

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.

[Jer Noble]

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

[Jer Noble]

(In reply to comment #10)
> bits … backed … BCD 

Bots … blocked … once.

(Typing on a bus)

[Eric Carlson]

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"

[Jer Noble]

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

[Jer Noble]

Created attachment 276482 [details]
Patch

[WebKit Commit Bot]

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.

[Jer Noble]

Committed r199593: <http://trac.webkit.org/changeset/199593>