44714 - Icons not changed from IE to Mozilla when set in Desktop Integration (.url icons) (Internet shortcuts)

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect, P5)

Description

[Eli Goldberg]

* TITLE/SUMMARY
Icons not changed from IE to Mozilla when set in Desktop Integration

* STEPS TO REPRODUCE
0) Launch Internet Explorer 
1) Make it your default browser, located in preferences | programs .
2) Quit Internet Explorer. Launch Internet Explorer a second time, making it your 
default browser when prompted. 
	--> Notice that documents such as .html and .gif files now have an Internet 
Explorer logo. 
3) Launch mozilla 
4) Open preferences | Advanced | desktop integration 
5) Set mozilla to be the default application for the file types that you viewed 
in step 2. 
6) Quit mozilla, and restart your computer. 

* RESULT
 - What happened

The icons continue to evidence the Internet Explorer logo, rather than a Netscape 
or mozilla logo. 

 - What was expected 

The Internet Explorer icons should be replaced with Netscape or mozilla icons for 
the file types that mozilla now handles by default. 

* REGRESSION

 - Occurs On
        Mac OS Seamonkey (7.6.2000 Netscape Commercial build)		

 - Doesn't Occur On
        Internet Explorer 5.0 RTM




* CONFIGURATIONS TESTED

- [Mac] Power Mac G4 (450 Mhz), 256 MB RAM (VM off), 1024x768 (Thousands of 
Colors), Mac OS 9.0

- [Win32] Vectra VL (266 MHz P2), 96 MB RAM, 800x600 (True Color), NT 4.0 SP5.

- [Linux] Vectra VL (266 MHz P2), 96 MB RAM. Red Hat Linux 6.0 (GNOME).

Comment 1

[Chris McAfee]

*** Bug 43715 has been marked as a duplicate of this bug. ***

Comment 2

[Chris McAfee]

cc

Comment 3

[Blake Ross]

nominating for beta3

43715, a dup of this, was nominated for beta2 but I don't think we need to 
spend time on this for 2.

Comment 4

[sairuh (rarely reading bugmail)]

eli, does this occur on both Mac and win32, or only the mac? the os/platform
differ from the bug details entered --just wondering. thx!

Comment 5

[don]

Eli, if this only occurs on Mac then we're not going to fix it.  If it occurs on
Windows, then that's a bug.  This is a non-issue on Linux.

Can you get me some more data and re-assign this back to me?

Comment 6

[Blake Ross]

It occurs on Windows.

Comment 7

[Eli Goldberg]

Oops. Sorry, I was blowing out so many bugs so quickly that I flubbed. 

Desktop Integration is a Windows-only feature, so, yes, you're all correct.

Comment 8

[don]

OK, I've marked this "b3nav+" and "correctness" (in the hope that the PDT will 
mark this "nsbeta3+") because the program is not fucntioning in a very visible 
manner.

Comment 9

[Phil Peterson]

PDT agrees P2

Comment 10

[don]

Let's fix this in RTM.

Comment 11

[johng]

nav triage team:
[RTM NEED INFO] looking for a patch to fix this

Comment 12

[don]

Fix in hand, being reviewed.

Comment 13

[don]

The fix for this is also in the fix for bug #27187.  Should this be closed as a dup?

Comment 14

[don]

The fix for this is reviewed and approved in bug #27187 (also now "rtm+").

Comment 15

[Bill Law]

This is fixed on the trunk.

Comment 16

[Phil Peterson]

rtm++ since I just marked the other one rtm++

Comment 17

[Bill Law]

Fix checked in on branch.

Comment 18

[John Morrison]

Mea culpa for not fully checking this until now. I was seeing the icons change
in passing, but looking now, I don't fully get this to change over. I think
I need to speak with you tomorrow about what is supposed to occur, when in 
terms of icons being swapped in. 

But the larger problem, not really what this bug was about, but related to it, 
si that I am getting an error message when double clicking files and shortcuts 
when Mozilla/Netscape is registered as the handler for that type -- Seamonkey 
will load the content but it also forces an OS alert (on win2k and win98) that, 
quote:

  "Cannot find the file "the-URL-of-the-shortcut" (or one of its components).
   Make sure that the path and filename are correct and that all required
   libraries are available."

Reopening this bug just to get this back on the radar (I suck).

Comment 19

[Phil Peterson]

Marking [rtm need info] since it was reopened

Comment 20

[Bill Law]

Sigh.  See bug 58848; the message box is covered there.  The icons are just
another symptom of the same Win32 quirks as described there.  Sometimes the
icons change, sometimes they don't.  If you edit the File Type info in windows
and manually select the icon, Windows sets the registry to exactly what it was
already (but now it works).

This mystery will not be solved for rtm.  Recommend [rtm-] at this point.

Comment 21

[don]

Agreed.  Minus.

Comment 22

[Paul Chen]

nav triage team:

Would like to fix this for beta1, but very low priority. Nsbeta1, p5

Comment 23

[sairuh (rarely reading bugmail)]

moving Windows Integration [formerly Desktop Integration] bugs to PaulW as qa
contact.

Comment 24

[Jaime Rodriguez, Jr.]

Removing adequated PDT grafitti.

Comment 25

[Terri Preston]

Changing qa contact to me, as I am working on these now ;-)

Comment 26

[Michael Lefevre]

*** Bug 156140 has been marked as a duplicate of this bug. ***

Comment 27

[Michael Lefevre]

*** Bug 139669 has been marked as a duplicate of this bug. ***

Comment 28

[Terri Preston]

qa contact windows integration-> pmac

Comment 29

[Oliver Klee]

*** Bug 184924 has been marked as a duplicate of this bug. ***

Comment 30

[Matthias Versen [:Matti]]

*** Bug 182319 has been marked as a duplicate of this bug. ***

Comment 31

[Andrew Hagen]

*** Bug 150335 has been marked as a duplicate of this bug. ***

Comment 32

[Manuel Reimer]

You can fix the bug with the wrong icon for ".url"-files with this:

Set this list of registry-keys

HKEY_CLASSES_ROOT\http\DefaultIcon
HKEY_CLASSES_ROOT\https\DefaultIcon
HKEY_CLASSES_ROOT\ftp\DefaultIcon
HKEY_CLASSES_ROOT\gopher\DefaultIcon
HKEY_CLASSES_ROOT\mailto\DefaultIcon

to

%MozillaEXEPath%,0

Comment 33

[Andrew Hagen]

Is Bill Law still with the Mozilla Project? Should he still be assigned to the bug?

Comment 34

[Jonathan Wilson]

Attachment: patch

This patch fixes the icon problem.
Basicly, I cloned ProtocolRegistryEntry and made it so it would set the icon
registry keys properly.
Also, the patch corrects a small typo I found, ddexec instead of ddeexec. Plus,
it changes the call to RegSetValueEx to correct a compiler warning about
passing a NULL as a non-pointer type (as per what MSDN says to do, it now
passes 0, not NULL)

2 other issues:
1.Given that nswindowshooksutil.cpp is only ever used as a #include to
nswindowshooks.cpp, shouldnt it be nswindowshooksutil.h?
and 2.line 543 of nswindwshooks.cpp says "convert to dword" but then it doesnt
do anything with the resulting dword (the variable called dwordValue). Should
it be doing something with that value or should it be removed?

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 121781 [details] [diff] [review]
patch

+    nsresult set();
+    nsresult reset();
+};

Why are you overriding set and reset if the implementations of them just call
SavedRegistryEntry::set/reset anyway?

Comment 36

[Jonathan Wilson]

Attachment: patch v2

removed ProtocolIconRegistryEntry::Set and ::Reset

Comment 37

[Jonathan Wilson]

assinging to me

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 121824 [details] [diff] [review]
patch v2

Hmm... Wouldn't it be better to do this inside the ProtocolRegistryEntry
set/reset methods like we do for the DDE stuff?  (mind you, this is the first
time I look at the code).

Also:

foo = NS_LITERAL_CSTRING("bar") + baz + NS_LITERAL_CSTRING("end");

instead of using += a bunch.

Finally, the "+// Set/reset are trickier in this case." comment hardly applies
here, does it?	We're using the set/reset from the base class....

Comment 39

[Jonathan Wilson]

Attachment: patch v3

Fixed so that it does it inside ProtocolRegistryEntry::Set and ::Reset.
Removed that comment.
As for +=, thats how ProtocolRegistryEntry does it so I am assuming that there
is a reason for why its done that way and therefore why it should be done that
way for ProtocolIconRegistryEntry also.

Also, can anyone comment on:
1.Given that nswindowshooksutil.cpp is only ever used as a #include to
nswindowshooks.cpp, shouldnt it be nswindowshooksutil.h?
and 2.line 543 of nswindwshooks.cpp says "convert to dword" but then it doesnt
do anything with the resulting dword (the variable called dwordValue). Should
it be doing something with that value or should it be removed?

Comment 40

[Boris Zbarsky [:bzbarsky]]

> As for +=, thats how ProtocolRegistryEntry does it so I am assuming that there
> is a reason for why its done that way

Yeah.  The reason is that the author of that code didn't have a clue how to use
the Mozilla string classes.

Other than that issue, the patch looks fine and I could sr it.  But you need
review from whoever actually owns this code....

Comment 41

[Jonathan Wilson]

After staring at the code in nswindowshooks.cpp and nswindowshooksutil.cpp, I
can see that the best solution is to re-code both files to use proper string
handling throughout. However, thats not what this bug (and fix) is about. This
fix is a fix to write as little code as possible to get the icons displaying,
something that can get R=, SR= and A= and get into 1.4. Not displaying the
proper icons for these things is a very visible problem (in fact, it might lead
some people to believe that mozilla hasnt in fact set itself to be the handler
for these types)
If the "powers that be" want me to fix specific uses of strings in my patch,
please tell me what specific uses to fix & I will do it.
But its not my job to re-write code that Bill Law should have written properly
in the first place.

Comment 42

[Jonathan Wilson]

Attachment: patch v4

Sorry for anything I said in the last comment, I wasnt thinking straight.
(blame it on personal issues)
I have fixed the one specific item bz mentioned and have filed bug 203644 to
cover the general case of re-writing all of the old code (written by Law) to
use proper string handling.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 121900 [details] [diff] [review]
patch v4

Excellent.  sr=me (and yes, I wasn't saying you need to fix all the existing
code.  ;)

Comment 44

[(not reading, please use [e-mail] instead)]

Comment on attachment 121900 [details] [diff] [review]
patch v4

a=sspitzer, I assume if we are no longer the default browser, the right thing
happens?

Comment 45

[Jonathan Wilson]

Yes, the right thing happens. The icon registry entries are returned to what
they were.

Comment 46

[Andrew Hagen]

Using a 4/29 build in Windows 98, new .url icons are created with a Mozilla
icon. OTOH, .url icons that already existed on the Windows desktop when I
installed the 4/29 build were not automatically switched to Mozilla icons. They
are still IE icons. This is even though I made the 4/29 build the default browser. 

If I go to Properties of the desktop .url icons, I can change the icon to the
Mozilla icon. 

Not sure whether this is a problem or not.

In any case, I want to express my thanks to the developers, including Jonathan
Wilson.

Comment 47

[Frank Wein [:mcsmurf]]

Patch has been checked in:
25/2  	Bug 44714 Icons not changed from IE to Mozilla when set in Desktop
Integration (.url icons) (Internet shortcuts)
patch by [e-mail] r=sgehani sr=bz a=sspitzer

Old version: 1.24
New version: 1.25

Thanks jonwil for fixing this :)!

Comment 48

[Andrew Hagen]

Re: my comment in comment 46

At some point, all of the icons suddenly switched to Mozilla icons. Looks like
everything is indeed working fine. Thanks again!