Bug 522416 has changed the icon code using an async request, but we should use the favicon service, because it will avoid more http requests, and it will work also in offline mode. i had implemented a first mock here http://mozilla.pastebin.com/m365fe0b4, but we can do better using new NetUtil stuff, and we need a notification from the favicon service when an icon is available, or a favicon service async fetch method that will call a callback when the favicon is available. Actually the only possible way to listen to favicon notification is to observe onPageChanged, but adding an history observer just for that would perform bad.

we could probably add a callback param to setAndLoadFaviconForPage and add a further notification after onLinkIconAvailable, like onLinkIconStored

even if looking in mxr onLinkIconAvailable does not look like so much used as it is

Going to do this in parts so changes are small and simple to understand. Part 1 is a simple logic refactoring. We are going to pull some logic out of DoSetAndLoadFaviconForURI so the method isn't quite so long.

Part 2 - Refactor our asynchronous callbacks so they all use the same error handling.

Part 3 - This makes the expiration lookup asynchronous, and once the results are gotten, it loads the data as we used to. In the [common] case where we already have a favicon, we'll do a synchronous lookup to see what the current favicon URI is for a page, but we can fix that in a later patch.

Talked this over with Marco. I've got other projects I need to focus on, so he'll probably be driving this the rest of the way. This turned into a much bigger project very quickly.

I had changed the location of this code earlier. This version will apply and allow the later patches to apply as well.

Unbitrot and some more cleanup.

unbitrot.

unbitrot, fixed a missing early return in handleCompletion (don't read icon data if they're still valid), removed some leftover unused code.

Could you provide an interdiff between my patches and yours? I'm finding it hard to review this without that.

sure, generating them.

sorry but hg-interdiff does not work (or better, it works but returns an empty diff), so this is the patch applied on top of your patch. something better then pure patch.

same as above... notice there could be some misalignment since i put these up quickly

Comment on attachment 410265 [details] [diff] [review] Make expiration lookup async v1.1 - interdiff r=sdwilsh

(In reply to comment #13) > sorry but hg-interdiff does not work (or better, it works but returns an empty > diff), so this is the patch applied on top of your patch. something better then > pure patch. Did you change the newlines or something? This diff is really strange...

i'll recheck it.

after review these will also need a small unbitrot, due to landing of bug 526920. i have the patches locally but i don't want to pollute this more till i get the last review.

Comment on attachment 410652 [details] [diff] [review] Refactor DoSetAndLoadFaviconForPage v1.2 - interdiff r=sdwilsh

part1: http://hg.mozilla.org/mozilla-central/rev/4ac5c905ccf1 part2: http://hg.mozilla.org/mozilla-central/rev/e96353cf7500 part3: http://hg.mozilla.org/mozilla-central/rev/06decd4aee8f part4 will be to provide a notification or callback to DoSetAndLoadFaviconForPage.

pushed a linux bustage fix: http://hg.mozilla.org/mozilla-central/rev/fa1c13d58b85

Comment on attachment 419577 [details] [diff] [review] Part4: lazy statements creation (and minor cleanup) >+ if (pb) { >+ pb->GetIntPref("places.favicons.optimizeToDimension", >+ &mOptimizedIconDimension); not checking return, so cast to void. > mozIStorageStatement *stmts[] = { >+ GetStatement(mDBRemoveOnDiskReferences) >+ , GetStatement(mDBRemoveTempReferences) >+ , GetStatement(mDBRemoveAllFavicons) pretty sure trailing commas here is fine (even the last one) > mozIStorageStatement* stmts[] = { >+ mDBGetURL >+ , mDBGetData >+ , mDBGetIconInfo >+ , mDBInsertIcon >+ , mDBUpdateIcon >+ , mDBSetPageFavicon >+ , mDBRemoveOnDiskReferences >+ , mDBRemoveTempReferences >+ , mDBRemoveAllFavicons > }; ditto r=sdwilsh

fixed comments: http://hg.mozilla.org/mozilla-central/rev/ed1e9fa499b5 bah, i forgot to put bug number in the hg commit message :\ an hook would have saved me.

actually the only issue with this could be that the page could not be addable to history (an about: page liek about:robots for example), for these pages we would not get an icon from favicons service, and it won't tell us that it does not have an icon (we could maybe make it fire an empty notification and fetch from network in such a case).

(In reply to comment #29) > actually the only issue with this could be that the page could not be addable > to history (an about: page liek about:robots for example), for these pages we > would not get an icon from favicons service, and it won't tell us that it does > not have an icon (we could maybe make it fire an empty notification and fetch > from network in such a case). If we use moz-anno: for this, we'll get the default icon in this case which is what we want, right?

(In reply to comment #30) > If we use moz-anno: for this, we'll get the default icon in this case which is > what we want, right? I think we want a proper icon, about:robots for example has its own icon that is pretty valid (this is an edge case though). Also in PB mode new pages won't have an icon, same if history is disabled, and so on. Also, we need to know when the icon has been added to the db, we can know that through a notification that can be fired at browser level by setAndLoadFaviconForPage, can't be done with moz-anno:.

filed Bug 573638, we need an async protocol that fetches from the db and if the icon is not in the db from the network.