Bug 534822 Address books get added with no name if created via altering default preferences (e.g. extension/mcd). r/sr=bienvenu
authorMark Banner <bugzilla@standard8.plus.com>
Wed, 14 Apr 2010 12:16:56 +0100
changeset 5437 598c45ea36a05e61e119c8c2b67cb7e7badc0225
parent 5436 9ecee87ecb58c14bffd6e6c8a68eba6487e9dc7a
child 5438 1f508d2cf9057b427156cd2e51f27ae6bb53aee6
push idunknown
push userunknown
push dateunknown
bugs534822
Bug 534822 Address books get added with no name if created via altering default preferences (e.g. extension/mcd). r/sr=bienvenu
mailnews/addrbook/src/nsAbDirProperty.cpp
mailnews/addrbook/src/nsDirPrefs.cpp
mailnews/addrbook/test/unit/data/bug534822prefs.js
mailnews/addrbook/test/unit/test_bug534822.js
--- a/mailnews/addrbook/src/nsAbDirProperty.cpp
+++ b/mailnews/addrbook/src/nsAbDirProperty.cpp
@@ -94,20 +94,37 @@ NS_IMETHODIMP nsAbDirProperty::GetDirNam
 {
   if (m_DirPrefId.IsEmpty())
   {
     aDirName = m_ListDirName;
     return NS_OK;
   }
 
   nsCString dirName;
-  nsresult rv = GetLocalizedStringValue("description", EmptyCString(),
-                                        dirName);
+  nsresult rv = GetLocalizedStringValue("description", EmptyCString(), dirName);
   NS_ENSURE_SUCCESS(rv, rv);
 
+  // In TB 2 only some prefs had chrome:// URIs. We had code in place that would
+  // only get the localized string pref for the particular address books that
+  // were built-in.
+  // Additionally, nsIPrefBranch::getComplexValue will only get a non-user-set,
+  // non-locked pref value if it is a chrome:// URI and will get the string
+  // value at that chrome URI. This breaks extensions/autoconfig that want to
+  // set default pref values and allow users to change directory names.
+  //
+  // Now we have to support this, and so if for whatever reason we fail to get
+  // the localized version, then we try and get the non-localized version
+  // instead. If the string value is empty, then we'll just get the empty value
+  // back here.
+  if (dirName.IsEmpty())
+  {
+    rv = GetStringValue("description", EmptyCString(), dirName);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
   CopyUTF8toUTF16(dirName, aDirName);
   return NS_OK;
 }
 
 // XXX Although mailing lists could use the NotifyItemPropertyChanged
 // mechanism here, it requires some rework on how we write/save data
 // relating to mailing lists, so we're just using the old method of a
 // local variable to store the mailing list name.
--- a/mailnews/addrbook/src/nsDirPrefs.cpp
+++ b/mailnews/addrbook/src/nsDirPrefs.cpp
@@ -801,17 +801,17 @@ static char *DIR_GetStringPref(const cha
     if (NS_FAILED(rv))
         return nsnull;
 
     nsCString value;
     nsCAutoString prefLocation(prefRoot);
 
     prefLocation.Append('.');
     prefLocation.Append(prefLeaf);
- 
+
     if (NS_SUCCEEDED(pPref->GetCharPref(prefLocation.get(), getter_Copies(value))))
     {
         /* unfortunately, there may be some prefs out there which look like this */
         if (value.EqualsLiteral("(null)")) 
         {
             if (defaultValue)
                 value = defaultValue;
             else
@@ -832,44 +832,58 @@ static char *DIR_GetStringPref(const cha
 /*
   Get localized unicode string pref from properties file, convert into an UTF8 string 
   since address book prefs store as UTF8 strings.  So far there are 2 default 
   prefs stored in addressbook.properties.
   "ldap_2.servers.pab.description"
   "ldap_2.servers.history.description"
 */
 
-static char *DIR_GetLocalizedStringPref
-(const char *prefRoot, const char *prefLeaf, const char *defaultValue)
+static char *DIR_GetDescription(const char *prefRoot)
 {
   nsresult rv;
   nsCOMPtr<nsIPrefBranch> pPref(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
 
   if (NS_FAILED(rv))
     return nsnull;
 
   nsCAutoString prefLocation(prefRoot);
-  prefLocation.Append('.');
-  prefLocation.Append(prefLeaf);
+  prefLocation.AppendLiteral(".description");
 
   nsString wvalue;
   nsCOMPtr<nsIPrefLocalizedString> locStr;
 
   rv = pPref->GetComplexValue(prefLocation.get(), NS_GET_IID(nsIPrefLocalizedString), getter_AddRefs(locStr));
   if (NS_SUCCEEDED(rv))
     rv = locStr->ToString(getter_Copies(wvalue));
 
   char *value = nsnull;
   if (!wvalue.IsEmpty())
   {
     NS_ConvertUTF16toUTF8 utf8str(wvalue.get());
     value = ToNewCString(utf8str);
   }
   else
-    value = defaultValue ? strdup(defaultValue) : nsnull;
+  {
+    // In TB 2 only some prefs had chrome:// URIs. We had code in place that would
+    // only get the localized string pref for the particular address books that
+    // were built-in.
+    // Additionally, nsIPrefBranch::getComplexValue will only get a non-user-set,
+    // non-locked pref value if it is a chrome:// URI and will get the string
+    // value at that chrome URI. This breaks extensions/autoconfig that want to
+    // set default pref values and allow users to change directory names.
+    //
+    // Now we have to support this, and so if for whatever reason we fail to get
+    // the localized version, then we try and get the non-localized version
+    // instead. If the string value is empty, then we'll just get the empty value
+    // back here.
+    rv = pPref->GetCharPref(prefLocation.get(), &value);
+    if (NS_FAILED(rv))
+      value = nsnull;
+  }
 
   return value;
 }
 
 static PRInt32 DIR_GetIntPref(const char *prefRoot, const char *prefLeaf, PRInt32 defaultValue)
 {
   nsresult rv;
   nsCOMPtr<nsIPrefBranch> pPref(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
@@ -1104,17 +1118,17 @@ static void DIR_GetPrefsForOneServer(DIR
 
   // this call fills in tempstring with the position pref, and
   // we then check to see if it's locked.
   server->position = DIR_GetIntPref (prefstring, "position", kDefaultPosition);
 
   // For default address books, this will get the name from the chrome
   // file referenced, for other address books it'll just retrieve it from prefs
   // as normal.
-  server->description = DIR_GetLocalizedStringPref(prefstring, "description", "");
+  server->description = DIR_GetDescription(prefstring);
   
   server->dirType = (DirectoryType)DIR_GetIntPref (prefstring, "dirType", LDAPDirectory);
 
   server->fileName = DIR_GetStringPref (prefstring, "filename", "");
   // if we don't have a file name try and get one
   if (!server->fileName || !*(server->fileName)) 
     DIR_SetServerFileName (server);
   if (server->fileName && *server->fileName)
new file mode 100644
--- /dev/null
+++ b/mailnews/addrbook/test/unit/data/bug534822prefs.js
@@ -0,0 +1,3 @@
+pref("ldap_2.servers.extension.description", "extension");
+pref("ldap_2.servers.extension.filename", "ldap1.mab");
+pref("ldap_2.servers.extension.uri", "ldap://test.invalid:389/o=invalid??sub");
new file mode 100644
--- /dev/null
+++ b/mailnews/addrbook/test/unit/test_bug534822.js
@@ -0,0 +1,49 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/*
+ * Tests for bug 534822 - non-built-in address books specified in preferences
+ * don't appear in address book lists.
+ */
+
+function run_test() {
+  // Read in the prefs that will be default.
+  let specialPrefs = do_get_file("data/bug534822prefs.js");
+
+  specialPrefs.copyTo(gProfileDir, "");
+
+  specialPrefs = gProfileDir;
+  specialPrefs.append("bug534822prefs.js");
+
+  Cc["@mozilla.org/preferences-service;1"]
+    .getService(Ci.nsIPrefService)
+    .readUserPrefs(specialPrefs);
+
+  // Now load the ABs and check we've got all of them.
+  let dirs = Cc["@mozilla.org/abmanager;1"]
+               .getService(Ci.nsIAbManager)
+               .directories;
+
+  let dir;
+
+  let results = [
+    { name: "extension", result: false },
+    { name: kPABData.dirName, result: false },
+    { name: kCABData.dirName, result: false }
+  ];
+
+  // Check the OS X Address Book if available
+  if ("@mozilla.org/rdf/resource-factory;1?name=moz-abosxdirectory" in Cc)
+    results.push({ name: kOSXData.dirName, result: false });
+
+  while (dirs.hasMoreElements()) {
+    let dir = dirs.getNext().QueryInterface(Ci.nsIAbDirectory);
+    
+    for (let i = 0; i < results.length; ++i) {
+      if (results[i].name == dir.dirName) {
+        do_check_false(results[i].result);
+        results[i].result = true;
+      }
+    }
+  }
+
+  results.forEach(function (result) { do_check_true(result.result); });
+};