Closed Bug 721413 Opened 13 years ago Closed 13 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 12

People

(Reporter: ttaubert, Assigned: ttaubert)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch patch v1 (obsolete) — Splinter Review
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+
Summary: [New Tab Page] Don't fetch links on startup when disabled → [New Tab Page] Load links lazily when opening a new tab
Attached patch patch v2Splinter Review
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)
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()".
(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 :)
Yes :)
Status: ASSIGNED → RESOLVED
Closed: 13 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.

Attachment

General

Created:
Updated:
Size: