Closed
Bug 916588
Opened 11 years ago
Closed 11 years ago
No favicons (or wrong favicons) appearing in lists on about:home
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(firefox25 unaffected, firefox26+ verified, firefox27+ verified, fennec26+)
VERIFIED
FIXED
Firefox 27
People
(Reporter: Margaret, Assigned: ckitching)
References
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Margaret
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•11 years ago
|
||
Oops, submitted the form before I meant to...
Only some favicons are appearing, and some of them are wrong.
Is this a regression from bug 888326?
tracking-fennec: --- → ?
Keywords: regression
Summary: Favicons not appearing in lists on about:home → No favicons (or wrong favicons) appearing in lists on about:home
Reporter | ||
Comment 2•11 years ago
|
||
It looks like there might be a problem recycling views, since the nextbus.com icon is appearing next to facebook.com (nextbus.com is the top entry in my most visited list).
Assignee | ||
Comment 3•11 years ago
|
||
But Bug 888326 was almost entirely cleanup - something quite stupid has happened if it caused this.
Had it landed, I'd have thought Bug 905685 a more likely culprit, but it doesn't appear to have done so yet. (but it's making the sort of functional change that might cause this sort of problem if botched)
Hmm. I shall investigate. This one looks like it may prove interesting.
Assignee: nobody → ckitching
Assignee | ||
Comment 4•11 years ago
|
||
Currently, I can't reproduce the icon-transposition thing on the current build. I am able to make it make icons not appear, although this is a known failure of the Favicon caching system addressed in Bug 914296.
I'll be better able to fiddle with it in the morning when I have less patchy wifi for my phone to use.
I am unable to reproduce this bug at all after applying the patch from 914296.
I suggest backing out 888326 for now and landing Bug 905685. 905685 is changing a bunch of things in places that are liable to cause this sort of problem - if this truly is an underlying bug it may fix it anyway - if not, 914296 will deal with it later.
My best working theory at the moment is some funkyness with old OnFaviconLoadListener objects firing when their result is no longer desired by that view. You scroll, the views gets recycled, and the callback lands on the wrong one. (After the desired callback for that one has hit it).
Reporter | ||
Comment 5•11 years ago
|
||
I think you just need enough sites in your history to scroll in order to be able to see this.
If I set up sync with a new profile, I see the icons from the default bookmarks appear throughout my list of synced sites.
tracking-fennec: ? → 26+
status-firefox26:
--- → affected
Reporter | ||
Comment 6•11 years ago
|
||
I'm getting conflicts trying to back out the patches from bug 888326, so I also backed out the patch from bug 895423.
Doing that, I found that the guilty changeset is this one:
https://hg.mozilla.org/mozilla-central/rev/1fe12c9597f4
Since this patch just touches FaviconView, there's probably just some small mistake in here that we can fix. Chris, let's see if we can get a patch to fix this problem, rather than going through the pain of trying to back out all of bug 888326.
tracking-firefox26:
--- → ?
Reporter | ||
Comment 7•11 years ago
|
||
Looking back at this patch more, I did a bad job reviewing it
We shouldn't be holding onto bitmaps in this view, because the image will change as the view is recycled, so the stored bitmaps will be wrong. I think we should see if we can just back out this patch, but that would require also backing out bug 895423, or changing it to just use updateImage instead of updateAndScaleImage.
Reporter | ||
Comment 8•11 years ago
|
||
Chris, any progress looking into this? We should try to fix this this week, or else back out the patches that caused the problem.
Flags: needinfo?(ckitching)
Assignee | ||
Comment 9•11 years ago
|
||
This bug is not fixed by the patches from Bug 905685. My investigation continues.
Assignee | ||
Comment 10•11 years ago
|
||
This seems to do the job.
It's sort of strange - the old Favicon system seems to dispatch more than one result when it loads a Favicon, one of which will be the desired result, the others, null.
This behaviour plus the fact that the FaviconView's code to clear itself when it was set with a null seem to have been the cause of this issue. I am no longer able to reproduce the bug with this patch applied.
Attachment #806397 -
Flags: review?(margaret.leibovic)
Flags: needinfo?(ckitching)
Reporter | ||
Comment 11•11 years ago
|
||
Comment on attachment 806397 [details] [diff] [review]
Demadnessify the Favicon system
Testing with this patch, I don't see the wrong favicons, but I also don't see the default favicon appear. Looking at your original patch, I must have missed this when reviewing. I think you should replace these calls with setImageResource(R.drawable.favicon);
See here for what we did before:
https://hg.mozilla.org/mozilla-central/rev/1fe12c9597f4#l1.164
Attachment #806397 -
Flags: review?(margaret.leibovic) → review-
Assignee | ||
Comment 12•11 years ago
|
||
Yes - this was intentional - I thought this was requested in Bug 873243.
Never mind - replacing old behaviour.
Attachment #806397 -
Attachment is obsolete: true
Attachment #806733 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 13•11 years ago
|
||
(In reply to Chris Kitching [:ckitching] from comment #12)
> Created attachment 806733 [details] [diff] [review]
> V2 - Demadnessify the Favicon system.
>
> Yes - this was intentional - I thought this was requested in Bug 873243.
> Never mind - replacing old behaviour.
Sorry if there was any confusion, that bug is about not showing the default favicon in the case where another favicon replaces it. But we still want the default favicon if there's no icon for the site.
Reporter | ||
Comment 14•11 years ago
|
||
Comment on attachment 806733 [details] [diff] [review]
V2 - Demadnessify the Favicon system.
Excellent, thanks. I'll land this for you!
Attachment #806733 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Comment 15•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/1df3efcf41b0
Please request approval to uplift to Aurora as well.
Assignee | ||
Comment 16•11 years ago
|
||
Comment on attachment 806733 [details] [diff] [review]
V2 - Demadnessify the Favicon system.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 888326
User impact if declined: Favicons in the new about home will be completely wrong. Icons for different sites will be displayed by sites, some sites won't have icons when they should.
Testing completed (on m-c, etc.): Landed on fx-team. No oranges yet.
Risk to taking this patch (and alternatives if risky): Low risk - its a two line change in some UI code.
String or IDL/UUID changes made by this patch: None.
Attachment #806733 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Blocks: FaviconRevamp
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Comment 18•11 years ago
|
||
Steps to reproduce on Aurora 26.0a2 2013-09-18/ Nighlty 27.0a1 2013-09-18:
1) Load news.google.com and bookmark the page
2) Bookmark a link from the context menu
3) Enter about:home and remove the news.google.com bookmark from the context menu
Actual results:
The unvisited bookmarks take the favicons of existing bookmarks.
Updated•11 years ago
|
Attachment #806733 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 19•11 years ago
|
||
Comment 20•11 years ago
|
||
I am not able to reproduce the bug using Google Nexus 7 (Android 4.4.2) and Samsung Galaxy S (Android 2.3.4).
Feel free to open it again or a new one if you will see the wrong behavior again.
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•