Last Comment Bug 433697 - Enable .wdseml file opening support for Mail/News
: Enable .wdseml file opening support for Mail/News
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: x86 Windows Vista
: -- enhancement (vote)
: mozilla1.9
Assigned To: Siddharth Agarwal [:sid0] (inactive)
:
:
Mentors:
Depends on: 445554
Blocks: 430614
  Show dependency treegraph
 
Reported: 2008-05-14 07:20 PDT by Siddharth Agarwal [:sid0] (inactive)
Modified: 2008-07-31 04:30 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch to enable .wdseml support (3.28 KB, patch)
2008-05-14 07:20 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Splinter Review
updated patch to fix Windows file path case sensitivity issue (4.52 KB, patch)
2008-05-18 03:49 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Splinter Review
update, now compares URIs using nsIURI::Equals (5.46 KB, patch)
2008-05-18 10:23 PDT, Siddharth Agarwal [:sid0] (inactive)
mozilla: review-
Details | Diff | Splinter Review
update, addressing bienvenu's comments (9.05 KB, patch)
2008-05-18 16:12 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Splinter Review
fixed extremely small nit (9.05 KB, patch)
2008-05-18 16:15 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Splinter Review
Made unescape not just Win-specific in HandleIndexerResult(); added comment for MsgMailboxGetURI() change (9.17 KB, patch)
2008-05-21 15:37 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Splinter Review
<ignore please> (10.23 KB, application/octet-stream)
2008-05-28 09:14 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details
Separated #ifdefs, used nsILocalFile::GetRelativeDescriptor instead of nsIURL::Equals (10.23 KB, patch)
2008-05-28 09:25 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Splinter Review
uh, it's XP_MACOSX, not XP_MAC (10.23 KB, patch)
2008-05-28 09:33 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Splinter Review
changed MsgMailboxGetURI to take in an nsILocalFile instead of a char * (11.28 KB, patch)
2008-05-29 10:12 PDT, Siddharth Agarwal [:sid0] (inactive)
neil: superreview+
Details | Diff | Splinter Review
update addressing comments (11.21 KB, patch)
2008-05-30 07:13 PDT, Siddharth Agarwal [:sid0] (inactive)
mozilla: review+
sid.bugzilla: superreview+
Details | Diff | Splinter Review

Description Siddharth Agarwal [:sid0] (inactive) 2008-05-14 07:20:37 PDT
Created attachment 320904 [details] [diff] [review]
patch to enable .wdseml support

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.
Comment 1 Siddharth Agarwal [:sid0] (inactive) 2008-05-18 03:49:23 PDT
Created attachment 321473 [details] [diff] [review]
updated patch to fix Windows file path case sensitivity issue

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 2 Siddharth Agarwal [:sid0] (inactive) 2008-05-18 06:53:53 PDT
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.
Comment 3 Siddharth Agarwal [:sid0] (inactive) 2008-05-18 10:23:05 PDT
Created attachment 321499 [details] [diff] [review]
update, now compares URIs using nsIURI::Equals

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 4 David :Bienvenu 2008-05-18 12:54:40 PDT
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.
Comment 5 Siddharth Agarwal [:sid0] (inactive) 2008-05-18 16:12:20 PDT
Created attachment 321536 [details] [diff] [review]
update, addressing bienvenu's comments

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.
Comment 6 Siddharth Agarwal [:sid0] (inactive) 2008-05-18 16:15:15 PDT
Created attachment 321537 [details] [diff] [review]
fixed extremely small nit
Comment 7 David :Bienvenu 2008-05-18 18:27:30 PDT
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 8 Siddharth Agarwal [:sid0] (inactive) 2008-05-21 15:37:05 PDT
Created attachment 322012 [details] [diff] [review]
Made unescape not just Win-specific in HandleIndexerResult(); added comment for MsgMailboxGetURI() change
Comment 9 David :Bienvenu 2008-05-21 16:32:49 PDT
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.
Comment 10 Siddharth Agarwal [:sid0] (inactive) 2008-05-22 01:26:14 PDT
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.)
Comment 11 Siddharth Agarwal [:sid0] (inactive) 2008-05-22 01:53:43 PDT
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 12 neil@parkwaycc.co.uk 2008-05-22 03:23:32 PDT
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).
Comment 13 David :Bienvenu 2008-05-27 10:39:13 PDT
>>+  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?
Comment 14 Siddharth Agarwal [:sid0] (inactive) 2008-05-28 09:14:12 PDT
Created attachment 322788 [details]
<ignore please>
Comment 15 Siddharth Agarwal [:sid0] (inactive) 2008-05-28 09:25:36 PDT
Created attachment 322792 [details] [diff] [review]
Separated #ifdefs, used nsILocalFile::GetRelativeDescriptor instead of nsIURL::Equals

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 16 Siddharth Agarwal [:sid0] (inactive) 2008-05-28 09:33:15 PDT
Created attachment 322794 [details] [diff] [review]
uh, it's XP_MACOSX, not XP_MAC
Comment 17 David :Bienvenu 2008-05-28 09:52:04 PDT
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?)
Comment 18 Siddharth Agarwal [:sid0] (inactive) 2008-05-28 13:08:20 PDT
(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)
Comment 19 Andrew Sutherland [:asuth] 2008-05-28 13:35:09 PDT
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).
Comment 20 Siddharth Agarwal [:sid0] (inactive) 2008-05-28 13:42:35 PDT
(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.
Comment 21 neil@parkwaycc.co.uk 2008-05-28 16:17:36 PDT
(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 22 neil@parkwaycc.co.uk 2008-05-28 16:20:38 PDT
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?
Comment 23 Siddharth Agarwal [:sid0] (inactive) 2008-05-29 10:12:08 PDT
Created attachment 322954 [details] [diff] [review]
changed MsgMailboxGetURI to take in an nsILocalFile instead of a char *

Seems quite logical to do.
Comment 24 neil@parkwaycc.co.uk 2008-05-30 05:46:18 PDT
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)
Comment 25 Siddharth Agarwal [:sid0] (inactive) 2008-05-30 07:13:21 PDT
Created attachment 323066 [details] [diff] [review]
update addressing comments

(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.
Comment 26 Siddharth Agarwal [:sid0] (inactive) 2008-05-30 08:00:46 PDT
hg patch, so patch -p1 please.
Comment 27 Mark Banner (:standard8) 2008-06-02 01:11:52 PDT
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

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