649347 - about: pages do not have favicons on the thumbnails

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Alex Lakatos[:AlexLakatos]]

Attachment: 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

Comment 1

[Donovan M]

I can confirm.

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

Comment 2

[Alex Lakatos[:AlexLakatos]]

No, any other page without a favicon has a "white page" as its favicon in Panorama.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v2

Please apply the patch for bug 627966 and the this patch

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: v3

Merged some duplicate code

Comment 8

[Ian Gilman [:iangilman]]

Comment on attachment 527462 [details] [diff] [review]
v3

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

Looks good!

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Please land this after bug 627966 has landed

Comment 10

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/projects/cedar/rev/939ccc994fba

Comment 11

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/mozilla-central/rev/939ccc994fba

Comment 12

[AndreiD[QA]]

Verified Fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110509 Firefox/6.0a1