828284 - Force shortcut desktop icons off on Windows XP

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[noozillander]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121010144125

Steps to reproduce:

Windows XP SP3. Updated FF from 16.0.1 to 17. All desktop icons with the familiar FF shortcut icons to websites now look like default windows system icons. No amount of fiddling/repairing brought back the familiar FF icons. Icons could be reset individually via Folder Options but this proved to be tedious and not sticky.


Actual results:

Gave up and reverted to FF 16.0.1. Icons ok again.
A number of people with the same problem have posted their concerns on the Firefox Support Forum, which seems to be invisible to the FF folks who might be able to fix this bug. See here: https://support.mozilla.org/en-US/questions/942316?page=2#answer-394644


Expected results:

FF shortcut icons should display consistently regardless of newer FF versions, as they used to up to 16.0.1

Comment 1

[exie]

same problem as bug 820123 affecting all FF/SeaMonkey versions with implemeted use of favicons on webpage shortcuts in Windows under Windows XP.

The ability to read PNG images from ICO and CUR format images was introduced in Windows Vista, Windows XP can't cope with such files and replace the icon with the default windows system icon.

Even if the problem will be solved somehow, there should be option in about:config to disable such questionable feature like suggested in bug 827784

Comment 2

[Scoobidiver (away)]

It's also confirmed on the French support forum: http://forums.mozfr.org/viewtopic.php?f=5&t=112636 and http://forums.mozfr.org/viewtopic.php?f=5&t=111095.

Can you try Firefox 21 Beta (http://www.mozilla.org/firefox/beta/) that contains the fix of bug 827784 and set browser.shell.shortcutFavicons to false in about:config?

Comment 3

[exie]

I just installed FF 21.0, added the settings to config:about and it work. We see the nice FF icon again. Too bad the options is not predefined in about:config with true state, so it can be just switched to false.

Comment 4

[Brian R. Bondy [:bbondy]]

Mentored info:
We want to change this condition to always enter when the user is using anything less than Vista:

http://dxr.mozilla.org/mozilla-central/widget/windows/nsDataObj.cpp#l1117

The check for OS version should be done like this:

if (... || !IsVistaOrLater()) {

Comment 5

[Brian R. Bondy [:bbondy]]

(In reply to smz from comment #53)
> 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 6

[Brian R. Bondy [:bbondy]]

Personally I'd be fine with changing the default everywhere, unfortunately I'm not sure how to measure user perception for this feature.  I'm leaning towards just disabling it everywhere though by default.

Comment 7

[smz]

Brian,

I agree with you: as it is now it would be much better to turn off the shortcuts-with-favicons feature by default.

I don't think many like to have their FF profile littered by dead .ico files and/or have internet shortcuts with a generic icon when moved to a different system or when the profile is cleaned/changed.

It will be different if icons were stored the same way that IE does (used to do?) when it create .URL files, that is storing the icon in the :favicon:$DATA alternate data stream of the .URL file.

Alternate data streams have their shortcomings too: when you move such an .URL files through a files system that doesn't support ADS (e.g. FAT32) you loose your icon but you are warned about the loss and the .URL file revert to its default icon, based on your browser choice (generic FF icon in our case)

Of course all of the above is true in the Windows platform and things may be (a lot) different in other cases.

It is also not clear to me what the implications are for Win8. We are anyway talking about .url files and not .website files, so *I think* there should be a total compatibility.

About your comment about user perception, yes, I understand it is now difficult to measure it, but... was that a concern when the shortcuts-with-icons were implemented?

I think that a fair compromise now would be to:

1. turn them off under WinXP
2. Let them on under Vista and above
3. provide a *prominent* option to turn them off
4. re-implement icon storage using ADS(under Windows, of course), possibly with an up-scaling of the original favicon (as IE does, if I'm not mistaken)

Points 1 to 3 should be addressed ASAP, possibly with the next minor *fix* release.

Comment 8

[smz]

... it could be the other way round too: off everywhere, prominent option to have them back on again if someone so desires...

Comment 9

[dbkh999]

Fix posted here:
https://support.mozilla.org/en-US/questions/943001#answer-437000

Comment 10

[Brian R. Bondy [:bbondy]]

We could attempt to go with that plan but if we added an option in step #3 then we cannot request landing on aurora and beta (due to localized strings cannot be uplifted).  Step #4 would be its own separate bug.  

I think maybe the best thing to do is to just disable the pref in this bug, land it and uplift it to aurora and beta, and then post another bug for changes we want from there, such as ADS and perhaps UI in preferences.  If it got approval it could land on release 2013-06-25.

Comment 11

[Brian R. Bondy [:bbondy]]

About the checkbox UI please also read this to see if you want one:
http://limi.net/checkboxes-that-kill (again UI changes can only go live on release by 2013-09-17 due to string changes).

Comment 12

[smz]

Brian,

what do you think about this (revised) path:

1. have browser.shell.shortcutFavicons exposed in about:config
2. have it set to "false" if windows version <= WinXP, otherwise set to "true"

the above two points to be implemented ASAP, possibly for the 2013-06-25 release

3. open up discussion and see if it should be set to "false" by default on every platform.
4. give more visibility to the option
5. have a proper implementation for .url shortcuts, in line with what IE does.

the above three points to be addressed at a second time.

I understand the rationale for requesting to open "new bugs" for points 3 to 5, but aren't we going to loose "audience" interested in this topic? At the very list we should "advertise" the existences of the new bugs to everyone who subscribed to this and all other related bugs.

To those who think that this issue have "already been fixed", I would like to underline that, yes, I can consider it fixed too, but only from a personal (I mean "egoistical") point of view.

P.S.: Brian, I just noticed that while I was writing this comment you posted a link to an interesting article. I will read it later on, thank-you!!

Comment 13

[smz]

above, please, correct "very list" with "very least"! :-)

Comment 14

[Brian R. Bondy [:bbondy]]

> but aren't we going to loose "audience" interested in this topic?

No we simply copy the CC list in the new bug.
We don't keep bugs open that are already landed for various reasons, but one of them is because it makes it confusing when there is a need to uplift past the nightly builds. For example there are tracking flags and flags for verification, and it would be confusing which one it applied to.

Mentored info: See Comment 4. Also I forgot to mention that to expose the pref just add the pef's default value to browser\app\profile.

Comment 15

[smz]

Fantastic!

... but now, who can actually implement those two small fixes? I'd love to be able to do it but unfortunately I'm not, and if we want that implemented "real soon" we probably need somebody with a good experience with "rapid repair" fixes.

As far as regards point 3 to 5, Brian, can you open the "new bug(s)": your English is definitely better than mine and I think it will be easier for you to express the issues at hand with a better wording...

I've read the article you posted and I totally agree with it! But if something has been broken in the past we should try to fix it now, don't you think?

Comment 16

[Brian R. Bondy [:bbondy]]

I reached out to someone to see if they can help with a fix, if not I'll try a couple other people, if not I can pick it up myself. Yup let's get this bug done then I can post the next steps.

Comment 17

[smz]

Great! Thanks!

I see this bug has 8 users in the CC list, while the CC list for the 100% related https://bugzilla.mozilla.org/show_bug.cgi?id=827784 has 17. Wouldn't it be better to put the missing ones in this bug CC list?

Also, as a personal note/request, where I can learn how to properly format posts within bugzilla (e.g. like the way you do for reference to other comments)? Am I allowed to use just plain HTML?

Comment 18

[Brian R. Bondy [:bbondy]]

(In reply to smz from comment #17)
> Great! Thanks!
> 
> I see this bug has 8 users in the CC list, while the CC list for the 100%
> related https://bugzilla.mozilla.org/show_bug.cgi?id=827784 has 17. Wouldn't
> it be better to put the missing ones in this bug CC list?

They have seen the dependent bug so they can re-add themselves, but when we spin off other bugs from this we'll copy.

> Also, as a personal note/request, where I can learn how to properly format
> posts within bugzilla (e.g. like the way you do for reference to other
> comments)? Am I allowed to use just plain HTML?

Probably there is some documentation but it's really easy, all you do is put the word bug followed by an ID and it will make it a link.  If you want a comment link you just put bug followed by an ID followed by comment followed by an ID.  Normal URLs are auto formatted.

No HTML or other formatting will be interpreted.

Comment 19

[smz]

Thanks again, Brian!

Sergio

Comment 20

[Adri Hilviu]

Attachment: Bug 828284

Added patch for the first 2 requirements in Comment 12.

Comment 21

[Brian R. Bondy [:bbondy]]

smz I also posted this which should improve the situation for post XP:
bug 876700

Comment 22

[Brian R. Bondy [:bbondy]]

Comment on attachment 754824 [details] [diff] [review]
Bug 828284

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

Looks great, thanks!
After you re-attach I'll post to try for tests and then I'll land it to mozilla-central Nightly builds for you :)

::: widget/windows/nsDataObj.cpp
@@ +1114,5 @@
>  
>    const char *shortcutFormatStr;
>    int totalLen;
>    nsCString path;
> +  if (!Preferences::GetBool(kShellIconPref, true) || WinUtils::GetWindowsVersion() < WinUtils::VISTA_VERSION) {

Could you split this into 2 lines? Most of the code tries to stay within a max line width of 80 chars:
if (!Preferences::GetBool(kShellIconPref, true) ||
    WinUtils::GetWindowsVersion() < WinUtils::VISTA_VERSION) {

Comment 23

[Brian R. Bondy [:bbondy]]

Comment on attachment 754824 [details] [diff] [review]
Bug 828284

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

Mark the next patch with review+ by the way.

Comment 24

[Adri Hilviu]

Attachment: Bug 828284

Applied the suggested change.

Comment 25

[Adri Hilviu]

Attachment: Bug 828284

Had a unnecessary tab in the previous patch.

Comment 26

[Brian R. Bondy [:bbondy]]

Thanks for the patch Adri!
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb83343d0a26

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Brian R. Bondy [:bbondy]]

Comment on attachment 755107 [details] [diff] [review]
Bug 828284

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 110894
User impact if declined: Users on Windows XP that create website shortcuts will get a default system icon for those shortcuts instead of a firefox icon.
Testing completed (on m-c, etc.): This is on m-c now, will verify before uplifting
Risk to taking this patch (and alternatives if risky): Very low
String or IDL/UUID changes made by this patch: None

Comment 29

[Alex Keybl [:akeybl]]

Comment on attachment 755107 [details] [diff] [review]
Bug 828284

Can't imagine this causing critical regressions post-release; approving for Aurora/Beta.

Comment 30

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/160351d36ebc
https://hg.mozilla.org/releases/mozilla-beta/rev/f1fce8144989

Comment 31

[Simona Badau]

Verified as fixed on Firefox 22 beta 4 (and Windows XP SP 3):
Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20100101 Firefox/22.0
Build ID: 20130605070403

Comment 34

[Simona Badau]

Verified as fixed on Firefox 23 beta 8:
Mozilla/5.0 (Windows NT 5.1; rv:23.0) Gecko/20100101 Firefox/23.0
Build ID: 20130722172257

Comment 35

[Simona Badau]

Verified as fixed on Firefox 24 beta 10:
Mozilla/5.0 (Windows NT 5.1; rv:24.0) Gecko/20100101 Firefox/24.0
Build ID: 20130909203154