The default bug view has changed. See this FAQ.

about: pages do not have favicons on the thumbnails

VERIFIED FIXED in Firefox 6

Status

Firefox Graveyard
Panorama
--
minor
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: AlexLakatos, Assigned: raymondlee)

Tracking

Trunk
Firefox 6
Bug Flags:
in-testsuite ?

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 525389 [details]
screenshot

about: pages (about:home, about:robots) do not have favicons on the thumbnails in Panorama, even if in Tab View they have.

Steps to Reproduce:
1.Open about:home
2.Enter Panorama View

Actual Results:
2.The thumbnail showing about:home does not have a favicon on it

Expected Results
2.2.The thumbnail showing about:home has a favicon on it
(Assignee)

Updated

6 years ago
Depends on: 627966

Comment 1

6 years ago
I can confirm.

It appears to be this way for any page that does not have a favicon associated with it (?)
(Reporter)

Comment 2

6 years ago
No, any other page without a favicon has a "white page" as its favicon in Panorama.
(Assignee)

Comment 3

6 years ago
Created attachment 526699 [details] [diff] [review]
v1
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #526699 - Flags: feedback?(tim.taubert)
Attachment #526699 - Attachment is patch: true
Attachment #526699 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 526699 [details] [diff] [review]
v1

I just tried this patch and it works for "about:home" but not for "about:robots" (maybe this one is special because the icon is an animated PNG and embedded as a base64 string). These both have no favicon when they're pinned.
Attachment #526699 - Flags: feedback?(tim.taubert) → feedback-
(Assignee)

Comment 5

6 years ago
Created attachment 527034 [details] [diff] [review]
v2

Please apply the patch for bug 627966 and the this patch
Attachment #526699 - Attachment is obsolete: true
Attachment #527034 - Flags: feedback?(tim.taubert)
Version: unspecified → Trunk
Comment on attachment 527034 [details] [diff] [review]
v2

The overall approach looks good but we should clean up the code and remove code duplicates:

>diff --git a/browser/base/content/tabview/groupitems.js b/browser/base/content/tabview/groupitems.js
>+    let browser = xulTab.linkedBrowser;
>+    let iconUrl;
>+    if (gBrowser.shouldLoadFavIcon(browser.contentDocument.documentURIObject) ||
>+        browser.currentURI.schemeIs("about")) {
>+      if (xulTab.image) {
>+        iconUrl = xulTab.image;
>+        if (/^https?:/.test(iconUrl))
>+          iconUrl = "moz-anno:favicon:" + iconUrl;
>+      } else {
>+        iconUrl = gFavIconService.getFaviconImageForPage(browser.currentURI).spec;
>+      }
>+    } else {
>+      iconUrl = Utils.defaultFaviconURL;
>+    }

>@@ -2034,18 +2046,30 @@ let GroupItems = {
>+    let browser = xulTab.linkedBrowser;
>+    let iconUrl;
>+    if (gBrowser.shouldLoadFavIcon(browser.contentDocument.documentURIObject) ||
>+        browser.currentURI.schemeIs("about")) {
>+      if (xulTab.image) {
>+        iconUrl = xulTab.image;
>+        if (/^https?:/.test(iconUrl))
>+          iconUrl = "moz-anno:favicon:" + iconUrl;
>+      } else {
>+        iconUrl = gFavIconService.getFaviconImageForPage(browser.currentURI).spec;
>+      }
>+    } else {
>+      iconUrl = Utils.defaultFaviconURL;
>+    }

These two places do exactly the same, don't they? Let's merge them into one function. The if-condition looks pretty similar to what TabItem.shouldLoadFavicon() does - so shouldn't we rather use this function instead of duplicating the code here?

>diff --git a/browser/base/content/tabview/tabitems.js b/browser/base/content/tabview/tabitems.js
>+        let iconUrl;
>+        if (tab.image) {
>+          iconUrl = tab.image;
>+          if (/^https?:/.test(iconUrl))
>+            iconUrl = "moz-anno:favicon:" + iconUrl;
>+        } else {
>+          iconUrl = 
>+            gFavIconService.getFaviconImageForPage(tab.linkedBrowser.currentURI).spec;
>+        }

This looks a bit like the two diffs above, maybe this could be merged into this function, too?

F+ with those issues addressed.
Attachment #527034 - Flags: feedback?(tim.taubert) → feedback+
(Assignee)

Comment 7

6 years ago
Created attachment 527462 [details] [diff] [review]
v3

Merged some duplicate code
Attachment #527034 - Attachment is obsolete: true
Attachment #527462 - Flags: review?(ian)
Comment on attachment 527462 [details] [diff] [review]
v3

Review of attachment 527462 [details] [diff] [review]:

Looks good!
Attachment #527462 - Flags: review?(ian) → review+
(Assignee)

Comment 9

6 years ago
Created attachment 529661 [details] [diff] [review]
Patch for checkin

Please land this after bug 627966 has landed
Attachment #527462 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/projects/cedar/rev/939ccc994fba
Keywords: checkin-needed
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → Firefox 6
http://hg.mozilla.org/mozilla-central/rev/939ccc994fba
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]

Comment 12

6 years ago
Verified Fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110509 Firefox/6.0a1
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.