Closed Bug 988558 Opened 11 years ago Closed 6 years ago

Provide a Components.utils.lazyImport that defaults to lazily import the same-name-as-module symbol

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1431533

People

(Reporter: mak, Unassigned)

References

Details

(Whiteboard: [MemShrink:P3] p=5)

Attachments

(1 file, 1 obsolete file)

Cu.import is bad for some reasons:
1. it's synchronous and not lazy
2. enforces us to type long resource uris every time
3. it doesn't promote the way we most commonly use jsm modules (exporting just one symbol named as the module)

defineLazyModuleGetter solves (1) but has other issues, it requires importing XPCOMUtils, it's hard to remember and verbose to type. It also doesn't allow to import more than one symbol.

Cu.lazyImport may fix this by:
1. lazy loading any required symbol
2. allowing to type just Cu.lazyImport("SymbolName"); in the default case (it would be mapped to either resource:///modules/SymbolName.jsm, or gre in case the former fails, not sure if there's another way to tell if module is in gre or not, currently)
3. making single-symbol-named-as-module the default case
4. it may have a second optional argument to specify more symbols to import
Summary: Provide a Components.utils.lazyImport that defaults to importing the same-name-as-module symbol → Provide a Components.utils.lazyImport that defaults to lazily import the same-name-as-module symbol
Whiteboard: [MemShrink]
Blocks: fxdesktopbacklog
No longer blocks: fxdesktoptriage
Whiteboard: [MemShrink] → [MemShrink] p=0
Whiteboard: [MemShrink] p=0 → [MemShrink:P2] p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: [MemShrink:P2] p=0 → [MemShrink:P2] p=5
A very rough draft. I don't really know what I'm doing here and am making it up
as I go along. The plumbing is in place, though it's not yet working.
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
This version works, but isn't quite ready for prime-time.

- It doesn't implement point 4 from comment 1 (a second, optional argument).
  The current design doesn't require storing any state other than the module
  name, which we can store in the getter name for free. Having to store other
  state complicates things and I'm not sure how to do it.

- It only works with gre modules for now. I see two options for fixing this:
  - First try gre. If it fails, try non-gre.
  - Have separate lazyImportGre() and lazyImportNonGre() functions.

  I guess the former is preferable if failing is quick and does not cause other
  problems (such as spamming the console with warnings). Otherwise, the latter
  would be better.

Feedback is appreciated! mak, please forward the f? request onto somebody else
if you think that's appropriate.

And I guess the next step is to switch a lot of the Cu.import() calls to use
Cu.lazyImport() instead.
Attachment #8406031 - Attachment is obsolete: true
Attachment #8406553 - Flags: feedback?(mak77)
Being able to handle other paths is important. Here's an example -- it has the extra "addons/" in the path. There are lots of other similar cases.

> XPCOMUtils.defineLazyModuleGetter(this, "AddonRepository_SQLiteMigrator",
>                                   "resource://gre/modules/addons/AddonRepository_SQLiteMigrator.jsm");

Hmm.
(In reply to Nicholas Nethercote [:njn] from comment #2)
> - It only works with gre modules for now. I see two options for fixing this:
>   - First try gre. If it fails, try non-gre.

I would have said the opposite, so an app module can "overload" a gre module. Even if I think it's a non-goal and maybe also error-prone... not sure if there's an expected behavior, we should make one.

The API could also allow different input options, to overcome these limitations:
Cu.lazyImport("symbol"); /* try /// and //gre/ */
Cu.lazyImport([ "path", "to", "symbol" ]); /* // try /// and //gre/ unless the first entry is "app" or "gre" */
Cu.lazyImport("resource://path/to/symbol.jsm");

> And I guess the next step is to switch a lot of the Cu.import() calls to use
> Cu.lazyImport() instead.

Yep, that may be a mentored good first bug

(In reply to Nicholas Nethercote [:njn] from comment #3)
> Being able to handle other paths is important. Here's an example -- it has
> the extra "addons/" in the path. There are lots of other similar cases.

Just off the top of my head, I was wondering if we could build a sort of "modules register" that is constructed at build time (I think we already build such a list somewhere, it should just be converted to an .h file that the register may digest, since we wouldn't want to pay IO cost to read a disk file), then for each "symbol" we'd get the resource uri... I'm not sure how to make that though, off-hand.
At first I thought that we'd be able to mass-convert Cu.import calls to Cu.lazyImport, but it seems that's not the case. So this is more concise that defineLazyModuleGetter, but otherwise not much better. So I'm not planning to work on this any more.
Assignee: n.nethercote → nobody
Whiteboard: [MemShrink:P2] p=5 → [MemShrink:P3] p=5
Attachment #8406553 - Flags: feedback?(mak77)
sorry if I didn't reply shortly, I'm quite busy in this period :(

In my opinion implementing something behaving like comment 4 (3 possible input values: symbol, path array or whole url) would still make a good API for future use. Replacing all in-content consumers through a script should still be feasible.

About the fact it's in the end a compact defineLazyModuleGetter version, that's true, but I also think the compactness will end up being the right way to attract devs love, since who wouldn't want to just type Cu.lazyImport("symbol") and be done?

That said, not a priority, I agree, but it may be a good student/contributor project.
Component: General → XPConnect
Product: Toolkit → Core
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → INACTIVE
Resolution: INACTIVE → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: