595076 - App tab icons should be updated when the tab's favicon becomes available

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Ian Gilman [:iangilman]]

When we start up, the icons for app tabs often aren't yet loaded, so we use a default icon. What we need to do is listen for attrModified and update the icons when they get loaded, very similar to how we do it in TabItems.

Comment 1

[Sean Dunn]

Attachment: v1

Here's a very simple first stab. It works, in as far as it appears to set the icon. But, your apptabs patch doesn't really seem to work very robustly. When I create an apptab, it doesn't get put on the apptab list of the GroupItem. And when I change the webpage, it doesn't look like attrModified is getting called. Am I doing something obviously wrong?

Comment 2

[Ian Gilman [:iangilman]]

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

Yes, the pinning not having any effect is a known issue (bug 593871); sorry if it was getting in the way. 

A couple of issues with your work in progress:

* AllTabs watches all tabs in all windows; you only need to register once, rather than once per group. 
* Once you've registered, you should also unregister at the end (to avoid leaks)
* App tabs don't have .tabItems, so you've got to deal with them differently. 

Anyway, I'll take it from here.

Comment 3

[Ian Gilman [:iangilman]]

Attachment: patch v1

I'm hoping to get this in b6

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 474113 [details] [diff] [review]
patch v1

since you're already using DOMAttrModified, as we talked about on irc, not going to r- this over doing it here. if we start getting main-window responsiveness complaints before final, going to look here first though.

testing favicons is non-trivial, so set the litmus flag on the bug to request a manual test here. r=me assuming the existing tests pass w/ this change.

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #4)
> Comment on attachment 474113 [details] [diff] [review]
> patch v1
> 
> since you're already using DOMAttrModified, as we talked about on irc, not
> going to r- this over doing it here. if we start getting main-window
> responsiveness complaints before final, going to look here first though.

AllTabs.register("attrModified", ...) is the TabAttrModified event, which is different from DOMAttrModfied.

Comment 6

[Ian Gilman [:iangilman]]

Try server build is clean.

Comment 7

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/8afc7bd2f495

Comment 8

[Tracy Walker [:tracy]]

As part of Bug Week, thanks to Mathnerd314 for the new test case in Litmus.

Litmus ID 12878