[webkit-reviews] review denied: [Bug 128143] Add support for AVKit fullscreen to WebKit2 : [Attachment 223291] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Jeremy Jones
<jeremyj-wk at apple.com>'s request for review:
Bug 128143: Add support for AVKit fullscreen to WebKit2
https://bugs.webkit.org/show_bug.cgi?id=128143

Attachment 223291: Patch
https://bugs.webkit.org/attachment.cgi?id=223291&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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.