Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #3113: Fetch adblock data every 6 hours. #3130

Merged
merged 1 commit into from
Dec 11, 2020
Merged

Fix #3113: Fetch adblock data every 6 hours. #3130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Client/Application/Delegates/AppDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,8 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UIViewControllerRestorati
if let authInfo = KeychainWrapper.sharedAppContainerKeychain.authenticationInfo(), authInfo.isPasscodeRequiredImmediately {
authenticator?.willEnterForeground()
}

AdblockResourceDownloader.shared.startLoading()
Copy link
Contributor

@soner-yuksel soner-yuksel Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I have a suggestion. what do you think about adding resource downloading into also applicationDidEnterBackground.

Use background task and use that window efficiently. We can define a background task with

private var backgroundTaskIdentifier: UIBackgroundTaskIdentifier?
backgroundTaskIdentifier = UIApplication.shared.beginBackgroundTask {
    self.endBackgroundTask()
 }

and have an background task finish method like

private func endBackgroundTask() {
    guard let backgroundTaskIdentifier = backgroundTaskIdentifier else { return }
    UIApplication.shared.endBackgroundTask(backgroundTaskIdentifier)
    self.backgroundTaskIdentifier = nil
}

and also we can have completion block from AdBlockResourceDownloader and end the task If operation is success.

And since we have also resource downloading in foreground this will help us improving the download experience and utilize the background window

Copy link
Contributor Author

Choose a reason for hiding this comment

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

overkill imo, why would you want to download it both when the app goes in background and in foreground?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we will download while going to background using background task, it will not need to download while app is coming foreground and occupy a thread and do processing.
Why we need to add both(background/foreground) is background task can initiate processes that doesnt take too long and there is no guarantee it will succeed so in that case foreground can initialize the process if it is needed.

But yes this is a suggestion, not necessary.

}

func applicationWillResignActive(_ application: UIApplication) {
Expand Down
14 changes: 14 additions & 0 deletions Client/Frontend/Settings/AdblockDebugMenuTableViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class AdblockDebugMenuTableViewController: TableViewController {
if listNames.isEmpty { return }

self.dataSource.sections = [self.actionsSection,
self.fetchSection,
self.datesSection,
self.bundledListsSection(names: listNames),
self.downloadedListsSection(names: listNames)]
Expand All @@ -44,6 +45,19 @@ class AdblockDebugMenuTableViewController: TableViewController {
return section
}

private var fetchSection: Section {
var section = Section(footer: "Last time we pinged the server for new data. If adblock list hasn't changed `Last Time Updated` section does not update.")
let dateFormatter = DateFormatter().then {
$0.dateStyle = .short
$0.timeStyle = .short
}

section.rows = [.init(text: "Last fetch time", detailText:
dateFormatter.string(from: AdblockResourceDownloader.shared.lastFetchDate))]

return section
}

private var datesSection: Section {
var section = Section(header: "Last time updated",
footer: "When the lists were last time updated on the device")
Expand Down
14 changes: 12 additions & 2 deletions Client/WebFilters/AdblockResourceDownloader.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,19 @@ class AdblockResourceDownloader {
Preferences.Shields.useRegionAdBlock.observe(from: self)
}

/// Initialized with year 1970 to force adblock fetch at first launch.
private(set) var lastFetchDate = Date(timeIntervalSince1970: 0)

func startLoading() {
AdblockResourceDownloader.shared.regionalAdblockResourcesSetup()
AdblockResourceDownloader.shared.generalAdblockResourcesSetup()
let now = Date()
let fetchInterval = AppConstants.buildChannel.isPublic ? 6.hours : 10.minutes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

blocked to decide on fetch interval, i like to fetch more often than 24hours, set it to 6 for now

Copy link
Member

Choose a reason for hiding this comment

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

can we add some randomization to this so it's not as much of a user fingerprint? ex: each time the fetch is supposed to occur, delay it by a random amount between 1 and 10 minutes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@diracdeltas This will only be non-random if the user happens to open their phone every 6 hours since it only triggered on app foreground, not on a specific timer.


if now.timeIntervalSince(lastFetchDate) >= fetchInterval {
lastFetchDate = now

AdblockResourceDownloader.shared.regionalAdblockResourcesSetup()
AdblockResourceDownloader.shared.generalAdblockResourcesSetup()
}
}

func regionalAdblockResourcesSetup() {
Expand Down