854107 - Lazy load more JS objects in browser.js

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

browser.js has many JS objects in it. The file is over 8K lines long. We have talked about several ways to remove code from browser.js and lazy load it instead. We already do this for a few files:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#59

These scripts are not JSM files. They need a window object, so we load them a slightly different way.

Comment 1

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

Attachment: WIP 1: Lazy load notification-based objects

This patch adds a new mechanism. Many of this JS objects follow a pattern like this:

In startup:
Obj.init();

In shutdown:
Obj.uninit();

The purpose of these functions is to add an observer (or more) to nsIObserverService. This pattern means we can't use our normal lazy load mechanism since the object would be lazy loaded in startup when the Obj.init() method is called.

Instead, this patch defers adding the observer to a delegate method. The notification is then passed to the Obj.observe(...) method when the notification is fired. This means we can defer loading the JS object until the notification fires.

What do you think?

Comment 2

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

Attachment: WIP 2: More normal lazy loaded objects

This patch adds more non-notification based objects to the lazy load mechanism. Two of these are called during getting/setting Preferences. That should still be out of the startup path, but I am not 100% sure. I might need to use the raw preference names to help make it more lazy.

Comment 3

[:Margaret Leibovic]

Comment on attachment 728632 [details] [diff] [review]
WIP 2: More normal lazy loaded objects

Nice, I like this. Finding a way to optimize getting/setting preferences sounds good, but this is definitely a good start.

Also, it might be nice to look into moving some of the PluginHelper utility routines to toolkit, since there are a lot of very similar functions in browser-plugins.js. Having PluginHelper in its own file will make this easier because we could just load some utility jsm when we need it. But that's something that should be done in a different bug.

Comment 4

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

Attachment: Patch: Lazy load notification-based objects v1

I added a LazyNotificationGetter object to help maintain state. I planned to call it's shutdown method in BrowserApp.shutdown, but no one has ever called that method since we went native.

I'll file a bug to remove the shutdown method.

I have tested by using a console API test page here:
http://people.mozilla.com/~mfinkle/console-test.html

I see the console log output in logcat, so the objects must be getting loading and the observe method is getting re-routed.

Comment 5

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

Attachment: Patch: More normal lazy loaded objects v1

Same as the original WIP except I changed the "MasterPassword.pref" calls in get/setPreferences to use the raw pref string instead. getPreferences gets called during started and using "MasterPassword.pref" pulls in the MasterPassword code. I liked using the named pref, but this makes the code lazier.

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 729126 [details] [diff] [review]
Patch: Lazy load notification-based objects v1

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

Great! Lets try it!

::: mobile/android/chrome/content/browser.js
@@ +74,5 @@
>  
> +// Lazily-loaded browser scripts that use observer notifcations:
> +var LazyNotificationGetter = {
> +  observers: [],
> +  shutdown: function lng_shutdown() {

Yeah, I think we should just yank our shutdown... or at least remove all the things from it that probably don't need to happen.

@@ +102,5 @@
> +      observe: function(s, t, d) {
> +        window[name].observe(s, t, d);
> +      }
> +    };
> +    Services.obs.addObserver(o, aNotification, false);

I think we might be better to register an observer for each notification only once, and then have it call all the observers. I would guess that doing all of this in JS and avoiding the XPCOM pieces of the observer service would generally be faster, but its also a bit more complex. At this point, I think the code cleanup is more useful than having a crazy efficient service.

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a2dd943226e

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b1459d107c9

Comment 9

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

Attachment: Patch: More lazy notification-based objects v1

Looks like we can easily move FindHelper and PermissionHelper out of browser.js using the new lazy notification-based getter

Comment 10

[:Margaret Leibovic]

Comment on attachment 729234 [details] [diff] [review]
Patch: More lazy notification-based objects v1

Nice.

I'm trusting you just copy/pasted and then got rid of init/uninit. I didn't check more closely than that :)

Comment 11

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

(In reply to Mark Finkle (:mfinkle) from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4b1459d107c9

Backed out due to robocop failures
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b6a1e103c82

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a2dd943226e

Comment 13

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

Comment on attachment 729234 [details] [diff] [review]
Patch: More lazy notification-based objects v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/c459dfb9522b

Comment 14

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

(In reply to Mark Finkle (:mfinkle) from comment #11)
> (In reply to Mark Finkle (:mfinkle) from comment #8)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/4b1459d107c9
> 
> Backed out due to robocop failures
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5b6a1e103c82

And relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e34fd6bc2e29

Try run was green:
https://tbpl.mozilla.org/?tree=Try&rev=db0e460e457d

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e34fd6bc2e29
https://hg.mozilla.org/mozilla-central/rev/c459dfb9522b

Comment 16

[:Margaret Leibovic]

There hasn't been activity in here in a long time. File follow-ups if you want to land more patches.