Last Comment Bug 879088 - Use defineLazyModuleGetter for lazy modules
: Use defineLazyModuleGetter for lazy modules
Status: RESOLVED FIXED
[mentor=margaret][lang=js]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 24
Assigned To: Nicolas Carlo [:nickecarlo]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-03 17:28 PDT by :Margaret Leibovic
Modified: 2013-06-07 08:42 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Replace XPCOMUtils.defineLazyGetter with defineLazyModuleGetter (6.75 KB, patch)
2013-06-05 18:23 PDT, Nicolas Carlo [:nickecarlo]
margaret.leibovic: review+
Details | Diff | Splinter Review

Description :Margaret Leibovic 2013-06-03 17:28:51 PDT
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
Comment 1 Nicolas Carlo [:nickecarlo] 2013-06-04 18:55:44 PDT
I can work on this bug.
Comment 2 Nicolas Carlo [:nickecarlo] 2013-06-05 17:28:06 PDT
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.
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2013-06-05 17:52:35 PDT
(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.
Comment 4 Nicolas Carlo [:nickecarlo] 2013-06-05 18:19:39 PDT
(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.
Comment 5 Nicolas Carlo [:nickecarlo] 2013-06-05 18:23:47 PDT
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.
Comment 6 :Margaret Leibovic 2013-06-06 09:20:32 PDT
Comment on attachment 758916 [details] [diff] [review]
Replace XPCOMUtils.defineLazyGetter with defineLazyModuleGetter

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

Looks good.
Comment 7 Ryan VanderMeulen [:RyanVM] 2013-06-06 09:39:50 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9be31da8e36a
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-06-07 08:42:50 PDT
https://hg.mozilla.org/mozilla-central/rev/9be31da8e36a

Note You need to log in before you can comment on or make changes to this bug.