For the Windows Search indexer it's been decided to have the helper files have the .wdseml extension (this isn't used by any other program AFAIK). The code in the patch is derived from the Spotlight .mozeml code [1], yet is different enough to warrant a separate code block (the main difference is in the conversion of the native path provided by Windows to a file:// URI, required by the MsgMailboxGetURI() function). [1] https://bugzilla.mozilla.org/show_bug.cgi?id=290057 and trunk/mailnews/base/src/nsMessengerBootstrap.cpp. The code does nothing unless the path of a .wdseml file is passed as an argument to the application.

I hoped for a better solution to this than a blanket case insensitive comparator check, but nsIURL::GetCommonBaseSpec() doesn't take into account such issues, while nsIURI::Equals() does take into account such issues, but isn't suitable. Could someone suggest a better fix for this?

Comment on attachment 321473 [details] [diff] [review] updated patch to fix Windows file path case sensitivity issue Cancelling review, a patch has been checked in (bug 407295) that moves the file to frozen linkage.

Now the code compares strings using nsIURI::Equals(), which in turn uses file objects to actually do the comparison, thus bypassing any case sensitivity issues altogether.

Comment on attachment 321499 [details] [diff] [review] update, now compares URIs using nsIURI::Equals can you make sure you have spaces around '=', e.g., here: + unescapedMessageId=NS_UnescapeURL(messageId, esc_Minimal, unescapedMessageId); can you combine the common code with the .mozeml code, instead of duplicating it, perhaps by adding a method that's parameterized on the file extension? (it looks like the code is very similar, which makes sense since they're doing the same thing.

David, by combining the code do you mean this? I've used #ifdefs instead of checking for extension because the handling is platform dependent, not extension dependent. Unfortunately I can't test this on Mac, but I haven't changed the logic of Mac handling. By the way, why is it not necessary for the message ID to be unescaped on Mac before being passed to OpenMessengerWindowForMessageId() ? An unescaped message ID doesn't work, at least on Windows.

It could be a bug on the mac, or it could be that we don't escape the message ids that we use in the file name (though I think we do). I'll have to look into the spotlight code - I'll try it tomorrow. It might take me a bit of time since I've only got Leopard and I've heard that our spotlight integration has issues on Leopard.

Comment on attachment 322012 [details] [diff] [review] Made unescape not just Win-specific in HandleIndexerResult(); added comment for MsgMailboxGetURI() change an alternative would be, on windows only, convert both paths to lower case for comparison purposes - that way you wouldn't have to loop like this across the path for all platforms...asking Neil for sr since I think he might have some good ideas for alternative approaches.

From http://developer.mozilla.org/en/docs/nsILocalFile:getRelativeDescriptor : it says that an "opaque string" is returned which isn't intended for display. Will it be all right to use? If so, then I guess I can get the relative descriptor of the file path wrt the server path, and check whether the first character is a dot (as in ../). (Yes, _wscicmp is the right way, sorry, I was a bit confused about encodings.)

I don't think directly converting both paths to lower case is all right, as non-ASCII characters are in what looks to be escaped UTF-8 form (eg ḥỜᴁڧᾷ is %E1%B8%A5%E1%BB%9C%E1%B4%81%DA%A7%E1%BE%B7) at the time we'll be converting to lower case, and Windows does check for the case of these characters in file paths. Perhaps an NS_Convert function could be used to unescape these characters and change them to UTF16, and then the paths could be changed to lower case? I'm not too sure of this. Using file objects does do this, so _wscicmp will definitely work. There wouldn't be any platform-specific headaches either, as the headaches are already taken care of in the Equals() or GetRelativeDescriptor() functions :) Therefore I think it'll be best if one of these two functions are used, and strings aren't directly compared.

Comment on attachment 322012 [details] [diff] [review] Made unescape not just Win-specific in HandleIndexerResult(); added comment for MsgMailboxGetURI() change >- // We're going to have a native path file url: >- // file://<folder path>.mozmsgs/<message-id>.mozeml >+#if defined (XP_MACOSX) || defined (XP_WIN) >+ if (StringEndsWith(arg, NS_LITERAL_STRING(".mozeml"), nsCaseInsensitiveStringComparator()) >+ || StringEndsWith(arg, NS_LITERAL_STRING(".wdseml"), nsCaseInsensitiveStringComparator())) >+ HandleIndexerResult(arg); > #endif Should these be separate ifdefs? >+ PRInt32 mozmsgsIndex = aPath.Find(NS_LITERAL_STRING(".mozmsgs")); Should this be case insensitive? >+#ifdef XP_WIN >+ // use nsILocalFile to convert the path to a file:// URI According to the comment above it's already a file:// URI... can someone clarify exactly what the parameter is? The alternative would seem to be to get the local path for the parameter and each server and call StringBeginsWith (but case insensitively on Windows).

>>+ PRInt32 mozmsgsIndex = aPath.Find(NS_LITERAL_STRING(".mozmsgs")); >Should this be case insensitive? We're creating this directory, and looking for it, so we can make it the correct case in both places. Modern file systems do maintain the case correctly, I believe, even Windows :-) Or is there some case I don't know about?

GetRelativeDescriptor is the best way to handle this, I guess, as it'll produce relative paths starting with ../ if the file isn't within a directory. As David mentioned, it isn't really necessary for the mozmsgs to be case insensitive -- FAT32, NTFS, and HFS+ are all case preserving, and we're creating the folder ourselves.

Comment on attachment 322794 [details] [diff] [review] uh, it's XP_MACOSX, not XP_MAC I bet Neil's going to point out that you can use swap here, instead of NS_IF_ADDREF +nsresult MsgGetLocalFileFromURI(const char *escapedUTF8Path, nsILocalFile **aFile) +{ + nsresult rv; + nsCOMPtr<nsIURI> argURI; + NS_NewURI(getter_AddRefs(argURI), escapedUTF8Path); You probably want to check if this fails: + NS_NewURI(getter_AddRefs(argURI), escapedUTF8Path); I wonder if it's preferred to go through nsIOService instead of using NS_NewURI (is NS_NewURI available in frozen linkages?)

(In reply to comment #17) > (From update of attachment 322794 [details] [diff] [review]) > I bet Neil's going to point out that you can use swap here, instead of > NS_IF_ADDREF OK, swap sounds nice. > > You probably want to check if this fails: > + NS_NewURI(getter_AddRefs(argURI), escapedUTF8Path); Hm, right, though I guess it'd fail at the next step anyway. > > I wonder if it's preferred to go through nsIOService instead of using NS_NewURI > (is NS_NewURI available in frozen linkages?) > I think NS_NewURI's there in frozen linkage. (I'm waiting for Neil's review)

Is this going to need the new .wdseml extension to be associated with Thunderbird by the installer? (Currently nsMailWinIntegration is also involved, but Vista changes are moving all the actual settings out to the installer).

(In reply to comment #19) > Is this going to need the new .wdseml extension to be associated with > Thunderbird by the installer? (Currently nsMailWinIntegration is also > involved, but Vista changes are moving all the actual settings out to the > installer). > Possibly, it still isn't 100% sure whether this will be part of Tb or an extension. Will it be possible to add other arbitrary registry entries as part of the install? (I guess a couple of dormant .wdseml associations wouldn't do any harm) This is going to be supported on XP too, btw.

(In reply to comment #18) > (In reply to comment #17) > > (From update of attachment 322794 [details] [diff] [review]) > > I bet Neil's going to point out that you can use swap here, instead of > > NS_IF_ADDREF > OK, swap sounds nice. Note that technically you need to clear the arg you're swapping with. (getter_AddRefs does this but the caller isn't obliged to clear it for you) > > You probably want to check if this fails: > > + NS_NewURI(getter_AddRefs(argURI), escapedUTF8Path); > Hm, right, though I guess it'd fail at the next step anyway. You'd get a better rv though ;-)

Comment on attachment 322794 [details] [diff] [review] uh, it's XP_MACOSX, not XP_MAC >+ // Get the nsILocalFile corresponding to uriPath. >+ nsCOMPtr<nsILocalFile> argLocalFile; >+ rv = MsgGetLocalFileFromURI(uriPath, getter_AddRefs(argLocalFile)); >+ NS_ENSURE_SUCCESS(rv, rv); So, it turns out that two of your callers (one here and one in nsMailboxUrl.cpp) already have a local file. I don't suppose you could change this to take a local file directly, and move the call to MsgGetLocalFileFromURI into the Mac section?

Seems quite logical to do.

Comment on attachment 322954 [details] [diff] [review] changed MsgMailboxGetURI to take in an nsILocalFile instead of a char * >+nsMessengerBootstrap::HandleIndexerResult(nsAutoString &aPath) Never use nsAutoString& - I'm not sure what will compile but you should prefer nsA(C)String& to ns(C)String& and use const if you can. >+ // We're going to have a native path file url: >+ // file://<folder path>.mozmsgs/<message-id>.mozeml >+ // need to convert to 8 bit chars...i.e., a local path. >+ nsCString nativeArg; >+ NS_CopyUnicodeToNative(folderPath, nativeArg); >+ >+ // Get the nsILocalFile for this file:// URI. >+ rv = MsgGetLocalFileFromURI(nativeArg, getter_AddRefs(msgFolder)); NS_NewURI accepts a unicode URI, does it not? >+nsresult MsgGetLocalFileFromURI(nsACString &aUTF8Path, nsILocalFile **aFile) Again, the string should be const. >+ nsCOMPtr<nsILocalFile> file = do_QueryInterface(argFile, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ file.swap(*aFile); >+ return NS_OK; You can actually write this as return CallQueryInterface(argFile, aFile); (Note that for .swap you should null out *aFile first)

(carrying forward sr) Thanks, Neil, for all the corrections. (I had corrected the in string parameters to use const, but had somehow missed it in the patch) HandleIndexerResult didn't compile with nsAString. > NS_NewURI accepts a unicode URI, does it not? It does, but I'd have to either move the NS_NewURI call into the Mac section, or have another function which accepts an nsAString. I wouldn't gain anything from it either, as the UTF-16 version of NS_NewURI converts the string to UTF-8 before proceeding.

hg patch, so patch -p1 please.

Checking in mailnews/base/src/nsMessengerBootstrap.cpp; /cvsroot/mozilla/mailnews/base/src/nsMessengerBootstrap.cpp,v <-- nsMessengerBootstrap.cpp new revision: 1.67; previous revision: 1.66 done Checking in mailnews/base/src/nsMessengerBootstrap.h; /cvsroot/mozilla/mailnews/base/src/nsMessengerBootstrap.h,v <-- nsMessengerBootstrap.h new revision: 1.13; previous revision: 1.12 done Checking in mailnews/base/util/nsMsgUtils.cpp; /cvsroot/mozilla/mailnews/base/util/nsMsgUtils.cpp,v <-- nsMsgUtils.cpp new revision: 1.147; previous revision: 1.146 done Checking in mailnews/base/util/nsMsgUtils.h; /cvsroot/mozilla/mailnews/base/util/nsMsgUtils.h,v <-- nsMsgUtils.h new revision: 1.72; previous revision: 1.71 done Checking in mailnews/local/src/nsMailboxUrl.cpp; /cvsroot/mozilla/mailnews/local/src/nsMailboxUrl.cpp,v <-- nsMailboxUrl.cpp new revision: 1.117; previous revision: 1.116 done