-
-
Notifications
You must be signed in to change notification settings - Fork 390
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
Unset "failed to initialize" error upon successful initialization #2971
Conversation
if (self.criticalError == "Privacy Badger failed to initialize") { | ||
delete self.criticalError; | ||
chrome.tabs.query({ active: true, lastFocusedWindow: true }, (tabs) => { | ||
if (tabs[0]) { | ||
self.updateBadge(tabs[0].id); | ||
} | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we setting self.criticalError to "Privacy Badger failed to initialize" in dispatcher
if we always delete it? Should we check for another condition before deleting? Mainly asking to make sure I understand what's going on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We set "failed to initialize" when a content script or PB's popup or options want something, and badger.INITIALIZED
is still false ten seconds after PB's background process started running.
If PB does in fact finish initializing, PB didn't fail to initialize, it was just really slow. We should unset "failed to initialize" at this point, right? Not sure what other condition we should check before deleting the error.
We could take out all of the "failed to initialize" logic, but then we would back at not knowing if PB were to ever fail to initialize again. I think not knowing is quite bad because then we're silently failing some proportion of our users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, so do we not have to check any condition to confirm that PB actually initialized because this code won't run unless PB finished initializing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if we got this far, badger.INITIALIZED
is true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Tested locally before and after code changes. Reduced the 10 second initialization timeout to 10 ms to confirm that PB settings changes persist and the error symbol doesn't appear if PB initializes after the timeout is triggered!
Unset "failed to initialize" error upon successful initialization.
Following up on #2967. Fixes another part of #2966. This PR is this bit: