674794 - favicons of app tabs aren't updated on Panorama

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Reuben Morais [:reuben]]

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]]

This is an Add-on for Firefox? If yes, can you provide a link to test this?
Thx.

Comment 2

[Reuben Morais [:reuben]]

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]]

I'm referring in comment1 about Gmail Labs or is it a feature of gmail that can be activated?

Comment 4

[Reuben Morais [:reuben]]

(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]]

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]]

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]]

(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]]

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

Oh, I completely forgot to mention, it only happens on app tabs.

Comment 10

[Raymond Lee [:raymondlee]]

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

Attachment: video

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: 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] (inactive)]

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]]

Attachment: 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] (inactive)]

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]]

(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] (inactive)]

(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]]

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

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] (inactive)]

http://hg.mozilla.org/integration/fx-team/rev/aa126a9aebbd

Comment 21

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 555100 [details] [diff] [review]
[checked-in] Patch for checkin

http://hg.mozilla.org/mozilla-central/rev/aa126a9aebbd

Comment 23

[Vlad [QA]]

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.