420459 - Move mailnews-specific profile code

Status: RESOLVED FIXED

(Core Graveyard :: Profile: BackEnd, defect)

Description

[Joshua Cranmer [:jcranmer]]

Currently four mailnews "magic" filenames are defined outside of mailnews: the profile directories "Mail" (MailD), "ImapMail" (IMapMD), "News" (NewsD), and "panacea.dat" (MFCaD). The filenames are defined in nsProfileDirServiceProvider.cpp and the keys to these names are in xpcom/io/nsAppDirectoryServiceDefs.h

Comment 1

[Robert Kaiser]

Yes, those should move to a mailnews-specific directory provider, just like http://mxr.mozilla.org/seamonkey/source/suite/profile/nsSuiteDirectoryProvider.cpp

Comment 2

[Mark Banner (:standard8)]

I've got a patch almost completed for this, taking...

Comment 3

[Mark Banner (:standard8)]

Attachment: The fix

This moves the mailnews specific profile directory/file definitions out of core and into mailnews. Mailnews already has a dir provider implementation, so hooking this up was fairly easy.

Benefits core and mailnews, especially as mailnews now needs to alter these definitions (in a different bug).

Note I've renamed one of the #defines from _DIR to _FILE because that's what it is actually referencing.

Benjamin can you review the core parts? - Neil can review the mailnews specific parts.

Comment 4

[Benjamin Smedberg]

Comment on attachment 306871 [details] [diff] [review]
The fix

My day is brighter because of this patch.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 306871 [details] [diff] [review]
The fix

>+nsresult
>+nsMailDirProvider::EnsureProfileDirectory(nsIFile *aDirectory)
>+{
>+  PRBool exists;
>+  nsresult rv = aDirectory->Exists(&exists);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!exists)
>+    rv = aDirectory->Create(nsIFile::DIRECTORY_TYPE, 0700);
>+
>+  return rv;
>+}

I don't see anything profile-specific about this method.  Does it want to be named EnsureDirectory?

>+void
>+nsMailDirProvider::EnsureProfileFile(const nsACString& aLeafName,
>+				     nsIFile* aParentDir,
>+				     nsIFile* aTarget)

Why does this method return void?

>+{
>+  nsCOMPtr<nsIFile> defaults;
>+  NS_GetSpecialDirectory(NS_APP_PROFILE_DEFAULTS_50_DIR,
>+                         getter_AddRefs(defaults));
>+  if (!defaults)
>+    return;
>+
>+  defaults->AppendNative(aLeafName);

The return value here needs to be checked, otherwise CopyToNative will be called with two copies of the same argument (which might well not do anything bad, but I wouldn't want to count on that).

>+  defaults->CopyToNative(aParentDir, aLeafName);

I suspect this return value wants to be propagated up the stack.

> NS_IMPL_ISUPPORTS2(nsMailDirProvider,
> 		   nsIDirectoryServiceProvider,
> 		   nsIDirectoryServiceProvider2)
> 
> NS_IMETHODIMP
> nsMailDirProvider::GetFile(const char *aKey, PRBool *aPersist,
>-			       nsIFile* *aResult)
>+			   nsIFile **aResult)

The above line changed, but still appears to have tabs in it.  Please fix.

> [...]
>+
>+  nsDependentCString leafStr(leafName);
>+  file->AppendNative(leafStr);

This wants error-checking too, I think.

>+  /*
>+   * makeDirectoryService
>+   *
>+   */
>+  makeDirectoryService : function () {
>+    // Register our own provider for the profile directory.
>+    // It will simply return the current directory.
>+    const provider = {
>+      getFile : function(prop, persistent) {
>+        persistent.value = true;
>+        if (prop == NS_APP_USER_PROFILE_50_DIR) {
>+          var processDir = dirSvc.get("CurProcD", nsIFile);
>+
>+          processDir.append("addrbook");

Why append "addrbook" here?

>+
>+      QueryInterface : function(iid) {
>+        if (iid.equals(Components.interfaces.nsIDirectoryServiceProvider) ||
>+            iid.equals(Components.interfaces.nsISupports)) {
>+          return this;
>+        }
>+        throw Components.results.NS_ERROR_NO_INTERFACE;
>+      }
>+    };

Does the above want to be XPCOMUtils.generateQI()?

>+    dirSvc.QueryInterface(Components.interfaces.nsIDirectoryService)
>+          .registerProvider(provider);
>+  }
>+};
>+
>+var testnum = 0;
>+
>+function run_test() {
>+  var i = -1;
>+  try {
>+
>+    const items = [ ["MailD", "Mail"],
>+                    ["IMapMD", "ImapMail"],
>+                    ["NewsD", "News"],
>+                    ["MFCaF", "panacea.dat"] ];

This code would be more readable, I think, if you used JS objects as hashes here (eg with "key" and "value") rather than sub-arrays that are indexed numerically.

>+
>+    ++testnum; // Test 1 - Mail Dir
>+
>+    for (i = 0; i < items.length; ++i) {

You might (or might not) want to consider using Array.forEach() for readability here as well.

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> (From update of attachment 306871 [details] [diff] [review])
> The above line changed, but still appears to have tabs in it.  Please fix.
I've fixed that and a few others.

> >+  /*
> >+   * makeDirectoryService
> >+   *
> >+   */
> >+  makeDirectoryService : function () {
> >+    // Register our own provider for the profile directory.
> >+    // It will simply return the current directory.
> >+    const provider = {
> >+      getFile : function(prop, persistent) {
> >+        persistent.value = true;
> >+        if (prop == NS_APP_USER_PROFILE_50_DIR) {
> >+          var processDir = dirSvc.get("CurProcD", nsIFile);
> >+
> >+          processDir.append("addrbook");
> 
> Why append "addrbook" here?

That was part of a copy/paste from elsewhere. I've changed it to "mailtest" - basically it means we're not dumping the files/re-using the dist/bin directory but somewhere else which makes it easier to find things.

Comment 7

[Mark Banner (:standard8)]

Attachment: The fix v2 (diff -w)

Patch with comments fix (diff -w)

Comment 8

[Mark Banner (:standard8)]

Attachment: The fix v2 (normal patch)

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 308987 [details] [diff] [review]
The fix v2 (diff -w)

Looking good; just a few tweaks needed:

>+  PRBool exists;
>+  if (NS_SUCCEEDED(file->Exists(&exists)) && !exists)
>+    if (isDirectory)
>+      rv = EnsureDirectory(file);
>+    else
>+      EnsureProfileFile(leafStr, parentDir, file);

Need to propagate the return value here.

>+// If there's no location registered for the profile direcotry, 
>+var testnum = 0;

No longer needed.

>+function run_test() {
>+  var i = -1;

No longer used.

>+  try {
>+
>+    const items = [ { key: "MailD", value: "Mail" },
>+                    { key: "IMapMD", value: "ImapMail" },
>+                    { key: "NewsD", value: "News" },
>+                    { key: "MFCaF", value: "panacea.dat" } ];
>+
>+    ++testnum; // Test 1 - Mail Dir

No longer used.

>+    items.forEach(function(item) {
>+      var dir = dirSvc.get(item.key, nsIFile);
>+
>+      do_check_true(profileDir.equals(dir.parent));
>+
>+      do_check_eq(dir.leafName, item.value);
>+    });
>+  }
>+  catch (e) {
>+    throw "FAILED in test #" + testnum + ", i: " + i + ",  " + e;
>+  }

I'm not convinced you really need to bother having a try/catch here at all.

sr=dmose with that stuff addressed

Comment 10

[Mark Banner (:standard8)]

Attachment: The fix v3 (diff -w)

In fixing the return value for the previous patch, I noticed that EnsureProfileFile was returning NS_ERROR_FAILURE. On further investigation I realised this function is useless in the panacea.dat case - because we don't have a default file in defaults/profile.

Therefore I've dropped the EnsureProfileFile function but maintained the EnsureDirectory - as I think that the existing is probably useful to us.

Hence re-requesting sr to check we're still happy with this.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 309797 [details] [diff] [review]
The fix v3 (diff -w)

sr=dmose

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 309797 [details] [diff] [review]
The fix v3 (diff -w)

Requesting 1.9 approval. This patch moves mailnews specific directory/file definitions out of core and into mailnews.

As these definitions are not used by FF, it should be low risk as the patch just removes the code from the core, and only modifies the mailnews code.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 309797 [details] [diff] [review]
The fix v3 (diff -w)

a1.9=beltzner

Comment 14

[Mark Banner (:standard8)]

Patch checked in -> fixed.

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 309797 [details] [diff] [review]
The fix v3 (diff -w)

>RCS file: mailnews/base/test/unit/test_nsMailDirProvider.js
>+ * Test suite for nsMsgMailSession functions relating to listeners.

<http://mxr.mozilla.org/seamonkey/search?string=Test+suite+for+nsMsgMailSessi&case=on&tree=seamonkey>
{{
Found 3 matching lines
Test suite for nsMsgMailSessi
/mailnews/compose/test/unit/test_nsMsgCompose2.js,
    * line 3 -- * Test suite for nsMsgMailSession functions relating to listeners.
/mailnews/base/test/unit/test_nsMailDirProvider.js,
    * line 3 -- * Test suite for nsMsgMailSession functions relating to listeners.
/mailnews/base/test/unit/test_nsMsgMailSession_Listeners.js,
    * line 3 -- * Test suite for nsMsgMailSession functions relating to listeners.
}}

Non updated copy+paste in the first two files !?