Closed Bug 1396104 Opened 7 years ago Closed 7 years ago

Use rich icons when getting top sites

Categories

(Firefox :: New Tab Page, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: Mardak, Assigned: ursula)

References

Details

Attachments

(1 file)

Ursula says 1 line change:

This is done via newtabutils's _addFavicons function - passing in a 0 to here: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/NewTabUtils.jsm?q=file%3Anewtabutils.jsm&redirect_type=single#877 to pull out largest favicon available
Blocks: 1394533
Assignee: nobody → usarracini
Comment on attachment 8906070 [details]
Bug 1396104 - Use rich icons when getting top sites

https://reviewboard.mozilla.org/r/177808/#review182876

::: toolkit/modules/NewTabUtils.jsm:961
(Diff revision 1)
>                  link.faviconLength = len;
> +                link.faviconSize = size;
>                }
>                return resolve(link);
>              });
> -        }).catch(() => {
> +        }, 0).catch(() => {

The indentation is all funky here. Your 0 here is the 2nd argument to `new Promise()` (notice the `.catch`)

Does this mean we don't actually need to pass in 0 to `getFaviconDataForPage`?
For styling, I could see the 0 on its own line with a comment, e.g.,

new Promise(resolve => {
  return Places.getFavicon(
    uriStuff,
    callbackStuff => {
    },
    0); // preferredWidth: get the biggest width available
}).catch

Where you only touch the line of the callback closing brace (add comma) and the 0 line.
Comment on attachment 8906070 [details]
Bug 1396104 - Use rich icons when getting top sites

https://reviewboard.mozilla.org/r/177808/#review182888
Attachment #8906070 - Flags: review?(edilee) → review+
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d923714255be
Use rich icons when getting top sites r=Mardak
https://hg.mozilla.org/mozilla-central/rev/d923714255be
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.