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

Migrate to targeted Swift Concurrency checking #937

Merged
merged 3 commits into from
Jan 19, 2023

Conversation

samsymons
Copy link
Collaborator

@samsymons samsymons commented Jan 17, 2023

Task/Issue URL: https://app.asana.com/0/1199230911884351/1203759366656777/f
Tech Design URL:
CC: @mallexxx

Description:

This PR sets the macOS browser to use Targeted Swift Concurrency checks, and resolves warnings that came up when making that change.

Steps to test this PR:

  1. Build the app and check that no concurrency warnings appear (including for the test suite)
  2. Check that the Fire button works as expected
  3. Check that Lottie animations work as expected
  4. To be really cautious, check that the NSException extension works as expected. Something like this should do it:
        do {
            try NSException.catch {
                let name = NSExceptionName(rawValue: "Name")
                let exception = NSException(name: name, reason: "Reason", userInfo: ["key": "value"])
                exception.raise()
            }
        } catch {
            print(error)
        }

Testing checklist:

  • Test with Release configuration
  • Test proper deallocation of tabs
  • Make sure committed submodule changes are desired
  • Make sure configuration is changed only in xcconfig files, not in the Xcode project file directly

Internal references:

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

Comment on lines +24 to +25
let errorDescription: String
let debugDescription: String
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These still conform to the error protocols that require them, so this shouldn't affect the behaviour of the app when receiving an exception. This change was made as NSException doesn't conform to Sendable but Swift.Error does, so I've extracted out only the values of the exception that we use.

@@ -17,7 +17,7 @@
//

import Cocoa
import Lottie
@preconcurrency import Lottie
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should go away once we upgrade our Lottie version, but the latest version of Lottie doesn't work perfectly with our animations. I plan to request permission to pull our animation files out into a sample app and send it to the Lottie maintainers during my next maintenance days, as we're blocked from upgrading until they work again.

@tomasstrba tomasstrba self-assigned this Jan 18, 2023
@tomasstrba tomasstrba self-requested a review January 18, 2023 10:58
Comment on lines +32 to +34
@MainActor func dataRecords(ofTypes dataTypes: Set<String>) async -> [WKWebsiteDataRecord]
@MainActor func removeData(ofTypes dataTypes: Set<String>, modifiedSince date: Date) async
@MainActor func removeData(ofTypes dataTypes: Set<String>, for records: [WKWebsiteDataRecord]) async
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't really change any behaviour since this protocol wraps a type that needs to be on the main thread anyway, but the compiler isn't yet smart enough to know that so this just makes it happy.

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! 👍 This is great! 🥇

@tomasstrba tomasstrba assigned samsymons and unassigned tomasstrba Jan 19, 2023
@samsymons samsymons merged commit fff4c3b into develop Jan 19, 2023
@samsymons samsymons deleted the sam/move-to-targeted-swift-concurrency-checking branch January 19, 2023 23:26
samsymons added a commit that referenced this pull request Jan 23, 2023
* develop: (61 commits)
  Version Bump to 0.31.7
  Update embedded files
  Update BSK with autofill 6.1.2 (#942)
  Migrate to targeted Swift Concurrency checking (#937)
  Add bundler configuration to limit Gemfile.lock noise (#939)
  update openssl depedency (#918)
  Remove tab content when showing TabContent.none (such as for new tabs opened from navigation links) (#938)
  Update GRDB to 2.0.0 (upstream 6.6.0, SQLCipher 4.5.3) (#933)
  Sparkle 2 (#934)
  Bump Submodules/privacy-reference-tests from `de75d51` to `4fdbbb6` (#932)
  Add Danger (#928)
  Update BSK with autofill 6.1.1 (#920)
  Set version to 0.31.6
  Fix GCC_OPTIMIZATION_LEVEL for Release and Review builds (#931)
  Disable testability for Release and Review builds (#929)
  update version
  update embedded files
  Remove the QuackDev reference. (#926)
  Bump find-in-page version (#889)
  Use ‘Settings’ in place of ‘Preferences’ on macOS Ventura (#925)
  ...
samsymons added a commit that referenced this pull request Jan 26, 2023
# By Dominik Kapusta (4) and others
# Via GitHub
* develop:
  Blockable JS Alerts inside tabs (#904)
  Fix add bookmark / folder modal in fullscreen (#930)
  Add Mac App Store target (#927)
  Pull request review checklist added to the pull request template (#935)
  Version Bump to 0.31.7
  Update embedded files
  Update BSK with autofill 6.1.2 (#942)
  Migrate to targeted Swift Concurrency checking (#937)
  Add bundler configuration to limit Gemfile.lock noise (#939)
  update openssl depedency (#918)
  Remove tab content when showing TabContent.none (such as for new tabs opened from navigation links) (#938)
  Update GRDB to 2.0.0 (upstream 6.6.0, SQLCipher 4.5.3) (#933)
  Sparkle 2 (#934)
  Bump Submodules/privacy-reference-tests from `de75d51` to `4fdbbb6` (#932)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
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