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.

Sign up for GitHub

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

#6649: Avoid background messaging to handle AJAX page updates #7030

Merged
merged 5 commits into from
Dec 21, 2023
Merged

#6649: Avoid background messaging to handle AJAX page updates #7030

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
384fb0f
Use navigation API
fregante Nov 29, 2023
41884b4
Merge remote-tracking branch 'origin/main' into F/perf/navigation-api
fregante Dec 10, 2023
531675f
Merge remote-tracking branch 'origin/main' into F/perf/navigation-api
fregante Dec 15, 2023
b5f022d
Reduce events
fregante Dec 15, 2023
d44fb9c
Merge remote-tracking branch 'origin/main' into F/perf/navigation-api
fregante Dec 21, 2023
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
7 changes: 7 additions & 0 deletions package-lock.json
View file
Open in desktop

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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@
"@testing-library/user-event": "^14.5.1",
"@total-typescript/ts-reset": "^0.5.1",
"@types/chrome": "^0.0.216",
"@types/dom-navigation": "^1.0.3",
"@types/dompurify": "^3.0.5",
"@types/downloadjs": "^1.4.6",
"@types/gapi": "0.0.47",
Expand Down
8 changes: 1 addition & 7 deletions src/background/navigation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import { reactivateTab, handleNavigate } from "@/contentScript/messenger/api";
import { reactivateTab } from "@/contentScript/messenger/api";
import { forEachTab } from "@/utils/extensionUtils";
import { type Target } from "@/types/messengerTypes";
import { canAccessTab } from "@/permissions/permissionsUtils";
Expand Down Expand Up @@ -51,12 +51,6 @@ async function onNavigation({ tabId, frameId }: Target): Promise<void> {
if (syncFlagOn("navigation-trace")) {
await traceNavigation({ tabId, frameId });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not removed the listeners due to this logging.

What would you like to do?

  • Leave as is
  • Drop logging
  • Attach/remove the listeners depending on the flag (via addAuthListener), instead of checking the flag at every navigation

Copy link
Contributor

@twschiller twschiller Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attach/remove the listeners depending on the flag (via addAuthListener), instead of checking the flag at every navigation

Lets do this. Unfortunately navigation tracing code is still helpful to understand wacky SPA navigation behavior of some in-house applications

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #7178

}

if (await canAccessTab({ tabId, frameId })) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that messaging + this check added at least 100ms to the navigation event.

// The content script will already be injected, so we don't have to call ensureContentScript before messaging the
// content script with handleNavigate.
handleNavigate({ tabId, frameId });
}
}

// Some sites use the hash to encode page state (e.g., filters). There are some non-navigation scenarios where the hash
Expand Down
3 changes: 2 additions & 1 deletion src/contentScript/contentScriptCore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import registerMessenger from "@/contentScript/messenger/registration";
import registerBuiltinBlocks from "@/bricks/registerBuiltinBlocks";
import registerContribBlocks from "@/contrib/registerContribBlocks";
import brickRegistry from "@/bricks/registry";
import { handleNavigate } from "@/contentScript/lifecycle";
import { handleNavigate, initNavigation } from "@/contentScript/lifecycle";
import { initTelemetry } from "@/background/messenger/api";
import { ENSURE_CONTENT_SCRIPT_READY } from "@/contentScript/ready";
import { initToaster } from "@/utils/notify";
Expand Down Expand Up @@ -66,6 +66,7 @@ export async function init(): Promise<void> {
initToaster();

await handleNavigate();
Copy link
Contributor

@twschiller twschiller Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we comment on the ordering here? I.e., why is initNavigation after handleNavigate. I think the answer is just: "handle initial load, then watch for additional navigation events"

Copy link
Contributor Author

@fregante fregante Dec 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #7178

initNavigation();

void initSidebarActivation();

Expand Down
4 changes: 4 additions & 0 deletions src/contentScript/lifecycle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,3 +638,7 @@ export async function reactivateTab(): Promise<void> {
// Force navigate event even though the href hasn't changed
await handleNavigate({ force: true });
}

export function initNavigation() {
window.navigation?.addEventListener("navigate", async () => handleNavigate());
}
1 change: 0 additions & 1 deletion src/contentScript/messenger/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ export const getInstalledExtensionPoints = getMethod(
"INSTALLED_EXTENSION_POINTS",
);
export const checkAvailable = getMethod("CHECK_AVAILABLE");
export const handleNavigate = getNotifier("HANDLE_NAVIGATE");
export const runBrick = getMethod("RUN_BRICK");
export const cancelSelect = getMethod("CANCEL_SELECT_ELEMENT");
export const selectElement = getMethod("SELECT_ELEMENT");
Expand Down
3 changes: 0 additions & 3 deletions src/contentScript/messenger/registration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import { handleMenuAction } from "@/contentScript/contextMenus";
import {
ensureInstalled,
getActiveExtensionPoints,
handleNavigate,
queueReactivateTab,
reactivateTab,
removePersistedExtension,
Expand Down Expand Up @@ -128,7 +127,6 @@ declare global {
INSTALLED_EXTENSION_POINTS: typeof getActiveExtensionPoints;
ENSURE_EXTENSION_POINTS_INSTALLED: typeof ensureInstalled;
CHECK_AVAILABLE: typeof checkAvailable;
HANDLE_NAVIGATE: typeof handleNavigate;
RUN_BRICK: typeof runBrick;
CANCEL_SELECT_ELEMENT: typeof cancelSelect;
SELECT_ELEMENT: typeof selectElement;
Expand Down Expand Up @@ -195,7 +193,6 @@ export default function registerMessenger(): void {
INSTALLED_EXTENSION_POINTS: getActiveExtensionPoints,
ENSURE_EXTENSION_POINTS_INSTALLED: ensureInstalled,
CHECK_AVAILABLE: checkAvailable,
HANDLE_NAVIGATE: handleNavigate,

RUN_BRICK: runBrick,
CANCEL_SELECT_ELEMENT: cancelSelect,
Expand Down
2 changes: 0 additions & 2 deletions src/pageEditor/pageEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import "@/extensionContext";
import "@/development/darkMode.js";

import { initMessengerLogging } from "@/development/messengerLogging";
import registerMessenger from "@/pageEditor/messenger/registration";

import ReactDOM from "react-dom";
import React from "react";
Expand All @@ -37,7 +36,6 @@ markAppStart();

void initMessengerLogging();
void initRuntimeLogging();
registerMessenger();
watchNavigation();
initToaster();

Expand Down
2 changes: 0 additions & 2 deletions src/tsconfig.strictNullChecks.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,8 +301,6 @@
"./pageEditor/fields/makeLockableFieldProps.tsx",
"./pageEditor/fields/selectorListItem/SelectorListItem.tsx",
"./pageEditor/hooks/useEscapeHandler.ts",
"./pageEditor/messenger/api.ts",
"./pageEditor/messenger/registration.ts",
"./pageEditor/panes/BetaPane.tsx",
"./pageEditor/panes/IntroButtons.tsx",
"./pageEditor/panes/NoModSelectedPane.tsx",
Expand Down