Add support for AVKit fullscreen to WebKit2
Created attachment 223052 [details] Patch
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.
> > > 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?
(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.
(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. 👍
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.
Created attachment 223291 [details] Patch
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.
> > 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.
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.
Created attachment 223358 [details] Patch
Created attachment 223400 [details] Patch
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?
Comment on attachment 223400 [details] Patch Clearing flags on attachment: 223400 Committed r163600: <http://trac.webkit.org/changeset/163600>
All reviewed patches have been landed. Closing bug.
*** Bug 127302 has been marked as a duplicate of this bug. ***