Last Comment Bug 827784 - Provide an option to disable favicons on webpage shortcuts in Windows
: Provide an option to disable favicons on webpage shortcuts in Windows
Status: RESOLVED FIXED
[mentor=bbondy][lang=c++]
:
Product: Firefox
Classification: Client Software
Component: Shell Integration (show other bugs)
: Trunk
: x86 Windows 2000
: -- enhancement with 9 votes (vote)
: Firefox 21
Assigned To: Josh Yuan
:
Mentors:
Depends on: 110894
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-08 05:12 PST by Brian R. Bondy [:bbondy]
Modified: 2013-05-15 18:37 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch#1 first attempt (6.48 KB, patch)
2013-01-28 21:34 PST, Josh Yuan
no flags Details | Diff | Splinter Review
Attempt to patch #2, hopefully it works this time! (4.14 KB, patch)
2013-01-30 00:55 PST, Josh Yuan
netzen: feedback+
Details | Diff | Splinter Review
fixed my errors from previous patch (4.32 KB, patch)
2013-02-10 16:12 PST, Josh Yuan
no flags Details | Diff | Splinter Review
resubmitting patch so that changes show (4.15 KB, patch)
2013-02-11 00:46 PST, Josh Yuan
no flags Details | Diff | Splinter Review
changed according to above requirements (112 bytes, patch)
2013-02-11 14:30 PST, Josh Yuan
no flags Details | Diff | Splinter Review
should work this time! (4.52 KB, patch)
2013-02-11 14:35 PST, Josh Yuan
netzen: feedback+
Details | Diff | Splinter Review
fixed the problems from comment #30 (4.49 KB, patch)
2013-02-11 18:57 PST, Josh Yuan
no flags Details | Diff | Splinter Review
fixes made from comment #32 (4.49 KB, patch)
2013-02-12 10:17 PST, Josh Yuan
no flags Details | Diff | Splinter Review
fixed issues from comment #34 (4.50 KB, patch)
2013-02-12 11:19 PST, Josh Yuan
no flags Details | Diff | Splinter Review
Patch (4.52 KB, patch)
2013-02-12 16:50 PST, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Splinter Review

Description Brian R. Bondy [:bbondy] 2013-01-08 05:12:09 PST
+++ 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.
Comment 1 Roasted 2013-01-08 08:08:02 PST
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?
Comment 2 Brian R. Bondy [:bbondy] 2013-01-08 08:09:42 PST
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.
Comment 3 N Brooks 2013-01-09 10:17:56 PST
> 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 !
Comment 4 smz 2013-01-09 14:12:30 PST
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.
Comment 5 Peter Baerwald 2013-01-09 22:09:48 PST
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
Comment 6 noozillander 2013-01-09 22:42:04 PST
Awesome if this could be fixed!
Comment 7 pell_lee_pell 2013-01-09 23:44:25 PST
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!
Comment 8 Josh Yuan 2013-01-15 22:01:10 PST
Hello there,

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

Thanks.
Comment 9 Brian R. Bondy [:bbondy] 2013-01-16 05:32:18 PST
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.
Comment 10 Colin Templeman 2013-01-25 04:48:27 PST
+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
Comment 11 Brian R. Bondy [:bbondy] 2013-01-25 05:15:37 PST
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.
Comment 12 Josh Yuan 2013-01-25 13:20:31 PST
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.
Comment 13 Brian R. Bondy [:bbondy] 2013-01-25 17:42:28 PST
Sounds great! Thanks for the update!
Comment 14 Josh Yuan 2013-01-26 18:04:50 PST
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?)
Comment 15 Brian R. Bondy [:bbondy] 2013-01-26 18:27:54 PST
Yup, please see comment 9.  Let me know if something in particular is not clear from that comment.
Comment 16 Josh Yuan 2013-01-28 21:34:42 PST
Created attachment 707437 [details] [diff] [review]
Patch#1 first attempt

First patch submit ever!  I think I did it right?
Comment 17 Brian R. Bondy [:bbondy] 2013-01-29 06:59:52 PST
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.
Comment 18 Josh Yuan 2013-01-29 23:22:46 PST
hmm, I'll try again now.  Hopefully it'll work this time
Comment 19 Josh Yuan 2013-01-30 00:55:17 PST
Created attachment 708022 [details] [diff] [review]
Attempt to patch #2, hopefully it works this time!
Comment 20 Brian R. Bondy [:bbondy] 2013-02-08 05:30:46 PST
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 21 Brian R. Bondy [:bbondy] 2013-02-08 07:23:58 PST
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
Comment 22 riokami 2013-02-10 04:07:36 PST
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
Comment 23 Josh Yuan 2013-02-10 16:12:38 PST
Created attachment 712307 [details] [diff] [review]
fixed my errors from previous patch
Comment 24 Josh Yuan 2013-02-10 16:13:48 PST
grr.. the differences aren't showing again in this patch... working on that
Comment 25 Josh Yuan 2013-02-11 00:46:47 PST
Created attachment 712366 [details] [diff] [review]
resubmitting patch so that changes show
Comment 26 Brian R. Bondy [:bbondy] 2013-02-11 06:44:16 PST
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.
Comment 27 Josh Yuan 2013-02-11 14:30:34 PST
Created attachment 712629 [details] [diff] [review]
changed according to above requirements
Comment 28 Josh Yuan 2013-02-11 14:31:33 PST
Im sorry! I did it wrong again!
Comment 29 Josh Yuan 2013-02-11 14:35:20 PST
Created attachment 712633 [details] [diff] [review]
should work this time!
Comment 30 Brian R. Bondy [:bbondy] 2013-02-11 15:23:18 PST
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 31 Josh Yuan 2013-02-11 18:57:10 PST
Created attachment 712750 [details] [diff] [review]
fixed the problems from comment #30
Comment 32 Brian R. Bondy [:bbondy] 2013-02-12 08:41:47 PST
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 33 Josh Yuan 2013-02-12 10:17:42 PST
Created attachment 712982 [details] [diff] [review]
fixes made from comment #32
Comment 34 Brian R. Bondy [:bbondy] 2013-02-12 10:30:50 PST
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;
Comment 35 Josh Yuan 2013-02-12 11:19:51 PST
Created attachment 713015 [details] [diff] [review]
fixed issues from comment #34
Comment 36 Brian R. Bondy [:bbondy] 2013-02-12 16:50:17 PST
Created attachment 713188 [details] [diff] [review]
Patch

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
Comment 37 Brian R. Bondy [:bbondy] 2013-02-13 06:29:28 PST
Congratulations on your first patch landing and thanks for the contribution.
https://hg.mozilla.org/integration/mozilla-inbound/rev/696dc68a45e7
Comment 38 Ed Morley [:emorley] 2013-02-14 03:04:36 PST
https://hg.mozilla.org/mozilla-central/rev/696dc68a45e7
Comment 39 smz 2013-02-14 03:56:02 PST
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.
Comment 40 Brian R. Bondy [:bbondy] 2013-02-14 04:22:32 PST
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.
Comment 41 Roasted 2013-02-14 05:43:08 PST
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)?
Comment 42 noozillander 2013-02-14 08:12:38 PST
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.
Comment 43 Brian R. Bondy [:bbondy] 2013-02-14 08:24:55 PST
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.
Comment 44 Bruce Young 2013-03-06 04:13:02 PST
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.
Comment 45 Brian R. Bondy [:bbondy] 2013-03-06 06:15:11 PST
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.
Comment 46 smz 2013-05-15 04:30:05 PDT
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
Comment 47 Brian R. Bondy [:bbondy] 2013-05-15 05:40:06 PDT
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.
Comment 48 exie 2013-05-15 12:59:14 PDT
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.
Comment 49 Brian R. Bondy [:bbondy] 2013-05-15 13:23:50 PDT
In that case please post a bug to disable it by default on Windows XP.
Comment 50 exie 2013-05-15 13:26:36 PDT
Actually the bug is reported since 2013-01-09  828284
Comment 51 exie 2013-05-15 13:29:24 PDT
sorry, bug 828284
(there is also explanation what is going on in comment 1)
Comment 52 Brian R. Bondy [:bbondy] 2013-05-15 13:37:35 PDT
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.
Comment 53 smz 2013-05-15 18:22:48 PDT
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.
Comment 54 smz 2013-05-15 18:37:24 PDT
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... :-(

Note You need to log in before you can comment on or make changes to this bug.