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

Improved IPC support #536

Merged
merged 12 commits into from
Oct 22, 2023

Conversation

diegoreymendez
Copy link
Contributor

@diegoreymendez diegoreymendez commented Oct 17, 2023

Task/Issue URL: https://app.asana.com/0/0/1205738275545702/f

iOS PR: duckduckgo/iOS#2101
macOS PR: duckduckgo/macos-browser#1690

What kind of version bump will this require?: Minor

Description

Offers improved IPC support in BSK.

How to test

See iOS and macOS PRs for testing instructions.


Internal references:

Software Engineering Expectations
Technical Design Template

// This is actually an improved way to send messages.
// Please avoid adding new messages to this enum, and instead
// add them to `ExtensionRequest`
case request(_ request: ExtensionRequest)
Copy link
Contributor Author

@diegoreymendez diegoreymendez Oct 17, 2023

Choose a reason for hiding this comment

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

Comment has it... ExtensionRequest is meant to replace ExtensionMessage.

case sendTestNotification
}

public enum ExtensionRequest: Codable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The advantage of using this is two folded:

  • Much simpler: adding a case is enough to be able to receive it in the extension, there's no need to add code to encode the data.
  • Less likely to cause trouble: because the app and extension use this enum, adding a new case will present an error if the case handling is missing in either side.

import Combine
import Foundation

extension UserDefaults {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit annoying to have to add these, but this is how we can use UserDefaults as an IPC mechanism through its inter-process KVO notifications. Even if the user edits a value from the command line, we'll get notified of the change.

<dict>
<key>_XCCurrentVersionName</key>
<string>HTTPSUpgrade 3.xcdatamodel</string>
</dict>
<dict/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, any reason why this changed?

@@ -17,25 +17,13 @@
//

import Foundation

/*
protocol NetworkProtectionSelectedServerStore: AnyObject {
Copy link
Contributor

Choose a reason for hiding this comment

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

One note is that we'll have to add something like this back to handle geoswitching, I believe

Copy link
Contributor

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @diegoreymendez

@diegoreymendez diegoreymendez merged commit c427dc6 into main Oct 22, 2023
5 checks passed
@diegoreymendez diegoreymendez deleted the diego/feature/change-netp-vpn-ownership-prototype branch October 22, 2023 06:44
diegoreymendez added a commit to duckduckgo/macos-browser that referenced this pull request Oct 22, 2023
diegoreymendez added a commit to duckduckgo/iOS that referenced this pull request Oct 22, 2023
Task/Issue URL: https://app.asana.com/0/0/1205738275545703/f

BSK PR: duckduckgo/BrowserServicesKit#536
macOS PR: duckduckgo/macos-browser#1690

## Description

Integrates the latest changes from BSK.
Comment on lines +38 to +44
private func selectedServerFromRawValue(_ rawValue: String?) -> TunnelSettings.SelectedServer {
guard let selectedEndpoint = networkProtectionSettingSelectedServerRawValue else {
return .automatic
}

return .endpoint(selectedEndpoint)
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional to use networkProtectionSettingSelectedServerRawValue directly here instead of rawValue? I see the same thing with registrationKeyValidityFromRawValue(_:) @diegoreymendez

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nopes... I think I missed this one, but didn't realize since it's unlikely to fail. Good catch!

samsymons added a commit that referenced this pull request Nov 7, 2023
* main:
  Fix syncing empty favorites folders (#546)
  Alert user about abnormal app conditions (#539)
  Bump Tests/BrowserServicesKitTests/Resources/privacy-reference-tests (#543)
  NetP iOS notifications settings (#541)
  Secure vault app group support (#532)
  Update autofill to 9.0.0 (#537)
  linting (#542)
  feat(dashboard): updating feedback form (#533)
  Improved IPC support (#536)
  bump C-S-S for duckplayer on big sur (#538) (#540)
  Append build number to metricKit crash version (#534)
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.

3 participants