[New Tab Page] Load links lazily when opening a new tab

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 591817 [details] [diff] [review]
patch v1
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #591817 - Flags: review?(mak77)
Comment on attachment 591817 [details] [diff] [review]
patch v1

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

Thanks, this should ideally solve the current regressions, will we figure them out.
Attachment #591817 - Flags: review?(mak77) → review+
(Assignee)

Updated

5 years ago
Summary: [New Tab Page] Don't fetch links on startup when disabled → [New Tab Page] Load links lazily when opening a new tab
(Assignee)

Comment 3

5 years ago
Created attachment 591951 [details] [diff] [review]
patch v2

Links are now lazily loaded when opening the New Tab Page for the first time. This should hopefully help with Ts/Tshutdown (pushed to try to verify) and serves the purpose of not doing anything when the user opts out of the New Tab Page.
Attachment #591817 - Attachment is obsolete: true
Attachment #591951 - Flags: review?(mak77)
(Assignee)

Comment 4

5 years ago
Nice, that totally solved the problem. I pushed the New Tab Page without and then with this patch to try:

without this patch: 0d7ece8c1f0e
with this patch:    c43df53c5f69

http://graphs-new.mozilla.org/graph.html#tests=[[51,23,14]]&sel=none&displayrange=7&datatype=running
http://graphs-new.mozilla.org/graph.html#tests=[[54,23,14]]&sel=none&displayrange=7&datatype=running
Comment on attachment 591951 [details] [diff] [review]
patch v2

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

Yes, initing resources when they are actually needed looks much healthier!

::: browser/modules/NewTabUtils.jsm
@@ +483,3 @@
>     */
> +  populateCache: function Links_populateCache(aCallback, aForce) {
> +    // There's a callback waiting already.

this comment can be improved, adding "thus the cache has not yet been populated"

@@ +492,4 @@
>  
> +      this._provider.getLinks(function (aLinks) {
> +        this._links = aLinks;
> +        this._populateCallbacks.forEach(function (cb) cb && cb());

may the callback ever throw and interrupt the loop? should you rather try/catch so that it will proceed regardless? Should be safer

@@ +492,5 @@
>  
> +      this._provider.getLinks(function (aLinks) {
> +        this._links = aLinks;
> +        this._populateCallbacks.forEach(function (cb) cb && cb());
> +        this._populateCallbacks = [];

or alternativaly, to avoid gc of the forEach callback, you may just do a plain
while (this._populateCallbacks) {
  let cb = this._populateCallbacks.shift();
  ...
}
Attachment #591951 - Flags: review?(mak77) → review+
> > +      this._provider.getLinks(function (aLinks) {
> > +        this._links = aLinks;
> > +        this._populateCallbacks.forEach(function (cb) cb && cb());
> > +        this._populateCallbacks = [];
> 
> or alternativaly, to avoid gc of the forEach callback, you may just do a
> plain
> while (this._populateCallbacks) {
>   let cb = this._populateCallbacks.shift();
>   ...
> }

While you're at it, please change "cb && cb()" to "if (cb)\n  cb()".
(Assignee)

Comment 7

5 years ago
(In reply to Marco Bonardo [:mak] from comment #5)
> > +  populateCache: function Links_populateCache(aCallback, aForce) {
> > +    // There's a callback waiting already.
> 
> this comment can be improved, adding "thus the cache has not yet been
> populated"

Fixed.

> > +      this._provider.getLinks(function (aLinks) {
> > +        this._links = aLinks;
> > +        this._populateCallbacks.forEach(function (cb) cb && cb());
> 
> may the callback ever throw and interrupt the loop? should you rather
> try/catch so that it will proceed regardless? Should be safer

Good point, done.

> > +      this._provider.getLinks(function (aLinks) {
> > +        this._links = aLinks;
> > +        this._populateCallbacks.forEach(function (cb) cb && cb());
> > +        this._populateCallbacks = [];
> 
> or alternativaly, to avoid gc of the forEach callback, you may just do a
> plain
> while (this._populateCallbacks) {
>   let cb = this._populateCallbacks.shift();
>   ...
> }

Done.

(In reply to Dão Gottwald [:dao] from comment #6)
> While you're at it, please change "cb && cb()" to "if (cb)\n  cb()".

Done.
I forgot a .length in the while pseudocode, though i guess you figured that out already :)
(Assignee)

Comment 9

5 years ago
Yes :)
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/d0b8df7e28ea
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/mozilla-central/rev/d0b8df7e28ea
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 12
You need to log in before you can comment on or make changes to this bug.