favicons of app tabs aren't updated on Panorama

VERIFIED FIXED in Firefox 9

Status

Firefox Graveyard
Panorama
VERIFIED FIXED
6 years ago
11 months ago

People

(Reporter: reuben, Assigned: raymondlee)

Tracking

Trunk
Firefox 9

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
This is an Add-on for Firefox? If yes, can you provide a link to test this?
Thx.
(Reporter)

Comment 2

6 years ago
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

6 years ago
I'm referring in comment1 about Gmail Labs or is it a feature of gmail that can be activated?
(Reporter)

Comment 4

6 years ago
(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

6 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

6 years ago
OS: Mac OS X → All
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 7

6 years ago
(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?
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Comment 8

6 years ago
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.
(Reporter)

Comment 9

6 years ago
Oh, I completely forgot to mention, it only happens on app tabs.
(Reporter)

Updated

6 years ago
Summary: favicons aren't updated on Panorama → favicons of app tabs aren't updated on Panorama
(Assignee)

Comment 10

6 years ago
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.
(Reporter)

Comment 11

6 years ago
Created attachment 554377 [details]
video
(Reporter)

Updated

6 years ago
Attachment #554377 - Attachment mime type: application/octet-stream → video/quicktime
(Assignee)

Comment 12

6 years ago
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

+    }
Attachment #554519 - Flags: review?(tim.taubert)
(Assignee)

Updated

6 years ago
Assignee: nobody → raymond
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.
Attachment #554519 - Flags: review?(tim.taubert) → review+
(Assignee)

Comment 14

6 years ago
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.
Attachment #554519 - Attachment is obsolete: true
Attachment #554832 - Flags: review?(tim.taubert)
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.
Attachment #554832 - Flags: review?(tim.taubert) → review+
(Assignee)

Comment 16

6 years ago
(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)
(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.
(Assignee)

Comment 18

6 years ago
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
Attachment #554832 - Attachment is obsolete: true
(Assignee)

Comment 19

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

Passed Try
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=a214eebfe008
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/fx-team/rev/aa126a9aebbd
Keywords: checkin-needed
Hardware: x86 → All
Whiteboard: [fixed-in-fx-team]
Version: 7 Branch → Trunk
Comment on attachment 555100 [details] [diff] [review]
[checked-in] Patch for checkin

http://hg.mozilla.org/mozilla-central/rev/aa126a9aebbd
Attachment #555100 - Attachment description: Patch for checkin → [checked-in] Patch for checkin
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 9
Duplicate of this bug: 682720

Comment 23

6 years ago
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.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.