Closed Bug 1434245 Opened 4 years ago Closed 1 month ago

Use defineLazyModuleGetters more often with PlacesUtils

Categories

(Firefox :: Bookmarks & History, enhancement, P3)

55 Branch
enhancement

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox95 --- fixed

People

(Reporter: mak, Assigned: lesore0789, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [fxsearch][lang=js])

Attachments

(1 file)

Now that PlacesUtils only exports a single symbol, we can defineLazyModuleGetter it more often:
https://searchfox.org/mozilla-central/search?q=Cu.import%28%22resource%3A%2F%2Fgre%2Fmodules%2FPlacesUtils.jsm%22&path=
In modules and components we can use defineLazyModuleGetters, in other places it is better to stick to defineModuleGetter, to avoid a cross compartment wrapper taken by XPCOMUtils to define the getter.
Priority: P2 → P3
Depends on: 406371

There is only one use left that can be removed because https://searchfox.org/mozilla-central/source/toolkit/components/places/tests/chrome/head.js is already including it. Also NetUtil can be removed if we add <script type="application/javascript" src="head.js" />.
We should also replace NetUtil with Services and use Services.io.newURI instead of NetUtil.newURI

Mentor: mak
Keywords: good-first-bug
Whiteboard: [fxsearch] → [fxsearch][lang=js]

Hi there, I am an outreachy applicant and would like to work on this bug. Can I be assigned to it?

Flags: needinfo?(mak)

Feel free to start working on it, bugs are assigned when the first patch is submitted. Thank you.

To clarify the issue at hand, ChromeUtils.import does a synchronous load of the jsm module, but for this specific module we prefer to load it lazily. So the scope was to replace all the ChromeUtils.import calls with defineLazyModuleGetter (or defineLazyModuleGetters). Searching for it in the source code, like in comment 1, shows that only one entry is left. Rather than replacing that entry, we should remove it though, because it's included in head.js (an header file) thus we could just load the head.js script in the test itself.
While there we could also remove NetUtil from head.js, because it's only used for NetUtil.newURI, that helper is no more necessary because Services.io.newURI does the same thing. Consumers of NetUtil should also be replaced.
Searchfox is a good way to search through the codebase.

To run the tests one should use ./mach test toolkit/components/places/tests/chrome/ check it works before starting changing the tests, then check again after the changes.

Flags: needinfo?(mak)
Assignee: nobody → lesore0789
Status: NEW → ASSIGNED
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/14ee535024d7
Use defineLazyModuleGetters more often with PlacesUtils. r=mak
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch
You need to log in before you can comment on or make changes to this bug.