From 226d9a401dc2fc3872b984f2b88c0cc73aa189b8 Mon Sep 17 00:00:00 2001 From: David Murdoch <187813+davidmurdoch@users.noreply.github.com> Date: Tue, 3 Sep 2024 14:27:52 -0400 Subject: [PATCH] perf(cherry-pick): use an interstitial page to load `popup.html`; load scripts using `defer`ed script tags (#26555) (#26822) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR removes `load-*.js` helpers and load scripts using the `defer` property instead. This change drastically improves the performance of the extension but greatly reduces perceived performance because the browser won't display the popup until the DOMContentLoaded event. But improve perceived performance is mostly regained by initially loading an asset-less HTML file, `popup-init.html`, that then redirects to the slower (but much faster than before this PR) `popup.html`. This was initially authored by @Gudahtt, I've just updated and optimized it to work with the both build systems. One change I did not make was moving scripts to the `head`. I don't think putting the scripts in the head does anything in our case[^1] other than potentially require that we wait for `DOMContentLoaded` before querying for the app container. Note: In Firefox we continue to use `popup.html`. Firefox is very slow to render the `popup-init.html` redirect, and it renders the page only partially anyway, making the UX feel very janky. It also doesn't delay opening the popup until `DOMContentLoaded`, like Chrome does, so the issue `popup-init.html` solves doesn't do anything anyway. --- Here is side-by-side video comparison of `develop` vs `popup-defer-scripts`. This video displays the elapsed time from clicking the MetaMask Fox to Lock Screen render. The end of the video is a frame-by-frame comparison of the new "jank" this PR introduces. The develop branch renders the fox immediately, whereas the popup-defer-scripts branch renders an empty page for about 15 milliseconds. These stats aren't representative of real world performance, but are intended to illustrate relative performance and perceived performance. Both builds were created by running `yarn build dist`. https://github.com/user-attachments/assets/38b856ca-d269-48f7-b305-19f4b9c6dce8 --- 1. build with `yarn dist` 2. install into your browser and go through onboarding 3. open MetaMask 4. Marvel at how much faster it is --- Closes [#25721](https://github.com/MetaMask/metamask-extension/issues/25721) [^1]: our HTML files are very tiny, and thus will be loaded all in one go, we don't have to worry about packets being lost and retransmitted over the network, delaying the browser's preload scanner. ## **Description** Cherry picks the popup loading changes. > [!WARNING] > There was a conflict around webpack changes since those aren't included in v12.2.0 > > Specifically, the build's script and some html files changed in the webpack PR as well as in https://github.com/MetaMask/metamask-extension/pull/26555. > [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26822?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. Co-authored-by: Mark Stacey --- app/background.html | 3 +- app/home.html | 2 +- app/manifest/v2/_base.json | 2 +- app/manifest/v2/firefox.json | 16 ++++++- app/manifest/v3/_base.json | 2 +- app/notification.html | 2 +- app/popup-init.html | 39 +++++++++++++++++ app/popup.html | 2 +- app/scripts/load-app.js | 26 ----------- development/build/scripts.js | 84 ++++++++++++++++++++---------------- development/build/static.js | 9 ---- offscreen/offscreen.html | 2 +- 12 files changed, 107 insertions(+), 82 deletions(-) create mode 100644 app/popup-init.html delete mode 100644 app/scripts/load-app.js diff --git a/app/background.html b/app/background.html index c0068295d730..a140318ba8d2 100644 --- a/app/background.html +++ b/app/background.html @@ -4,7 +4,6 @@ - - + diff --git a/app/home.html b/app/home.html index b94800b13d78..11cfed10242e 100644 --- a/app/home.html +++ b/app/home.html @@ -16,6 +16,6 @@
- + diff --git a/app/manifest/v2/_base.json b/app/manifest/v2/_base.json index 9843a20529de..31b1c82224fd 100644 --- a/app/manifest/v2/_base.json +++ b/app/manifest/v2/_base.json @@ -15,7 +15,7 @@ "512": "images/icon-512.png" }, "default_title": "MetaMask", - "default_popup": "popup.html" + "default_popup": "popup-init.html" }, "commands": { "_execute_browser_action": { diff --git a/app/manifest/v2/firefox.json b/app/manifest/v2/firefox.json index 6b0a122cfd9e..b2858bb8612f 100644 --- a/app/manifest/v2/firefox.json +++ b/app/manifest/v2/firefox.json @@ -4,5 +4,19 @@ "id": "webextension@metamask.io", "strict_min_version": "89.0" } - } + }, + "browser_action": { + "default_icon": { + "16": "images/icon-16.png", + "19": "images/icon-19.png", + "32": "images/icon-32.png", + "38": "images/icon-38.png", + "64": "images/icon-64.png", + "128": "images/icon-128.png", + "512": "images/icon-512.png" + }, + "default_title": "MetaMask", + "default_popup": "popup.html" + }, + "manifest_version": 2 } diff --git a/app/manifest/v3/_base.json b/app/manifest/v3/_base.json index f3ffbf07c173..71d083208b55 100644 --- a/app/manifest/v3/_base.json +++ b/app/manifest/v3/_base.json @@ -10,7 +10,7 @@ "512": "images/icon-512.png" }, "default_title": "MetaMask", - "default_popup": "popup.html" + "default_popup": "popup-init.html" }, "author": "https://metamask.io", "background": { diff --git a/app/notification.html b/app/notification.html index 70f02e9ab421..0356d2278800 100644 --- a/app/notification.html +++ b/app/notification.html @@ -50,6 +50,6 @@ />
- + diff --git a/app/popup-init.html b/app/popup-init.html new file mode 100644 index 000000000000..2fb79fefcfde --- /dev/null +++ b/app/popup-init.html @@ -0,0 +1,39 @@ + + + + + + + MetaMask + + + diff --git a/app/popup.html b/app/popup.html index 296b0ceae711..3cb985b27a25 100644 --- a/app/popup.html +++ b/app/popup.html @@ -12,6 +12,6 @@
- + diff --git a/app/scripts/load-app.js b/app/scripts/load-app.js deleted file mode 100644 index 4b92255a5bc2..000000000000 --- a/app/scripts/load-app.js +++ /dev/null @@ -1,26 +0,0 @@ -// eslint-disable-next-line import/unambiguous -'use strict'; - -setTimeout(() => { - // eslint-disable-next-line spaced-comment - const scriptsToLoad = [ - /* SCRIPTS */ - ]; - - const loadScript = (src) => { - const script = document.createElement('script'); - script.type = 'text/javascript'; - script.async = true; - script.onload = loadNext; - script.src = src; - document.body.appendChild(script); - }; - - loadNext(); - - function loadNext() { - if (scriptsToLoad.length) { - loadScript(scriptsToLoad.shift()); - } - } -}, 10); diff --git a/development/build/scripts.js b/development/build/scripts.js index c87197bfaf1c..a5f6b7084a13 100644 --- a/development/build/scripts.js +++ b/development/build/scripts.js @@ -661,6 +661,12 @@ function createFactoredBuild({ const isTest = buildTarget === BUILD_TARGETS.TEST || buildTarget === BUILD_TARGETS.TEST_DEV; + const scripts = getScriptTags({ + groupSet, + commonSet, + shouldIncludeSnow, + applyLavaMoat, + }); switch (groupLabel) { case 'ui': { renderHtmlFile({ @@ -669,36 +675,39 @@ function createFactoredBuild({ shouldIncludeSnow, applyLavaMoat, isMMI: buildType === 'mmi', + scripts, }); renderHtmlFile({ htmlName: 'popup', browserPlatforms, shouldIncludeSnow, applyLavaMoat, + scripts, }); renderHtmlFile({ - htmlName: 'notification', + htmlName: 'popup-init', browserPlatforms, shouldIncludeSnow, applyLavaMoat, - isMMI: buildType === 'mmi', - isTest, + scripts, }); renderHtmlFile({ - htmlName: 'home', + htmlName: 'notification', browserPlatforms, shouldIncludeSnow, applyLavaMoat, isMMI: buildType === 'mmi', isTest, + scripts, }); - renderJavaScriptLoader({ - groupSet, - commonSet, + renderHtmlFile({ + htmlName: 'home', browserPlatforms, shouldIncludeSnow, applyLavaMoat, - destinationFileName: 'load-app.js', + isMMI: buildType === 'mmi', + isTest, + scripts, }); break; } @@ -710,14 +719,7 @@ function createFactoredBuild({ browserPlatforms, shouldIncludeSnow, applyLavaMoat, - }); - renderJavaScriptLoader({ - groupSet, - commonSet, - browserPlatforms, - shouldIncludeSnow, - applyLavaMoat, - destinationFileName: 'load-background.js', + scripts, }); if (isEnableMV3) { const jsBundles = [ @@ -747,17 +749,19 @@ function createFactoredBuild({ browserPlatforms, shouldIncludeSnow, applyLavaMoat: false, + scripts, }); break; } case 'offscreen': { - renderJavaScriptLoader({ + renderHtmlFile({ + htmlName: 'offscreen', groupSet, commonSet, browserPlatforms, shouldIncludeSnow, applyLavaMoat, - destinationFileName: 'load-offscreen.js', + scripts, }); break; } @@ -1140,13 +1144,11 @@ async function createBundle(buildConfiguration, { reloadOnChange }) { } } -function renderJavaScriptLoader({ +function getScriptTags({ groupSet, commonSet, - browserPlatforms, shouldIncludeSnow, applyLavaMoat, - destinationFileName, }) { if (applyLavaMoat === undefined) { throw new Error( @@ -1180,17 +1182,8 @@ function renderJavaScriptLoader({ ...jsBundles, ]; - browserPlatforms.forEach((platform) => { - const appLoadFilePath = './app/scripts/load-app.js'; - const appLoadContents = readFileSync(appLoadFilePath, 'utf8'); - - const scriptDest = `./dist/${platform}/${destinationFileName}`; - const scriptOutput = appLoadContents.replace( - '/* SCRIPTS */', - `...${JSON.stringify(requiredScripts)}`, - ); - - writeFileSync(scriptDest, scriptOutput); + return requiredScripts.map((src) => { + return ``; }); } @@ -1201,6 +1194,7 @@ function renderHtmlFile({ applyLavaMoat, isMMI, isTest, + scripts = [], }) { if (applyLavaMoat === undefined) { throw new Error( @@ -1213,15 +1207,29 @@ function renderHtmlFile({ ); } - const htmlFilePath = `./app/${htmlName}.html`; + const scriptTags = scripts.join('\n '); + + const htmlFilePath = + htmlName === 'offscreen' + ? `./offscreen/${htmlName}.html` + : `./app/${htmlName}.html`; const htmlTemplate = readFileSync(htmlFilePath, 'utf8'); const eta = new Eta(); - const htmlOutput = eta.renderString(htmlTemplate, { - isMMI, - isTest, - shouldIncludeSnow, - }); + const htmlOutput = eta + .renderString(htmlTemplate, { + isMMI, + isTest, + shouldIncludeSnow, + }) + // these replacements are added to support the webpack build's automatic + // compilation of html files, which the gulp-based process doesn't support. + .replace('', scriptTags) + .replace( + '', + `${scriptTags}\n `, + ) + .replace('', scriptTags); browserPlatforms.forEach((platform) => { const dest = `./dist/${platform}/${htmlName}.html`; // we dont have a way of creating async events atm diff --git a/development/build/static.js b/development/build/static.js index cf0cd1078423..ff14657b621b 100644 --- a/development/build/static.js +++ b/development/build/static.js @@ -166,10 +166,6 @@ function getCopyTargets(shouldIncludeLockdown, shouldIncludeSnow) { src: './app/scripts/init-globals.js', dest: 'scripts/init-globals.js', }, - { - src: './app/scripts/load-app.js', - dest: 'scripts/load-app.js', - }, { src: shouldIncludeLockdown ? `./app/scripts/lockdown-run.js` @@ -192,11 +188,6 @@ function getCopyTargets(shouldIncludeLockdown, shouldIncludeSnow) { dest: `scripts/runtime-lavamoat.js`, pattern: '', }, - { - src: `./offscreen/`, - pattern: `*.html`, - dest: '', - }, { src: getPathInsideNodeModules('@blockaid/ppom_release', '/'), pattern: '*.wasm', diff --git a/offscreen/offscreen.html b/offscreen/offscreen.html index ea5ad76dd7a7..7137f2406213 100644 --- a/offscreen/offscreen.html +++ b/offscreen/offscreen.html @@ -5,6 +5,6 @@ MetaMask Offscreen Page - +