search

ghostery / ghostery-extension

Ghostery Browser Extension for Firefox, Chrome, Opera, Edge and Safari
Mozilla Public License 2.0
1.4k stars 142 forks source link

fix(sync): move options sync to background process #2062

Closed smalluban closed 5 days ago

smalluban commented 1 week ago

This PR moves out the sync of options into the background process.

The trigger of the sync process programmatically stays as it was - every time the background starts up the sync is triggered. However, if any other context wants to force the sync, the message syncOptions must be sent (added to the settings page and panel).

The background/session.js can be cleaned up from alarms. As the sync is triggered with every background process start-up, the alarms don't give much, as any request made by the browser brings the BG back to life. For the same reason, it is safe to trigger sync then, as from my testing, BG is not killed that often (many sites make some background calls, or the user moves the mouse over the page or something similar).

Also, the sync option in the Account section is unlocked. I found a use case, where you can log in, switch sync to false, and then log out, keeping the sync set to false. On the other hand, currently, it is impossible to log in without sync turned off. It is pointless to block this setting before the user is logged in, as it might be "hacked" anyway.

After code review, the PR requires manual testing.

smalluban commented 1 week ago

@AdamGhst, I did my testing between Firefox and Chrome, but please do a round of smokes related to the sync process.

smalluban commented 6 days ago

@AdamGhst I made some tweaks, but even without them, I cannot reproduce the issue with sync off/on. However, with the latest changes, it is even better, as the sync process wasn't triggered when a user switched the toggle of that option (only after a refresh of the page). This is why you might have thought that it does not work.

AdamGhst commented 6 days ago

@AdamGhst I made some tweaks, but even without them, I cannot reproduce the issue with sync off/on. However, with the latest changes, it is even better, as the sync process wasn't triggered when a user switched the toggle of that option (only after a refresh of the page). This is why you might have thought that it does not work.

It is very, very possible that what I found was misunderstood from my side. Looking forward to testing it again. :]

github-actions[bot] commented 6 days ago

Builds for commit 2855db47f719dc9935871b9c65c6211ff606f0dc: