221872 - Method GetWindowsFolder return paths in upper case

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Stefan Borggraefe]

The method GetWindowsFolder

http://lxr.mozilla.org/seamonkey/source/xpcom/io/SpecialSystemDirectory.cpp#178

return paths in upper case. This makes them hard to compare with paths in
mixed-case when working with those paths in JavaScript.

I noticed this problem while working on Bug 220891. The Default Profile Folder
is returned with the APPDATA-part in upper case while a folder selected with a
FilePicker is returned in mixed-case. This makes it hard to compare both paths
for equality in a cross-platform way.

So GetWindowsFolder should return paths in mixed-case, too.

Comment 1

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

this is due to this MakeUpperCase call, it seems:
196         return NS_NewNativeLocalFile(nsDependentCString(MakeUpperCase(path)), 

Comment 2

[Stefan Borggraefe]

Bug 162361 from comment #12 on has some discussion whether MakeUpperCase is
needed or not.

Comment 3

[Stefan Borggraefe]

Attachment: Patch to remove MakeUpperCase completely

This patch removes the MakeUpperCase completely. This fixes the bug and I found
no negative effects so far.

Comment 4

[Doug Turner (:dougt)]

paths on windows are always case insenstive. therefore, your js code will ALWAYS
have to compare without regard to case.

Comment 5

[Stefan Borggraefe]

> paths on windows are always case insenstive. therefore, your js code will
> ALWAYS have to compare without regard to case.

But the same JS code also runs on platforms with case-sensitive file systems. So
I can't just use an case-insensitve comparison since this would be wrong when
using the ext2 file system on Linux for example. While it is possible to write
platform dependent JS code, I don't think it is desirable.

When Windows-paths would be mixed-case everywhere in Mozilla, then a
case-sensitive comparison would work for all platforms, including Windows. Also
this would be more consistent than having mixed-case paths in some parts and
upper-case paths in other parts of Mozilla.

Furthermore these uppercase-paths look ugly when they are visible to the user.
The Create New Profile wizard currently shows the %APPDATA%-part of the default
profile path in uppercase and the rest of the path in lowercase. The
%APPDATA%-part comes from the GetWindowsFolder-method.

So I think, it would be a good thing to fix this bug. I'm currently working with
a build which has the patch applied and everything works just fine.

I have to admit this is my first C++ patch for Mozilla and I can't really check
at code level, whether there is code that depends on uppercase-paths (but this
would be kind of odd, wouldn't it?). So if you think this makes no sense or this
is to risky, feel free to deny the review or mark this bug WONTFIX.

Comment 6

[Brodie]

This patch is bitrot now by the patch in bug 202366 which STILL didn't get rid
of the uppercasing. I can't see any reason for modifying the string returned
from the OS call. No one in this bug or that bug has provided a reason, why do
it? Let's just remove it (completely irrespective of the JS angle).

Comment 7

[Brodie]

Attachment: patch 2

Updated for current code. I argue that it is a bug to change the case from that
which has been returned by OS calls. This bug is valid and should be fixed.

Comment 8

[Brodie]

Comment on attachment 151917 [details] [diff] [review]
patch 2

Very simple patch removing CharUpper calls from the Windows code.

Comment 9

[Stefan Borggraefe]

Can you also remove these comments when you fix this bug?

http://lxr.mozilla.org/mozilla/source/profile/resources/content/createProfileWizard.js#124
http://lxr.mozilla.org/mozilla/source/toolkit/profile/content/createProfileWizard.js#104

Comment 10

[Darin Fisher]

Comment on attachment 151917 [details] [diff] [review]
patch 2

r=darin

Comment 11

[Brodie]

Stefan:
I've found and updated profile/resources/content/createProfileWizard.js, but I
don't have a toolkit directory in my mozilla source tree, so I can't update
toolkit/profile/content/createProfileWizard.js. Where is it?

Comment 12

[Jungshik Shin]

(In reply to comment #11)

> toolkit/profile/content/createProfileWizard.js. Where is it?

'toolkit' is a part of aviary (firefox, thunderbird, sunbird, etc). You have to
checkout the directory explicitly. 

Comment 13

[Brodie]

Attachment: checked in patch

Fix checked in (checked in patch with comments removed is attached). 

For some reason PatchMaker/CVS didn't read my log message file and so the
checked in patch has no log message. I've searched the CVS docs for how to add
a log message after the fact but can't find anything. How do I fix this?

Comment 14

[[e-mail]]

cvs admin -m "1.14:Windows special system directories should preserve case
b=221872 r=darin sr=dougt" mozilla/xpcom/io/SpecialSystemDirectory.cpp
(or something like that); similarly for revision 1.45 of createProfileWizard.js;
note that this won't update bonsai.

Oh, and thanks for fixing this :-)

Comment 15

[Brodie]

CVS log message is now fixed. Pity about bonsai (there's no way to fix it?).
I'll make sure the message works next time. This bug is done.