Closed Bug 1461548 Opened 3 years ago Closed 3 years ago

Only load RefreshBlocker code when necessary

Categories

(Firefox :: Disability Access, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: Felipe, Assigned: Felipe)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p1])

Attachments

(2 files)

The RefreshBlocker code in tab-content.js is parsed and compiled for every new tab, even when not necessary. The code is already nice enough that it doesn't add it's progress listeners if the feature is not enabled, but we could go further and avoid it entirely.
For this patch, I didn't use the new lazy proxies from bug 1457988. Instead, I just split the code into a separate frame script, and the parent process will only load that frame script if the pref is true, or if it becomes true.
Comment on attachment 8975703 [details]
Bug 1461548 - Part 0. Move RefreshBlocker to a new frame script.

https://reviewboard.mozilla.org/r/243926/#review249952

::: browser/base/content/content-refreshblocker.js:7
(Diff revision 1)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* eslint-env mozilla/frame-script */
> +
> +var RefreshBlocker = {

This is going to sound super-lame, but in order to keep blame, could you copy tab-content.js and then remove everything except RefreshBlocker in order to generate this file? We did something similar when converting tabbrowser.xml to tabbrowser.js, and it makes doing code archaeology easier.

::: browser/base/content/content-refreshblocker.js:173
(Diff revision 1)
> +
> +      refreshURI.forceRefreshURI(URI, null, data.delay, true);
> +    }
> +  },
> +
> +  QueryInterface: ChromeUtils.generateQI([Ci.nsIWebProgressListener2, Ci.nsIWebProgressListener, Ci.nsISupportsWeakReference]),

While you're here, can you break this up over a few more lines?
Attachment #8975703 - Flags: review?(mconley) → review+
Comment on attachment 8975704 [details]
Bug 1461548 - Only load the RefreshBlocker frame script when necessary.

https://reviewboard.mozilla.org/r/243928/#review249962

Thanks!

::: browser/base/content/browser.js:5307
(Diff revision 1)
> +    }
> +  },
> +
> +  loadFrameScript() {
> +    let mm = window.getGroupMessageManager("browsers");
> +    mm.loadFrameScript("chrome://browser/content/content-refreshblocker.js", true);

Remind me again... loadFrameScript is idempotent, right? We don't have to worry about this firing multiple times? I know we've got this thing only registering itself as an observer if it's preffed off, and unregistering itself as soon as its preffed on... but I wonder if it'd be worth a sanity check to ensure that loadFrameScript can't run more than once in the same window?

::: browser/base/content/content-refreshblocker.js:182
(Diff revision 1)
> +addEventListener("unload", () => {
> +  RefreshBlocker.uninit();
> +});

This looks like an extra.
Attachment #8975704 - Flags: review?(mconley) → review+
Triaging as P1 given it has patches etc. and looks set to land this cycle.
Priority: -- → P1
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ed202b3c435
Part 0. Move RefreshBlocker to a new frame script. r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a22b2c85767
Only load the RefreshBlocker frame script when necessary. r=mconley
https://hg.mozilla.org/mozilla-central/rev/6ed202b3c435
https://hg.mozilla.org/mozilla-central/rev/2a22b2c85767
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.