Fixes by code review #2

ios/brave-ios/Sources/Brave/Frontend/Browser/HomePanel/NTPDataSource.swift

@@ -26,7 +26,7 @@ enum NTPWallpaper {
     case .image(let background):
       imagePath = background.imagePath
     case .sponsoredImage(let background):
-      if isSponsoredVideo(background) {
+      if background.isVideoFile {
         return nil
       }
       imagePath = background.imagePath
@@ -59,10 +59,6 @@ enum NTPWallpaper {
       return background.focalPoint
     }
   }
-
-  private func isSponsoredVideo(_ background: NTPSponsoredImageBackground) -> Bool {
-    return background.imagePath.pathExtension == "mp4"
-  }
 }
 
 public class NTPDataSource {

ios/brave-ios/Sources/Brave/Frontend/Browser/New Tab Page/Backgrounds/NewTabPageBackgroundButtonsView.swift

@@ -58,7 +58,8 @@ class NewTabPageBackgroundButtonsView: UIView, PreferencesObserver {
       activeView?.isHidden = false
     }
   }
-  var tappedPlayButton: ((Bool) -> Void)?
+  var tappedPlayButton: (() -> Void)?
+  var tappedBackgroundDuringAutoplay: (() -> Void)?
 
   private let imageCreditButton = ImageCreditButton().then {
     $0.isHidden = true
@@ -167,15 +168,15 @@ class NewTabPageBackgroundButtonsView: UIView, PreferencesObserver {
   }
 
   @objc private func tappedVideoPlayButton() {
-    tappedPlayButton?(true)
+    tappedPlayButton?()
   }
 
   @objc private func tappedVideDuringAutoplay() {
-    tappedPlayButton?(false)
+    tappedBackgroundDuringAutoplay?()
   }
 
   func videoAutoplayStarted() {
-    let tapGesture: UITapGestureRecognizer = UITapGestureRecognizer(
+    let tapGesture = UITapGestureRecognizer(
       target: self,
       action: #selector(self.tappedVideDuringAutoplay)
     )
@@ -191,12 +192,13 @@ class NewTabPageBackgroundButtonsView: UIView, PreferencesObserver {
     if let playButtonGestureRecognizer = playButtonGestureRecognizer {
       removeGestureRecognizer(playButtonGestureRecognizer)
     }
+    playButtonGestureRecognizer = nil
   }
 
   private func updatePlayButtonVisibility() {
     let isLandscape = frame.width > frame.height
 
-    // Hide the play button if the video is in landscape mode on a phone
+    // Hide the play button if the video is in landscape mode on iPhone
     if isLandscape && UIDevice.isPhone {
       playButton.isHidden = true
     } else {

ios/brave-ios/Sources/Brave/Frontend/Browser/New Tab Page/Backgrounds/NewTabPageVideoButtonsView.swift

@@ -29,16 +29,19 @@ class NewTabPageVideoButtonsView: UIView {
     backgroundColor = .clear
 
     addSubview(cancelButton)
-
     addSubview(playPauseButtonImage)
 
-    let tapGesture: UITapGestureRecognizer = UITapGestureRecognizer(
+    let tapGesture = UITapGestureRecognizer(
       target: self,
       action: #selector(self.videoBackgroundTapped(sender:))
     )
     tapGesture.numberOfTapsRequired = 1
     addGestureRecognizer(tapGesture)
 
+    cancelButton.snp.makeConstraints {
+      $0.top.equalTo(self.safeAreaLayoutGuide.snp.top).offset(20)
+      $0.right.equalTo(self.safeAreaLayoutGuide.snp.right).offset(-20)
+    }
     playPauseButtonImage.snp.makeConstraints {
       $0.centerX.equalToSuperview()
       $0.centerY.equalToSuperview().offset(20)

ios/brave-ios/Sources/Brave/Frontend/Browser/New Tab Page/Backgrounds/NewTabPageVideoPlayer.swift

@@ -16,20 +16,19 @@ import UIKit
 /// playback on the New Tab Page. It handles events such as play finished,
 /// played 25 percent, autoplay finished, play cancelled, and video loaded.
 class NewTabPageVideoPlayer {
-  var autoplayFinishedEvent: ((Bool) -> Void)?
-  var videoLoadedEvent: ((Bool) -> Void)?
+  var autoplayFinishedEvent: (() -> Void)?
+  var autoplayStartedEvent: (() -> Void)?
   var playFinishedEvent: (() -> Void)?
   var playStartedEvent: (() -> Void)?
   var played25PercentEvent: (() -> Void)?
-  var playCancelledEvent: ((Bool) -> Void)?
+  var playCancelledEvent: (() -> Void)?
 
   var player: AVPlayer?
 
-  private var playStarted = false
-  private var previewAutoplayFinished = false
+  private var isPlayStarted = false
   private var isLoadFinished = false
-  private var isAutoplayStartedOnce = false
-  private var shouldStartAutoplayAfterLoad = false
+  private var isAutoplayRequestedOnce = false
+  private var isAutoplayFinished = false
   private var timeObserver: Any?
   private var playerObserver: NSKeyValueObservation?
 
@@ -38,24 +37,16 @@ class NewTabPageVideoPlayer {
   private var stopFrame: Int?
   private var frameRate: Float?
 
-  init(_ backgroundVideoPath: URL) {
-    let asset: AVURLAsset = AVURLAsset(url: backgroundVideoPath)
-    let item = AVPlayerItem(asset: asset)
-    player = AVPlayer(playerItem: item)
+  init(backgroundVideoPath: URL, videoInitiallyVisible: Bool) {
+    // Do not start autoplay if video is not initially visible
+    if !videoInitiallyVisible {
+      isAutoplayRequestedOnce = true
+      isAutoplayFinished = true
+    }
 
     stopFrame = parseStopFrameFromFilename(filename: backgroundVideoPath.lastPathComponent)
-
-    loadVideoTrackParams(
-      asset: asset
-    )
-
-    NotificationCenter.default
-      .addObserver(
-        self,
-        selector: #selector(self.playerDidFinishPlaying),
-        name: .AVPlayerItemDidPlayToEndTime,
-        object: item
-      )
+    resetPlayer(backgroundVideoPath)
+    loadVideoTrackParams(backgroundVideoPath)
   }
 
   deinit {
@@ -82,8 +73,8 @@ class NewTabPageVideoPlayer {
   }
 
   func startVideoPlayback() {
-    if !previewAutoplayFinished {
-      autoplayFinished(animated: false)
+    if !isAutoplayFinished {
+      autoplayFinished()
     }
 
     player?.isMuted = false
@@ -102,27 +93,27 @@ class NewTabPageVideoPlayer {
       }
 
     player?.play()
-    playStarted = true
+    isPlayStarted = true
 
     playStartedEvent?()
   }
 
-  func maybeCancelPlay(animated: Bool) {
-    if !previewAutoplayFinished && isLoadFinished {
-      autoplayFinished(animated: false)
+  func maybeCancelPlay() {
+    if !isAutoplayFinished && isLoadFinished {
+      autoplayFinished()
       player?.pause()
     }
 
-    if playStarted {
+    if isPlayStarted {
       player?.pause()
       if let timeObserver = timeObserver {
         player?.removeTimeObserver(timeObserver)
       }
       timeObserver = nil
 
-      playCancelledEvent?(animated)
+      playCancelledEvent?()
     }
-    playStarted = false
+    isPlayStarted = false
   }
 
   func togglePlay() -> Bool {
@@ -137,66 +128,66 @@ class NewTabPageVideoPlayer {
     return !isVideoInProgress
   }
 
-  private func loadVideoTrackParams(asset: AVURLAsset) {
+  func maybeStartAutoplay() {
+    // Start autoplay only once.
+    if isAutoplayRequestedOnce {
+      return
+    }
+    isAutoplayRequestedOnce = true
+
+    // Start autoplay after video is loaded
+    if !isLoadFinished {
+      return
+    }
+
+    startAutoplay()
+  }
+
+  private func loadVideoTrackParams(_ backgroundVideoPath: URL) {
     Task {
-      var loadedFrameRate: Float?
+      var frameRate: Float?
+      let asset: AVURLAsset = AVURLAsset(url: backgroundVideoPath)
       let isPlayable = try? await asset.load(.isPlayable)
       if let isPlayable = isPlayable,
         isPlayable,
         let videoTrack = try? await asset.loadTracks(withMediaType: .video).first
       {
-        loadedFrameRate = try? await videoTrack.load(.nominalFrameRate)
+        frameRate = try? await videoTrack.load(.nominalFrameRate)
       }
-      let frameRate = loadedFrameRate
 
       finishPlayerSetup(isPlayable: isPlayable, frameRate: frameRate)
     }
   }
 
   private func finishPlayerSetup(isPlayable: Bool?, frameRate: Float?) {
     DispatchQueue.main.async { [weak self] in
+      guard let self = self else { return }
+
       guard let isPlayable = isPlayable else {
-        self?.videoLoaded(succeeded: false)
+        self.videoLoaded(succeeded: false)
         return
       }
-      self?.videoLoaded(succeeded: isPlayable)
+      self.frameRate = frameRate
+      self.videoLoaded(succeeded: isPlayable)
     }
   }
 
-  func maybeStartAutoplay(shouldShowVideoBackground: Bool) {
-    if isAutoplayStartedOnce {
-      return
-    }
-    isAutoplayStartedOnce = true
-
-    if !shouldShowVideoBackground {
+  private func startAutoplay() {
+    if isAutoplayFinished {
       autoplayFinished()
       return
     }
 
-    if previewAutoplayFinished {
-      return
-    }
-
-    if !isLoadFinished {
-      shouldStartAutoplayAfterLoad = true
-      return
-    }
-
-    startAutoplay()
-  }
-
-  private func startAutoplay() {
     guard let duration = player?.currentItem?.duration else {
       autoplayFinished()
       return
     }
 
-    var autoplayLengthSeconds: Float64 = self.kMaxAutoplayDuration
+    var autoplayLengthSeconds = self.kMaxAutoplayDuration
     if let frameRate = frameRate,
       let stopFrame = stopFrame
     {
-      let stopFrameTime = Float64(stopFrame) / Float64(frameRate)
+      let stopFrameTime = Double(stopFrame) / Double(frameRate)
       if stopFrameTime < autoplayLengthSeconds {
         autoplayLengthSeconds = stopFrameTime
       }
@@ -215,27 +206,30 @@ class NewTabPageVideoPlayer {
     // event if needed.
     playerObserver = player?.observe(\.timeControlStatus, options: [.new, .old]) {
       [weak self] (player, change) in
-      if player.timeControlStatus == .paused && self?.previewAutoplayFinished == false {
-        self?.autoplayFinished()
+      guard let self = self else { return }
+      if player.timeControlStatus == .paused && !self.isAutoplayFinished {
+        self.autoplayFinished()
       }
     }
 
     player?.isMuted = true
     player?.play()
+
+    autoplayStartedEvent?()
   }
 
   private func isVideoInProgress() -> Bool {
     return player?.timeControlStatus != .paused
   }
 
   @objc private func playerDidFinishPlaying(note: NSNotification) {
-    if !previewAutoplayFinished {
+    if !isAutoplayFinished {
       autoplayFinished()
       return
     }
 
     playFinishedEvent?()
-    playStarted = false
+    isPlayStarted = false
   }
 
   private func checkPlayPercentage() {
@@ -260,21 +254,21 @@ class NewTabPageVideoPlayer {
 
   private func videoLoaded(succeeded: Bool) {
     isLoadFinished = true
-    videoLoadedEvent?(succeeded)
 
     if !succeeded {
       autoplayFinished()
       return
     }
 
-    if shouldStartAutoplayAfterLoad {
+    // Autoplay didn't start before because the video wasn't loaded
+    if isAutoplayRequestedOnce {
       startAutoplay()
     }
   }
 
-  private func autoplayFinished(animated: Bool = true) {
-    previewAutoplayFinished = true
-    autoplayFinishedEvent?(animated)
+  private func autoplayFinished() {
+    isAutoplayFinished = true
+    autoplayFinishedEvent?()
   }
 
   private func cleanupObservers() {