fix(sync): move options sync to background process #2062
+189
−172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@AdamGhst, I did my testing between Firefox and Chrome, but please do a round of smokes related to the sync process. |
|
Builds for commit 822ccda: |
chrmod
reviewed
Nov 15, 2024
| @@ -36,6 +36,9 @@ store | |||
| }; | |||
| } | |||
|
|
|||
| // Sync options with background | |||
| chrome.runtime.sendMessage({ action: 'syncOptions' }); | |||
There was a problem hiding this comment.
should we await here? we don't want UI to flicker in case option change
There was a problem hiding this comment.
We already discussed awaiting the sync process in another context, and our decision was to not wait for it. It can be long or failing for many reasons (bad internet connection etc) - The settings page are available immediately for the user (they are local).
AdamGhst
requested changes
Nov 15, 2024
There was a problem hiding this comment.
Setting Sync stops working.
Steps to reproduce:
- Login to Ghostery Account both browsers. Setting Sync is ON on both.
- Chrome - FF Setting sync works great.
- Turn off the sync option in Chrome and leave it enabled in FF.
- I make changes to the Ghostery settings in Chrome. No reaction in FF. As expected.
- Turn the sync option back on in Chrome.
- Make changes to the Ghostery settings in Chrome.
- Syncing stops working. No changes in FF after reloading the Ghostery settings page.
The same happens other way around. (FF - Chrome)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
syncOptionsmust be sent (added to the settings page and panel).The
background/session.jscan 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.