Closed Bug 1790234 Opened 2 years ago Closed 2 years ago

Add a method to nsIFaviconService that allows for manually setting and inserting a favicon entry without fetching over the network

Categories

(Toolkit :: Places, enhancement)

enhancement

Tracking

()

RESOLVED INVALID

People

(Reporter: mconley, Unassigned)

References

(Blocks 2 open bugs)

Details

The nsIFaviconService has a method setAndFetchFaviconForPage (see here for the documentation for that method) that fetches a favicon for a page over the network and then stores it in the DB.

Looking through the rest of nsIFaviconService, it doesn't look like there's a great way of doing the same thing, but instead of supplying a URI endpoint, supplying a binary blob as the favicon data.

We should add a method to nsIFaviconService that lets us supply a blob of favicon data for a page without hitting the network.

Component: Migration → Places
Product: Firefox → Toolkit
Blocks: 1789769

Hey mak,

Do you think I'm missing anything here, or does it make sense to expose a new method for this kind of favicon insertion?

Flags: needinfo?(mak)

It's also worth noting that since this is to supporting the Migrator stuff, it might make sense to have this method accept a collection of favicons, rather than one at a time.

You don't need a new API afaict, you can use replaceFaviconData or replaceFaviconDataFromDataURL depending on which format you have the icon, followed by setAndFetchFaviconForPage. The former fills a memory cache that is then used by the latter, so network won't be hit as far as the faviconURI is the same.
Fetching from the network directly in the favicon service is deprecated, long term I think the service will only accept a data url for the favicon data, thus I'd suggest using replaceFaviconDataFromDataURL (followed by SetAndFetch...).

If you don't have the favicon uri, then you'll have to make a "randomized" one, but I hope other browsers are storing the favicon uri.

Note: the annoyance of having to invoke 2 APIs for setting one icon could be removed, and we have Bug 1552815 covering that. Short, setAndFetchFaviconForPage should just be setIconForPage.

Flags: needinfo?(mak)

There isn't a batch API for insertion, since for each icon there's various steps to resize/convert/undupe it anyway, it wouldn't save much.

Ah, I see. Do you know if we already have utilities in place to convert image data to DataURLs off of the main thread? I worry a little bit about doing a bunch of these (maybe on the order of thousands?) in a row on the main thread, despite being small images.

Flags: needinfo?(mak)

Not sure about off the main thread, so far most code (for example https://searchfox.org/mozilla-central/source/browser/modules/FaviconLoader.jsm) used FileReader::readAsDataURL starting from a blob, but that is doing one icon at a time when a page is visited, not hundreds.
FileReader is available in Workers... Of course having the whole batch fetch and conversion happen in a worker would be better, I suppose though you'll still have to pass back [favicon_url, data_url] tuples once done.

Flags: needinfo?(mak)

Alright, cool, thanks mak. :)

In that case, from what I've read here, we already have what we need for inserting favicons from other browsers into the database, so I think we can close this as INVALID.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.