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

Status: VERIFIED FIXED

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

Description

[Chris Kitching [:ckitching]]

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.

Comment 1

[Aaron Train [:aaronmt]]

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.

Comment 3

[:Margaret Leibovic]

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! :)

Comment 4

[Richard Newman [: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.

Comment 5

[Richard Newman [:rnewman]]

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.

Comment 6

[Zhanpeng Zeng (:marcus)]

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.

Comment 7

[Richard Newman [:rnewman]]

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.

Comment 8

[Zhanpeng Zeng (:marcus)]

(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

Comment 9

[Mike Hoye [:mhoye]]

Zhanpeng - are you working on this bug?

RNewman - can you take a look at the question in comment 8?

Comment 10

[Richard Newman [:rnewman]]

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

Comment 12

[Richard Newman [:rnewman]]

I think we want this for 33 at the latest, so I'm stealing this.

Comment 13

[Richard Newman [:rnewman]]

Attachment: Part 1: remove redundant data from bookmarks.json.in. v1

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?

Comment 14

[Richard Newman [:rnewman]]

Attachment: Part 2: bundle current SUMO favicon in default bookmarks. v1

Does exactly what it says on the tin.

Comment 15

[Nick Alexander :nalexander [he/him]]

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.

Comment 16

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/cadbfcd5e867
https://hg.mozilla.org/integration/fx-team/rev/f35f7745d1b7

Comment 17

[Richard Newman [:rnewman]]

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.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cadbfcd5e867
https://hg.mozilla.org/mozilla-central/rev/f35f7745d1b7

Comment 19

[Aaron Train [:aaronmt]]

Finally.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a48cbea17696
https://hg.mozilla.org/releases/mozilla-aurora/rev/81d3346f1ef6

Comment 21

[Teodora Vermesan (:TeoVermesan)]

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