Last Comment Bug 674794 - favicons of app tabs aren't updated on Panorama
: favicons of app tabs aren't updated on Panorama
Status: VERIFIED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 9
Assigned To: Raymond Lee [:raymondlee]
:
:
Mentors:
: 682720 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-27 17:44 PDT by Reuben Morais [:reuben]
Modified: 2016-04-12 14:00 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Video (655.29 KB, video/quicktime)
2011-08-18 20:47 PDT, Raymond Lee [:raymondlee]
no flags Details
video for app tabs (2.28 MB, video/quicktime)
2011-08-18 22:21 PDT, Raymond Lee [:raymondlee]
no flags Details
video (2.85 MB, video/quicktime)
2011-08-19 05:17 PDT, Reuben Morais [:reuben]
no flags Details
v1 (1.13 KB, patch)
2011-08-19 13:06 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
v2 (2.29 KB, patch)
2011-08-22 05:37 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
[checked-in] Patch for checkin (2.88 KB, patch)
2011-08-23 07:54 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Reuben Morais [:reuben] 2011-07-27 17:44:43 PDT
For example, if you turn on Gmail Labs' "Unread message icon" feature, the favicon on Panorama will not change unless you close and reopen the tab.
Comment 1 Vlad [QA] 2011-07-29 02:51:56 PDT
This is an Add-on for Firefox? If yes, can you provide a link to test this?
Thx.
Comment 2 Reuben Morais [:reuben] 2011-07-29 04:18:47 PDT
No, Panorama a.k.a. TabCandy is a built-in feature since 4.0 (?).
Just click the "Group your tabs" button on the top right of the window.
Comment 3 Vlad [QA] 2011-07-29 04:36:20 PDT
I'm referring in comment1 about Gmail Labs or is it a feature of gmail that can be activated?
Comment 4 Reuben Morais [:reuben] 2011-07-29 04:43:47 PDT
(In reply to comment #3)
> I'm referring in comment1 about Gmail Labs or is it a feature of gmail that
> can be activated?

(comment #0)
> Gmail Labs' "Unread message icon" feature

Look for "Unread message icon" on your Gmail Labs settings.
Comment 5 Vlad [QA] 2011-07-29 05:18:20 PDT
Reproducible for me on  Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a1) Gecko/20110728 Firefox/8.0a1  
and also on Mozilla/5.0 (Windows NT 6.1; rv:8.0a1) Gecko/20110728 Firefox/8.0a1

Changing platform to All since is reproducible on Windows 7 as well.

Setting resolution to NEW.
Comment 6 Raymond Lee [:raymondlee] 2011-08-08 22:11:28 PDT
This works for me.

When an attribue (e.g. title, icon url, etc)  is modified, the fav icon, title and thumbnail would be updated (see TabItems_update()) immediately or after a delay if there are other tab items waiting to be updated or the tab item itself has just been updated less than 200 million seconds ago.

Reporter: please try
1. open a tab with gmail
2. go into Panorama
3. open a new window or use other email client to send an email your emil account
4. wait a few minutes and the fav icon and title should get updated.
Comment 7 Reuben Morais [:reuben] 2011-08-18 20:35:40 PDT
(In reply to Raymond Lee [:raymondlee] from comment #6)
> 
> Reporter: please try
> 1. open a tab with gmail
> 2. go into Panorama
> 3. open a new window or use other email client to send an email your emil
> account
> 4. wait a few minutes and the fav icon and title should get updated.
>

I can still reproduce this, and I don't think I should have to wait, why can't it be instantaneous?
Comment 8 Raymond Lee [:raymondlee] 2011-08-18 20:47:40 PDT
Created attachment 554293 [details]
Video

> I can still reproduce this, and I don't think I should have to wait, why
> can't it be instantaneous?

It works fine for me.  Please see the attached video.

Two google mail tabs with the same account in each window.  One in Panorama and one isn't.  The fav and title are updated almost at the same time.
Comment 9 Reuben Morais [:reuben] 2011-08-18 20:55:57 PDT
Oh, I completely forgot to mention, it only happens on app tabs.
Comment 10 Raymond Lee [:raymondlee] 2011-08-18 22:21:50 PDT
Created attachment 554304 [details]
video for app tabs

(In reply to Reuben Morais [:reuben] from comment #9)
> Oh, I completely forgot to mention, it only happens on app tabs.

Still works for me.
Comment 11 Reuben Morais [:reuben] 2011-08-19 05:17:40 PDT
Created attachment 554377 [details]
video
Comment 12 Raymond Lee [:raymondlee] 2011-08-19 13:06:38 PDT
Created attachment 554519 [details] [diff] [review]
v1

-    // use the tab image if it doesn't start with http e.g. data:image/png, chrome://
-    if (tab.image && !(/^https?:/.test(tab.image)))
-      url = tab.image;
-    else
+    if (tab.image) {
+      // if starts with http/https, fetch icon from favicon service via the moz-anno protocal
+      if (/^https?:/.test(tab.image))
+        url = "moz-anno:favicon:" + tab.image;

This would get the icon from fav icon service using the icon url

+      else
+        url = tab.image;
+    } else {
       url = gFavIconService.getFaviconImageForPage(tab.linkedBrowser.currentURI).spec;

This doesn't return the latest fav icon by url so we use "moz-anno:favicon:" above

+    }
Comment 13 Tim Taubert [:ttaubert] 2011-08-22 04:46:40 PDT
Comment on attachment 554519 [details] [diff] [review]
v1

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

Nice, thanks!

r=me with the one line fixed.

::: browser/base/content/tabview/ui.js
@@ +1569,5 @@
>  
> +    if (tab.image) {
> +      // if starts with http/https, fetch icon from favicon service via the moz-anno protocal
> +      if (/^https?:/.test(tab.image))
> +        url = "moz-anno:favicon:" + tab.image;

Please use "gFavIconService.getFaviconLinkForIcon(gWindow.makeURI(tab.image)).spec" here.
Comment 14 Raymond Lee [:raymondlee] 2011-08-22 05:37:26 PDT
Created attachment 554832 [details] [diff] [review]
v2

(In reply to Tim Taubert [:ttaubert] from comment #13)
> Please use
> "gFavIconService.getFaviconLinkForIcon(gWindow.makeURI(tab.image)).spec"
> here.

Done.

I've updated the test and request a review again.
Comment 15 Tim Taubert [:ttaubert] 2011-08-22 06:26:50 PDT
Comment on attachment 554832 [details] [diff] [review]
v2

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

Thanks!

r=me with the test fix.

::: browser/base/content/test/tabview/browser_tabview_bug600645.js
@@ +40,5 @@
>      // fired, a delay is used here to avoid the test code run before the browser 
>      // code.
>      executeSoon(function() {
> +      ok(/^moz-anno:favicon:/.test($icon.attr("src")), 
> +         "The icon url starts with moz-anno:favicon so the default fav icon would be displayed");

As noted in https://developer.mozilla.org/en/nsIFaviconService#getFaviconLinkForIcon%28%29 the moz-anno protocol handler automatically redirects to the default favIcon if the given url is invalid. This isn't really obvious and the test doesn't check if the default favIcon is displayed...

Please add a comment about the fallback and compare the actual favIcon with the expected one.
Comment 16 Raymond Lee [:raymondlee] 2011-08-22 07:04:43 PDT
(In reply to Tim Taubert [:ttaubert] from comment #15)
> As noted in
> https://developer.mozilla.org/en/
> nsIFaviconService#getFaviconLinkForIcon%28%29 the moz-anno protocol handler
> automatically redirects to the default favIcon if the given url is invalid.
> This isn't really obvious and the test doesn't check if the default favIcon
> is displayed...
> 
> Please add a comment about the fallback and compare the actual favIcon with
> the expected one.

I am not sure how we can compare the actual favicon with the expected one (default one).  Since both faviconService.getFaviconData() and faviconService.getFaviconDataAsDataURL() throws an error for the invalid url (moz-anno:favicon:http://mochi.test:8888/browser/browser/base/content/test/tabview/favicon.ico)
Comment 17 Tim Taubert [:ttaubert] 2011-08-23 02:12:55 PDT
(In reply to Raymond Lee [:raymondlee] from comment #16)
> I am not sure how we can compare the actual favicon with the expected one
> (default one).  Since both faviconService.getFaviconData() and
> faviconService.getFaviconDataAsDataURL() throws an error for the invalid url
> (moz-anno:favicon:http://mochi.test:8888/browser/browser/base/content/test/
> tabview/favicon.ico)

Ok, well I'd say then let's just check that faviconService.getFaviconDataAsDataURL() throws an error and add a nice comment that we expect this error and why we do this.
Comment 18 Raymond Lee [:raymondlee] 2011-08-23 07:54:29 PDT
Created attachment 555100 [details] [diff] [review]
[checked-in] Patch for checkin

(In reply to Tim Taubert [:ttaubert] from comment #17)
> Ok, well I'd say then let's just check that
> faviconService.getFaviconDataAsDataURL() throws an error and add a nice
> comment that we expect this error and why we do this.

Done
Comment 19 Raymond Lee [:raymondlee] 2011-08-23 19:29:31 PDT
Comment on attachment 555100 [details] [diff] [review]
[checked-in] Patch for checkin

Passed Try
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=a214eebfe008
Comment 20 Tim Taubert [:ttaubert] 2011-08-24 04:40:35 PDT
http://hg.mozilla.org/integration/fx-team/rev/aa126a9aebbd
Comment 21 Rob Campbell [:rc] (:robcee) 2011-08-25 11:46:52 PDT
Comment on attachment 555100 [details] [diff] [review]
[checked-in] Patch for checkin

http://hg.mozilla.org/mozilla-central/rev/aa126a9aebbd
Comment 22 Tim Taubert [:ttaubert] 2011-08-28 10:46:59 PDT
*** Bug 682720 has been marked as a duplicate of this bug. ***
Comment 23 Vlad [QA] 2011-08-29 04:45:25 PDT
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 2

The favicon is updating accordingly when new mails are arriving, in Panorama and also in normal mode.

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