Talk:Improve display of location bar results

The current Places database associates each moz_places entry with a single moz_favicon entry through a favicon_id column. To address bug 751712, we will need a way to store multiple favicons per moz_places entry. We also need to store which sizes each favicon is appropriate for according to the sizes attribute on its <link> tag.

Schema

I think the ideal way to do this is to add a separate moz_favicon_sizes table:

 CREATE TABLE moz_favicon_sizes (
   place_id INTEGER NOT NULL,
   width INTEGER NOT NULL,
   height INTEGER NOT NULL,
   favicon_id INTEGER NOT NULL,
   PRIMARY KEY(place_id, width, height)
 )

Each favicon will have an entry in this new table for each size it supports in the given moz_place. The existing favicon_id column in moz_places would go away[1] no longer be maintained, and you would instead join based on the place_id and favicon_id in moz_favicon_sizes to find favicons associated with a moz_places row.

[1] https://www.sqlite.org/lang_altertable.html "It is not possible to ... remove a column"

For example, assume we have a clean places database. If google.com has link tags like this:

 <link rel="icon" sizes="any" href="a.svg"/>
 <link rel="icon" sizes="16x16 32x32" href="b.ico"/>
 <link rel="icon" sizes="64x64" href="c.png"/>

The favicons will each get a row in moz_favicons with ids 1, 2, and 3 respectively. google.com has a row in moz_places with an id of 1. moz_favicon_sizes will end up like this:

To migrate from the current schema, we can add an entry to moz_favicon_sizes associating each moz_place entry with its moz_favicon for the 16x16 size. We'll need to update the expiration query to make sure it goes through the moz_favicon_sizes table: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsPlacesExpiration.js#244

Getting Favicons

Since we're trying to move away from the synchronous nsIFaviconService (bug 713642), we shouldn't worry about changing it to allow consumers to get/set specific sizes. Instead, the mozIAsyncFavicons interface should provide this functionality. To complement getFaviconURLForPage and getFaviconDataForPage, we should add these methods:

 void getAllFaviconURLsForPage(
   in nsIURI aPageURI,
   in nsIAllFaviconDataCallback aCallback
 );

 void getAllFaviconDataForPage(
   in nsIURI aPageURI,
   in nsIAllFaviconDataCallback aCallback
 );

And these helper interfaces:

 [scriptable, function, uuid(blah)]
 interface nsIAllFaviconDataCallback : nsISupports
 {
   void onComplete(in nsIArray faviconDescriptors);
 };

 [scriptable, uuid(blah)]
 interface nsIFaviconDescriptor : nsISupports
 {
   attribute readonly nsIURI faviconURI,
   attribute readonly unsigned long dataLen,
   attribute readonly [const,array,size_is(dataLen)] octet data,
   attribute readonly AUTF8String mimeType,
   attribute readonly short width,
   attribute readonly short height
 };

The getter methods take in a page URI and provide an object that will allow you to find the appropriate favicon URI/Data/mime for all sizes. The way I did it, this is just an array of {URI, data, mime, width, height}. A collection with a friendlier interface that will let you ask for specific sizes or provide other convenience methods might be better to use.

It's necessary to return the set of all associated favicons in order to allow consumers to choose the "best" icon for their purposes without hitting the database many times. For example, when the awesomebar needs 32x32 icons next to results, it will want to use an "any" size icon if available, then fall back to 32x32, then fall back to the next largest favicon, then fall back to the default icon. Other consumers may have other preferences, so I don't think it makes sense to only provide a method like "getBestFavicon" and assume it will work for everybody (though it might be helpful in the future).

Setting Favicons

The favicon service should contain a set of desired dimensions. By default, this could just be 16x16. This set can be changed by the consumer depending on its needs for favicons in the UI. When we want to start collecting "any" and 32x32 icons in Firefox for autocomplete results, it should be as simple as registering those dimensions with the service when Firefox starts up.

However, the service shouldn't guarantee that only favicons of the desired dimensions can make it into the database. Consumers are responsible for setting favicons it thinks are appropriate based on the set of desired dimensions. For example, whatever handles link tags will look at this attribute and set the "most appropriate" favicons it can find based on which ones we ideally want.

setAndFetchFaviconForPage should be changed to take an optional sizes string. This specifies which sizes the favicon is appropriate for so the method can associate the favicon properly. This method should behave as it did before, except the favicons are now associated to pages through the moz_favicon_sizes table for the given sizes. This string takes the same form as the sizes attribute on link tags. The sizes string is "16x16" by default for backwards compatibility.

 void setAndFetchFaviconForPage(
   in nsIURI aPageURI,
   in nsIURI aFaviconURI,
   in boolean aForceReload,
   [optional] in nsIFaviconDataCallback aCallback,
   [optional] in AString sizes
 );

A favicon set through this method should be optimized like before. However, if the favicon data is over the size limit, it should be resized to the dimensions they were indicated as appropriate for. The data size limit will need to be adjusted according to the appropriate dimensions. "any" size favicons use the data size limit appropriate for the largest desired dimension, and are resized to that largest dimension if they are over the limit. If bug 419588 gets fixed, I imagine we would need the ability to optimize each part of an ICO/ICNS file.

Backwards Compatibility

Existing methods will need to be updated to work with the new schema. setFaviconUrlForPage and setAndLoadFaviconForPage should assume that the consumer wants to set the 16x16 favicon associated with the page. To ensure that getFaviconForPage, getFaviconImageForPage, getFaviconURLForPage, and getFaviconDataForPage still work correctly, we can change them to assume that the user is looking for a 16x16 favicon and return the 16x16 URI that's associated with the page through the moz_favicon_sizes table. If there is no associated 16x16 favicon, it should try its best to find an appropriate favicon. This is necessary so these methods aren't broken on sites that only provide a 32x32 favicon, for example. The current API never guarantees that returned favicons must be 16x16. If we get a 17x17 favicon, it won't be resized as long as it's under the data size limit.

There are some things in our codebase that depend on the current database structure where each place is associated with one favicon. We could avoid breaking them by maintaining the old favicon_id column so it always points to the 16x16 favicon, but I think it's better to update these queries to behave properly with the new schema/API. Any extensions that muck with the database probably shouldn't be doing so in the first place since we provide an API. Here's are some places in mozilla-central that perform joins based on the old assumption:

- nsINavHistoryResultNode has an "icon" attribute, and it looks like that's always found by joining moz_places with the moz_favicons table:

 https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#104
 https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavBookmarks.cpp#1791
 https://mxr.mozilla.org/mozilla-central/search?string=moz_favicons&find=nsNavHistory.cpp&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

- nsIAutoCompleteResult has an "image" attribute, which is found like the nsINavHistoryResultNode:

 https://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsIAutoCompleteResult.idl#82
 https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsPlacesAutoComplete.js#1185
 https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsPlacesAutoComplete.js#270

I think we'll also have to update the moz-anno:favicon handler: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsAnnoProtocolHandler.cpp#380

Problems

Assume there's a 32x32 favicon at google.com/favicon.ico and the user hasn't visited google yet. Say the user visits malicioussite.biz which only has this link tag:

 <link rel="icon" sizes="2x2" href="google.com/favicon.ico"/>

The favicon file size will be larger than we expect for a 2x2 favicon, so it will be resized to 2x2 to optimize it. Then the user visits google.com, which only has this link tag:

 <link rel="icon" sizes="32x32" href="/favicon.ico"/>

Since google.com/favicon.ico is already in the database, we don't try to fetch it again. Consequently, google.com gets a yucky resized 2x2 favicon in the tab, and it will stay that way until the favicon expires or we do a force refresh.

We could almost resolve this by always fetching an icon again if it has a different size associated with it than we already know about on the page. That would mean malicioussite.biz could still mess up the favicon associated with google in things like awesomebar results and bookmarks, but it would be fixed once the user visits google's site with proper dimensions.

The schema results in at least one additional row in moz_favicon_sizes per moz_place that references a given favicon. We also have to join across three tables instead of two to reach a moz_favicon from a moz_place. This takes up more disk space and results in slower queries, but I'm not sure if this is a serious degradation.