Bug 153221 – Enable WebVideoFullscreenManager for Cocoa

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Ada Chan]

Enable WebVideoFullscreenManager for Cocoa.  Right now it's iOS only.

This will involve moving these classes to Cocoa:
- WebVideoFullscreenInterface
- WebVideoFullscreenModel
- WebVideoFullscreenModelVideoElement
- WebVideoFullscreenManagerProxy

and also adding a stub implementation of WebVideoFullscreenInterfaceMac for now.

Part of <rdar://problem/24171440>

[Ada Chan]

Created attachment 269826 [details]
Patch

[Ada Chan]

I had trouble getting the bots to apply my patch and it looks like the file moves look like a file remove + new file in the patch, which makes it really hard to review.  I'm going to make a separate patch just to move the files so it'll be easier to review the actual changes: https://bugs.webkit.org/show_bug.cgi?id=153473

[Ada Chan]

Created attachment 269917 [details]
Patch

[Build Bot]

Comment on attachment 269917 [details]
Patch

Attachment 269917 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/742915

New failing tests:
media/media-controls-drag-timeline-set-controls-property.html

[Build Bot]

Created attachment 269933 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Ada Chan]

I have trouble reproducing the hang in media/media-controls-drag-timeline-set-controls-property.html.

[Ada Chan]

Created attachment 270065 [details]
Patch

[Eric Carlson]

Comment on attachment 270065 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270065&action=review

> Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.h:69
> +    WEBCORE_EXPORT virtual void resetMediaState() override;
> +    WEBCORE_EXPORT virtual void setDuration(double) override;
> +    WEBCORE_EXPORT virtual void setCurrentTime(double currentTime, double anchorTime) override;
> +    WEBCORE_EXPORT virtual void setBufferedTime(double) override;
> +    WEBCORE_EXPORT virtual void setRate(bool isPlaying, float playbackRate) override;
> +    WEBCORE_EXPORT virtual void setVideoDimensions(bool hasVideo, float width, float height) override;
> +    WEBCORE_EXPORT virtual void setSeekableRanges(const TimeRanges&) override;
> +    WEBCORE_EXPORT virtual void setCanPlayFastReverse(bool) override;
> +    WEBCORE_EXPORT virtual void setAudioMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override;
> +    WEBCORE_EXPORT virtual void setLegibleMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override;
> +    WEBCORE_EXPORT virtual void setExternalPlayback(bool enabled, ExternalPlaybackTargetType, WTF::String localizedDeviceName) override;
> +    WEBCORE_EXPORT virtual void setWirelessVideoPlaybackDisabled(bool) override;

Nit: these don't need "virtual", it is implied by "override".

> Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:58
> +void WebVideoFullscreenInterfaceMac::resetMediaState()
> +{
> +}
> +
> +void WebVideoFullscreenInterfaceMac::setDuration(double)
> +{
> +}

Nit: you might want to put all of these empty methods in the .h file so the ones you do override are more obvious: WEBCORE_EXPORT void setDuration(double) override { }

[Ada Chan]

Created attachment 270096 [details]
Patch

[Ada Chan]

(In reply to comment #8)
> Comment on attachment 270065 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270065&action=review
> 
> > Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.h:69
> > +    WEBCORE_EXPORT virtual void resetMediaState() override;
> > +    WEBCORE_EXPORT virtual void setDuration(double) override;
> > +    WEBCORE_EXPORT virtual void setCurrentTime(double currentTime, double anchorTime) override;
> > +    WEBCORE_EXPORT virtual void setBufferedTime(double) override;
> > +    WEBCORE_EXPORT virtual void setRate(bool isPlaying, float playbackRate) override;
> > +    WEBCORE_EXPORT virtual void setVideoDimensions(bool hasVideo, float width, float height) override;
> > +    WEBCORE_EXPORT virtual void setSeekableRanges(const TimeRanges&) override;
> > +    WEBCORE_EXPORT virtual void setCanPlayFastReverse(bool) override;
> > +    WEBCORE_EXPORT virtual void setAudioMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override;
> > +    WEBCORE_EXPORT virtual void setLegibleMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override;
> > +    WEBCORE_EXPORT virtual void setExternalPlayback(bool enabled, ExternalPlaybackTargetType, WTF::String localizedDeviceName) override;
> > +    WEBCORE_EXPORT virtual void setWirelessVideoPlaybackDisabled(bool) override;
> 
> Nit: these don't need "virtual", it is implied by "override".
> 
> > Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:58
> > +void WebVideoFullscreenInterfaceMac::resetMediaState()
> > +{
> > +}
> > +
> > +void WebVideoFullscreenInterfaceMac::setDuration(double)
> > +{
> > +}
> 
> Nit: you might want to put all of these empty methods in the .h file so the
> ones you do override are more obvious: WEBCORE_EXPORT void
> setDuration(double) override { }

Thanks for the review!  Addressed your feedback and sent it to EWS to make sure the bots pass.

[Ada Chan]

Created attachment 270097 [details]
Patch

[Ada Chan]

Committed:
http://trac.webkit.org/changeset/195755

[Csaba Osztrogonác]

(In reply to comment #12)
> Committed:
> http://trac.webkit.org/changeset/195755

Mac cmake buildfix landed in https://trac.webkit.org/changeset/195957
and https://trac.webkit.org/changeset/195958

but the build is still broken:
/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:155:45: error: expected ';' after return statement
        return "com.apple.WebKit.WebContent" WK_XPC_SERVICE_SUFFIX;
                                            ^
                                            ;
/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:157:45: error: expected ';' after return statement
        return "com.apple.WebKit.Networking" WK_XPC_SERVICE_SUFFIX;
                                            ^
                                            ;
/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:160:44: error: expected ';' after return statement
        return "com.apple.WebKit.Databases" WK_XPC_SERVICE_SUFFIX;
                                           ^
                                           ;
/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:166:48: error: expected ';' after return statement
            return "com.apple.WebKit.Plugin.32" WK_XPC_SERVICE_SUFFIX;
                                               ^
                                               ;
/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:168:48: error: expected ';' after return statement
            return "com.apple.WebKit.Plugin.64" WK_XPC_SERVICE_SUFFIX;
                                               ^
                                               ;