Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix video fullscreen exit crash #295

Merged
merged 11 commits into from
Feb 21, 2022
Merged

Fix video fullscreen exit crash #295

merged 11 commits into from
Feb 21, 2022

Conversation

mallexxx
Copy link
Collaborator

@mallexxx mallexxx commented Nov 2, 2021

Task/Issue URL: https://app.asana.com/0/1199237043630360/1201203279491064/f
Tech Design URL:
CC: @tomasstrba @samsymons

Description:
Fixes crash when fullscreen playing Tab/Window is closed

Steps to test this PR:

  1. Open youtube.com full screen, exit full screen
  2. Open one tab with youtube.com; open full screen, hit cmd+w
  3. Open one New Tab, second tab with youtube.com; open full screen, hit cmd+w
  4. Open one window with youtube tab and a New Tab, enter full screen; Open another window on external display with youtube.com, enter full screen, hit cmd+w in 2nd window, hit ctrl+Tab in 1st window

Testing checklist:

  • Test with Release configuration
  • Validate Tab switching
  • Tab resizing
  • Preferences/Bookmarks management

Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

@mallexxx mallexxx force-pushed the alex/fix-fullscreen-crash branch from bc2e86a to e9d9ebe Compare November 2, 2021 11:06
@tomasstrba tomasstrba self-assigned this Nov 3, 2021
Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alex, unfortunately the test 2 is failing for me.

I get this when ad is playing and I hit CMD + W
Screen Shot 2021-11-03 at 13 32 00

and crash when video is playing and I hit CMD + W
Screen Shot 2021-11-03 at 13 32 24

(macOS 12, Xcode 13.1)

@mallexxx mallexxx requested a review from tomasstrba February 9, 2022 12:51
@mallexxx
Copy link
Collaborator Author

mallexxx commented Feb 9, 2022

@tomasstrba could you take a look again at this please

Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍 🎖️ 🏅 Thanks for taking this of my shoulders 🙏

Just one thing below and then feel free to merge :)

@@ -20,6 +20,12 @@ import Cocoa
import Combine
import os.log

#if DEBUG
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general this is a bad technique. The point of #if DEBUG #else is to resolve the condition during the compilation. When using isDebugBuild both branches are part of the binary.

I can't see a usage of isDebugBuild, was it merged by mistake? (it was previously there and I remember refactoring it)

@@ -170,4 +170,13 @@ final class WebView: WKWebView {
inspectorPerform("showResources")
}

var fullscreenWindowController: NSWindowController? {
guard let fullscreenWindowController = self.window?.windowController,
fullscreenWindowController.className.contains("FullScreen")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 what a cool hack

@tomasstrba tomasstrba assigned mallexxx and unassigned tomasstrba Feb 18, 2022
@mallexxx mallexxx merged commit a785c70 into develop Feb 21, 2022
@mallexxx mallexxx deleted the alex/fix-fullscreen-crash branch February 21, 2022 07:00
samsymons added a commit that referenced this pull request Feb 21, 2022
* develop:
  Fix video fullscreen exit crash (#295)
  Set BrowserServicesKit to 8.0.2.
  Fix crash on SecureVault corruption (#417)
  ensure app usage is sent (#429)
samsymons added a commit that referenced this pull request Feb 21, 2022
…ntication

* sam/logins-filtering-and-sorting:
  Fix video fullscreen exit crash (#295)
  Set BrowserServicesKit to 8.0.2.
  Fix crash on SecureVault corruption (#417)
  ensure app usage is sent (#429)
samsymons added a commit that referenced this pull request Feb 22, 2022
* develop:
  Waiting for ContentBlockingRules to be applied before navigation (#402)
  Cookie prompt management (#312)
  Pass config data to Autofill UserScript (#418)
  Fix video fullscreen exit crash (#295)
jonathanKingston pushed a commit that referenced this pull request Feb 25, 2022
* Fix crash on fullscreen exit

* fix resizing

* trying to fix Xcode 13.0 compatibility

* Xcode 13.1 fix

* Unit tests fixed for macOS 12

* fix tab switching

* Fix window closing when video playing in full screen

* cleanup

Co-authored-by: Tomas Strba <[email protected]>
@tomasstrba tomasstrba mentioned this pull request Mar 7, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants