+++ This bug was initially created as a clone of Bug #110894 +++ This bug is to add an about:config option to disable having the favicon used as the webpage shortcut. Some users think it's too small to use and some like to copy the favicons to a USB drive and move them computers.

Regarding the amount of good work that some people invested into Bug 110894, wouldn't it be good not to just add an option to disable this again but also add an option that sets the proportion of icon and white-fill so that we just could start using and loving these icons as a feature?

Setting the base icon size to resize to sounds feasible and would accomplish this. The default would be set to the current icon size on the shortcut icons.

> Since updating to Firefox 17, shortcut desktop icons have reverted to generic windows icons I get exactly the same. This is WORSE than the original which just gave a FF icon for everything, now not only do we not get the FavIcon, we don't get any !

I just registered to cast my vote to this bug. Actually, IMHO, the importance of it should be upgraded from "enhancement" to something higher as the new mechanism is broken in Windows XP and the new metroish icons with the tiny favicon encrusted in the middle are just plain ugly, at least as I see them on my Windows 7 system with a 1680x1050 22" monitor.

I don't know how to vote--I tried and it didn't seem to click --I vote to fix this too --it's kept me from updating from 16.02

Awesome if this could be fixed!

I really hope this can be resolved, as well. The generic Windows-icon shortcuts produced in FF 17 (and presumably 18) on an XP system just do not do the trick, and frankly look awful. I couldn't agree more with N. Brooks' statement, "This is WORSE than the original which just gave a FF icon for everything, now not only do we not get the FavIcon, we don't get any!" I wasn't expecting Mozilla to get more than a one-year headstart on Microsoft's cessation of extended support for XP users. ;) I am sticking with 16.02 for now, although it makes me a bit uncomfortable. Any forthcoming resolution or configuration workaround for this issue would be greatly appreciated!

Hello there, My friend and I are interested in helping out. Is this bug still active? If so, where/how do we begin? Thanks.

Yup it's still valid, thanks for offering to help! Let's start with just adding the preference to drop the functionality but leave the functionality as is for the default. We'll use follow up bugs for other ideas discussed in this bug after this which you can optionally take. So here's an example of how to obtain a preference: http://dxr.mozilla.org/mozilla-central/content/media/DecoderTraits.cpp.html#l159 The lines of code with the "-" in front of them here is what we want to do when that preference is set: https://hg.mozilla.org/mozilla-central/diff/aa003aa3a18f/widget/windows/nsDataObj.cpp#l1.58 https://hg.mozilla.org/mozilla-central/diff/aa003aa3a18f/widget/windows/nsDataObj.cpp#l1.98 The other lines close to that with the "+" is what we want to no longer do when the preference is set.

+1 to display at least a default FF icon once again for XP users please. I don't require a favicon per site myself but understand why some users may find it useful to do so. On this basis I also support the suggestion to make it user configurable through about:config

Hi Josh, are you still interested in working on this? If you need any help with anything just let me know here, or you can message bbondy on IRC or you can email me at netzen@gmail.com.

Yes, my friend and I are still working on this! We've just had a lot of obligations to work on. I've built the mozilla source code and everything. So, I'll be working on this over the weekend.

Sounds great! Thanks for the update!

I think for the most part I understand the process that I need to do. Although, was there a particular file of code that I am supposed to look at? (Or at least start with?)

Yup, please see comment 9. Let me know if something in particular is not clear from that comment.

First patch submit ever! I think I did it right?

Comment on attachment 707437 [details] [diff] [review] Patch#1 first attempt Review of attachment 707437 [details] [diff] [review]: ----------------------------------------------------------------- Good job submitting your first patch! I think maybe you have 2 patches in your patch queue though. Can you go to your srcdir and type: hg qapplied I it lists more than one patch can you do: hg qpop hg qfold name_of_patch_that_was_popped.diff Then if you have only one patch left, re-attach to this bug and obsolete the one posted. The reason I ask is because it looks like the diff is not made from m-c but based on some other changes.

hmm, I'll try again now. Hopefully it'll work this time

Comment on attachment 708022 [details] [diff] [review] Attempt to patch #2, hopefully it works this time! Review of attachment 708022 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I didn't see this attachment until now, please flag for "feedback?" or "review?" next time to be sure I'll see it, thanks! I'll get this reviewed today.

Comment on attachment 708022 [details] [diff] [review] Attempt to patch #2, hopefully it works this time! Review of attachment 708022 [details] [diff] [review]: ----------------------------------------------------------------- Looks like you're almost there, good work. Please re-add a patch and then request feedback from me once more. ::: widget/windows/nsDataObj.cpp @@ +1103,5 @@ > // will need to change if we ever support iDNS > nsAutoCString asciiUrl; > LossyCopyUTF16toASCII(url, asciiUrl); > > + //-------------------modifications done here!---------------------------------------------- nit: Please remove these lines, the patch file itself shows where the modifications are. @@ +1104,5 @@ > nsAutoCString asciiUrl; > LossyCopyUTF16toASCII(url, asciiUrl); > > + //-------------------modifications done here!---------------------------------------------- > + if (!Preferences) You need to actually check a real new preference here. In Comment 9 I provided a code example link for how to do that. @@ +1112,5 @@ > + const int totalLen = formatLen + asciiUrl.Length(); // we don't want a null character on the end > + } > + > + else > + { nit: Please reformat to: if (...) { } else { } @@ +1113,5 @@ > + } > + > + else > + { > + nsCOMPtr<nsIFile> icoFile; nit: Please align code via 2 spaces and to the surrounding block. @@ +1138,2 @@ > > +//-----------------modifications end here!----------------------------------------------------- ditto remove these lines. @@ +1153,5 @@ > // terminate strings which reach the maximum size of the buffer. Since we know that the > // formatted length here is totalLen, this call to _snprintf will format the string into > // the buffer without appending the null character. > + > +// modifications start here ------------------------------------------------------ ditto remove line @@ +1163,5 @@ > + else > + { > + _snprintf( contents, totalLen, shortcutFormatStr, asciiUrl.get(), path.get() ); > + } > +// modifications end here ------------------------------------------------------- ditto remove line

It would be very good to have an option in about:config which will turn on/off this functionality. I mean if it's "false", for example, then firefox will not write any extra data in .URL files, just address: [InternetShortcut] URL=http://example.com/foo?bar=baz

grr.. the differences aren't showing again in this patch... working on that

Comment on attachment 712366 [details] [diff] [review] resubmitting patch so that changes show Review of attachment 712366 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/nsDataObj.cpp @@ +1110,5 @@ > NS_NewURI(getter_AddRefs(aUri), url); > > + if (!Preferences::GetBool("browser.mozilla.favicon", false)) { > + static const char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n"; > + static const int formatLen = strlen(shortcutFormatStr) - 2; // don't include %s in the lne Since these can sometimes change between function calls now please remove the word static for both shortcutFormatStr and formatLen here and below. @@ +1111,5 @@ > > + if (!Preferences::GetBool("browser.mozilla.favicon", false)) { > + static const char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n"; > + static const int formatLen = strlen(shortcutFormatStr) - 2; // don't include %s in the lne > + const int totalLen = formatLen + asciiUrl.Length(); // we don't want a null character on the end Before the if statement above please put: int totalLen; Then set totalLen in both this block and the else block. The reason is because totalLen falls out of scope as you have it and won't compile. @@ +1151,5 @@ > // terminate strings which reach the maximum size of the buffer. Since we know that the > // formatted length here is totalLen, this call to _snprintf will format the string into > // the buffer without appending the null character. > + > + if (!Preferences::GetBool("browser.mozilla.favicon", false)) { Let's call this: browser.shell.favicon Also please put this at the top of the file: static const char * kShellIconPref = "browser.shell.favicon" And then use kShellIconPref at both places that you put the string.

Im sorry! I did it wrong again!

Comment on attachment 712633 [details] [diff] [review] should work this time! Review of attachment 712633 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/nsDataObj.cpp @@ +1113,5 @@ > + int totalLen; > + if (!Preferences::GetBool(kShellIconPref, false)) { > + const char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n"; > + const int formatLen = strlen(shortcutFormatStr) - 2; // don't include %s in the lne > + const int totalLen = formatLen + asciiUrl.Length(); // we don't want a null character on the end The totalLen outside of this scope won't be set. Please change to: totalLen = formatLen + asciiUrl.Length(); and also the same below. Thanks!

Comment on attachment 712750 [details] [diff] [review] fixed the problems from comment #30 Review of attachment 712750 [details] [diff] [review]: ----------------------------------------------------------------- The structure of the fix looks correct to me, but there are some issues causing it to not compile. By the way if you want to build in a faster way just build these 2 sub-directories: build inside mozillasrcdir/objdir/widget build inside mozillasrcdir/objdir/toolkit/library You can see most build problems by redirecting the output to a file and then searching inside that file after build for ": error" (without quotes) ::: widget/windows/nsDataObj.cpp @@ +28,5 @@ > #include "prtypes.h" > #include "nsDirectoryServiceDefs.h" > #include "nsITimer.h" > #include "nsThreadUtils.h" > +#include "Preferences.h" I think you need this to compile: #include "mozilla/Preferences.h" @@ +1111,5 @@ > NS_NewURI(getter_AddRefs(aUri), url); > > + int totalLen; > + if (!Preferences::GetBool(kShellIconPref, false)) { > + const char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n"; Just noticed that shortcutFormatStr is used outside of this scope, so you have to do like you did totalLen and bring it outside to the parent scope. It won't compile as is. @@ +1126,3 @@ > > + nsresult rv = mozilla::widget::FaviconHelper::GetOutputIconPath(aUri, icoFile, true); > + NS_ENSURE_SUCCESS(rv, rv); This won't compile because the return type is different, please change to: NS_ENSURE_SUCCESS(rv, E_FAIL); @@ +1126,4 @@ > > + nsresult rv = mozilla::widget::FaviconHelper::GetOutputIconPath(aUri, icoFile, true); > + NS_ENSURE_SUCCESS(rv, rv); > + nsCString path; You'll have to move "nsCString path;" to the parent scope or else the block below won't see it at compile time. @@ +1127,5 @@ > + nsresult rv = mozilla::widget::FaviconHelper::GetOutputIconPath(aUri, icoFile, true); > + NS_ENSURE_SUCCESS(rv, rv); > + nsCString path; > + rv = icoFile->GetNativePath(path); > + NS_ENSURE_SUCCESS(rv, rv); NS_ENSURE_SUCCESS(rv, E_FAIL); @@ +1131,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + const char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n" > + "IDList=\r\nHotKey=0\r\nIconFile=%s\r\n" > + "IconIndex=0\r\n"; ditto here. @@ +1132,5 @@ > + > + const char* shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n" > + "IDList=\r\nHotKey=0\r\nIconFile=%s\r\n" > + "IconIndex=0\r\n"; > + const int formatLen = strlen(shortcutFormatStr) - 2*2 // don't include %s (2 times) in the len This won't compile because of a missing semicolon after 2*2; While you're on this line please also fix the formatting to put a space before and after the * @@ +1134,5 @@ > + "IDList=\r\nHotKey=0\r\nIconFile=%s\r\n" > + "IconIndex=0\r\n"; > + const int formatLen = strlen(shortcutFormatStr) - 2*2 // don't include %s (2 times) in the len > + totalLen = formatLen + asciiUrl.Length() > + + path.Length() // we don't want a null character on the end ditto: missing a semicolon.

Comment on attachment 712982 [details] [diff] [review] fixes made from comment #32 Review of attachment 712982 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/nsDataObj.cpp @@ +1109,5 @@ > nsCOMPtr<nsIFile> icoFile; > nsCOMPtr<nsIURI> aUri; > NS_NewURI(getter_AddRefs(aUri), url); > > + char shortcutFormatStr; Almost there but this still won't compile because this needs to be a const char pointer: - char shortcutFormatStr; + const char *shortcutFormatStr;

Thanks this works now and testing went well. I made a couple small nit changes for you and I'll push it to the try server. If it passes then I'll land the patch and you'll have landed your first patch! Nits changed: - Changed pref name to browser.shell.shortcutFavicons - Added commit message - Fixed a couple of formatting nits - Set default of pref to true

Congratulations on your first patch landing and thanks for the contribution. https://hg.mozilla.org/integration/mozilla-inbound/rev/696dc68a45e7

Congratulations and many thanks to Josh and Brian for creating/mentoring this fix. I see that the target rev is Firefox 21. Honestly I was hoping for an earlier release. It's a real pity that I'll need to skip 4 major releases before upgrading (assuming that no other "improvement" of this kind happens in the meanwhile):-/ Again, my sincere thank-you to everybody who contributed.

Hi smz, This is the normal process that every bug follows. There are special cases for security bugs and really serious regressions where they can be requested to be uplifted to aurora / beta or even release for really serious things, but this bug won't meet that criteria. I know some people will think of this as a regression but I do not think it would be approved. If you need this fix early though the best bet would be to use Nightly (which already has the fix) or Aurora which will have the fix soon.

Since this bug is fixed (thanks to all who were involved) I wonder what will happen to the basic goal of bug 110894. That bug's target was to have favicons on desktop shortcuts. The result were these tiny white-spaced shortcuts icons. That led to this bug which made the result of the first bug optional. So we are still without usable favicons on desktop shortcuts which is a pity. What about my Comment #1 (https://bugzilla.mozilla.org/show_bug.cgi?id=827784#c1)?

Hi all, It's been educative and fascinating to follow how you coder folks collaborate. I had no idea of the work and mentoring that goes into fixing bugs. Thanks so much to all involved for your time and effort!!! I echo smz's comment about having to put my favorite browser's updates on hold until FF v21. I'll try using the Nightly build alternative.

Hey Roasted, one of the problems with that is that many (possibly most?) sites only provide their favicons in 16x16 format. Furthermore we only have support for extracting the 16x16 ones in our /gfx code. Resizing these icons looks pixelated and bbad. There is a bug to fix the icon extraction for any requested size but I don't think it was implemented yet. And as mentioned it wouldn't work across all sites because some sites don't provide all icon sizes. Could you post a new bug with your suggestion in Comment 1. I don't mind mentoring such a bug to add an about:config option to control that.

I would like to request that there be a user or About:config option which allows for dragged urls to be created with only the MINIMUM necessary data. That is, just the single line from your code snippet reference page, https://hg.mozilla.org/mozilla-central/diff/aa003aa3a18f/widget/windows/nsDataObj.cpp#l1.58 , as: shortcutFormatStr = "[InternetShortcut]\r\nURL=%s\r\n" Which creates a two-line, reliable url file. In my case, not only is the extra code causing the Windows OS to interpret it as a generic icon, but I wish to have a plain and simple url data file that I can RELIABLY transport between computers running different OS versions, and even different Mac OS versions. I have found that the old-style Windows minimal URL (two data lines) works well in this regard. Please consider retaining this feature of simplicity - essentially a request to not break something that was working very well, in an attempt to make it somehow cooler... Thank you.

Hi Bruce, Which of these lines are causing problems for you? - HotKey= - IconFile= - IconIndex= Could you try a bit more saving a different filename each time so Windows won't use a cached version? We can probably simplify the pref'ed version that was created in this bug. Also please let me know which OS you are using.

OK, FF 21.0 is out and the browser.shell.shortcutFavicons option has been implemented, *BUT* it doesn't comes pre-configured in about:config, not even with its supposed "true" default status. You must manually add the config string in about:config and IMHO this makes the option unacceptably hidden. I would like to ask to re-open this ticket and have the option pre-configured and visible in about:config. This is the minimum, I think: in reality I'd love to have this option exposed in Options->Options->Advanced

Hi smz, if you'd like to have this option listed in about:config that's acceptable and you can post a new bug for that, it's a really quick change. But I don't think it's a good idea to add an option for something obscure like this, that most users won't want to change inside the options UI.

To Brian R. Bondy [:bbondy] Actually you are WRONG. *ALL* WindowsXP users would like to switch it off, coz under that OS, favicons in shortcut doesn't work at all and we see just default windows system icon.

In that case please post a bug to disable it by default on Windows XP.

Actually the bug is reported since 2013-01-09 828284

sorry, bug 828284 (there is also explanation what is going on in comment 1)

Awesome, thanks so much for the insight! I put info on how to fix the bug in bug 828284 and put myself as the mentor. Usually a community member will come along relatively quickly to work on mentored bugs.

Initially I was about posting this in https://bugzilla.mozilla.org/show_bug.cgi?id=827784, but as it is now flagged as "fixed" and this "bug" seems to be the active one about issues related to the shortcuts-with-favicon issue, I'm posting here hoping to be not totally "off topic". Now, sorry, I don't want to step on anyone's toes, but I would set it as the default on any OS. Actually I can't talk about "any OS", but while it is a given fact that it is a real bug to try to use the "favicon shortcut" on WinXP, I find it particularly ugly and bad implemented on Win7 as well. I'll try to justify such a "strong" wording and please keep in mind that while some absolute measures in the following analysis may differ according to your setting, the relative ones should not: Point 1: With Win7 set up to show "small icons" (I'm allowed to do it, I can see them, so I think it is kosher for me to do it): - A FF shortcut is displayed as 37x37 pixels white square. - Within it I have a 13x13 "favicon" - So on each side of the favicon I have a 12 pixels "border" - The area covered by the favicon is about 12.3% of the square shortcut area - I find this to be bad "real estate management" Point 2: Shortcuts favicons are stored "off line"(pardon the inaccurate term) in /%userprofile%/AppData/..../shortcutCache and not "in line" (somehow serialized) within the shortcut itself. Whenever I copy/move/send my shortcut out of the domain of my user account, I am copying/moving/sending an "half baked" shortcut, castrated of much of its meaning. Point 3: When I delete a FF shortcut, the corresponding .ico file in /%userprofile%/AppData/..../shortcutCache is not removed, potentially leaving behind a lot of garbage in my profile So, this is what I think: *IF* we want favicons in shortcut's icons (I can trace a request for this to https://bugzilla.mozilla.org/show_bug.cgi?id=110894 reported on 2001-11-19, and I’m not "a priori" against that), well, please, let us implement it in "nice" and "correct" way, otherwise let's go back to the generic FF icon. Let me also state this: I can't talk "about others" in a general way, but all the people I've talked to really do *hate* the new "tiny favicon" shortcuts, so I don't think I'm part of minority of "nuts" (but maybe I’m wrong...) My apologies to everybody if I’ve been to strong with my words: English is not my mother tongue so I may have used impolite wording.

OMG! I made a mistake: I was willing to post my previous comment in https://bugzilla.mozilla.org/show_bug.cgi?id=828284 Brian, is it OK for me to re-post there? Sorry for mixing things up: I had toomany tabs open... :-(