Closed Bug 1084134 Opened 5 years ago Closed 5 years ago

Move Reader into a lazy-loaded JS file

Categories

(Firefox for Android :: Reader View, defect)

35 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Slowly trying to reduce the size of browser.js.
Copy/paste.
Attachment #8506519 - Flags: review?(rnewman)
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.
Attachment #8506520 - Flags: review?(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.
Attachment #8506519 - Flags: review?(rnewman) → review+
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?
(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.
(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.
Attachment #8506520 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/3f4673b89e04
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.