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)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 12
People
(Reporter: ttaubert, Assigned: ttaubert)
Details
Attachments
(1 file, 1 obsolete file)
6.98 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•13 years ago
|
||
Comment 2•13 years ago
|
||
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•13 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•13 years ago
|
||
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•13 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 5•13 years ago
|
||
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+
Comment 6•13 years ago
|
||
> > + 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•13 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.
Comment 8•13 years ago
|
||
I forgot a .length in the while pseudocode, though i guess you figured that out already :)
Assignee | ||
Comment 9•13 years ago
|
||
Yes :)
Assignee | ||
Comment 10•13 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 11•13 years ago
|
||
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.
Description
•