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

VERIFIED FIXED in Firefox 4.0b7

Status

defect
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: iangilman, Assigned: iangilman)

Tracking

unspecified
Firefox 4.0b7
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

5.94 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
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
Posted 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-
Posted 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: 9 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.