600645 - App tabs don't appear in the group app tab tray if their icon fails to load

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Ian Gilman [:iangilman]]

We have a default icon we use if the app tab has no icon, but if it lists an icon that's broken, we use that and consequently the app tab doesn't show up at all.

I found this making one of my own pages an app tab: 

http://iangilman.com/germany/

... which gives a favicon link of:

http://iangilman.com/favicon.ico

... even though it's not actually listed anywhere on the page; must be Firefox making assumptions?

Obviously I can fix it by fixing my page, but presumably this is an issue for other pages as well. 

So, on the one hand, I don't know that people will be picking favicon-less pages to make into app tabs, but on the other hand, if they do it kind of looks like data loss.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[Ian Gilman [:iangilman]]

Comment on attachment 488824 [details] [diff] [review]
v1

>   addAppTab: function GroupItem_addAppTab(xulTab) {
>     let self = this;
> 
>+    xulTab.addEventListener("error", this._onAppTabError, false);

Is a failed favIcon the only thing that triggers a xul:tab error event? I would imagine not... how do we detect the specific error we're interested in?

>                  search1.html \
>                  search2.html \
>+                 browser_tabview_bug600645.html \

I believe a file, even an html file, prefixed with "browser" will be run as a test. Please remove "browser_tabview_" from the html file's name.

>+<link reL="shortcut icon" href="favicon.ico">

Nit: reL should be rel.

>+  let errorHandler = function(event) {
>+    newTab.removeEventListener("error", errorHandler, false);
>+
>+    // clean up
>+    gBrowser.removeTab(newTab);
>+    let endGame = function() {
>+      window.removeEventListener("tabviewhidden", endGame, false);
>+
>+      ok(!TabView.isVisible(), "Tab View is hidden");
>+      finish();
>+    }
>+    window.addEventListener("tabviewhidden", endGame, false);
>+    TabView.toggle();
>+  };
>+  newTab.addEventListener("error", errorHandler, false);
>+
>+  newTab.linkedBrowser.loadURI(
>+    "http://mochi.test:8888/browser/browser/base/content/test/tabview/browser_tabview_bug600645.html");

You're testing that the error is fired from the xul:tab, but you still need to test to see whether the app tabs respond correctly with their icon.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v1

(In reply to comment #2)
> Comment on attachment 488824 [details] [diff] [review]
> v1
> 
> >   addAppTab: function GroupItem_addAppTab(xulTab) {
> >     let self = this;
> > 
> >+    xulTab.addEventListener("error", this._onAppTabError, false);
> 
> Is a failed favIcon the only thing that triggers a xul:tab error event? I would
> imagine not... how do we detect the specific error we're interested in?

A failed favicon should be the only thing that triggers the error event.  See the below links, the code removes the image attribute (the fav icon src) when an error event occurs.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1123
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2558

> 
> >                  search1.html \
> >                  search2.html \
> >+                 browser_tabview_bug600645.html \
> 
> I believe a file, even an html file, prefixed with "browser" will be run as a
> test. Please remove "browser_tabview_" from the html file's name.

Changed the name to bug600645.html

> 
> >+<link reL="shortcut icon" href="favicon.ico">
> 
> Nit: reL should be rel.

Fixed

> 
> >+  let errorHandler = function(event) {
> >+    newTab.removeEventListener("error", errorHandler, false);
> >+
> >+    // clean up
> >+    gBrowser.removeTab(newTab);
> >+    let endGame = function() {
> >+      window.removeEventListener("tabviewhidden", endGame, false);
> >+
> >+      ok(!TabView.isVisible(), "Tab View is hidden");
> >+      finish();
> >+    }
> >+    window.addEventListener("tabviewhidden", endGame, false);
> >+    TabView.toggle();
> >+  };
> >+  newTab.addEventListener("error", errorHandler, false);
> >+
> >+  newTab.linkedBrowser.loadURI(
> >+    "http://mochi.test:8888/browser/browser/base/content/test/tabview/browser_tabview_bug600645.html");
> 
> You're testing that the error is fired from the xul:tab, but you still need to
> test to see whether the app tabs respond correctly with their icon.

Added extra code for that.

Comment 4

[Ian Gilman [:iangilman]]

Comment on attachment 489110 [details] [diff] [review]
v1

> A failed favicon should be the only thing that triggers the error event.  See
> the below links, the code removes the image attribute (the fav icon src) when
> an error event occurs.
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1123
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2558

Fair enough, thanks!

>+  // change the src to something else.
>+  let otherFavIconURL = "http://mochi.test:8888/fav.ico";
>+  $icon.attr("src", otherFavIconURL);
>+  is($icon.attr("src"), otherFavIconURL, "The icon src is using another fav icon");

I'm not sure what the purpose of this bit is; I'd leave it out. 

If it's to make sure we're not getting a false positive later on, it doesn't do any good, since as soon as you change the url of the app tab (with .linkedBrowser.loadURI), the icon src is changed to the new thing (at least once the TabAttrModified gets to it).

F+ otherwise.

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v1

Updated the patch based on Ian's comment.

f=ian

Comment 6

[Raymond Lee [:raymondlee]]

Sent it to try 0effc839caf3, waiting for the result

Comment 7

[Raymond Lee [:raymondlee]]

(In reply to comment #6)
> Sent it to try 0effc839caf3, waiting for the result

Passed Try

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 489373 [details] [diff] [review]
v1

a=beltzner

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 12

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/597e4c5ded14

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This bug was part of a mass backout to fix the permanent leak on OS X 64 that this push caused.

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

Comment 14

[Raymond Lee [:raymondlee]]

Passed Try

Comment 15

[(no longer active)]

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

Comment 16

[Tracy Walker [:tracy]]

Verified in recent nightly minefield build