522855 - Use the favicon service for the taskbar preview icon

Status: NEW

(Firefox :: Shell Integration, enhancement)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Marco Bonardo [:mak]]

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

Comment 2

[Marco Bonardo [:mak]]

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

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: Refactor DoSetAndLoadFaviconForPage v1.0

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.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: Refactor Async Callbacks v1.0

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

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: Make expiration lookup async v1.0

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.

Comment 6

[Shawn Wilsher :sdwilsh]

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.

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: Refactor DoSetAndLoadFaviconForPage v1.1

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

Comment 8

[Marco Bonardo [:mak]]

Attachment: Refactor DoSetAndLoadFaviconForPage v1.2

Unbitrot and some more cleanup.

Comment 9

[Marco Bonardo [:mak]]

Attachment: 407159: Refactor Async Callbacks v1.1

unbitrot.

Comment 10

[Marco Bonardo [:mak]]

Attachment: Make expiration lookup async v1.1

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

Comment 11

[Shawn Wilsher :sdwilsh]

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

Comment 12

[Marco Bonardo [:mak]]

sure, generating them.

Comment 13

[Marco Bonardo [:mak]]

Attachment: Refactor DoSetAndLoadFaviconForPage v1.2 - interdiff

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.

Comment 14

[Marco Bonardo [:mak]]

Attachment: Make expiration lookup async v1.1 - interdiff

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

Comment 15

[Shawn Wilsher :sdwilsh]

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

r=sdwilsh

Comment 16

[Shawn Wilsher :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...

Comment 17

[Marco Bonardo [:mak]]

i'll recheck it.

Comment 18

[Marco Bonardo [:mak]]

Attachment: Refactor DoSetAndLoadFaviconForPage v1.2 - interdiff

Comment 19

[Marco Bonardo [:mak]]

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 20

[Shawn Wilsher :sdwilsh]

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

r=sdwilsh

Comment 21

[Marco Bonardo [:mak]]

Attachment: part1: refactor DoSetAndLoadFaviconForPage

Comment 22

[Marco Bonardo [:mak]]

Attachment: part2: refactor async callbacks

Comment 23

[Marco Bonardo [:mak]]

Attachment: part3: make expiration lookup async

Comment 24

[Marco Bonardo [:mak]]

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.

Comment 25

[Marco Bonardo [:mak]]

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

Comment 26

[Marco Bonardo [:mak]]

Attachment: Part4: lazy statements creation (and minor cleanup)

Comment 27

[Shawn Wilsher :sdwilsh]

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

Comment 28

[Marco Bonardo [:mak]]

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.

Comment 29

[Marco Bonardo [:mak]]

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).

Comment 30

[Shawn Wilsher :sdwilsh]

(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?

Comment 31

[Marco Bonardo [:mak]]

(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:.

Comment 32

[Marco Bonardo [:mak]]

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.