972138 - Default bookmarks' favicons should use the new mozilla.org M favicon, not the red dino logo

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: screenshot-of-menu-favicons.png

Please see the attached screenshot of the Bookmarks menu's incorrect favicons. If you visit the links, then the destination pages' new favicon will replace the obsolete favicons (as seen for the "About Us" bookmark in the screenshot).

These default bookmarks have red dino heads, but should have M logo:
• About Us
• Get Involved
• Customize Firefox
• Help and Tutorials

This default bookmark has no favicon, but should have a Firefox logo:
• Getting Started

Comment 1

[Marco Bonardo [:mak]]

it's just matter of updating mxr.mozilla.org/mozilla-central/source/browser/locales/generic/profile/bookmarks.html.in with new ICON datauris

Comment 2

[Manish Goregaokar [:manishearth]]

A simple solution for this bug would be to download https://www.mozilla.org/favicon.ico , convert it to a base64 datauri, and replace the four datauris in [1] with it.

Using command line tools I was able to produce a datauri[2] which seems to work, however it seems to be uncompressed or something (lots of repetitive characters), so I suggest trying to use one of those online datauri generator tools and see if you come up with something better (or converting it to compressed png first)


 [1]: http://mxr.mozilla.org/mozilla-central/source/browser/locales/generic/profile/bookmarks.html.in
 [2]: http://pastebin.com/UBeFjUDz

Comment 3

[Manish Goregaokar [:manishearth]]

Manan wants to work on this.

Comment 4

[Manan Doshi]

Attachment: bookmark-icon-2.patch

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8535532 [details] [diff] [review]
bookmark-icon-2.patch

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

You're on the right track, but the image is wrong. I'm getting two superimposed Ms of different sizes while opening the link:

data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAABmJLR0QA/wD/AP+gvaeTAAAACXBIWXMAAAsTAAALEwEAmpwYAAAAB3RJTUUH3gwMCgonObRdcQAAAehJREFUWMPtlTFrFFEUhb/zkEisxFIwICz4A7Swt3ay4C7R0kaxtLK2tbKzEGKdZB+6b8BGsbbRX6BoESyFFEEQecdiZhJ3ZiKJWwg6p1iYmTdzvnfveXfhf5cAivWJbSP0OKXZ/WJ9YgCq362UZjeL8fQ2OW8SAuQM0uc0n11cGqAoJs8JGpONMUKVcwg45+bqu2AFhIKwjSSc87uU4pVlAALiJTlXNCHgyvwh9jMBSAitICFpE/MUg7ORwuVlK3AK87ruBK5A3jvnu8C+EEhgY/uL8S1gtepdVYm2rhc3rgJPgEvNWmAP+ATcK1N8283AeOrapNUgfZV0rv1MNTAC2x/LFEe1+RYwbb57hLbLFDc6IZSqd3LdjraZWQRo8lLf/1A/Gh2z8g/KFB9VGajTbhtnd8379hLUvj06gTnAncMQNjIoLH7W9RGod+m6l5aPZfLt4CB3db4LENSpADYKQogyxVCmeBZY6wvfL3oDXChTPAOsAT961qy2AAzZELr1bkOVKe5W69SpGLBRpnitTHH3YC3s/34OAISaI/ft7OjdtgNbprh98kG0EIL+ad2ckEPnqloKYen/ggCQXuzI0DdY9lKaaT7f0WJN/Mo5N4NreYC/qQFgABgABoAB4E8BTjNo0L+in3/P0tKi9oPpAAAAAElFTkSuQmCC

what steps did you use to make this?

(Also, you should change your commit message to be r=mak or something, I don't have review powers)

Comment 6

[Manan Doshi]

Attachment: bookmark-favicon.patch

Fixed daturi

Comment 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8535559 [details] [diff] [review]
bookmark-favicon.patch

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

(patch applies onto previous patch, explained fix in chat)

Comment 8

[Manan Doshi]

Attachment: bookmark-icon-2.patch

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8535565 [details] [diff] [review]
bookmark-icon-2.patch

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

The commit message needs editing (you can edit this by hand), aside from that this seems good!

Comment 10

[Manish Goregaokar [:manishearth]]

Attachment: Screenshot of new icon in action

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8535565 [details] [diff] [review]
bookmark-icon-2.patch

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

The commit message on the patch has two near duplicate lines. It should also be changed to have r=jaws instead of r=mak.

The base64 image is only the 16x16 resolution, so it is lacking a HiDPI variant. It would be nice to add this, although I'm not sure how to do that with base64 images. Regardless, this is not a regression from the previous icon in the tree, so I'll approve this change.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Manish/Manan: did this get forgotten?

Comment 13

[Manish Goregaokar [:manishearth]]

Ah, sorry, somehow missed this (thought it still needed review, didn't get the email)

Comment 14

[Manish Goregaokar [:manishearth]]

Wait, the commit message still needs fixing, sorry.


Manan, can you fix this?

Comment 15

[Manan Doshi]

Attachment: Bug 972138 - Changed default bookmarks' favicons to the new mozilla.org M favicon

Changed r=mak to r=jaws. Am I doing it right?

Comment 16

[Manish Goregaokar [:manishearth]]

Comment on attachment 8547135 [details] [diff] [review]
Bug 972138 - Changed default bookmarks' favicons to the new mozilla.org M favicon

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

LGTM. Needs a rubber-stamp from :jaws and we should be good to go.

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 19

[Ashwini K H]

Bug Verification-2015-14-01

Comment 20

[Fariha Afrin]

I've tried to reproduce this Bug on Firefox nightly 
41.0a1 (2015-05-21) version.
Build ID 20150521030204
I also find the same thing as I selected the default bookmark it has no favicon, but it  should have a Firefox logo at 'Getting Started' option.

Comment 21

[Muktasib Un Nur]

Tested this bug in,
OS: 		Windows 7 (64bit)

Version : 	Nightly 41.0a1 (2015-05-21)
User Agent :	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID :	20150521030204

Version :	Dev Edition 40.0a2 (2015-05-21)
User Agent :	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID :	20150521004003

Version :	Beta 38.0.5
User Agent : 	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID :	20150518141916

It’s partly fixed.  Details:

[[[FIXED]]]
These default bookmark icons have been fixed with “m” icon istead of dino icon
*About Us
*Get Involved
*Customize Firefox
*Help and Tutorials

[[[NOT FIXED]]]
This default bookmark has still no favicon. 
*Getting Started

Comment 22

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

According to comment 20 and comment 21 above, the red dino icons have correctly been replaced by the "m" icon. However, there is still no Firefox logo for "Getting Started". 

Chris, do we need (or is there already) a follow up bug on file for showing the Firefox logo for "Getting Started"?

Comment 23

[Chris Peterson [:cpeterson]]

Florian, thanks for reminding me! I missed the notes above about the "Getting Started" favicon not being fixed.

We should probably file a new bug for the "Getting Started" favicon because this bug has a long history. I filed bug 1171520 for this issue and needinfo'd Manan, asking if he has time to fix that favicon. :)