553687 - Lazy load JavaScript Modules

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

We Cu.import any needed JavaScript modules. We could lazy load those modules when they are needed. This can help our startup time.

This patch: 
* Lazy loads some modules
* Removes unused SpatialNavigation

Even though we have spatial nav turned off, the callback method is broken if someone _did_ try to turn it back on.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 433628 [details] [diff] [review]
patch

>-Cu.import("resource://gre/modules/SpatialNavigation.js");
>-Cu.import("resource://gre/modules/PluralForm.jsm");
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+
>+XPCOMUtils.defineLazyGetter(this, "PluralForm", function() {
>+  Cu.import("resource://gre/modules/PluralForm.jsm");
>+  return PluralForm;
>+});

Do we really need to have these import both in browser-ui.js and browser.js?


Also we're still setting pref("snav.enabled", false); in app/mobile.js.
Looking at mxr I can't find it defined to true all.js, so I bet we can just remove this pref and everything should works fine. (Spatial Navigation is defaulting to false if the pref is not found: http://mxr.mozilla.org/mozilla-central/source/toolkit/spatial-navigation/SpatialNavigation.js#486)

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #1)
> (From update of attachment 433628 [details] [diff] [review])

> >+XPCOMUtils.defineLazyGetter(this, "PluralForm", function() {
> >+  Cu.import("resource://gre/modules/PluralForm.jsm");
> >+  return PluralForm;
> >+});
> 
> Do we really need to have these import both in browser-ui.js and browser.js?


Nope. I'll remove the one from browser.js since it will have no need for any imported modules.

> Also we're still setting pref("snav.enabled", false); in app/mobile.js.
> Looking at mxr I can't find it defined to true all.js, so I bet we can just
> remove this pref and everything should works fine. (Spatial Navigation is
> defaulting to false if the pref is not found:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/spatial-navigation/SpatialNavigation.js#486)

I'll remove the pref from mobile.js

I really don't want any Spatial Nav code at all. Even initializing the code hooks up a key event handler, which slows the code down for no good reason.

http://mxr.mozilla.org/mozilla-central/source/toolkit/spatial-navigation/SpatialNavigation.js#58

r+ with those changes?

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

> I really don't want any Spatial Nav code at all. Even initializing the code
> hooks up a key event handler, which slows the code down for no good reason.
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/spatial-navigation/SpatialNavigation.js#58
> 
> r+ with those changes?

Sure.

Sorry for the late response, I've not seen your reply because I wasn't CC'ed on this bug.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

with review changes

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/fdb9b8bce411