1117228 - Investigate lazy-loading Reader.js

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[:Margaret Leibovic]

From bug 1111142 comment 12:

> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> 
> >+// XXX: Make this into a module?
> >+Services.scriptloader.loadSubScript("chrome://browser/content/Reader.js", this);
> 
> I guess it makes no sense to lazy load this?

I decided to get rid of the lazy loading because I need to register the message listeners, but maybe we could find a way to lazily do that, similarly to what we do for notifications. Since content.js is doing the parse-on-load business now, we really don't need Reader.js to wake up until we find an article or load about:reader. I could also file a follow-up bug to investigate this, it might be a good optimization for memory-constrained devices where parse-on-load is disabled.

Comment 1

[:Margaret Leibovic]

It looks like we'll need to do this, since not lazy loading Reader.js caused a tcheck regression when I tried to land bug 1111142.

Comment 2

[:Margaret Leibovic]

Attachment: Lazy load Reader.js

Looks like this fixes the regression:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6aaa4ecd605

Comment 3

[:Margaret Leibovic]

(This builds on top of the patches in bug 1111142 and bug 1117224, I'll land them all together).

Comment 4

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

Comment on attachment 8544867 [details] [diff] [review]
Lazy load Reader.js

>diff --git a/mobile/android/chrome/content/Reader.js b/mobile/android/chrome/content/Reader.js
>   observe: function Reader_observe(aMessage, aTopic, aData) {
>     switch (aTopic) {
>       case "Reader:Added": {
>-        window.messageManager.broadcastAsyncMessage("Reader:Added", { url: aData });
>+        let mm = window.getGroupMessageManager("browsers");
>+        mm.broadcastAsyncMessage("Reader:Added", { url: aData });


Is it possible to add the Message listeners here? Will a message be fired into the parent process/scope *before* "Reader:Added" is caught? If so, then don't worry about it. If not, you might want to consider moving the addMessageListener into here in a followup.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js


>+// Lazily-loaded browser scripts that use message listeners.

Someday, I want to move this helper shims, for lazy loading based on notifications, events and messages, into a separate JS file. Just for cleanup.

I do find it to be a useful pattern for lazy loading.

Comment 5

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Comment on attachment 8544867 [details] [diff] [review]
> Lazy load Reader.js
> 
> >diff --git a/mobile/android/chrome/content/Reader.js b/mobile/android/chrome/content/Reader.js
> >   observe: function Reader_observe(aMessage, aTopic, aData) {
> >     switch (aTopic) {
> >       case "Reader:Added": {
> >-        window.messageManager.broadcastAsyncMessage("Reader:Added", { url: aData });
> >+        let mm = window.getGroupMessageManager("browsers");
> >+        mm.broadcastAsyncMessage("Reader:Added", { url: aData });
> 
> 
> Is it possible to add the Message listeners here? Will a message be fired
> into the parent process/scope *before* "Reader:Added" is caught? If so, then
> don't worry about it. If not, you might want to consider moving the
> addMessageListener into here in a followup.

Yes, the two messages that are fired first are "Reader:UpdateIsArticle" (fired from child -> parent when we parse an article on load) and "Reader:ArticleGet" (fired from child -> parent when we load about:reader).

I experimented with lazily adding the rest of the listeners, but it became a bit of a mess.

> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> 
> 
> >+// Lazily-loaded browser scripts that use message listeners.
> 
> Someday, I want to move this helper shims, for lazy loading based on
> notifications, events and messages, into a separate JS file. Just for
> cleanup.
> 
> I do find it to be a useful pattern for lazy loading.

Yeah, that would be nice clean-up, since we keep adding more lazy patterns.

https://hg.mozilla.org/integration/fx-team/rev/5b030e48e8b0

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5b030e48e8b0