1434245 - Use defineLazyModuleGetters more often with PlacesUtils

Status: RESOLVED FIXED

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

Description

[Marco Bonardo [:mak]]

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=

Comment 1

[Marco Bonardo [:mak]]

Better search https://searchfox.org/mozilla-central/search?q=.import%28%22resource%3A%2F%2Fgre%2Fmodules%2FPlacesUtils.jsm%22&path=

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Marco Bonardo [:mak]]

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

Comment 4

[Leslie]

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

Comment 5

[Marco Bonardo [: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.

Comment 6

[Leslie]

Attachment: Bug 1434245 - Use defineLazyModuleGetters more often with PlacesUtils. r?mak

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14ee535024d7
Use defineLazyModuleGetters more often with PlacesUtils. r=mak

Comment 8

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/14ee535024d7