317839 - Favicons

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

Store and display favicons for bookmarks and history.

Comment 1

[Brett Wilson]

We should only save 16x16 images, regardless of input. Some icons have lots of different images in them at different resolutions, some are animated. We should only extract the first part. See bug 175787

Comment 2

[Brett Wilson]

Attachment: Favicon service implementation

Comment 3

[Brett Wilson]

Attachment: Hook service into browser.js

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 205317 [details] [diff] [review]
Favicon service implementation

Drive by style review: 

>+[scriptable, uuid(6c60f476-4c8d-4046-a11c-6923d486178b)]
>+interface nsIFaviconService : nsISupports
>+{
>+  /**
>+   * Declares that a given page uses a favicon with the given URI.
>+   *
>+   * You needn't have specified any data at this point. An entry linking
>+   * the favicon with the page will be create with no data. You can populate
>+   * it later with SetFaviconData.
>+   */
>+  void SetFaviconUrlForPage(in nsIURI aPage, in nsIURI aFavicon);

use lower case for the first character of method names. 

>+  void SetFaviconData(in nsIURI aFavicon,
>+                      [const,array,size_is(aDataLen)] in octet aData,
>+                      in PRUint32 aDataLen, in AUTF8String aMimeType,
>+                      in PRTime aExpiration);

use IDL types where possible, e.g. unsigned long aDataLen vs. PRUint32.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 205320 [details] [diff] [review]
Hook service into browser.js

See my comments on the other patch for style comments... also:

>+#ifdef MOZ_PLACES
>+  // Save this favicon in the favicon service
>+  if (aURL) {
>+    var faviconService = Components.classes["@mozilla.org/browser/favicon-service;1"].
>+      getService(Components.interfaces.nsIFaviconService);
>+    var uri = Components.classes["@mozilla.org/network/io-service;1"]
>+                              .getService(Components.interfaces.nsIIOService)
>+                              .newURI(aURL, null, null);

Your following line indentation is weird. Line up the dots, or do something different ;-) 

>+    faviconService.SetAndLoadFaviconForPage(gBrowser.currentURI, uri, false);
>+  }
>+#endif

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 205317 [details] [diff] [review]
Favicon service implementation

>+  /**
>+   * Declares that a given page uses a favicon with the given URI.
>+   *
>+   * You needn't have specified any data at this point. An entry linking
>+   * the favicon with the page will be create with no data. You can populate
>+   * it later with SetFaviconData.
>+   */
>+  void SetFaviconUrlForPage(in nsIURI aPage, in nsIURI aFavicon);

Let's also all use param comments like this:

/**
 * Lead in comments
 * 
 * @param   foo
 *          Some description of foo
 *          ...
 * @returns Some description of the return value
 * @throws  NS_ERROR_GOAT
 *          Some description of the condition when the error condition 
 *          NS_ERROR_GOAT (specific to this function, aside from standard
 *          out-of-memory, or propagated errors) is thrown. 
 *          ...
 */

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 205320 [details] [diff] [review]
Hook service into browser.js

This browser change looks fine.

Comment 8

[Brett Wilson]

This initial patch punts on most of the current favicon problems. In previous cases where the favions were wrong/clipped/otherwise weird, this patch is mostly the same. In particular, if the favicon contains only a 32x32 version (like salon.com), you will get clipping in tree views.

Firefox considers the favicon of an image to be that image. So if you bookmark an image, that image (if its < 16K) will be stored in bookmarks. This service does not store favicons if the favicon is the same as the page itself, which works around this problem.

Comment 9

[Annie Sullivan]

Comment on attachment 205317 [details] [diff] [review]
Favicon service implementation

Just some minor nitpicks:

>+nsFaviconService::SetFaviconData(nsIURI* aFavicon, const PRUint8* aData,
>+                                 PRUint32 aDataLen, const nsACString& aMimeType,
>+                                 PRTime aExpiration)
>+{
>+  mozStorageStatementScoper scoper(mDBGetIconInfo);
>+  nsresult rv = BindStatementURI(mDBGetIconInfo, 0, aFavicon);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRBool hasResult;
>+  rv = mDBGetIconInfo->ExecuteStep(&hasResult);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (hasResult) {
>+    // update old one
>+    PRInt64 id;
>+    rv = mDBGetIconInfo->GetInt64(0, &id);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    mDBGetIconInfo->Reset();
>+    scoper.Abandon();
>+    mozStorageStatementScoper scoper2(mDBUpdateIcon);
>+
>+    rv = mDBUpdateIcon->BindInt64Parameter(0, id);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = mDBUpdateIcon->BindBlobParameter(1, aData, aDataLen);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = mDBUpdateIcon->BindUTF8StringParameter(2, aMimeType);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = mDBUpdateIcon->BindInt64Parameter(3, aExpiration);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    return mDBUpdateIcon->Execute();
>+
>+  } else {
>+    // insert new one
>+    mDBGetIconInfo->Reset();
>+    scoper.Abandon();
>+    mozStorageStatementScoper scoper2(mDBInsertIcon);
>+
>+    nsresult rv = BindStatementURI(mDBInsertIcon, 0, aFavicon);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = mDBInsertIcon->BindBlobParameter(1, aData, aDataLen);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = mDBInsertIcon->BindUTF8StringParameter(2, aMimeType);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = mDBInsertIcon->BindInt64Parameter(3, aExpiration);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    return mDBInsertIcon->Execute();
>+  }
>+}

Seems like you don't have to duplicate quite so much code here.


>+        expiration = (PRInt64)seconds * 1000000; 
...
>+    expiration += (PRInt64)(24 * 60 * 60) * (PRInt64)1000000;

Why not cast (PRInt64)1000000 in both places?

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 205317 [details] [diff] [review]
Favicon service implementation

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ browser/components/places/public/nsIFaviconService.idl	8 Dec 2005 19:50:03 -0000
>+  void SetAndLoadFaviconForPage(in nsIURI aPage, in nsIURI aFavicon,
>+    in boolean aForceReload);

Line up the second line with the start of the argument list in the first line.

>+  /**
>+   * Retrieves the given favicon data. Throws if we don't have data.
>+   *
>+   * If there is no data but we have an entry for this favicon, aDataLen will
>+   * be 0 and aData will be NULL.
>+   */
>+  void GetFaviconData(in nsIURI aFavicon,
>+                      [array,size_is(aDataLen)] out octet aData,
>+                      out PRUint32 aDataLen,
>+                      out AUTF8String aMimeType);
>+

Might be more natural to use this from JS if aData was |retval| ?

>+  /**
>+   * For a given page, this will give you a URI that, when displayed in chrome,
>+   * will result in the given page's favicon.
>+   *
>+   * For a normal page with a favicon we've stored, this will be an annotation
>+   * URI which will then cause the corresponding favicon data to be loaded from
>+   * this service. For pages where we don't have a favicon, this will be a
>+   * chrome URI of the default icon for a web page.
>+   */
>+  nsIURI GetFaviconLinkForPage(in nsIURI aPage);

GetFaviconImageForPage, maybe?  "Link" seems overloaded considering <link rel="icon">

>--- browser/components/places/src/nsAnnoProtocolHandler.cpp	17 Nov 2005 18:34:26 -0000	1.1
>+++ browser/components/places/src/nsAnnoProtocolHandler.cpp	8 Dec 2005 19:50:03 -0000
>+ * imglib could be told to use it's cached version from our NewChannel, but

s/it's/its/

>+  if (annoName.EqualsLiteral(FAVICON_ANNOTATION_NAME)) {
>+    // special handling for favicons: ask favicon service
>+    nsCOMPtr<nsIFaviconService> faviconService = do_GetService(
>+                                "@mozilla.org/browser/favicon-service;1", &rv);
>+    if (NS_FAILED(rv))
>+      return GetDefaultIcon(_retval);

This should at least warn if the service is unavailable, and maybe just return immediately with no icon.  Also, should this use GetFaviconService() instead?

>+    rv = faviconService->GetFaviconData(annoURI, &data, &dataLen, mimeType);
>+    if (NS_FAILED(rv))
>+      return GetDefaultIcon(_retval);
>+
>+    // don't allow icons with no MIME types

I know this was already here, but a quick nitpick for clarity:

"don't allow icons without MIME types"

>+nsresult
>+nsAnnoProtocolHandler::GetDefaultIcon(nsIChannel** aChannel)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIIOService> ioservice = do_GetIOService(&rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIURI> uri;
>+  rv = NS_NewURI(getter_AddRefs(uri), NS_LITERAL_CSTRING(FAVICON_DEFAULT_URL));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  return ioservice->NewChannelFromURI(uri, aChannel);
>+}

I think this can just use NS_NewChannel() for convenience.

>--- browser/components/places/src/nsBookmarksHTML.cpp	3 Dec 2005 23:35:47 -0000	1.2
>+++ browser/components/places/src/nsBookmarksHTML.cpp	8 Dec 2005 19:50:03 -0000
>+nsresult
>+BookmarkContentSink::SetFaviconForURI(nsIURI* aURI, nsCString aData)
>+{
>+  PRUint32 available;
>+  rv = stream->Available(&available);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (available == 0)
>+    return NS_ERROR_FAILURE;

I wonder if this should actually just read in a while() loop until all the data is consumed (numRead returns as 0).  I'm not sure all the data is guaranteed to be available in a single read.

more later.

Comment 11

[Darin Fisher]

+  // blocking stream is OK for data URIs
+  nsCOMPtr<nsIInputStream> stream;
+  rv = channel->Open(getter_AddRefs(stream));
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  PRUint32 available;
+  rv = stream->Available(&available);
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (available == 0)
+    return NS_ERROR_FAILURE;
+
+  // read all the decoded data
+  PRUint8* buffer = NS_STATIC_CAST(PRUint8*,
+                                   nsMemory::Alloc(sizeof(PRUint8) * available));
+  if (! buffer)
+    return NS_ERROR_OUT_OF_MEMORY;
+  PRUint32 numRead;
+  rv = stream->Read(NS_REINTERPRET_CAST(char*, buffer), available, &numRead);
+  if (NS_FAILED(rv) || numRead != available) {
+    nsMemory::Free(buffer);
+    return rv;
+  }
+
+  nsCAutoString mimeType;
+  rv = channel->GetContentType(mimeType);
+  NS_ENSURE_SUCCESS(rv, rv);

Looks like buffer is leaked if GetContentType returns failure.

Bryner is right.  Available isn't required to tell you the full length of the data stream (even though the stream returned from the data channel probably does).  I also recommend using a nsCString instead of allocating a byte array by hand.

It probably makes sense to have a function defined in xpcom/io/nsStreamUtils.h that can be used to read an entire blocking input stream into a nsCString (or a PRUint8 array if really desired).  Casting nsCString::get() to |const PRUint8*| is not such a bad idea IMO ;-)

I can help write this function if desired.

Comment 12

[Darin Fisher]

brettw: One nit-pick...

+  void SetFaviconUrlForPage(in nsIURI aPage, in nsIURI aFavicon);

Methods declared in XPIDL should always begin with a lowercase letter.  Otherwise, the method will not reflect to Javascript with a lowercase starting letter, which ends up going against Javascript coding conventions.

Comment 13

[Brian Ryner (not reading)]

Comment on attachment 205317 [details] [diff] [review]
Favicon service implementation

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ browser/components/places/src/nsFaviconService.cpp	8 Dec 2005 19:50:03 -0000
>+nsFaviconService::~nsFaviconService()
>+{
>+  if (gFaviconService == this)

Assert that it's equal.

>+nsresult
>+nsFaviconService::Init()
>+{
>+  nsresult rv;
>+
>+  nsNavHistory* historyService = nsNavHistory::GetHistoryService();
>+  NS_ENSURE_TRUE(historyService, NS_ERROR_NO_INTERFACE);

This seems like an odd error code to use. OUT_OF_MEMORY is more likely.

>+NS_IMETHODIMP
>+nsFaviconService::SetFaviconData(nsIURI* aFavicon, const PRUint8* aData,
>+                                 PRUint32 aDataLen, const nsACString& aMimeType,
>+                                 PRTime aExpiration)
>+{
>+  mozStorageStatementScoper scoper(mDBGetIconInfo);
>+  nsresult rv = BindStatementURI(mDBGetIconInfo, 0, aFavicon);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRBool hasResult;
>+  rv = mDBGetIconInfo->ExecuteStep(&hasResult);
>+  NS_ENSURE_SUCCESS(rv, rv);

Since you abandon the scoper in all cases here, maybe you should instead just scope it to this block?

>+NS_IMPL_ISUPPORTS3(FaviconLoadListener,
>+                   nsIStreamListener, // is a nsIRequestObserver

This doesn't automatically support base interfaces, so you need to explicitly list nsIRequestObserver.

>+FaviconLoadListener::~FaviconLoadListener()
>+{
>+  if (mData)
>+    delete[] mData;

This needs to use nsMemory::Free to match how it was allocated.

>+NS_IMETHODIMP
>+FaviconLoadListener::OnStopRequest(nsIRequest *aRequest, nsISupports *aContext,
>+                                 nsresult aStatusCode)
>+{
>+  if (NS_FAILED(aStatusCode)) {
>+    // load failed FIXME
>+    return NS_OK;
>+  }
>+
>+  // sniff the MIME type
>+  nsresult rv;
>+  nsCOMPtr<nsICategoryManager> categoryManager =
>+    do_GetService("@mozilla.org/categorymanager;1", &rv);

Use NS_CATEGORYMANAGER_CONTRACTID

>+  nsCOMPtr<nsISimpleEnumerator> sniffers;
>+  rv = categoryManager->EnumerateCategory("content-sniffing-services",

Might want to define a constant for this string somewhere.

>+  // extract the expiration time, if there is an error, make up an expiration
>+  // time of tomorrow
>+  PRTime expiration = -1;
>+  nsCOMPtr<nsICachingChannel> cachingChannel = do_QueryInterface(mChannel, &rv);
>+  if (NS_SUCCEEDED(rv)) {
>+    nsCOMPtr<nsISupports> cacheToken;
>+    rv = cachingChannel->GetCacheToken(getter_AddRefs(cacheToken));
>+    if (NS_SUCCEEDED(rv)) {
>+      nsCOMPtr<nsICacheEntryInfo> cacheEntry = do_QueryInterface(cacheToken, &rv);
>+      PRUint32 seconds;
>+      rv = cacheEntry->GetExpirationTime(&seconds);
>+      if (NS_SUCCEEDED(rv))
>+        expiration = (PRInt64)seconds * 1000000;

Maybe use PR_USEC_PER_SEC, for clarity?

>+  if (expiration < 0) {
>+    expiration = PR_Now();
>+    expiration += (PRInt64)(24 * 60 * 60) * (PRInt64)1000000;

Ditto.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ browser/components/places/src/nsFaviconService.h	8 Dec 2005 19:50:03 -0000
>+class nsFaviconService : public nsIFaviconService
>+{
>+public:
>+  nsFaviconService();
>+  nsresult Init();
>+
>+  // called by nsNavHistory::Init
>+  static nsresult InitTables(mozIStorageConnection* aDBConn);
>+
>+  // called by nsNavHistory::VacuumDB
>+  static nsresult VacuumFavicons(mozIStorageConnection* aDBConn);
>+
>+  /**
>+   * Returns a cached pointer to the favicon service for consumers in the
>+   * places directory.
>+   */
>+  static nsFaviconService* GetFaviconService()
>+  {
>+    if (! gFaviconService) {
>+      // note that we actually have to set the service to a variable here
>+      // because the work in do_GetService actually happens in "operator=" >:(
>+      nsresult rv;
>+      nsCOMPtr<nsIFaviconService> serv(do_GetService("@mozilla.org/browser/favicon-service;1", &rv));
>+      NS_ENSURE_SUCCESS(rv, nsnull);
>+
>+      // our constructor should have set the static variable. If it didn't,
>+      // something is wrong.
>+      NS_ASSERTION(gFaviconService, "Favicon service creation failed");
>+    }
>+    // the service manager will keep the pointer to our service around, so
>+    // this should always be valid even if nobody currently has a reference.
>+    return gFaviconService;
>+  }
>+
>+  // addition to API for strings to prevent excessive parsing of URIs
>+  nsresult GetFaviconLinkForIconString(const nsCString& aIcon, nsIURI** aOutput);
>+  void GetFaviconSpecForIconString(const nsCString& aIcon, nsACString& aOutput);
>+
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIFAVICONSERVICE
>+
>+
>+private:
>+  ~nsFaviconService();
>+
>+protected:

These may as well be "private", too.  Declaring a private destructor says "I don't want to be subclassed."

>--- browser/components/places/src/nsNavHistoryResult.cpp	3 Dec 2005 23:35:47 -0000	1.14
>+++ browser/components/places/src/nsNavHistoryResult.cpp	8 Dec 2005 19:50:03 -0000
> NS_IMETHODIMP nsNavHistoryResult::GetImageSrc(PRInt32 row, nsITreeColumn *col,
>                                               nsAString& _retval)
> {
...
faviconService->GetFaviconSpecForIconString(VisibleElementAt(row)->mFaviconURL, spec);
>+  _retval = NS_ConvertUTF8toUTF16(spec);

This is a bit more efficient, I think (avoids constructing a temporary):

CopyUTF8ToUTF16(spec, _retval);

Looks ok to me otherwise.

Comment 14

[Brett Wilson]

On trunk

Comment 15

[Brian Ryner (not reading)]

Comment on attachment 205317 [details] [diff] [review]
Favicon service implementation

(clearing obsolete request)

Comment 16

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv