Closed Bug 921433 Opened 11 years ago Closed 10 years ago

The favicon bundled with Fennec for does not match the actual favicon of the site


(Firefox for Android Graveyard :: Favicon Handling, defect)

Not set


(firefox25 wontfix, firefox26 wontfix, firefox27 wontfix, firefox28 wontfix, firefox29 wontfix, firefox30 wontfix, firefox31 wontfix, firefox32 verified, firefox33 verified)

Firefox 33
Tracking Status
firefox25 --- wontfix
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- verified
firefox33 --- verified


(Reporter: ckitching, Assigned: rnewman)



(Keywords: polish, reproducible)


(2 files)

The favicon bundled with Fennec for does not match the actual favicon of the site.

Is this intentional, or an omission to update the bundled icon when the real icon changed? Seems like a relatively simple fix, anyway. Can provide screenshots on request if the issue isn't immediately clear.
The last time we updated bug 822820, SUMO was just using a Firefox icon so we just reused ours. We should match what they serve now.
Blocks: 926139
OS: Linux → Android
Hardware: x86_64 → All
Keywords: reproducible
Summary: The favicon bundled with Fennec for does not match the actual favicon of the site. (Intentional?) → The favicon bundled with Fennec for does not match the actual favicon of the site
Is there something blocking us from fixing this bug? One thing to note is that we only ship the Firefox icon for release/beta, so we would still need to bundle an icon if we want this to work properly on nightly/aurora/unbranded builds.

rnewman, if this bug isn't too complicated to fix, maybe this could be a good bug for you to mentor. Get more people in on the favicon fun! :)
Flags: needinfo?(rnewman)
The only complication is in how the favicon is bundled -- SUMO uses distinct link tags to different icon sizes, which doesn't align with the *one* icon we can put in the DB.

(Unless we re-pack it as a .ico and store that...)

If we're happy to continue shipping just the 64px version, then yeah, this is trivial.
Blocks: 822820
Flags: needinfo?(rnewman)
Whiteboard: [mentor=rnewman][lang=java][good first bug]
Now that we've addressed Bug 748100, we should be able to bundle several sizes of favicon into one .ico file, and store that in our database instead of what we do now.
Depends on: ICODecoder
Keywords: polish
Component: Theme and Visual Design → Favicon Handling
Blocks: 999920
I'm interested it this bug but favicon handling seem like a little complicated(a folder named Favicon with so many source files).

I am new to this and have no idea about where lead to this bug or where i should begin with. Are there someone who can do me a favor? Thanks for your kindess.
There's a file in your source tree:


The entry for bookmarks_support in that file needs a better base64-encoded icon. Given that we now support .ico files, you might be able to combine several entries:

into a single .ico directory, and use that. Otherwise, just pick the 32x32 png and replace the one in that JSON file.

Rebuilding and installing, clearing data on the device, then launching Fennec, should show the correct icon (the Firefox globe) for that bookmark.
(In reply to Richard Newman [:rnewman] from comment #7)

Excuse me. I think I am not totally understand.

Do you mean that I should follow two step below?
1.use base64 to encode the firefox-32.png and replace in file
2.combine serval entries firefox-xx.png into a single .ico directory and use that

Do I understand alright? And what do you mean by "use that" in the second step? Can you introduce it to me with further infomation? Thanks
Mentor: rnewman
Whiteboard: [mentor=rnewman][lang=java][good first bug] → [lang=java][good first bug]
Zhanpeng - are you working on this bug?

RNewman - can you take a look at the question in comment 8?
(In reply to Zhanpeng Zeng (:marcus) from comment #8)

> Do I understand alright? And what do you mean by "use that" in the second
> step? Can you introduce it to me with further infomation? Thanks

The thing that goes in is a base64-encoded 'file' hiding in a data: URI.

That file is provided as input to our favicons code. Our favicons code understands both PNGs and icon 'directories' (.ico).

The simple solution is to just use a single newer PNG, replacing the current base64 string with a new one.

The perhaps better solution is to use an icon directory format to store several sizes of icon instead of a single PNG.
Mentor: rnewman
I think we want this for 33 at the latest, so I'm stealing this.
Assignee: nobody → rnewman
Whiteboard: [lang=java][good first bug]
Experimentation suggests that this file does nothing useful, but untangling the build dependency on is more than I want to take on right now. It also might break locale-specific bookmarks, so let's just do this.

Seems sane?
Attachment #8457607 - Flags: review?(nalexander)
Does exactly what it says on the tin.
Attachment #8457609 - Flags: review?(nalexander)
Comment on attachment 8457607 [details] [diff] [review]
Part 1: remove redundant data from v1

Review of attachment 8457607 [details] [diff] [review]:


::: mobile/locales/generic/profile/
@@ +1,4 @@
>  #filter substitution
>  {"type":"text/x-moz-place-container","root":"placesRoot","children":
>    [{"type":"text/x-moz-place-container","title":"@bookmarks_title@","annos":[{"name":"mobile/bookmarksRoot","expires":4,"type":1,"value":1}],
> +    "children": []

Consider adding a #preprocessor comment to explain that this is unused.
Attachment #8457607 - Flags: review?(nalexander) → review+
Attachment #8457609 - Flags: review?(nalexander) → review+
Comment on attachment 8457609 [details] [diff] [review]
Part 2: bundle current SUMO favicon in default bookmarks. v1

(Applies to both patches.)

Approval Request Comment
[Feature/regressing bug #]:
  None. This has been stale for a while.

[User impact if declined]:
  Old icon for SUMO. This is just an image change.

[Describe test coverage new/current, TBPL]:
  Just an image change.

[Risks and why]: 
  Beauty and consistency are important.

[String/UUID change made/needed]:
Attachment #8457609 - Flags: approval-mozilla-aurora?
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Attachment #8457609 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tested with:
Device: Samsung Galaxy Nexus
OS: Android 4.2.1

SUMO helping hand favicon is replaced by the the Firefox favicon, so verified fixed on:
Build: Firefox for Android 32 Beta 1
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.