Closed Bug 1075226 Opened 10 years ago Closed 10 years ago

Use Array.map in Updater_fillEmptyCells

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: evold, Assigned: evold)

References

Details

Attachments

(1 file, 1 obsolete file)

At the moment Updater_fillEmptyCells is using a combination of `forEach` and `Array.push` to achieve what Array.map does natively.
Assignee: nobody → evold
Attached patch 1075226.diff (obsolete) — Splinter Review
Attachment #8497846 - Flags: review?(ttaubert)
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+
(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.
Attached patch 1075226.diffSplinter Review
Attachment #8497846 - Attachment is obsolete: true
Attachment #8498705 - Flags: review?(ttaubert)
OS: Mac OS X → All
Hardware: x86 → All
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+
Status: NEW → ASSIGNED
Depends on: 1083206
No longer depends on: 1083206
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.