1461548 - Only load RefreshBlocker code when necessary

Status: RESOLVED FIXED

(Firefox :: Disability Access, enhancement, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

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 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1461548 - Part 0. Move RefreshBlocker to a new frame script.

Review commit: https://reviewboard.mozilla.org/r/243926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243926/

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1461548 - Only load the RefreshBlocker frame script when necessary.

Review commit: https://reviewboard.mozilla.org/r/243928/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243928/

Comment 4

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 6

[:Gijs (away/busy until Mar 10)]

Triaging as P1 given it has patches etc. and looks set to land this cycle.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/6ed202b3c435
https://hg.mozilla.org/mozilla-central/rev/2a22b2c85767