Closed Bug 595076 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(blocking2.0 betaN+)

VERIFIED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: iangilman, Assigned: iangilman)

References

Details

(Whiteboard: [in-litmus-bug-week])

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → seanedunn
Attached patch v1 (obsolete) — Splinter Review
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?
Attachment #474007 - Flags: feedback?(ian)
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.
Attachment #474007 - Flags: feedback?(ian) → feedback-
Attached patch patch v1Splinter Review
I'm hoping to get this in b6
Attachment #474113 - Flags: review?(dietrich)
Attachment #474007 - Attachment is obsolete: true
Assignee: seanedunn → ian
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.
Attachment #474113 - Flags: review?(dietrich) → review+
Flags: in-litmus?
(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.
Summary: App tab icons are generic → App tab icons should be updated when the tab's favicon becomes available
Attachment #474113 - Flags: approval2.0?
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Try server build is clean.
Attachment #474113 - Flags: approval2.0?
http://hg.mozilla.org/mozilla-central/rev/8afc7bd2f495
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b6
As part of Bug Week, thanks to Mathnerd314 for the new test case in Litmus.

Litmus ID 12878
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
Whiteboard: [in-litmus-bug-week]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.