Closed Bug 420459 Opened 13 years ago Closed 13 years ago

Move mailnews-specific profile code

Categories

(Core Graveyard :: Profile: BackEnd, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: jcranmer, Assigned: standard8)

Details

Attachments

(1 file, 3 obsolete files)

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
Yes, those should move to a mailnews-specific directory provider, just like http://mxr.mozilla.org/seamonkey/source/suite/profile/nsSuiteDirectoryProvider.cpp
I've got a patch almost completed for this, taking...
Assignee: nobody → bugzilla
Attached patch The fix (obsolete) — Splinter Review
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.
Attachment #306871 - Flags: review?(neil)
Attachment #306871 - Flags: review?(benjamin)
Attachment #306871 - Flags: review?(neil) → review+
Attachment #306871 - Flags: superreview?(dmose)
Comment on attachment 306871 [details] [diff] [review]
The fix

My day is brighter because of this patch.
Attachment #306871 - Flags: review?(benjamin) → review+
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.
Attachment #306871 - Flags: superreview?(dmose) → superreview-
(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.
Attached patch The fix v2 (diff -w) (obsolete) — Splinter Review
Patch with comments fix (diff -w)
Attachment #306871 - Attachment is obsolete: true
Attachment #308987 - Flags: superreview?(dmose)
Attachment #308987 - Flags: review+
Attached patch The fix v2 (normal patch) (obsolete) — Splinter Review
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
Attachment #308987 - Flags: superreview?(dmose) → superreview+
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.
Attachment #308987 - Attachment is obsolete: true
Attachment #308989 - Attachment is obsolete: true
Attachment #309797 - Flags: superreview?(dmose)
Comment on attachment 309797 [details] [diff] [review]
The fix v3 (diff -w)

sr=dmose
Attachment #309797 - Flags: superreview?(dmose) → superreview+
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.
Attachment #309797 - Flags: approval1.9?
Comment on attachment 309797 [details] [diff] [review]
The fix v3 (diff -w)

a1.9=beltzner
Attachment #309797 - Flags: approval1.9? → approval1.9+
Patch checked in -> fixed.
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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 !?
Target Milestone: --- → mozilla1.9beta5
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.