Closed Bug 972138 Opened 10 years ago Closed 9 years ago

Default bookmarks' favicons should use the new M favicon, not the red dino logo


(Firefox :: Bookmarks & History, defect)

Not set



Firefox 38
38.1 - 26 Jan


(Reporter: cpeterson, Assigned: manan.doshi96, Mentored)



(Whiteboard: [good first bug][lang=xul])


(3 files, 3 obsolete files)

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
it's just matter of updating with new ICON datauris
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: p=0 → p=1
Points: --- → 1
Flags: qe-verify?
Whiteboard: p=1
A simple solution for this bug would be to download , 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)

Whiteboard: [good first bug][lang=xul]
Manan wants to work on this.
Assignee: nobody → manan.doshi96
Mentor: manishearth
Attached patch bookmark-icon-2.patch (obsolete) — Splinter Review
Attachment #8535532 - Flags: review?(manishearth)
Comment on attachment 8535532 [details] [diff] [review]

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:


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)
Attachment #8535532 - Flags: review?(manishearth) → feedback-
Attached patch bookmark-favicon.patch (obsolete) — Splinter Review
Fixed daturi
Attachment #8535532 - Attachment is obsolete: true
Attachment #8535559 - Flags: review?(manishearth)
Comment on attachment 8535559 [details] [diff] [review]

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

(patch applies onto previous patch, explained fix in chat)
Attachment #8535559 - Flags: review?(manishearth) → feedback-
Attached patch bookmark-icon-2.patch (obsolete) — Splinter Review
Attachment #8535559 - Attachment is obsolete: true
Attachment #8535565 - Flags: review?(manishearth)
Comment on attachment 8535565 [details] [diff] [review]

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

The commit message needs editing (you can edit this by hand), aside from that this seems good!
Attachment #8535565 - Flags: review?(manishearth)
Attachment #8535565 - Flags: review?(mak77)
Attachment #8535565 - Flags: feedback+
Comment on attachment 8535565 [details] [diff] [review]

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.
Attachment #8535565 - Flags: review?(mak77) → review+
Manish/Manan: did this get forgotten?
Flags: needinfo?(manishearth)
Flags: needinfo?(manan.doshi96)
Ah, sorry, somehow missed this (thought it still needed review, didn't get the email)
Flags: needinfo?(manishearth)
Keywords: checkin-needed
Wait, the commit message still needs fixing, sorry.

Manan, can you fix this?
Keywords: checkin-needed
Changed r=mak to r=jaws. Am I doing it right?
Attachment #8535565 - Attachment is obsolete: true
Flags: needinfo?(manan.doshi96)
Comment on attachment 8547135 [details] [diff] [review]
Bug 972138 - Changed default bookmarks' favicons to the new M favicon

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

LGTM. Needs a rubber-stamp from :jaws and we should be good to go.
Attachment #8547135 - Flags: review?(jaws)
Attachment #8547135 - Flags: feedback+
Attachment #8547135 - Flags: review?(jaws) → review+
Keywords: checkin-needed
Whiteboard: [good first bug][lang=xul] → [good first bug][lang=xul][fixed-in-fx-team]
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=xul][fixed-in-fx-team] → [good first bug][lang=xul]
Target Milestone: --- → Firefox 38
Iteration: --- → 38.1 - 26 Jan
Bug Verification-2015-14-01
QA Whiteboard: [good first verify]
Flags: qe-verify? → qe-verify-
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.
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:

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

This default bookmark has still no favicon. 
*Getting Started
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"?
Flags: needinfo?(cpeterson)
Blocks: 1171520
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. :)
Flags: needinfo?(cpeterson)
You need to log in before you can comment on or make changes to this bug.