Skip to content

Commit

Permalink
perf(cherry-pick): use an interstitial page to load popup.html; loa…
Browse files Browse the repository at this point in the history
…d scripts using `defer`ed script tags (#26555) (#26822)

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](#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.


<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **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
#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**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **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 <[email protected]>
  • Loading branch information
davidmurdoch and Gudahtt authored Sep 3, 2024
1 parent 8fa9cc0 commit 226d9a4
Show file tree
Hide file tree
Showing 12 changed files with 107 additions and 82 deletions.
3 changes: 1 addition & 2 deletions app/background.html
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
<meta charset="utf-8">
</head>
<body>
<script src="./load-background.js"></script>
<script src="./chromereload.js"></script>
<script src="./load-background.js" defer></script>
</body>
</html>
2 changes: 1 addition & 1 deletion app/home.html
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@
<img class="loading-spinner" src="./images/spinner.gif" alt="" loading="lazy" />
</div>
<div id="popover-content"></div>
<script src="./load-app.js" async></script>
<script src="./load-app.js" defer></script>
</body>
</html>
2 changes: 1 addition & 1 deletion app/manifest/v2/_base.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
16 changes: 15 additions & 1 deletion app/manifest/v2/firefox.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,19 @@
"id": "[email protected]",
"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
}
2 changes: 1 addition & 1 deletion app/manifest/v3/_base.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
2 changes: 1 addition & 1 deletion app/notification.html
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,6 @@
/>
</div>
<div id="popover-content"></div>
<script src="./load-app.js" async></script>
<script src="./load-app.js" defer></script>
</body>
</html>
39 changes: 39 additions & 0 deletions app/popup-init.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<!DOCTYPE html>
<html dir="ltr">
<head>
<meta http-equiv="refresh" content="0;url=/popup.html">
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
<title>MetaMask</title>
<style>
/**
* We try to match the user's system theme for the background color, but
* since the actual popup will use the theme from the user's extension
* settings we can't get it right 100% of the time. The browser doesn't
* allow a height or width of 0, otherwise we wouldn't bother with the
* background color.
* Unfortunately (and conveniently?), if the user *does* have a
* non-system theme set popup.html will flash the wrong theme, as the
* loading screen itself always uses the system theme. So while this page
* doesn't match popup.html's loading screen, the backgrounds will match,
* so the redirect isn't all that jarring.
*
* Implementing https://github.com/MetaMask/metamask-extension/issues/26545
* would improve the situation.
*
* Porting the loading screen (inlining all assets and CSS) to this init
* page may be a further UX improvement.
*/
html {
width: 357px;
height: 600px;
background: #f2f4f6;
}
@media screen and (prefers-color-scheme: dark) {
html {
background: #24272a;
}
}
</style>
</head>
</html>
2 changes: 1 addition & 1 deletion app/popup.html
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,6 @@
<img class="loading-spinner" src="./images/spinner.gif" alt="" loading="lazy" />
</div>
<div id="popover-content"></div>
<script src="./load-app.js" async></script>
<script src="./load-app.js" defer></script>
</body>
</html>
26 changes: 0 additions & 26 deletions app/scripts/load-app.js

This file was deleted.

84 changes: 46 additions & 38 deletions development/build/scripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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;
}
Expand All @@ -710,14 +719,7 @@ function createFactoredBuild({
browserPlatforms,
shouldIncludeSnow,
applyLavaMoat,
});
renderJavaScriptLoader({
groupSet,
commonSet,
browserPlatforms,
shouldIncludeSnow,
applyLavaMoat,
destinationFileName: 'load-background.js',
scripts,
});
if (isEnableMV3) {
const jsBundles = [
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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 `<script src="${src}" defer></script>`;
});
}

Expand All @@ -1201,6 +1194,7 @@ function renderHtmlFile({
applyLavaMoat,
isMMI,
isTest,
scripts = [],
}) {
if (applyLavaMoat === undefined) {
throw new Error(
Expand All @@ -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('<script src="./load-app.js" defer></script>', scriptTags)
.replace(
'<script src="./load-background.js" defer></script>',
`${scriptTags}\n <script src="./chromereload.js" async></script>`,
)
.replace('<script src="./load-offscreen.js" defer></script>', scriptTags);
browserPlatforms.forEach((platform) => {
const dest = `./dist/${platform}/${htmlName}.html`;
// we dont have a way of creating async events atm
Expand Down
9 changes: 0 additions & 9 deletions development/build/static.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand All @@ -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',
Expand Down
2 changes: 1 addition & 1 deletion offscreen/offscreen.html
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@
<title>MetaMask Offscreen Page</title>
</head>
<body>
<script src="./load-offscreen.js"></script>
<script src="./load-offscreen.js" defer></script>
</body>
</html>

0 comments on commit 226d9a4

Please sign in to comment.