403651 - for pages that are images, store a scaled (16 x 16 px) and resampled version of the image as the favicon

Status: REOPENED

(Firefox :: Bookmarks & History, enhancement, P5)

Description

[(not reading, please use [e-mail] instead)]

for pages that are images, store a scaled (16 x 16 px) and resampled version of the image as the favicon

from the code (http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsFaviconService.cpp#452)

  // If have an image loaded in the main frame, that image will get set as its
  // own favicon. It would be nice to store a resampled version of the image,
  // but that's prohibitive for now. This workaround just refuses to save the
  // favicon in this case.
  PRBool pageEqualsFavicon;
  rv = page->Equals(aFavicon, &pageEqualsFavicon);
  NS_ENSURE_SUCCESS(rv, rv);
  if (pageEqualsFavicon)
    return NS_OK;

this sounds like a job for dolske's code in bug #389273

Comment 1

[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

Comment 2

[Felix Fung (:felix)]

I've been looking into this and know about how 50% of this could be done. Since bug 389273 has been fixed, calls to SetFaviconData (amongst others) will optimize images before saving them to the database. However, we still short-circuit images from being saved here:

(from toolkit/components/places/AsyncFaviconHelpers.cpp)
--- In AsyncFetchAndSetIconForPage::start
>  // If the page url points to an image, the icon's url will be the same.
>  // In future evaluate to store a resample of the image.  For now avoid that
>  // for database size concerns.
>  // Don't store favicons for error pages too.
>  if (icon.spec.Equals(page.spec) ||
>      icon.spec.Equals(FAVICON_ERRORPAGE_URL)) {
>    return NS_OK;
>  }

So naively, if get rid of the short-circuit and voila you'll see thumbnails in your bookmarks. However, AsyncFetchAndSetIconForPage will as the name implies send another request for the image rather than reusing the document itself. Relying on the browser cache for this is pretty gross and doesn't always work. So what we should do is in ImageDocument.cpp wait for the request to complete and call nsFaviconService::SetFaviconData which will store the icon. Then AsyncFetchAndSetIconForPage will just call AsyncAssociateIconToPage that will reuse the data stored in SetFaviconData instead of opening a new request.

I'm stuck on how to get the raw request data (SetFaviconData takes a pointer to the binary image data and the image length) in ImageDocument.cpp...

Comment 3

[Felix Fung (:felix)]

Attachment: For ImageDocuments Store Thumb As Favicon

Summary of Changes:
- Once an image document has finished decoding the image, we pass the pointer to the image to the favicon service which will copy and resample the image as necessary.
- When an image page is saved as a bookmark, it will then find the icon stored in the favicondb and not have to refetch.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 573956 [details] [diff] [review]
For ImageDocuments Store Thumb As Favicon

Review of attachment 573956 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/document/src/ImageDocument.cpp
@@ +584,5 @@
> +
> +    nsCOMPtr<nsIFaviconService> favSvc =
> +      do_GetService("@mozilla.org/browser/favicon-service;1", &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    favSvc->SetFaviconData(mDocumentURI, data, dataLen, mimeType, 0);

This is mainthread IO, you really don't want to do this.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So, provided you have a way to recognize when an image is going to be used as a favicon, you should add an async method to mozIAsyncFavicons to set its data. The old favicons interface will be deprecated asap.

You also have to check with care you are not creating large long-living image buffers, iirc someone was discussing about that in another bug, but don't remember it atm...

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

note that, once you have an async method, all changes are serialized, so calling SetFaviconDataAsync (whatever is named) before the usual favicon call, will serialize all changes.

Comment 7

[Felix Fung (:felix)]

Attachment: For ImageDocuments Store Thumb As Favicon

WORK IN PROGRESS

Additional Changes:
- New nsFaviconService API call that takes an imgIContainer instead
-- (Otherwise we end up re-encoding and re-decoding the image.)

This still isn't async yet, but will be...

Comment 8

[Felix Fung (:felix)]

Attachment: For ImageDocuments Store Thumb As Favicon

- Made it use mozIAsyncFavicons::SetFaviconDataAsync

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 574853 [details] [diff] [review]
For ImageDocuments Store Thumb As Favicon

Review of attachment 574853 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/document/src/ImageDocument.cpp
@@ +574,5 @@
> +      do_GetService("@mozilla.org/browser/favicon-service;1", &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = favSvc->SetFaviconImageContainer(mDocumentURI, imgContainer, 0);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }

So, there is something to discuss about this.
First it should be wrapped in #ifdef moz_places
Then you should first try to get the favicon service, and if that succeeds proceed with the rest. Getting the service may fail but that should not interrupt this code, so manage errors appropriately to proceed with whatever this code was trying to do.

But the mail problem is not one of these, the problem is that some Places implementers may not use favicons, just thinking of Thunderbird for example that only relies on link coloring, but with this change opening an image document in Thunderbird will store a favicon for it, that then will have to be discarded. It's not an easy problem, since ideally the task to add a favicon for this kind of document should be of browser, not of content.
If we could make the existing path from browser more performant (maybe by passing the imagecontainer or whatever avoids multiple decodes/encodes) that would be the way to go, so who is not a browser can avoid caring about these.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

s/mail/main/ obviously :)

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

If there's no way to go through browser, in bug 699843 I suggested an alternative, that is to temporarily store data for nonexisting icons in a memory hash, so they are discarded without causing IO.

Comment 12

[Justin Dolske [:Dolske]]

Hmm. UX was actually talking about just not using scaled favicons for images (since they're generally not very useful, scaling down to 16x16 just mangles most images into an unrecognizable tiny blob).

Comment 13

[Felix Fung (:felix)]

I kinda remember that on IRC. I think the contrary argument is that those are useful semi-recognizable blobs!

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 574853 [details] [diff] [review]
For ImageDocuments Store Thumb As Favicon

clearing request while the favicons service changes are being worked on.

Comment 15

[Justin Dolske [:Dolske]]

Comment on attachment 574853 [details] [diff] [review]
For ImageDocuments Store Thumb As Favicon

Review of attachment 574853 [details] [diff] [review]:
-----------------------------------------------------------------

I'm actually still not sure if we should do this for image documents (pre previous comment), but since you wrote some code I'll review it. :)

::: toolkit/components/places/mozIAsyncFavicons.idl
@@ +187,5 @@
> +   * @throws NS_ERROR_FAILURE
> +   *         Thrown if the favicon is overbloated and won't be saved to the db.
> +   */
> +  void setFaviconImageContainer(in nsIURI aFaviconURI,
> +                                in imgIContainer aImgContainer,

Hmm. I don''t feel super strongly about this, but I'd prefer to pass in an nsIDOMHTMLImageElement (and internally get at the container). imgIContainers are a little odd to pass around through interfaces.

::: toolkit/components/places/nsFaviconService.cpp
@@ +435,5 @@
> +                                           PRTime aExpiration) {
> +  NS_ENSURE_ARG(aFaviconURI);
> +  NS_ENSURE_ARG(aImgContainer);
> +
> +  nsCString newData, newMimeType;

nsCAutoString, will just use stack storage when the data's < 64 bytes.

@@ +443,5 @@
> +  const PRUint8* data = reinterpret_cast<PRUint8*>(const_cast<char*>(newData.get()));
> +  PRUint32 dataLen = newData.Length();
> +
> +  rv = AsyncSetFaviconData::start(
> +    aFaviconURI, data, dataLen, newMimeType, aExpiration, nsnull);

Couple of potential problems here:

If AsyncSetFaviconData() isn't immediately copying the data from data/dataLen, then that pointer will become invalid when |newData| is deallocated.

If it is copying the data, that sucks because it's copying a lot of data. Oh, well, it's generally scaled to 16x16 so that's not likely.

Might need to pass the xpcom image object (aImgContainer, or the HTML image per previous comment) into AsyncSetFaviconData() so it can hold a reference until it's ready to read the bits.

@@ +995,5 @@
> +  if (aImgtool) {
> +    imgtool = do_QueryInterface(aImgtool);
> +  } else {
> +    imgtool = do_CreateInstance("@mozilla.org/image/tools;1");
> +  }

Eww. :) Don't pass this in as an argument, hoist it up to be a global.

Or maybe modernize things by adding it to xpcom/build/ServiceList.h (separate bug), so you can just use mozilla::services::GetImageTools().

Comment 16

[Felix Fung (:felix)]

I don't think it's too radical to say that, images should have some favicon that is not the NULL placeholder. The opposing argument then is to either have a scaled version or some placeholder favicon for images in general. So here's why I support showing a scaled version of an image.

1) When we open an image tab, the tab icon is that of the scaled image. It maintains consistency to see that tab icon match the bookmarks manager/dropdown w.e.

2) Note that these favicons are (almost guaranteed) to be shown to someone who has seen the image before. Hence, as a person having seen the image the first time, the 16x16 is very often sufficient for a person to recognize what the image is of.

Comment 17

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 18

[Dave Townsend [:mossop]]

Bug 1453751 is going to change things so that for now we just show the site's default (favicon.ico) icon for image documents. We could also easily implement the current state, in fact an earlier patch did that in a way that solves this bug. We could also do something else but we need to decide what that something else should be. Looking for UX guidance here...

Comment 19

[Daniel Holbert [:dholbert]]

I'm not sure about the exact APIs involved here, but it's possible this Just Works via the imagelib "SurfaceCache" structure (which aims to cache scaled versions of images that have been drawn): https://searchfox.org/mozilla-central/source/image/SurfaceCache.h

(And if it doesn't already Just Work, we may want to try to implement it via the SurfaceCache, to share similar code/logic & make use of existing functionality that does approximately the right thing.)

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:mak, could you have a look please?
For more information, please visit auto_nag documentation.