760218 - Missing favicons for pages that don't specify favicon size

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Regression from bug 715263?

Comment 1

[Aaron Train [:aaronmt]]

Yep.

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=65fa5cb6f79c&tochange=3aa566994890

Comment 2

[:Margaret Leibovic]

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)]

(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]

This isn't in Firefox 14 (introduced by bug 715263), so it shouldn't need to be a blocker.

Comment 5

[Brian Nicholson (:bnicholson)]

Oh, I'll take that away then.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: 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]

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)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e64eb13b8b5b

Comment 9

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/e64eb13b8b5b

(Merged by Ed Morley)

Comment 10

[Brian Nicholson (:bnicholson)]

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)]

Oh, I guess bug 715263 just made the 15 cutoff, and this one didn't?

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 631206 [details] [diff] [review]
patch

Safe, low risk fix. Mobile-only

Comment 13

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/c7a66cfaa20b