1045843 - Consider loading missing favicons from the network when viewing about:home lists.

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Favicon Handling, defect)

Description

[Chris Kitching [:ckitching]]

Attachment: Capella's patch addressing this problem.

Giving Capella's patch from Bug 1044940 its own bug: it's a cool patch, but it doesn't address that bug.

The question: Should we go to network and try to download a favicon when the user just scrolls through the the bookmarks/history lists?

Pros:
Prettier lists.
Fewer blanks in favicon database.
People who use Sync will actually *have* icons (or has the situation Wes described over in Bug 813585 since been fixed?)

Cons:
Extra network traffic.
Very trivial extra complexity (I have a feeling I wrote it like this first time round and RNewman said no when reviewing the monster favicon patch from hell. Let's see...)

Comment 1

[Richard Newman [:rnewman]]

We shouldn't do so unrestrainedly. Slamming through your history could result in hundreds of KB of favicons being loaded unnecessarily. If you can make sure that doesn't happen -- maybe only do it for visible bookmarks and when the history view stops? -- then fine.

We also want to make sure this doesn't happen when the user is trying to do something else -- CPU or network.

Comment 2

[Mark Capella [:capella]]

TwoLinePageRows are displayed not only for Bookmark/History listitems, but also for Topsites, ReadingList and RecentTabs.

Favicons can also be requested by TopSites when Gridviews can't locate a thumb image, and when HomeFragment tries to add an item to the Desktop.

Favicons exist in these situations, after a user has already visited a page. I see this as a fix for favicons that are not currently loaded, but might be available via network, and have been lost by whatever method they are currently *not* being loaded.

The favicon if available, should already exist(?). Calls to the network should be rare.

Bug 1016686 (a pre-req to this bug) moves both types of LoadFaviconTask(s) off the GeckoBackgroundThread, to the newSingleThreadExecutor (DB-only, and network).

With this patch, loadFaviconTask(s) generated from getSizedFaviconForPageFromLocal() (always requested "onlyFromLocal") are moved back to GeckoBackgroundThread where they started, and mingle with other Backgroundthread tasks as before but in a less intrusive manner, as DB look-ups don't block anywhere near as long as network.

On failing the DB lookup, the patch then has the task listener load the defaultIcon, and call loadUncachedFavicon() which generates an additional LoadFaviconTask to the network, now segregated on the newSingleThreadExecutor. If this fails, we're no worse than we started. If it succeeds, we display the favicon, and persist it to the DB for later, avoiding future network tasks.

DB persistence is assumed, as new items added in this manner were initially determined to be persistable, based on the manner their page was displayed in BrowserApp.loadFavicon() which observes tab-type privacy state.

That's the plan. Now, help keep me honest :-)

Comment 3

[Richard Newman [:rnewman]]

(In reply to Mark Capella [:capella] from comment #2)

> The favicon if available, should already exist(?). Calls to the network
> should be rare.

Not if you're a Sync user. Sync doesn't sync favicons.

And be careful of fingerprinting -- fetching a bunch of favicons at the same time as a side-effect of scrolling through history pretty much leaks your (synced) history to the network you're on.

Comment 4

[Mark Capella [:capella]]

Closing this concept patch. It doesn't lead anywhere that other patches don't address.