Display Privacy Stats on HTML New Tab Page #3611
Conversation
| let count: Int64 | ||
| let displayName: String | ||
|
|
||
| static func otherCompanies(count: Int64) -> TrackerCompany { |
There was a problem hiding this comment.
Minor: should be called otherCompany as the return is singular
There was a problem hiding this comment.
It's the sum of all other companies (the non-top-100 companies), so I'd rather keep it plural if that makes sense.
| } | ||
|
|
||
| private enum Const { | ||
| static let maxTopCompaniesCount: Int = 100 |
There was a problem hiding this comment.
Minor: Maybe add documentation explaining why 100 is the max here.
| public static func make(location: URL) -> CoreDataDatabase { | ||
| let bundle = PrivacyStats.bundle | ||
| guard let model = CoreDataDatabase.loadModel(from: bundle, named: "PrivacyStats") else { | ||
| fatalError("Failed to load model") |
There was a problem hiding this comment.
Should this be a fatalError? Could we gracefully handle privacy stats not working as expected?
There was a problem hiding this comment.
Maybe we could throw an error and handle it?
There was a problem hiding this comment.
Yep, I've clearly taken the shortcut here. All other database stacks use fatalError here but it's a weak justification, as users could probably live without privacy stats, rather than without Bookmarks. Let me try to do something about it.
There was a problem hiding this comment.
Ok, now I know what's up. I've been here already, trying to make it not crash with a fatal error, but unfortunately this initialization is asynchronous and while we have the database object ready, we can only learn that it's unusable in an asynchronous callback. Reworking privacy stats to be asynchronous would require major changes to entire NTP loading mechanism. I will add it to the project to consider adding privacy stats module to NTP after a delay, but given it requires extra work I'd skip it for now.
After all, if the database crashes here, it's either:
- the user doesn't have space on disk, which is recoverable and they should take care of it anyway
- we messed up database migration which should really be caught earlier during the release process (and also doesn't have a chance of happening for the v1 ;))
There was a problem hiding this comment.
Reasonable justification.
|
Ran through tests @ayoy and it works great, nice work! 👍🏼 |
# By Anka (2) and others # Via GitHub (2) and Michal Smaga (1) * main: Revert to empty RMF config for debug builds (#3616) Fix test Display Privacy Stats on HTML New Tab Page (#3611) point to BSK branch and updates (#3606) Bump version to 1.116.0 (319) Client displays correct subscription (#3581) Bump version to 1.116.0 (318) [Release PR] Add StoreKit debugging menu options (#3607) # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
# By Anka (2) and others # Via GitHub (2) and Michal Smaga (1) * main: Add support for RMF survey `locale` parameter (#3613) Revert to empty RMF config for debug builds (#3616) Fix test Display Privacy Stats on HTML New Tab Page (#3611) point to BSK branch and updates (#3606) Bump version to 1.116.0 (319) Client displays correct subscription (#3581) Bump version to 1.116.0 (318) [Release PR] Add StoreKit debugging menu options (#3607) # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Task/Issue URL: https://app.asana.com/0/72649045549333/1208246350498754/f
Tech Design URL: https://app.asana.com/0/481882893211075/1208848285586302
Description:
This change adds support for displaying Privacy Stats on New Tab Page.
Steps to test this PR:
Test that reporting multiple blocked trackers concurrently works well:
Test evicting old stats:
Definition of Done:
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation