350006 - favicon not listed in media tab of Page Info

Status: NEW

(Firefox :: Page Info Window, enhancement)

Description

[Robert Rothenberg]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.8.0.5) Gecko/20060731 Ubuntu/dapper-security Firefox/1.5.0.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.8.0.5) Gecko/20060731 Ubuntu/dapper-security Firefox/1.5.0.5

When one visits a page on a site with no default icon explicitly mentioned on the page, the default icon is not listed in the Media tab of the page info. It should be (since it is an image that is being displayed as part of the page).

Perhaps the media tab could change the color of the URL of the favorite icon to note that it is not being explicitly used in a page?





Reproducible: Always

Steps to Reproduce:
1. Visit a page on a site where the favorite icon is at the root and is called
   favicon.ico but is not explicitly given in the header (e.g. http://www.cnn.com)
2. View Page Info (Ctrl-I).
3. Media tab does not show the favorite icon.



Expected Results:  
URL of favicon.ico should be listed.

Comment 1

[Philip Chee]

This should be either WONTFIX or controlled by the "browser.chrome.favicons" preference.

Comment 2

[Thomas D. (:thomas8)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090826 Namoroka/3.6a2pre

The "wontfix?" nomination on whiteboard should be removed, as this bug was already confirmed, and is currently marked "new". It's also still present, and it looks perfectly valid. Wontfix recommendation was based on a misunderstanding.

Philip, I think you misunderstood. The preference "browser.chrome.favicons" doesn't have much to do with this bug: It's not about favicons not showing on tabs, this is about a missing entry in the list of all media belonging to the page (Tools > Page Info > Media). Currently, a global favicon.ico (from the server root) is not listed there unless it is explicitly referenced in the page markup. I think it's beyond doubt that even a global favicon forms part of the current page and therefore should be listed in the media list, just like any other external image that belongs to the page.

Comment 3

[Daniel]

Attachment: Patch #1

- Add the favicon to "Page Info" when the link is not stated explicitly in the page source.

- Ensure the favicon is only displayed once.

Comment 4

[Daniel]

Comment on attachment 433705 [details] [diff] [review]
Patch #1

Hi Gavin,

To my knowledge, this "Page Info" utility is likely to be in "Toolkit" section. Therefore I'm asking for your review.

Daniel

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 433705 [details] [diff] [review]
Patch #1

>@@ -550,7 +558,7 @@
> 
> function addImage(url, type, alt, elem, isBg)
> {
>-  if (!url)
>+  if (!url || gImageHash[url] != null)
>     return;
> 
>   if (!gImageHash.hasOwnProperty(url))

The addImage function keeps a count of how many time each image appears in the page. This count is visible in the "Count" column of the tree (see bug 201264 for details).

If I understood correctly how favicons work, the file favicon.ico is used only if there's no link tag indicating the URL of the favicon of the page, so you should probably test that instead of adding the favicon.ico file unconditionally to the list of images.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 433705 [details] [diff] [review]
Patch #1

>diff -r ca15f0ccd62c browser/base/content/pageinfo/pageInfo.js

>+  /* add favicon */
>+  let documentURI = gDocument.location.toString();
>+  let rootURI = documentURI.substring(0, 
>+            documentURI.indexOf("/", documentURI.indexOf("://") + 4) + 1);
>+  let faviconURI = rootURI + "favicon.ico";

This should probably just use gDocument.documentURIObject.prePath + "/favicon.ico" instead.

Though that makes me wonder whether we just want to query the favicon service instead, since it should already have the right icon loaded. I think getFaviconLinkForIcon is what we'd want here, but I'm not sure. We probably actually want to avoid doing anything if the page has no favicon, as Florian points out... I'm not sure the favicon service supports those kinds of queries. Marco knows the API better (I've CCed him).

>+  addImage(faviconURI, gStrings.mediaLink, "", faviconURI, true);

Why are you passing faviconURI as "elem"? And "true" for isBg? Doesn't look right...

> function addImage(url, type, alt, elem, isBg)

Comment 7

[Philip Chee]

I'm extremely dubious about displaying the favicon in PageInfo, however why can't we get it from the image that is already in the tabbrowser tab and/or the locationbar?

Also we can probably optimize this slightly by not bothering if the webpage already has a <link rel="icon"> since the website operator then probably doesn't expect you to be checking for /favicon.ico.

Comment 8

[Thomas D. (:thomas8)]

Gavin, would you mind commenting on favico bug 216907 where a privacy issue has been completely ignored for more than a year? I really wonder: Why am I still trying to help improve your product if I don't get any replies whatsoever when raising a security/privacy related problem? Is there any mechanism in place to ensure such bugs get at least noticed and commented?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> can't we get it from the image that is already in the tabbrowser tab and/or the
> locationbar?

> Also we can probably optimize this slightly by not bothering if the webpage
> already has a <link rel="icon">

Using the favicon service would make both of these points moot.

Comment 10

[Daniel]

Attachment: Patch #2

Add favicon information only when the page doesn't provide a explicit link to the favicon in its source.

If we already have the favicon cached, use the favicon obtained from faviconService. Otherwise, fall back to the default "favicon.ico".

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 436648 [details] [diff] [review]
Patch #2

>diff -r ca15f0ccd62c browser/base/content/pageinfo/pageInfo.js

>+  try {
>+    faviconURI = faviconService.getFaviconForPage(pageURI);
>+        

I hope this thing can be done async in future (thus adding the entry to the media list async), since this API is most likely going to be changed to be async with a callback (like getFaviconForPage(pageURI, callback)) not sure when it will happen but not too far.

>+  } catch (ex) { /* we don't have this favicon in the cache, fall back
>+                            to the default favicon link */

this comment style looks completely out of place. Never seen a multi line comment hanging out in a side
Actually i'm not even sure why this file uses multiline comments style for single line comments, can we use the correct style in new code, please?

>+
>+    faviconURI = makeURI(gDocument.documentURIObject.prePath +
>+                            "/favicon.ico");
>+    
>+    /* load and store the favicon in the cache */                            
>+    faviconService.setAndLoadFaviconForPage(pageURI, faviconURI, false);

This is async (Actually it will wait 3s before fetching the icon due to LAZY_ADD), and could fail. If we have never seen the page or history is disabled or we are in pb mode the icon won't be loaded. in this case you should probably add the icon to your page only if this succeeds, thus if you get back a callback from the API (faviconService.setAndLoadFaviconForPage(pageURI, faviconURI, false, callback), where callback is nsIFaviconDataCallback, right now it is called only in case of success, i'm thinking to change that to fire a callback with null params in case of cancel)

>+  if (!faviconCached)
>+    /* get the new favicon from the cache */
>+    faviconURI = faviconService.getFaviconForPage(pageURI);

if without braces with 2 lines after it, bad.

>+  if (gImageHash[faviconURI.spec][count] == 0) {
>+    /* Favicon stored under chrome://mozapps/skin/places/ cannot be
>+     * displayed in the "Make preview" panel
>+     */
>+
>+    var faviconElem = new Image(); // create a new HTMLImageElement
>+    faviconElem.src = faviconURI.spec;

i think that if the icon is in faviconService you want to use moz-anno protocol like moz-anno:icon:uri_of_the_page and you'll get back your favicon async or the default icon it the icon is unknown (but at that point there is no reason to show the entry).

There are trailing spaces all around (on all newlines at least) and indentation is fancy.

So, looks like there are bigger issues, if we can improve favicons service API let me know your ideas

Comment 12

[Tanner M. Young [:tmyoung]]

What is the status on this patch?

Comment 13

[Daniel]

Hi everyone,

I'm trying to tackle this one more time. I noticed that there was a significant change to the nsIFaviconService. There are now asynchronous alternatives in mozIAsyncFavicons to the removed synchronous ones in nsIFaviconService.

The problem is I don't know how to access the asynchronous methods in mozIAsyncFavicons. From the documentation for mozIAsyncFavicons, it seems that nsIFaviconService is the only service that implements this interface. So do I just access the asynchronous methods through an instance of nsIFaviconService or are there any other services that implement mozIAsyncFavicons?

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Daniel: you can now direct your query to a person by specifying the name of him/her in the needinfo? textfield entry.

For example, I've set needinfo? from Marco for you. (whom I presume might be able to answer the question partly due to comment 11)

Comment 15

[Daniel]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #14)
> Daniel: you can now direct your query to a person by specifying the name of
> him/her in the needinfo? textfield entry.
> 
> For example, I've set needinfo? from Marco for you. (whom I presume might be
> able to answer the question partly due to comment 11)

Yup, that's exactly what I wanted to do. Thanks Gary.

Comment 16

[Marco Bonardo [:mak]]

(In reply to Daniel from comment #13)
> 
> The problem is I don't know how to access the asynchronous methods in
> mozIAsyncFavicons. From the documentation for mozIAsyncFavicons, it seems
> that nsIFaviconService is the only service that implements this interface.
> So do I just access the asynchronous methods through an instance of
> nsIFaviconService or are there any other services that implement
> mozIAsyncFavicons?

just use PlacesUtils.favicons getter from JS.

Comment 17

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

The bug assignee is inactive on Bugzilla, so the assignee is being reset.