1894633 - Stop fetching icons from the network in Places

Status: RESOLVED FIXED

(Toolkit :: Places, task, P3)

Description

[Marco Bonardo [:mak]]

Places should only be a store of icons, but never fetch them from the network.
There's privacy and security implications in doing that, we prefer to delegate the burden to some dedicated module.
This is already the case for icons stored from the browser tabs, though many components still rely on Places to do the fetching.
Any use of SetAndFetchFaviconForPage should be converted.
PlacesUIUtils.loadFavicon should be removed, or moved, or changed to only accept data uris (then maybe it won't be particularly useful to keep it).

We have browser/modules/FaviconLoader.sys.mjs, we could probably move more stuff into it and make it our centralized favicon load module.

Comment 1

[Marco Bonardo [:mak]]

This may need a breakdown, as we should likely start converting consumers one by one, and there is module refactoring to make.
The final scope is to remove SetAndFetchFaviconForPage.

Comment 2

[Marco Bonardo [:mak]]

Daisuke, this will likely need a breakdown, per comment 1. Probably multiple dependent bugs.

Comment 3

[Daisuke Akatsuka (:daisuke)]

Hi Marco, thanks!

Here is the approach I'm thinking of so far.
The nsIFaviconService.setAndFetchFaviconForPage() is doing three major things like below.

1. If there is favicon data for the page in database, get the data. (If aForceReload is false)
2. If no favicon data from above, get the data from network.
3. Save/notify the data.

So, I think we need the following 2 functions to correspond to the above.

1. getFaviconFromDB() in PlacesUtils.sys.mjs.
2. getFaviconAsDataURL() in FaviconLoader.sys.mjs.

Then, change the codes where use nsIFaviconService.setAndFetchFaviconForPage() to like below.

1. Get favicon data from PlacesUtils.getFaviconFromDB() (if not forced to reload).
2. If no favicon data of the above, get it from FaviconLoader.getFaviconAsDataURL().
3. Call nsIFaviconService.setFaviconForPage().

Next, regarding to PlacesUIUtils.loadFavicon(), it seems that this function is called from only one place. So, I think we can move the function to the place that calls it.

So, I think, as it seems that it will not be so huge, the following patches in this bug might be enough, not breaking down.

1. Replace nsIFaviconService.setAndFetchFaviconForPage() with new.
2. Move PlacesUIUtils.loadFavicon() to LinkHandlerParent.sys.mjs (and optimize)
3. Remove nsIFaviconService.setAndFetchFaviconForPage().

I will do the above first, then, if it looks better to fix each in another bug, I will file them.

Comment 4

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1894633: Use nsIFaviconService::setFaviconForPage() instead of setAndFetchFaviconForPage()

Comment 5

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1894633: Use nsIFaviconService::setFaviconForPage() for tests

Depends on D211692

Comment 6

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1894633: Move loadFavicon function to LinkHandlerParent

Depends on D211693

Comment 7

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1894633: Remove nsIFaviconService::setAndFetchFaviconForPage() api

Depends on D211694

Comment 8

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1894633: Remove unused telemetry about favicon size

Depends on D211695

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4cca836dfbd
Use nsIFaviconService::setFaviconForPage() for tests r=places-reviewers,home-newtab-reviewers,thecount,mak
https://hg.mozilla.org/integration/autoland/rev/a0717adf4b6a
Remove nsIFaviconService::setAndFetchFaviconForPage() api r=mak
https://hg.mozilla.org/integration/autoland/rev/b3bf3c4a2462
Remove unused telemetry about favicon size r=mak

Comment 10

[Noemi Erli[:noemi_erli]]

Backed out 3 changesets (Bug 1894633) as requested by daisuke CLOSED TREE
Backout link: https://hg.mozilla.org/integration/autoland/rev/8b55eb3aa72957c8bfc7526937329818c1ac426a

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/628af64728a1
Backed out 3 changesets as requested by daisuke CLOSED TREE

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83b1731abb9d
Use nsIFaviconService::setFaviconForPage() for tests r=places-reviewers,home-newtab-reviewers,thecount,mak
https://hg.mozilla.org/integration/autoland/rev/8b824e3cd55d
Remove nsIFaviconService::setAndFetchFaviconForPage() api r=mak
https://hg.mozilla.org/integration/autoland/rev/33cddf1e5c70
Remove unused telemetry about favicon size r=mak

Comment 13

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/83b1731abb9d
https://hg.mozilla.org/mozilla-central/rev/8b824e3cd55d
https://hg.mozilla.org/mozilla-central/rev/33cddf1e5c70