-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: upgrade to Manifest V3 #101
Conversation
Co-authored-by: Marcus <marcustyphoon@gmail.com>
fixes a theoretical bug where request error handling does not work due to the background script/sw being destroyed and recreated between onBeforeRequest firing and onErrorOccurred/onCompleted firing for the same request
cc @marcustyphoon, forgot you're not a collaborator here yet (sent you an invite now). Consider this a review request 😁 |
I... think this is fine. After reading https://bugzilla.mozilla.org/show_bug.cgi?id=1889402 and https://bugzilla.mozilla.org/show_bug.cgi?id=1893232 I believe that it would be an issue if we were to add another host permission in the future, but as-is Firefox users should be prompted on install and can toggle the permission later in the preference pane no matter what they choose. Chrome users are still, somewhat mysteriously, not given any UI for this, so I guess we're just hoping there's no way to deny the permission during the install flow. |
The issue of duplicate asks being saved is interesting. I do encounter duplicate asks, but I'm trying to pin down exactly when, and thus whether it's just due to the Ah—nope, I can trigger it with chrome.webRequest.onBeforeRequest.addListener(async ({ method, requestBody, requestId, timeStamp, url }) => {
+ await new Promise(resolve => setTimeout(resolve, 2000 * Math.random()));
const { [requestId]: savedTimeStamp } = await chrome.storage.session.get(requestId);
if (savedTimeStamp) { I can't say I fully understand what's happening here, but as you can probably tell from what I tried there, my first thought is that this is a symptom of session storage access being asynchronous. If there can be a time gap between the read of and write to session storage, then another Edit: One way to solve this is to leave the synchronous deduplication code in place, adding the session storage as a fallback. See the mentioning PR. |
Ping for re-review @marcustyphoon |
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.
Smoke tested in Firefox 128 and Chromium 121.
Links
Changelog
Removed the
webextension-polyfill
dependency in favour of using thechrome
global directly, since this namespace has promises enabled in Manifest V3.Upgraded the extension to cross-browser Manifest V3:
manifest_version
:2
→3
browser_action
/browserAction
→action
background.service_worker
for Chrome compatibilitybackground.persistent
(defaults tofalse
in MV3)host_permissions
optional_permissions
→optional_host_permissions
As per this comment, removed the
handledRequests
Map
in favour ofchrome.storage.session
interactions for increased robustness when considering the now non-persistent nature of the background script.I opted not to implement the host permissions banner since that shouldn't be a problem for the targeted browser versions, although it would be problematic for anyone trying to run this on Safari (which we do not officially support).
Testing steps
Chrome
(expected: one warning about
background.scripts
being unsupported)www.tumblr.com
(i.e. it only saves one copy)www.tumblr.com
www.tumblr.com/inbox
*.tumblr.com/ask
www.tumblr.com
(i.e. the error banner appears on that item in the outbox page)(easy repro: open the ask form for a blog you control, disable asks for that blog, then try to send an ask)
*.tumblr.com/ask
Firefox
(expected: one warning about
background.service_worker
being unrecognised)www.tumblr.com
www.tumblr.com
www.tumblr.com/inbox
*.tumblr.com/ask
www.tumblr.com
*.tumblr.com/ask