Skip to content
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

Start using webext-messenger (for background functions and messages) #1258

Merged
merged 17 commits into from
Sep 12, 2021
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@
"webext-content-scripts": "^0.9.0",
"webext-detect-page": "^3.0.2",
"webext-dynamic-content-scripts": "^8.0.0",
"webext-messenger": "^0.4.0",
"webext-patterns": "^1.1.1",
"webext-polyfill-kinda": "^0.1.0",
"webextension-polyfill-ts": "^0.26.0"
Expand Down
1 change: 1 addition & 0 deletions src/background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import "@/telemetry/rollbar";
import "webpack-target-webextension/lib/background";
import "webext-dynamic-content-scripts";

import "./background/messenger/registration";
import "./development/autoreload";
import "./background/installer";
import "./messaging/external";
Expand Down
158 changes: 75 additions & 83 deletions src/background/contextMenus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
*/

import pTimeout from "p-timeout";
import { liftBackground } from "@/background/protocol";
import { browser, Menus, Tabs } from "webextension-polyfill-ts";
import { isBackgroundPage } from "webext-detect-page";
import { reportError } from "@/telemetry/logging";
Expand Down Expand Up @@ -117,7 +116,11 @@ function menuListener(info: Menus.OnClickData, tab: Tabs.Tab) {
* Uninstall contextMenu for `extensionId`. Returns true if the contextMenu was removed, or false if the contextMenu was
* not found.
*/
export async function uninstall(extensionId: UUID): Promise<boolean> {
export async function uninstallContextMenu({
extensionId,
}: {
extensionId: UUID;
}): Promise<boolean> {
try {
const menuItemId = extensionMenuItems.get(extensionId);

Expand All @@ -141,101 +144,90 @@ export async function uninstall(extensionId: UUID): Promise<boolean> {
}
}

/**
* Uninstall context menu and return whether or not the context menu was uninstalled.
*/
export const uninstallContextMenu = liftBackground(
"UNINSTALL_CONTEXT_MENU",
async ({ extensionId }: { extensionId: UUID }) => uninstall(extensionId)
);

export const ensureContextMenu = liftBackground(
"ENSURE_CONTEXT_MENU",
async ({
extensionId,
contexts,
title,
documentUrlPatterns,
}: SelectionMenuOptions) => {
if (!extensionId) {
throw new Error("extensionId is required");
}
export async function ensureContextMenu({
extensionId,
contexts,
title,
documentUrlPatterns,
}: SelectionMenuOptions) {
if (!extensionId) {
throw new Error("extensionId is required");
}

// Handle the thundering herd of re-registrations when a contentScript.reactivate is broadcast
if (pendingRegistration.has(extensionId)) {
console.debug("contextMenu registration pending for %s", extensionId);
// Handle the thundering herd of re-registrations when a contentScript.reactivate is broadcast
if (pendingRegistration.has(extensionId)) {
console.debug("contextMenu registration pending for %s", extensionId);

// Is it OK to return immediately? Or do we need to track the common promise that all callers should see?
return;
}
// Is it OK to return immediately? Or do we need to track the common promise that all callers should see?
return;
}

pendingRegistration.add(extensionId);
pendingRegistration.add(extensionId);

const updateProperties: Menus.UpdateUpdatePropertiesType = {
type: "normal",
title,
contexts,
documentUrlPatterns,
};
const updateProperties: Menus.UpdateUpdatePropertiesType = {
type: "normal",
title,
contexts,
documentUrlPatterns,
};

const expectedMenuId = makeMenuId(extensionId);
const expectedMenuId = makeMenuId(extensionId);

try {
let menuId = extensionMenuItems.get(extensionId);
try {
let menuId = extensionMenuItems.get(extensionId);

if (menuId) {
try {
await browser.contextMenus.update(menuId, updateProperties);
return;
} catch (error: unknown) {
console.debug("Cannot update context menu", { error });
}
} else {
// Just to be safe if our `extensionMenuItems` bookkeeping is off, remove any stale menu item
await browser.contextMenus.remove(expectedMenuId).catch(noop);
if (menuId) {
try {
await browser.contextMenus.update(menuId, updateProperties);
return;
} catch (error: unknown) {
console.debug("Cannot update context menu", { error });
}
} else {
// Just to be safe if our `extensionMenuItems` bookkeeping is off, remove any stale menu item
await browser.contextMenus.remove(expectedMenuId).catch(noop);
}

// The update failed, or this is a new context menu
extensionMenuItems.delete(extensionId);

// The types of browser.contextMenus.create are wacky. I verified on Chrome that the method does take a callback
// even when using the browser polyfill
let createdMenuId: string | number;
menuId = await new Promise((resolve, reject) => {
// `browser.contextMenus.create` returns immediately with the assigned menu id
createdMenuId = browser.contextMenus.create(
{
...updateProperties,
id: makeMenuId(extensionId),
},
() => {
if (browser.runtime.lastError) {
reject(new Error(browser.runtime.lastError.message));
}

resolve(createdMenuId);
// The update failed, or this is a new context menu
extensionMenuItems.delete(extensionId);

// The types of browser.contextMenus.create are wacky. I verified on Chrome that the method does take a callback
// even when using the browser polyfill
let createdMenuId: string | number;
menuId = await new Promise((resolve, reject) => {
// `browser.contextMenus.create` returns immediately with the assigned menu id
createdMenuId = browser.contextMenus.create(
{
...updateProperties,
id: makeMenuId(extensionId),
},
() => {
if (browser.runtime.lastError) {
reject(new Error(browser.runtime.lastError.message));
}
);
});

extensionMenuItems.set(extensionId, menuId);
} catch (error: unknown) {
if (
getErrorMessage(error).includes("Cannot create item with duplicate id")
) {
// Likely caused by a concurrent update. In practice, our `pendingRegistration` set and `extensionMenuItems`
// should prevent this from happening
console.debug("Error registering context menu item", { error });
return;
}
resolve(createdMenuId);
}
);
});

console.error("Error registering context menu item", { error });
throw error;
} finally {
pendingRegistration.delete(extensionId);
extensionMenuItems.set(extensionId, menuId);
} catch (error: unknown) {
if (
getErrorMessage(error).includes("Cannot create item with duplicate id")
) {
// Likely caused by a concurrent update. In practice, our `pendingRegistration` set and `extensionMenuItems`
// should prevent this from happening
console.debug("Error registering context menu item", { error });
return;
}

console.error("Error registering context menu item", { error });
throw error;
} finally {
pendingRegistration.delete(extensionId);
}
);
}

if (isBackgroundPage()) {
browser.contextMenus.onClicked.addListener(menuListener);
Expand Down
10 changes: 5 additions & 5 deletions src/background/deployment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,10 @@ import { refreshRegistries } from "@/hooks/useRefresh";
import { liftBackground } from "@/background/protocol";
import * as contentScript from "@/contentScript/lifecycle";
import { selectExtensions } from "@/options/selectors";
import { uninstallContextMenu } from "@/background/contextMenus";
import { containsPermissions } from "@/utils/permissions";
import {
uninstallContextMenu,
containsPermissions,
} from "@/background/messenger/api";
import { deploymentPermissions } from "@/permissions";
import { IExtension } from "@/core";
import { ExtensionOptionsState } from "@/store/extensions";
Expand Down Expand Up @@ -83,9 +85,7 @@ function installDeployment(
extensionId: extension.id,
};

void uninstallContextMenu(identifier).catch((error) => {
reportError(error);
});
void uninstallContextMenu(identifier).catch(reportError);

returnState = reducer(returnState, actions.removeExtension(identifier));
}
Expand Down
9 changes: 0 additions & 9 deletions src/background/devtools/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import {
} from "@/background/devtools/internal";
import { ensureContentScript } from "@/background/util";
import { isEmpty } from "lodash";
import * as contextMenuProtocol from "@/background/contextMenus";
import { Target } from "@/background/devtools/contract";
import { DynamicDefinition } from "@/nativeEditor/dynamic";
import { RegistryId, UUID } from "@/core";
Expand Down Expand Up @@ -206,14 +205,6 @@ export const runReader = liftBackground(
})
);

export const uninstallContextMenu = liftBackground(
"UNINSTALL_CONTEXT_MENU",
// False positive - it's the inner method that should be async
// eslint-disable-next-line unicorn/consistent-function-scoping
() => async ({ extensionId }: { extensionId: UUID }) =>
contextMenuProtocol.uninstall(extensionId)
);

export const uninstallActionPanelPanel = liftBackground(
"UNINSTALL_ACTION_PANEL_PANEL",
// False positive - it's the inner method that should be async
Expand Down
Loading