Closed Bug 998171 Opened 6 years ago Closed 5 years ago

The site icon of a just-previously-visited site appears instead of the site icon of the currently viewed site when viewed in aero peek

Categories

(SeaMonkey :: General, defect)

SeaMonkey 2.26 Branch
x86_64
Windows 7
defect
Not set

Tracking

(seamonkey2.26 fixed, seamonkey2.27 fixed, seamonkey2.28 fixed, seamonkey2.29 fixed)

RESOLVED FIXED
seamonkey2.29
Tracking Status
seamonkey2.26 --- fixed
seamonkey2.27 --- fixed
seamonkey2.28 --- fixed
seamonkey2.29 --- fixed

People

(Reporter: thee.chicago.wolf, Assigned: neil)

References

Details

Attachments

(3 files)

Attached image wrong-aero-icon.jpg
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 SeaMonkey/2.26 (Beta/Release)
Build ID: 20140415200419

Steps to reproduce:

I visited CNN and then logged into and then out of eBay. When I viewed the Aero peek window of the logged out eBay session, I still saw the CNN site icon in the top left corner of the Aero peek window (see attached image).


Actual results:

The site icon of the previously viewed web site (CNN) showed as being the site icon of the currently viewed web site (eBay).


Expected results:

The eBay site icon should have appeared in the Aero peek window instead of the CNN site icon.
Work done in bug 839891 appears to be causing this issue. Setting browser.taskbar.previews.enable=false fixes the issue.
Ah, I bet the problem here is that eBay isn't showing a link rel=icon and so we should be clearing the previously loaded favicon.
Attached patch Proposed patchSplinter Review
Conveniently WindowsPreviewPerTab.jsm already handles null icons, resetting to the default icon, so I thought the easiest way to handle this was to make setIcon null-safe (like it is in Firefox) and also nsBrowserStatusHandler.

Unfortunately the code for setIcon is more complicated than it could have been because of the string compatibility requirement.
Assignee: nobody → neil
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8415838 - Flags: review?(bugzilla)
Comment on attachment 8415838 [details] [diff] [review]
Proposed patch

The patch does seem to work, but I wonder about another thing: Shouldn't the icon in aero UI always be the same as the icon in the url bar and the icon on the tab itself? This seems to be different when telling SeaMonkey to always load favicon.ico (pref browser.chrome.favicons set to true). For example when I go to http://edition.cnn.com/ I see the CNN logo in url bar and tab, but not in the aero preview of that tab.
So with rel=icon it looks like it runs this code http://hg.mozilla.org/comm-central/annotate/0927618bbc2e/suite/browser/tabbrowser.xml#l1085 which calls
"  1106               this.setIcon(this.tabs[index], iconUri);"

which is why the icon gets updated. Maybe the loadFavIcon method at http://hg.mozilla.org/comm-central/annotate/0927618bbc2e/suite/browser/tabbrowser.xml#l779 should call setIcon to set the icon (which will call onLinkIconAvailable). But we can deal with this in a new bug if needed.
(In reply to Frank Wein [:mcsmurf] from comment #4)
> Comment on attachment 8415838 [details] [diff] [review]
> Proposed patch
> 
> The patch does seem to work, but I wonder about another thing: Shouldn't the
> icon in aero UI always be the same as the icon in the url bar and the icon
> on the tab itself? This seems to be different when telling SeaMonkey to
> always load favicon.ico (pref browser.chrome.favicons set to true). For
> example when I go to http://edition.cnn.com/ I see the CNN logo in url bar
> and tab, but not in the aero preview of that tab.

If the site has an icon, I've always seen it displayed in the Aero peek.
The current navigator code wants to process favicon.ico itself so we'd have to rewrite those parts too (I guess we should start using the favicon service too).
Attached patch Test patchSplinter Review
I wonder if a patch like this does the correct thing?
(Actually it does not as in this line
 browser.mIconURL = aURI.spec;
browser is undefined. But somehow it sets the icon anyway in the aero and browser GUI).
(In reply to Frank Wein from comment #8)
> (Actually it does not as in this line
>  browser.mIconURL = aURI.spec;
> browser is undefined. But somehow it sets the icon anyway in the aero and
> browser GUI).

That's because it's called from navigator.js as well, and that's when you're getting the undefined browser.
Oh, and there's a difference in behaviour: tabs prefer the loading icon, then the link icon, then the image thumbnail, then favicon.ico, while the URL bar prefers favicon.ico until a link icon overrides it. (Obviously the URL bar has no loading icon, and it also doesn't try to thumbnail images either.)
Comment on attachment 8415838 [details] [diff] [review]
Proposed patch

Ok, r+ then, can deal with the other issue in a new bug.
Attachment #8415838 - Flags: review?(bugzilla) → review+
Pushed comm-central changeset b533f0b61aef.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.29
Comment on attachment 8415838 [details] [diff] [review]
Proposed patch

[Approval Request Comment]
Regression caused by (bug #): 839891
User impact if declined: Bogus icons appear in Aero Peek
Testing completed (on m-c, etc.): Landed on c-c
Risk to taking this patch (and alternatives if risky): Low, onLinkIconAvailble now passes null to indicate no icon, copying existing Firefox behaviour.
String changes made by this patch: None
Attachment #8415838 - Flags: approval-comm-beta?
Attachment #8415838 - Flags: approval-comm-aurora?
Comment on attachment 8415838 [details] [diff] [review]
Proposed patch

a=me comm-aurora and comm-beta
Attachment #8415838 - Flags: approval-comm-beta?
Attachment #8415838 - Flags: approval-comm-beta+
Attachment #8415838 - Flags: approval-comm-aurora?
Attachment #8415838 - Flags: approval-comm-aurora+
Blocks: 1018792
Landed on comm-release for SeaMonkey 2.26.1

$ hg tip
changeset:   20186:aa67f2715fd3
branch:      SEA_2_26_1_RELBRANCH
tag:         tip
user:        Neil Rashbrook <neil@parkwaycc.co.uk>
date:        Mon Jun 02 00:10:24 2014 +0100
summary:     Bug 998171 Clear the Aero Peek favicon when the document changes r=mcsmurf a=Ratty
Any ETA on when the 2.26.1 build will be up to test this?
testing ready should be tomorrow/this weekend, we're hoping for official release early next week.
Just tested with 2.26.1 and it look good. Thanks for fixing.
You need to log in before you can comment on or make changes to this bug.