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

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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();
  ...
}

Comment 6

[Dão Gottwald [:dao]]

> > +      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()".

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Yes :)

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/d0b8df7e28ea