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

VERIFIED FIXED in Firefox 32

Status

()

defect
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: ckitching, Assigned: rnewman)

Tracking

(Blocks 2 bugs, {polish, reproducible})

Trunk
Firefox 33
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(2 attachments)

The favicon bundled with Fennec for support.mozilla.org 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 support.mozilla.org does not match the actual favicon of the site. (Intentional?) → The favicon bundled with Fennec for support.mozilla.org does not match the actual favicon of the site
Duplicate of this bug: 947806
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:

  mobile/locales/generic/profile/bookmarks.json.in

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:

      http://support.cdn.mozilla.net/static/img/firefox-64.png?v=1
      http://support.cdn.mozilla.net/static/img/firefox-32.png?v=1
      http://support.cdn.mozilla.net/static/img/firefox-16.png?v=1

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 bookmarks.json.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 bookmarks.json.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
Duplicate of this bug: 826087
I think we want this for 33 at the latest, so I'm stealing this.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Whiteboard: [lang=java][good first bug]
Experimentation suggests that this file does nothing useful, but untangling the build dependency on bookmarks.inc 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 bookmarks.json.in. v1

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

OK.

::: mobile/locales/generic/profile/bookmarks.json.in
@@ +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]:
  None.
Attachment #8457609 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/cadbfcd5e867
https://hg.mozilla.org/mozilla-central/rev/f35f7745d1b7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Attachment #8457609 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Finally.
Status: RESOLVED → VERIFIED
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
You need to log in before you can comment on or make changes to this bug.