Drop vestigial Firefox support #7237
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7237 +/- ##
==========================================
+ Coverage 71.50% 71.63% +0.12%
==========================================
Files 1213 1208 -5
Lines 37797 37699 -98
Branches 7098 7078 -20
==========================================
- Hits 27028 27005 -23
+ Misses 10769 10694 -75 ☔ View full report in Codecov by Sentry. |
| @@ -5,7 +5,6 @@ | |||
| <content url="file://$MODULE_DIR$"> | |||
There was a problem hiding this comment.
What's this file? It has some ancient directories
There was a problem hiding this comment.
It's from IntelliJ - those excludeFolder entries tell IntelliJ not to index those folders. The outdated directories don't hurt anything, but could drop them to clean up the file
| @@ -51,7 +50,7 @@ const NetworkErrorDetail: React.FunctionComponent<{ | |||
| const absoluteUrl = selectAbsoluteUrl(error.config); | |||
|
|
|||
| const permissionsState = useAsyncState<boolean | undefined>( | |||
| async () => containsPermissions({ origins: [absoluteUrl] }), | |||
| async () => browser.permissions.contains({ origins: [absoluteUrl] }), | |||
There was a problem hiding this comment.
In Chrome there's no change, it was already short-circuited this way.
|
|
||
| import { expectContext } from "@/utils/expectContext"; | ||
|
|
||
| export async function onTabClose(watchedTabId: number): Promise<void> { |
There was a problem hiding this comment.
Will be in fregante/webext-events#12
| async function requestPermissionsFromUserGesture( | ||
| permissions: Permissions.Permissions, | ||
| ): Promise<boolean> { |
There was a problem hiding this comment.
This function has no special logic around browser.permissions.request that actually deals with user-gesture loss. Is it only named this way to be aware of the possibility?
I dropped the entire thing because it just had exceptions for Firefox, but ensurePermissionsFromUserGesture remains above, also without special logic.
There was a problem hiding this comment.
browser.permissions.request that actually deals with user-gesture
What do you mean it "deals" with it? If the gesture is lost, IIRC it just returns false (or maybe throws?)
Is it only named this way to be aware of the possibility?
Correct, requestPermissionsFromUserGesture is named that way to remind callers that it needs to come from a detectable gesture (so there should be no long-running operations between the gesture and the call)
There was a problem hiding this comment.
.request() will throw with "xx must be triggered by user gesture"
What do you mean it "deals" with it?
I think that PB does have a way to request permissions even outside user gestures, and I thought this was it. The "way" I remember looked something like "if .request() fails, then show a popup so that the user will have to click"
There was a problem hiding this comment.
The "way" I remember looked something like "if .request() fails, then show a popup so that the user will have to click"
I don't think so?
Even so, it should always have been called from a button click for activate or grant permissions. (I don't think we had any flows where it could trigger a popup at a seemingly arbitrary time). We had reworked network call ordering, etc. to ensure that the Chrome permissions call was always made within the timeout window
There was a problem hiding this comment.
Oh here it is, the copy-to-clipboard brick uses that:
The detection is here:
pixiebrix-extension/src/utils/clipboardUtils.ts
Lines 34 to 41 in dc328e8
What I was suggesting was basically:
permissions.request()- if it fails with "user gesture", inject "click anywhere to continue"
- call
permissions.request()indocument.onclick
I'll open an issue:
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
What does this PR do?
Drops all Firefox-related code as per #7236 (comment)
Checklist
src/tsconfig.strictNullChecks.json(if possible)