Last Comment Bug 760218 - Missing favicons for pages that don't specify favicon size
: Missing favicons for pages that don't specify favicon size
Status: RESOLVED FIXED
: regression
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 16
Assigned To: Brian Nicholson (:bnicholson)
:
:
Mentors:
Depends on:
Blocks: 715263
  Show dependency treegraph
 
Reported: 2012-05-31 11:29 PDT by Aaron Train [:aaronmt]
Modified: 2012-07-15 19:44 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed


Attachments
patch (1.01 KB, patch)
2012-06-07 16:21 PDT, Brian Nicholson (:bnicholson)
margaret.leibovic: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Aaron Train [:aaronmt] 2012-05-31 11:29:50 PDT
Regression from bug 715263?
Comment 2 :Margaret Leibovic 2012-06-01 09:28:55 PDT
I guess that means I should take this...

I'll do some debugging today. Does this affect all about: pages? Has anyone been seeing missing favicons on normal webpages?
Comment 3 Brian Nicholson (:bnicholson) 2012-06-04 13:15:45 PDT
(In reply to Margaret Leibovic [:margaret] from comment #2)
> I guess that means I should take this...
> 
> I'll do some debugging today. Does this affect all about: pages? Has anyone
> been seeing missing favicons on normal webpages?

Yes - http://nightly.mozilla.org.
Comment 4 :Margaret Leibovic 2012-06-04 13:41:27 PDT
This isn't in Firefox 14 (introduced by bug 715263), so it shouldn't need to be a blocker.
Comment 5 Brian Nicholson (:bnicholson) 2012-06-04 13:50:57 PDT
Oh, I'll take that away then.
Comment 6 Brian Nicholson (:bnicholson) 2012-06-07 16:21:53 PDT
Created attachment 631206 [details] [diff] [review]
patch

I noticed this while working on a number of other favicon bugs today. mFaviconSize is initialized to 0, so any favicons of undefined (0) size won't be used. This means that any page that that doesn't have a root favicon.ico won't have its favicon shown (unless it has a size attribute on its link item).
Comment 7 :Margaret Leibovic 2012-06-07 16:35:44 PDT
Comment on attachment 631206 [details] [diff] [review]
patch

That makes sense. Also if an additional favicon is added with the same size, I think the last one added is expected to be the one used, and this patch would make that true. Thank you, Brian!
Comment 8 Brian Nicholson (:bnicholson) 2012-06-07 17:20:30 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/e64eb13b8b5b
Comment 9 Graeme McCutcheon [:graememcc] 2012-06-08 04:18:35 PDT
https://hg.mozilla.org/mozilla-central/rev/e64eb13b8b5b

(Merged by Ed Morley)
Comment 10 Brian Nicholson (:bnicholson) 2012-07-14 01:32:52 PDT
Comment on attachment 631206 [details] [diff] [review]
patch

I'm confused about what happened here...bug 715263 is apparently in Aurora, but it's not marked or mentioned in the bug. There was a rollup patch uploaded that included this one, but it looks like that wasn't the one that got landed. Anyway, we should probably uplift this too.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 715263
User impact if declined: Missing icon on about:home, nightly.mozilla.org, and other sites
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): very low risk
String or UUID changes made by this patch: none
Comment 11 Brian Nicholson (:bnicholson) 2012-07-14 01:36:16 PDT
Oh, I guess bug 715263 just made the 15 cutoff, and this one didn't?
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-15 19:30:22 PDT
Comment on attachment 631206 [details] [diff] [review]
patch

Safe, low risk fix. Mobile-only
Comment 13 Brian Nicholson (:bnicholson) 2012-07-15 19:44:46 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/c7a66cfaa20b

Note You need to log in before you can comment on or make changes to this bug.