Closed
Bug 1075226
Opened 10 years ago
Closed 10 years ago
Use Array.map in Updater_fillEmptyCells
Categories
(Firefox :: New Tab Page, defect)
Firefox
New Tab Page
Tracking
()
RESOLVED
FIXED
Firefox 36
People
(Reporter: evold, Assigned: evold)
References
Details
Attachments
(1 file, 1 obsolete file)
1.09 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
At the moment Updater_fillEmptyCells is using a combination of `forEach` and `Array.push` to achieve what Array.map does natively.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → evold
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8497846 -
Flags: review?(ttaubert)
Comment 2•10 years ago
|
||
Comment on attachment 8497846 [details] [diff] [review] 1075226.diff Review of attachment 8497846 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/newtab/updater.js @@ +162,2 @@ > if (aSite || !aLinks[aIndex]) > + return null; Just tested and looks like Promise.all() does indeed ignore null values. A little magic but ok. OTOH we could also use a generator and call Promise.all(...promises()); @@ +164,2 @@ > > + let { promise, resolve } = Promise.defer(); We don't want to switch back to the old Promise API. @@ +175,5 @@ > + window.getComputedStyle(site.node).opacity; > + gTransformation.showSite(site, resolve); > + > + return promise; > + })).then(aCallback).catch(console.exception); Is |console| available? Guess we'd have to import JSM.
Attachment #8497846 -
Flags: review?(ttaubert) → feedback+
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #2) > Comment on attachment 8497846 [details] [diff] [review] > 1075226.diff > > Review of attachment 8497846 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/newtab/updater.js > @@ +162,2 @@ > > if (aSite || !aLinks[aIndex]) > > + return null; > > Just tested and looks like Promise.all() does indeed ignore null values. A > little magic but ok. OTOH we could also use a generator and call > Promise.all(...promises()); hmm not sure how that would look. > @@ +164,2 @@ > > > > + let { promise, resolve } = Promise.defer(); > > We don't want to switch back to the old Promise API. Ah I understand, reverted that bit. > @@ +175,5 @@ > > + window.getComputedStyle(site.node).opacity; > > + gTransformation.showSite(site, resolve); > > + > > + return promise; > > + })).then(aCallback).catch(console.exception); > > Is |console| available? Guess we'd have to import JSM. Yes it's available.
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8497846 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8498705 -
Flags: review?(ttaubert)
Assignee | ||
Updated•10 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 5•10 years ago
|
||
Comment on attachment 8498705 [details] [diff] [review] 1075226.diff Review of attachment 8498705 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: browser/base/content/newtab/updater.js @@ +164,2 @@ > > + return new Promise((resolve) => { nit: return new Promise(resolve => { @@ +173,5 @@ > // the fade-in transition work. > window.getComputedStyle(site.node).opacity; > gTransformation.showSite(site, resolve); > + }); > + })).then(aCallback).catch(console.exception); Oh, right. It's not a JSM so of course |console| is available.
Attachment #8498705 -
Flags: review?(ttaubert) → review+
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f170fd83bb9b
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f170fd83bb9b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in
before you can comment on or make changes to this bug.
Description
•