[webkit-reviews] review granted: [Bug 134890] WebVideoFullscreenInterfaceAVKit should only call the UI from main thread. : [Attachment 234872] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jeremy Jones
<jeremyj-wk at apple.com>'s request for review:
Bug 134890: WebVideoFullscreenInterfaceAVKit should only call the UI from main
thread.
https://bugs.webkit.org/show_bug.cgi?id=134890

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

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234872&action=review


> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:592
> +	   if (m_playerController)
> +	       playerController().delegate = m_videoFullscreenModel;

Why use both m_playerController and playerController()?

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:623
> +	   playerController.contentDuration = duration;
> +	   playerController.maxTime = duration;
> +	   playerController.contentDurationWithinEndTimes = duration;
> +	   playerController.loadedTimeRanges = @[@0, @(duration)];
> +	   
> +	   // FIXME: we take this as an indication that playback is ready.
> +	   playerController.canPlay = YES;
> +	   playerController.canPause = YES;
> +	   playerController.canTogglePlayback = YES;
> +	   playerController.hasEnabledAudio = YES;
> +	   playerController.canSeek = YES;
> +	   playerController.minTime = 0;
> +	   playerController.status = AVPlayerControllerStatusReadyToPlay;

In setWebVideoFullscreenModel you test to ensure that m_playerController isn't
NULL. Why is it unnecessary here?

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:637
> +	   NSTimeInterval anchorTimeStamp = ![playerController() rate] ? NAN :
anchorTime;
> +	   AVValueTiming *timing = [getAVValueTimingClass()
valueTimingWithAnchorValue:currentTime
> +	       anchorTimeStamp:anchorTimeStamp rate:0];
> +	   playerController().timing = timing;

Ditto. Also, why does this code use playerController() when above you use
playerController.?

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:648
> +	   playerController().rate = isPlaying ? playbackRate : 0.;

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:660
> +	   playerController().hasEnabledVideo = hasVideo;
> +	   playerController().contentDimensions = CGSizeMake(width, height);

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:682
> +	   playerController().seekableTimeRanges = seekableRanges;

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:707
> +	   playerController().audioMediaSelectionOptions = webOptions;
> +	   if (selectedIndex < webOptions.count)
> +	       playerController().currentAudioMediaSelectionOption =
webOptions[(size_t)selectedIndex];

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:721
> +	   playerController().legibleMediaSelectionOptions = webOptions;
> +	   if (selectedIndex < webOptions.count)
> +	       playerController().currentLegibleMediaSelectionOption =
webOptions[(size_t)selectedIndex];

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:740
> +	   playerController().externalPlaybackAirPlayDeviceLocalizedName =
localizedDeviceName;
> +	   playerController().externalPlaybackType = externalPlaybackType;
> +	   playerController().externalPlaybackActive = enabled;

Ditto.