1294999 - Move global imports and lazy module / service getters to preferences.js

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P3)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Various preference pages define their own imports and lazy module getters and lazy service getters. All of these pages are actually included in to one page during build time, and as such all of the global definitions share the same namespace.

These should all be moved to preferences.js to make it clearer that they are both available to all pages and that they don't need to be redefined for other pages.

The code is all available in /browser/components/preferences/in-content/

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Carlos, would you like to fix this bug?

Comment 2

[Carlos [nohamelin]]

Ok. I understand the problem; I have inspected before how the in-content preferences page is structured.

I need to check that artifact build first.

Comment 3

[Carlos [nohamelin]]

Attachment: Move global imports and lazy getters to preferences.js

I'm submitting my current wip patch to start to check some things, while I try these days to fix an error while building with mach. So, in fact, I haven't run it yet :/


* FxAccountsCommon has been being obtained via a lazyGetter taking directly the return value of Cu.import; I checked and I didn't find any usage of an internal property not exposed via EXPORTED_SYMBOLS in that module, so I changed it to a safer lazyModuleGetter.

* I moved some in-line module imports (Translation and offlineAppCache) to lazyModuleGetters too.

* Should I add a comment at the start of each cleaned file remembering to add new imports in preferences.js?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8782224 [details] [diff] [review]
Move global imports and lazy getters to preferences.js

Review of attachment 8782224 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just one typo that needs to be fixed before we can get this checked in.

You can ask for help with getting your first build compiled and running in the #introduction room on Mozilla's IRC server, irc.mozilla.org

::: browser/components/preferences/in-content/preferences.js
@@ +19,5 @@
> +Cu.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
> +Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource:///modules/ShellService.jsm");
> +Cu.import("resource:///modules/TransientPrefs.jsm");
> +Cu.import("resource://services-sync/main.js");

Can you please put these in alphabetical order since you're editing this list anyways? Same for the defineLazyModuleGetter list below too.

@@ +24,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "fxAccounts",
> +                                  "resource://gre/modules/FxAccounts.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "FxAccountsCommon",
> +                                  "resource://gre/modules/FxAccountsCommon.jsm");

This should be FxAccountsCommon.js, not .jsm

Comment 5

[Carlos [nohamelin]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> ::: browser/components/preferences/in-content/preferences.js
> @@ +19,5 @@
> > +Cu.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
> > +Cu.import("resource://gre/modules/Task.jsm");
> > +Cu.import("resource:///modules/ShellService.jsm");
> > +Cu.import("resource:///modules/TransientPrefs.jsm");
> > +Cu.import("resource://services-sync/main.js");
> 
> Can you please put these in alphabetical order since you're editing this
> list anyways?

They were grouped by path first, and then sorted alphabetically (excepting XPCOMUtils/Services, that it seems that they are usually on top anyway?). Should I ignore the path to do this?

> Same for the defineLazyModuleGetter list below too.

is it not sorted already?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Yes, you are right. I didn't think about the path first. The ordering in the patch is correct.

"resource://gre/modules/FxAccountsCommon.jsm" still needs to be changed to "resource://gre/modules/FxAccountsCommon.js"

Comment 7

[Carlos [nohamelin]]

Attachment: Bug 1294999 - in-content preferences: Move global imports and lazy getters to preferences.js (fixed typo)

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8783981 [details] [diff] [review]
Bug 1294999 - in-content preferences: Move global imports and lazy getters to preferences.js (fixed typo)

Review of attachment 8783981 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for doing this! I'll mark this as checkin-needed. Once it gets checked in the code changes should appear in Firefox Nightly a day or two later.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for check-in

Comment 10

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> Various preference pages define their own imports and lazy module getters
> and lazy service getters. All of these pages are actually included in to one
> page during build time, and as such all of the global definitions share the
> same namespace.

Have we considered making each file a JSM (or loaded in its own scope) instead so it's clear what is in scope for each file?

> These should all be moved to preferences.js to make it clearer that they are
> both available to all pages and that they don't need to be redefined for
> other pages.

IMO this is a step backwards for maintainability and it will probably make it harder to eslint the files for rules dealing with variable scope/use.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Matthew N. [:MattN] from comment #10)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> > Various preference pages define their own imports and lazy module getters
> > and lazy service getters. All of these pages are actually included in to one
> > page during build time, and as such all of the global definitions share the
> > same namespace.
> 
> Have we considered making each file a JSM (or loaded in its own scope)
> instead so it's clear what is in scope for each file?
> 
I don't think that was considered before. Having a separate scope would surely make it easier to reason about what the file has access to, as well as prevent bugs like bug 1294579.

The goal of this bug was to do some of the preliminary work of combining the preferences to a single-page. However, it would still probably be smart to try to compartmentalize as much logic here as we can. Using JSMs for grouped functionality could be a good approach that doesn't limit us from having a single searchable preference page.

Gijs, what do you think about this? Is there anything salvageable with the attached patch?

Matt, what would you recommend as some good next steps, and potentially something that Carlos could work on if we decide not to land this?

Comment 12

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> (In reply to Matthew N. [:MattN] from comment #10)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> > > Various preference pages define their own imports and lazy module getters
> > > and lazy service getters. All of these pages are actually included in to one
> > > page during build time, and as such all of the global definitions share the
> > > same namespace.
> > 
> > Have we considered making each file a JSM (or loaded in its own scope)
> > instead so it's clear what is in scope for each file?
> > 
> I don't think that was considered before. Having a separate scope would
> surely make it easier to reason about what the file has access to, as well
> as prevent bugs like bug 1294579.
> 
> The goal of this bug was to do some of the preliminary work of combining the
> preferences to a single-page. However, it would still probably be smart to
> try to compartmentalize as much logic here as we can. Using JSMs for grouped
> functionality could be a good approach that doesn't limit us from having a
> single searchable preference page.
> 
> Gijs, what do you think about this? Is there anything salvageable with the
> attached patch?

TBH, I'm not entirely sure how much we'd gain from making all the files JSMs - we'd need to somehow pass the window/document we're using to all of them, and update all the references to window/document globals in the files to match, which is a lot of boring work. I'm not sure that the trade-off in lintability is worth that work.

The patch goes in the opposite direction because if we use JSMs then we do need all the individual imports/lazy-getters. So we should decide what we want, and if we go the JSM route I doubt much of anything can be salvaged from the patch. :-(

Comment 13

[Matthew N. [:MattN]]

Good point about the document/window globals. My other suggestion was just to load each file in its own scope (which I guess means loadSubScript or a Sandbox) and it seems like that could help with knowing what's in scope:

"Any top-level functions or variables created by the loaded script via var are created as properties of the targetObj target object (but things declared via let and const are NOT). Additionally, properties of the targetObj target object can be referred to as variables in the loaded script."[1]

We may need to change the way some modules are imported (I'm not sure if Cu.import's will leak to the targetObj global by default) but that should be fairly straightforward to fix.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/mozIJSSubScriptLoader#loadSubScript()

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 15

[Garima Mazumdar]

Hi! I'm Garima, an Outreachy participant, and would like to work on this bug :)

Comment 16

[:Gijs (he/him)]

Let's just fix this. Although some of the .js files involved (looking at you, extensionControlled.js) might want to be JSMs, most of them don't.

Comment 17

[:Gijs (he/him)]

Attachment: Bug 1294999 - put module getters in preferences.js and de-duplicate them, r?jaws

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6495a7114de2
put module getters in preferences.js and de-duplicate them, r=jaws,preferences-reviewers

Comment 19

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/6495a7114de2