Use defineLazyModuleGetter for lazy modules

RESOLVED FIXED in Firefox 24

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Margaret, Assigned: nickecarlo)

Tracking

Trunk
Firefox 24
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=margaret][lang=js])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
In a bunch of places we use XPCOMUtils.defineLazyGetter to load a module. We should use defineLazyModuleGetter, since it's designed for that.

To fix this, we should look at all the places we use defineLazyGetter:
http://mxr.mozilla.org/mozilla-central/search?find=/mobile/android/&string=definelazygetter

And if it's used to load a module, e.g.:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#24

We should replace it with defineLazyModuleGetter, e.g.:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#34
(Assignee)

Comment 1

4 years ago
I can work on this bug.
(Assignee)

Updated

4 years ago
Assignee: nobody → nickecarlo
(Assignee)

Comment 2

4 years ago
Margaret,

I'm almost done with this bug but I can't figure out how I should approach the following two instances:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#43

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#60

Everything else was straight forward but the above two declare a variable in the lamda function which is then returned. Couldn't quite figure out from the documentation how to approach this situation. For now, I've left these two alone.

Also, I've only changed those instances of defineLazyGetter to defineLazyModuleGetter that were used to load modules. There were others that I left untouched. So its only browser.js, AddonUpdateService.js, and SessionStore.js that had relevant pieces of code that needed changing.

Once you advise me on how to tackle the above two situations then I can submit the patch.
(In reply to Nicolas Carlo from comment #2)
> Margaret,
> 
> I'm almost done with this bug but I can't figure out how I should approach
> the following two instances:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#43
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#60
> 
> Everything else was straight forward but the above two declare a variable in
> the lamda function which is then returned. Couldn't quite figure out from
> the documentation how to approach this situation. For now, I've left these
> two alone.

Actually, you can safely change those two as well. Look at the definition of defineLazyModuleGetter:
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/XPCOMUtils.jsm#220

It uses the same pattern almost exactly.
(Assignee)

Comment 4

4 years ago
(In reply to Mark Finkle (:mfinkle) from comment #3)
> Actually, you can safely change those two as well. Look at the definition of
> defineLazyModuleGetter:
> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/XPCOMUtils.
> jsm#220
> 
> It uses the same pattern almost exactly.

Thanks a lot. Should have checked there first myself. I still feel a little lost around here.
(Assignee)

Comment 5

4 years ago
Created attachment 758916 [details] [diff] [review]
Replace XPCOMUtils.defineLazyGetter with defineLazyModuleGetter

Changes made in files browser.js (thanks to Mark Finkle, I changed the remaining two instances as well), AddonUpdateService.js, and SessionStore.js.
Attachment #758916 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 6

4 years ago
Comment on attachment 758916 [details] [diff] [review]
Replace XPCOMUtils.defineLazyGetter with defineLazyModuleGetter

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

Looks good.
Attachment #758916 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9be31da8e36a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9be31da8e36a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
You need to log in before you can comment on or make changes to this bug.