1084134 - Move Reader into a lazy-loaded JS file

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

Slowly trying to reduce the size of browser.js.

Comment 1

[:Margaret Leibovic]

Attachment: (Part 1) Move Reader into a lazy-loaded JS file

Copy/paste.

Comment 2

[:Margaret Leibovic]

Attachment: (Part 2) Get rid of init method to make this actually lazy

We're still going to end up calling Reader.something while the first tab is loading, but this is one small step towards being lazier.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8506519 [details] [diff] [review]
(Part 1) Move Reader into a lazy-loaded JS file

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

Such rubberstamp, much file, wow.

::: mobile/android/chrome/content/Reader.js
@@ +45,5 @@
> +        type: "Reader:LongClick",
> +        tabID: tabID
> +      });
> +
> +      UITelemetry.addEvent("save.1", "pageaction", null, "reader");

Do we not need to import something to get a reference to UITelemetry (and the other references in this file)?

I'm guessing your answer is that loadSubScript means we don't, but I figured it's worth checking.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8506520 [details] [diff] [review]
(Part 2) Get rid of init method to make this actually lazy

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

::: mobile/android/chrome/content/Reader.js
@@ -28,3 @@
>  
> -    Services.obs.addObserver(this, "Reader:Add", false);
> -    Services.obs.addObserver(this, "Reader:Remove", false);

Who is responsible for adding these observers now?

Comment 5

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

(In reply to Richard Newman [:rnewman] from comment #3)

> > +      UITelemetry.addEvent("save.1", "pageaction", null, "reader");
> 
> Do we not need to import something to get a reference to UITelemetry (and
> the other references in this file)?
> 
> I'm guessing your answer is that loadSubScript means we don't, but I figured
> it's worth checking.

(In reply to Richard Newman [:rnewman] from comment #4)

> > -    Services.obs.addObserver(this, "Reader:Add", false);
> > -    Services.obs.addObserver(this, "Reader:Remove", false);
> 
> Who is responsible for adding these observers now?

Both questions are answered here in the first patch:

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

>+  ["Reader", ["Reader:Add", "Reader:Remove"], "chrome://browser/content/Reader.js"],

The lazy loader uses loadSubScript, pulling the code into the browser.js scope, meaning UITelemetry.jsm is available. It will also triggers the lazy load from observer service notifications, which are added here too.

Comment 6

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #5)
> (In reply to Richard Newman [:rnewman] from comment #3)
> 
> > > +      UITelemetry.addEvent("save.1", "pageaction", null, "reader");
> > 
> > Do we not need to import something to get a reference to UITelemetry (and
> > the other references in this file)?
> > 
> > I'm guessing your answer is that loadSubScript means we don't, but I figured
> > it's worth checking.
> 
> (In reply to Richard Newman [:rnewman] from comment #4)
> 
> > > -    Services.obs.addObserver(this, "Reader:Add", false);
> > > -    Services.obs.addObserver(this, "Reader:Remove", false);
> > 
> > Who is responsible for adding these observers now?
> 
> Both questions are answered here in the first patch:
> 
> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> 
> >+  ["Reader", ["Reader:Add", "Reader:Remove"], "chrome://browser/content/Reader.js"],
> 
> The lazy loader uses loadSubScript, pulling the code into the browser.js
> scope, meaning UITelemetry.jsm is available. It will also triggers the lazy
> load from observer service notifications, which are added here too.

Sorry, at first I made one patch, but then I decided it would be smarter to pull the non-copy-paste changes into a separate patch. I'll land them folded together, since the first patch on its own has double the observers.

Comment 7

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/3f4673b89e04

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f4673b89e04