[webkit-reviews] review granted: [Bug 127762] Refactor WebVideoFullscreenController separating AVKit from MediaElement. : [Attachment 222405] Patch

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Jeremy Jones
<jeremyj-wk at apple.com>'s request for review:
Bug 127762: Refactor WebVideoFullscreenController separating AVKit from
MediaElement.
https://bugs.webkit.org/show_bug.cgi?id=127762

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

------- Additional Comments from Jer Noble <jer.noble at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=222405&action=review


> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:46
> +// Changes here must be reflected in
WebVideoFullscreenManagerProxy.messages.in

I'd leave this comment out, or move it into the ChangeLog.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:47
> +class WebVideoFullscreenInterface {

This class should go into its own header.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:61
> +// Changes here must be reflected in WebVideoFullscreenManager.messages.in
> +class WebVideoFullscreenModel {

Ditto about the comment and this class going into its own header.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:72
> +class WebVideoFullscreenModelMediaElement : public WebVideoFullscreenModel,
public EventListener {

This class (and implementation) can be pulled into their own files as well.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:95
> +class WebVideoFullscreenInterfaceAVKit : public WebVideoFullscreenInterface,
public RefCounted<WebVideoFullscreenInterfaceAVKit> {

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:212
> +    initAVPlayerLayer();
> +    initAVValueTiming();

If this is really a MediaElement model, why does it have to init AVPlayerLayer
and AVValueTiming?

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:293
> +    ref();
> +    WebThreadRun(^{
> +	   m_mediaElement->pause();
> +	   m_mediaElement->exitFullscreen();
> +	   deref();
> +    });

The standard way to do this in WebCore is to use a RefPtr protector.  I.e.:

RefPtr<WebVideoFullscreenModelMediaElement> protect(this);
WebThreadRun(^{
   ...
   protect.clear();
});

That way, if the block exits early or is dealloced without being executed, the
deref is called correctly.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:302
> +void WebVideoFullscreenModelMediaElement::play() {
> +    ref();
> +    WebThreadRun(^{
> +	   m_mediaElement->play();
> +	   deref();
> +    });
> +}

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:311
> +void WebVideoFullscreenModelMediaElement::pause()
> +{
> +    ref();
> +    WebThreadRun(^{
> +	   m_mediaElement->pause();
> +	   deref();
> +    });
> +}

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:320
> +void WebVideoFullscreenModelMediaElement::togglePlayState()
> +{
> +    ref();
> +    WebThreadRun(^{
> +	   m_mediaElement->togglePlayState();
> +	   deref();
> +    });
> +}

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:328
> +    ref();
> +    WebThreadRun(^{
> +	   m_mediaElement->setCurrentTime(time);
> +	   deref();
> +    });

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:338
> +    ref();
> +    WebThreadRun(^{
> +	   setMediaElement(nullptr);
> +	   deref();
> +    });
> +}

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:354
> +    m_playerController = [[[WebAVPlayerController alloc] init] autorelease];


This should be:  m_playerController = adoptNS([[[WebAVPlayerController alloc]
init]);

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:383
> +    NSTimeInterval anchorTimeStamp = (![m_playerController.get() rate]) ?
NAN : anchorTime;

The parens are unnecessary here.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:513
> +    WebVideoFullscreenController* strongSelf = [self retain];
> +    
> +    _interface->exitFullscreen([=](){
> +	   strongSelf->_model->setMediaElement(nullptr);
> +	   strongSelf->_interface->setWebVideoFullscreenModel(nullptr);
> +	   strongSelf->_model->setWebVideoFullscreenInterface(nullptr);
> +	   strongSelf->_model = nullptr;
> +	   strongSelf->_interface = nullptr;
> +	   [strongSelf release];

Again, use a RetainPtr here (passing into the lambda by value) rather than
manually retaining and releasing.