Open Bug 522855 Opened 10 years ago Updated 5 years ago

Use the favicon service for the taskbar preview icon

Categories

(Firefox :: Shell Integration, enhancement)

enhancement
Not set

Tracking

()

People

(Reporter: mak, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(14 obsolete files)

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
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 3.7a1
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.
Attachment #407145 - Flags: review?(mak77)
Attachment #407145 - Attachment description: Refactor v1.0 → Refactor DoSetAndLoadFaviconForPage v1.0
Attached patch Refactor Async Callbacks v1.0 (obsolete) — Splinter Review
Part 2 - Refactor our asynchronous callbacks so they all use the same error handling.
Attachment #407159 - Flags: review?(mak77)
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.
Attachment #407181 - Flags: review?(mak77)
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.
Assignee: sdwilsh → nobody
Status: ASSIGNED → NEW
I had changed the location of this code earlier.  This version will apply and allow the later patches to apply as well.
Attachment #407145 - Attachment is obsolete: true
Attachment #407309 - Flags: review?(mak77)
Attachment #407145 - Flags: review?(mak77)
Assignee: nobody → mak77
Unbitrot and some more cleanup.
Attachment #407309 - Attachment is obsolete: true
Attachment #408489 - Flags: review?(sdwilsh)
Attachment #407309 - Flags: review?(mak77)
unbitrot.
Attachment #407159 - Attachment is obsolete: true
Attachment #408492 - Flags: review+
Attachment #407159 - Flags: review?(mak77)
unbitrot, fixed a missing early return in handleCompletion (don't read icon data if they're still valid), removed some leftover unused code.
Attachment #407181 - Attachment is obsolete: true
Attachment #408589 - Flags: review?(sdwilsh)
Attachment #407181 - Flags: review?(mak77)
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.
Attachment #410258 - Flags: review?(sdwilsh)
same as above... notice there could be some misalignment since i put these up quickly
Attachment #410265 - Flags: review?(sdwilsh)
Attachment #410265 - Flags: review?(sdwilsh) → review+
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...
Attachment #408589 - Flags: review?(sdwilsh) → review+
i'll recheck it.
Attachment #410258 - Attachment is obsolete: true
Attachment #410652 - Flags: review?(sdwilsh)
Attachment #410258 - Flags: review?(sdwilsh)
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
Attachment #410652 - Flags: review?(sdwilsh) → review+
Attachment #408489 - Flags: review?(sdwilsh)
Attachment #408489 - Flags: review+
Attachment #410265 - Attachment is obsolete: true
Attachment #410652 - Attachment is obsolete: true
Attachment #408489 - Attachment is obsolete: true
Attachment #408492 - Attachment is obsolete: true
Attachment #408589 - Attachment is obsolete: true
Attached patch part2: refactor async callbacks (obsolete) — Splinter Review
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.
Attachment #419577 - Flags: review?(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
Attachment #419577 - Flags: review?(sdwilsh) → review+
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.
Depends on: 540765
Whiteboard: [waiting for bug 540765]
Depends on: 553489
No longer depends on: 540765
Whiteboard: [waiting for bug 540765]
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).
Attachment #419577 - Attachment is obsolete: true
Attachment #411645 - Attachment is obsolete: true
Attachment #411644 - Attachment is obsolete: true
Attachment #411643 - Attachment is obsolete: true
(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:.
Depends on: 573638
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.
Assignee: mak77 → nobody
Depends on: 619233
Severity: normal → enhancement
Target Milestone: Firefox 3.7a1 → ---
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.