Investigate lazy-loading Reader.js

RESOLVED FIXED in Firefox 37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

35 Branch
Firefox 37
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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.
Assignee: nobody → margaret.leibovic
Looks like this fixes the regression:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6aaa4ecd605
Attachment #8544867 - Flags: review?(mark.finkle)
Blocks: 1111142
No longer depends on: 1111142
(This builds on top of the patches in bug 1111142 and bug 1117224, I'll land them all together).
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.
Attachment #8544867 - Flags: review?(mark.finkle) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/5b030e48e8b0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
You need to log in before you can comment on or make changes to this bug.