Favicons should sniff the content type (using bug 247981 stuff) before saving the icon to bookmarks.html, to make sure we don't save invalid non-image data as an icon.
Created attachment 156379 [details] [diff] [review] favicon-sniffing-0.patch patch to add sniffing support.
15 years ago
Attachment #156379 - Flags: approval-aviary?
15 years ago
Status: NEW → ASSIGNED
Comment on attachment 156379 [details] [diff] [review] favicon-sniffing-0.patch firstname.lastname@example.org
Attachment #156379 - Flags: approval-aviary? → approval-aviary+
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
It looks like your changes to nsBookmarksService::UpdateBookmarkIcon also fixes the issue of the wrong MIME type being saved in bookmarks.html (eg, text/plain). See: http://bugzilla.mozilla.org/show_bug.cgi?id=174265#c85 Not sure if a seperate bug on the issue was filed elsewhere. Will try to verify this soon.
With todays builds, I can verify the issue I mentioned previously is now fixed. When we receive a BMP icon sent as text/plain, it is now listed in the bookmarks store with the proper type, image/x-icon. However, I noticed a seperate issue while testing this. When I initially bookmark a page, the icon isn't saved. Only after first loading the bookmark is the ICON= attribute added. If we're bookmarking a page that's currently displayed, the icon data should be available for saving in the initial bookmarks update. (and if not, refetech it).
Comment on attachment 156379 [details] [diff] [review] favicon-sniffing-0.patch >Index: src/nsBookmarksService.cpp >+ PRInt32 len = ((aIconDataLen + 2) / 3) * 4; This is not necessary at all. >+ nsString dataUri; >+ dataUri += NS_LITERAL_STRING("data:"); >+ dataUri += NS_ConvertASCIItoUTF16(aMIMEType); >+ dataUri += NS_LITERAL_STRING(";base64,"); >+ dataUri += NS_ConvertASCIItoUTF16(iconDataBase64, len); For the trunk, it'd have been better to use AppendLiteral and AppendASCII in place of '+= NS_LITERAL_STRING' and '+= NS_ConvertASCIItoUTF16'. For the branch, 'dataUri += NS_ConvertASCIItoUTF16(,)' should have been replaced by 'AppendASCIItoUTF16(icondDataBase64)'.
(In reply to comment #6) > For the branch, 'dataUri += NS_ConvertASCIItoUTF16(,)' should have been > replaced by 'AppendASCIItoUTF16(icondDataBase64)'. I meant 'AppendASCIItoUTF16(icondDataBase64, dataUri);' (note PL_base64Encode ? returns a null-terminated string, which is why you don't need to calculate 'len'). The same is true of aMimeType.
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → bookmarks
You need to log in before you can comment on or make changes to this bug.