Last Comment Bug 649347 - about: pages do not have favicons on the thumbnails
: about: pages do not have favicons on the thumbnails
Status: VERIFIED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- minor
: Firefox 6
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on: 627966
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-12 08:28 PDT by Alex Lakatos[:AlexLakatos]
Modified: 2016-04-12 14:00 PDT (History)
6 users (show)
bzbarsky: in‑testsuite?
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshot (46.55 KB, image/jpeg)
2011-04-12 08:28 PDT, Alex Lakatos[:AlexLakatos]
no flags Details
v1 (4.36 KB, patch)
2011-04-18 05:05 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback-
Details | Diff | Review
v2 (5.63 KB, patch)
2011-04-19 10:32 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Review
v3 (7.78 KB, patch)
2011-04-20 20:30 PDT, Raymond Lee [:raymondlee]
ian: review+
Details | Diff | Review
Patch for checkin (8.04 KB, patch)
2011-05-03 03:29 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Review

Description Alex Lakatos[:AlexLakatos] 2011-04-12 08:28:16 PDT
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
Comment 1 Donovan M 2011-04-17 01:36:08 PDT
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] 2011-04-17 23:17:09 PDT
No, any other page without a favicon has a "white page" as its favicon in Panorama.
Comment 3 Raymond Lee [:raymondlee] 2011-04-18 05:05:36 PDT
Created attachment 526699 [details] [diff] [review]
v1
Comment 4 Tim Taubert [:ttaubert] 2011-04-18 05:16:57 PDT
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] 2011-04-19 10:32:57 PDT
Created attachment 527034 [details] [diff] [review]
v2

Please apply the patch for bug 627966 and the this patch
Comment 6 Tim Taubert [:ttaubert] 2011-04-20 06:02:48 PDT
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] 2011-04-20 20:30:52 PDT
Created attachment 527462 [details] [diff] [review]
v3

Merged some duplicate code
Comment 8 Ian Gilman [:iangilman] 2011-04-26 10:07:30 PDT
Comment on attachment 527462 [details] [diff] [review]
v3

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

Looks good!
Comment 9 Raymond Lee [:raymondlee] 2011-05-03 03:29:20 PDT
Created attachment 529661 [details] [diff] [review]
Patch for checkin

Please land this after bug 627966 has landed
Comment 10 Boris Zbarsky [:bz] 2011-05-03 16:36:25 PDT
http://hg.mozilla.org/projects/cedar/rev/939ccc994fba
Comment 11 Boris Zbarsky [:bz] 2011-05-04 13:51:46 PDT
http://hg.mozilla.org/mozilla-central/rev/939ccc994fba
Comment 12 AndreiD[QA] 2011-05-10 02:14:21 PDT
Verified Fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110509 Firefox/6.0a1

Note You need to log in before you can comment on or make changes to this bug.