Bug 128143 – Add support for AVKit fullscreen to WebKit2

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Jones]

Add support for AVKit fullscreen to WebKit2

[Jeremy Jones]

Created attachment 223052 [details]
Patch

[Eric Carlson]

Comment on attachment 223052 [details]
Patch

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

> Source/WebCore/ChangeLog:21
> +        * WebCore.exp.in:
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * platform/ios/WebVideoFullscreenControllerAVKit.mm:
> +        (-[WebVideoFullscreenController exitFullscreen]):
> +        * platform/ios/WebVideoFullscreenInterfaceAVKit.h:
> +        * platform/ios/WebVideoFullscreenInterfaceAVKit.mm:
> +        (WebVideoFullscreenInterfaceAVKit::enterFullscreenCompletion):
> +        (WebVideoFullscreenInterfaceAVKit::enterFullscreen):
> +        (WebVideoFullscreenInterfaceAVKit::exitFullscreenCompletion):
> +        (WebVideoFullscreenInterfaceAVKit::exitFullscreen):
> +        * platform/ios/WebVideoFullscreenModelMediaElement.h:

Nit: I think it is helpful to have per-method comments in ChangeLog files.

> Source/WebKit2/ChangeLog:15
> +        * DerivedSources.make:
> +        * UIProcess/WebPageProxy.cpp:
> +        (WebKit::WebPageProxy::WebPageProxy):
> +        (WebKit::WebPageProxy::videoFullscreenManager):
> +        * UIProcess/WebPageProxy.h:

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:67
>      void setVideoLayerID(uint32_t videoContextID) override {UNUSED_PARAM(videoContextID);};

Nit: remove "videoContextID" and you won't need the UNUSED_PARAM() macro.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:69
> +    void enterFullscreenCompletion(std::function<void()> completion);

Nit: "completion" adds no information, so it should be removed.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:71
> +    void exitFullscreenCompletion(std::function<void()> completion);

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:302
> -    enterFullscreen(nullptr);
> +    exitFullscreenCompletion(nullptr);

Do you really want to call *exit*FullscreenCompletion here?

> Source/WebKit2/UIProcess/WebVideoFullscreenManagerProxy.cpp:93
> +
> +    

Nit: extra blank line.

> Source/WebKit2/UIProcess/WebVideoFullscreenManagerProxy.h:51
> +    void setVideoLayerID(unsigned videoContextID) override;

Nit: "videoContextID" adds no information, so it should be removed.

> Source/WebKit2/UIProcess/WebVideoFullscreenManagerProxy.h:57
> +    void seekToTime(double time) override;

Nit: "time" adds no information, so it should be removed.

> Source/WebKit2/UIProcess/WebVideoFullscreenManagerProxy.h:60
> +    WebPageProxy* m_page;

You should use a reference instead of a pointer, the page can never be NULL.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:52
> +#include "WebVideoFullscreenManager.h"

Nit: This include is only needed on iOS so you can include it conditionally.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:90
> +#include "WebVideoFullscreenManager.h"

Ditto.

> Source/WebKit2/WebProcess/WebVideoFullscreenManager.cpp:37
> +#include <WebCore/HTMLMediaElement.h>
> +#include <WebCore/HTMLVideoElement.h>

Nit: HTMLVideoElement.h includes HTMLMediaElement.h so you don't need to include both.

> Source/WebKit2/WebProcess/WebVideoFullscreenManager.cpp:44
> +PassRefPtr<WebVideoFullscreenManager> WebVideoFullscreenManager::create(WebPage* page)

The page can never be NULL so you should use a reference instead of a pointer.

> Source/WebKit2/WebProcess/WebVideoFullscreenManager.cpp:76
> +void WebVideoFullscreenManager::exitFullscreenForNode(Node* node)

This won't build in the Release configuration, "node" is unused.

> Source/WebKit2/WebProcess/WebVideoFullscreenManager.cpp:100
> +

Nit: extra blank line.

> Source/WebKit2/WebProcess/WebVideoFullscreenManager.cpp:103
> +void WebVideoFullscreenManager::setVideoLayer(PlatformLayer* videoLayer)

This won't build in the Release configuration, "videoLayer" is unused.

[Jeremy Jones]

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:69
> > +    void enterFullscreenCompletion(std::function<void()> completion);
> 
> Nit: "completion" adds no information, so it should be removed.
> 

I added "completion" to disambiguate the overloaded functions. The code generated from .messages.in contains a function pointer passed to a template method and the compiler can't infer what the type of the function pointer is when it is overloaded.

Side note: when you have a function pointer to a member that is overloaded, who do you specify which member you want a pointer to?

[Jeremy Jones]

(In reply to comment #3)
> > 
> > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:69
> > > +    void enterFullscreenCompletion(std::function<void()> completion);
> > 
> > Nit: "completion" adds no information, so it should be removed.
> > 
> 
> I added "completion" to disambiguate the overloaded functions. The code generated from .messages.in contains a function pointer passed to a template method and the compiler can't infer what the type of the function pointer is when it is overloaded.
> 
> Side note: when you have a function pointer to a member that is overloaded, who do you specify which member you want a pointer to?

Oh! You meant the parameter, not the function name.

[Eric Carlson]

(In reply to comment #4)
> (In reply to comment #3)
> > > 
> > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:69
> > > > +    void enterFullscreenCompletion(std::function<void()> completion);
> > > 
> > > Nit: "completion" adds no information, so it should be removed.
> > > 
> > 
> > I added "completion" to disambiguate the overloaded functions. The code generated from .messages.in contains a function pointer passed to a template method and the compiler can't infer what the type of the function pointer is when it is overloaded.
> > 
> > Side note: when you have a function pointer to a member that is overloaded, who do you specify which member you want a pointer to?
> 
> Oh! You meant the parameter, not the function name.

👍

[Jon Lee]

Comment on attachment 223052 [details]
Patch

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

>> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:69
>> +    void enterFullscreenCompletion(std::function<void()> completion);
> 
> Nit: "completion" adds no information, so it should be removed.

If I may bike shed a bit, the name of this function could be improved. It sounds like you are entering fullscreen completion, not entering fullscreen with a callback.

I'd suggest
void enterFullscreenWithCompletionCallback(std::function<void()>);

>> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:71
>> +    void exitFullscreenCompletion(std::function<void()> completion);
> 
> Ditto.

Similarly here.

[Jeremy Jones]

Created attachment 223291 [details]
Patch

[Eric Carlson]

Comment on attachment 223291 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Rename overloaded exitFullscreen to exitFullscreenCompletionHandler.
> +        Rename overloaded enterFullscreen to enterFullscreenCompletionHandler.

Nit: you renamed them enterFullscreenWithCompletionHandler and exitFullscreenWithCompletionHandler.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:91
> +#include "WebVideoFullscreenManagerProxy.h"
> +#include "WebVideoFullscreenManagerProxyMessages.h"

Nit: these are both iOS-only so they don't need to be included by all ports.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:2303
> +		3F418EF81887BD97002795FD /* WebVideoFullscreenManagerProxyMessages.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WebVideoFullscreenManagerProxyMessages.h; sourceTree = "<group>"; };

WebVideoFullscreenManagerProxyMessages.h isn't in the patch, did you forget to add it?

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:54
> +#if PLATFORM(IOS)
> +#include "WebVideoFullscreenManager.h"
> +#endif

Nit: a conditionally included header should be after the unconditional headers.

[Jeremy Jones]

> > Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:2303
> > +		3F418EF81887BD97002795FD /* WebVideoFullscreenManagerProxyMessages.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WebVideoFullscreenManagerProxyMessages.h; sourceTree = "<group>"; };
> 
> WebVideoFullscreenManagerProxyMessages.h isn't in the patch, did you forget to add it?
> 

Nope. It and WebVideoFullscreenManagerMessages.h are derived sources.

[Simon Fraser (smfr)]

Comment on attachment 223291 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.h:33
> +#include <WebCore/HTMLMediaElement.h>

We try to avoid including files in headers. Why this include here and not in the impl?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2971
> +WebVideoFullscreenManagerProxy* WebPageProxy::videoFullscreenManager()

This should return a reference if it can never be null.

> Source/WebKit2/UIProcess/WebVideoFullscreenManagerProxy.cpp:29
> +#if PLATFORM(IOS)

This file should live in UIProcess/ios if it's iOS-only.

> Source/WebKit2/UIProcess/WebVideoFullscreenManagerProxy.cpp:57
> +void WebVideoFullscreenManagerProxy::setVideoLayerID(unsigned videoContextID)

It's called setVideoLayerID but sets a videoContextID. Which is it?

Can we use a stronger type than "unsigned"?

> Source/WebKit2/UIProcess/WebVideoFullscreenManagerProxy.h:42
> +class WebVideoFullscreenManagerProxy : public WebCore::WebVideoFullscreenInterfaceAVKit, public WebCore::WebVideoFullscreenModel, public IPC::MessageReceiver {

I think IPC::MessageReceiver can be privately inherited.

> Source/WebKit2/UIProcess/WebVideoFullscreenManagerProxy.h:58
> +    void setVideoLayerID(unsigned) override;
> +    
> +    void requestExitFullScreen() override;
> +    void play() override;
> +    void pause() override;
> +    void togglePlayState() override;
> +    void seekToTime(double) override;
> +    void didExitFullscreen() override;

Please use 'virtual' for these.

> Source/WebKit2/UIProcess/WebVideoFullscreenManagerProxy.h:60
> +    WebPageProxy* m_page;

Can this be a reference?

>> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:2303
>> +		3F418EF81887BD97002795FD /* WebVideoFullscreenManagerProxyMessages.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WebVideoFullscreenManagerProxyMessages.h; sourceTree = "<group>"; };
> 
> WebVideoFullscreenManagerProxyMessages.h isn't in the patch, did you forget to add it?

It's generated.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:210
> +    WebVideoFullscreenManager* videoFullscreenManager();

Can this return a reference?

> Source/WebKit2/WebProcess/WebVideoFullscreenManager.cpp:28
> +#if PLATFORM(IOS)

This file should live in an /ios directory.

> Source/WebKit2/WebProcess/WebVideoFullscreenManager.cpp:51
> +    setWebVideoFullscreenInterface(this);

Seems odd to call this here. Also odd that it's never unset in the dtor.

> Source/WebKit2/WebProcess/WebVideoFullscreenManager.h:28
> +#if PLATFORM(IOS)

This file should live in an /ios directory.

> Source/WebKit2/WebProcess/WebVideoFullscreenManager.h:51
> +class WebVideoFullscreenManager : public WebCore::WebVideoFullscreenModelMediaElement, public WebCore::WebVideoFullscreenInterface, public IPC::MessageReceiver {

It's weird that the existing WebVideoFullscreenModelMediaElement doesn't inherit from HTMLMediaElement.

> Source/WebKit2/WebProcess/WebVideoFullscreenManager.h:58
> +    bool supportsFullscreen(const WebCore::Node*);

const method?

> Source/WebKit2/WebProcess/WebVideoFullscreenManager.h:64
> +    bool operator==(const EventListener& rhs) override {return static_cast<WebCore::EventListener*>(this) == &rhs;}

Need spaces inside the {}

> Source/WebKit2/WebProcess/WebVideoFullscreenManager.h:73
> +    void setDuration(double) override;
> +    void setCurrentTime(double currentTime, double anchorTime) override;
> +    void setRate(bool isPlaying, float playbackRate) override;
> +    void setVideoDimensions(bool hasVideo, float width, float height) override;
> +    void setVideoLayer(PlatformLayer*) override;
> +    void setVideoLayerID(uint32_t) override;
> +    void enterFullscreen() override;
> +    void exitFullscreen() override;

virtual please.

> Source/WebKit2/WebProcess/WebVideoFullscreenManager.h:75
> +    RefPtr<WebPage> m_page;

Seems very odd for this to retain Page.

[Jeremy Jones]

Created attachment 223358 [details]
Patch

[Jeremy Jones]

Created attachment 223400 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 223400 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:60
> +    WebPageProxy* m_page;

Can this be a reference?

[WebKit Commit Bot]

Comment on attachment 223400 [details]
Patch

Clearing flags on attachment: 223400

Committed r163600: <http://trac.webkit.org/changeset/163600>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jon Lee]

*** Bug 127302 has been marked as a duplicate of this bug. ***