1089368 - Promisify _removeLegacySites from content/newtab/updater.js

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: 1089368.diff

https://github.com/erikvold/gecko-dev/pull/5/files

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8511670 [details] [diff] [review]
1089368.diff

Review of attachment 8511670 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! r=me with the two callback args removed.

::: browser/base/content/newtab/updater.js
@@ +139,5 @@
>            node.parentNode.removeChild(node);
>            resolve();
>          });
> +      });
> +    }).then(aCallback).catch(console.exception);

There's no reason to still support the callback, right? Catching errors should probably be moved to the caller as well.

@@ +152,4 @@
>      let {cells, sites} = gGrid;
>  
>      // Find empty cells and fill them.
> +    return Promise.all(sites.map((aSite, aIndex) => {

We can remove the callback argument here for _fillEmptyCells() as well.

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Tim Taubert [:ttaubert] from comment #2)
> Comment on attachment 8511670 [details] [diff] [review]
> 1089368.diff
> 
> Review of attachment 8511670 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks! r=me with the two callback args removed.
> 
> ::: browser/base/content/newtab/updater.js
> @@ +139,5 @@
> >            node.parentNode.removeChild(node);
> >            resolve();
> >          });
> > +      });
> > +    }).then(aCallback).catch(console.exception);
> 
> There's no reason to still support the callback, right? Catching errors
> should probably be moved to the caller as well.
> 
> @@ +152,4 @@
> >      let {cells, sites} = gGrid;
> >  
> >      // Find empty cells and fill them.
> > +    return Promise.all(sites.map((aSite, aIndex) => {
> 
> We can remove the callback argument here for _fillEmptyCells() as well.

sounds good!

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

https://tbpl.mozilla.org/?tree=Try&rev=368c829e9c17

Comment 5

[Ed Lee :Mardak]

Code was removed in bug 1433133 https://hg.mozilla.org/mozilla-central/rev/fc80dd3e4fae