The default bug view has changed. See this FAQ.

Refactor the Address Book interfaces

ASSIGNED
Assigned to

Status

MailNews Core
Address Book
P3
normal
ASSIGNED
9 years ago
2 years ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

(Depends on: 5 bugs, Blocks: 4 bugs)

Trunk
Thunderbird 3.0b2
Dependency tree / graph
Bug Flags:
blocking-thunderbird3.0a2 -
blocking-thunderbird3 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [branch @ http://hg.mozilla.org/users/Pidgeot18_gmail.com/ab_rewrite], URL)

Attachments

(3 attachments, 15 obsolete attachments)

126.54 KB, patch
Details | Diff | Splinter Review
302.20 KB, patch
standard8
: review+
Details | Diff | Splinter Review
1.07 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
Refactor addressbook to use the interfaces currently planned out by Standard8 on his talk page.
(Assignee)

Comment 1

9 years ago
Created attachment 298593 [details] [diff] [review]
WIP #1

First off: I want to kill mork, I want to clobber it, I want to mutilate it, maim it, torture it even (it's certainly doing that to me!).

Mork complaints aside, this is my first WIP for refactoring abook. Currently, test_basic_nsIAbCard leaks 12 strings, and an assertion is thrown in test_cardForEmail.js (at least I removed ~250 lines of code from nsAddrDatabase!). Both of these are known problems, and leaks are going to be ignored until after I ensure that all consumers of nsIAbCard are not using the nsString properties. After these consumers are changed, the new contract will be gracefully inserted.

The one-line change in mork is to fix a bizarre null-param error in mork.

Some commonly-accessed properties may want to have the property access still. DisplayName and PrimaryEmail are the ones that seem to be the most heavily used.

Finally, why is aimScreenName stored locally as _AimScreenName? It looks clearer to me to strip off the leading underscore and only worry about it at database migration time.
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
(Assignee)

Comment 2

9 years ago
Created attachment 299215 [details] [diff] [review]
WIP #2

Remember what I said about mork in my last comment? Forget it. I cannot imagine a suitable punishment for mork. All I can say is that I can't wait for demorkification to finish.

My code no longer leaks (hallelujah), but it is still incapable of passing the fourth test because reification in the database is still using hardcoded property lists.

I also have a list of the properties being set for debugging purposes (mostly due to mork aggravation).
Attachment #298593 - Attachment is obsolete: true
(Assignee)

Comment 3

9 years ago
Created attachment 299630 [details] [diff] [review]
WIP #3

This patch does some more conversion, adds a gob of documentation, and fixes some more stuff.

This fixes:
* An error in LDAP I hadn't noticed before.
* I18n problems and the first gross leaks I saw.
* Uses more A{C}String, less {w}string
* Passes all tests now.

I know that palmsync and much of import will fail with this patch. Both SM and TB shouldn't crash on this patch.

No more mork complaints for the time being, just astonishment of the fragile code in some places (*cough* nsImportFieldMap *cough*).
Attachment #299215 - Attachment is obsolete: true
(Assignee)

Comment 4

9 years ago
Created attachment 300526 [details] [diff] [review]
WIP #4

The latest WIP; it should work for the most part, however I don't think it fully conforms to the proper IDL spec (excluding mailing list stuff, of course). Usages in dozens of files and large parts in code that I cannot build make it difficult to tell.
(Everything to date has been concerned with only converting nsIAbCard; the other interfaces are expected to be contained in another patch for ease of reviews).
Attachment #299630 - Attachment is obsolete: true
(Assignee)

Comment 5

9 years ago
Created attachment 301044 [details] [diff] [review]
WIP #5 (warning: possibly bitrotted)

This patch passes all 4 tests I have. One major warning though -- it is potentially bitrotted by Mark's recent checkin. It is also liable to fail for import and palmsync.

nsIAbMDBCard is on its last legs here and should be removed by the next update, although nsAbMDBCard will probably stay a bit longer.
Attachment #300526 - Attachment is obsolete: true
(Assignee)

Comment 6

9 years ago
Created attachment 302667 [details] [diff] [review]
Patch ready for review

A nice, big patch for review...

Known to build on Linux and Mac OS X, Windows/palmsync not known.

(Raw hg mq patch)
Attachment #301044 - Attachment is obsolete: true
Attachment #302667 - Flags: review?(bugzilla)
Comment on attachment 302667 [details] [diff] [review]
Patch ready for review

diff --git a/db/mork/src/morkRowCellCursor.cpp b/db/mork/src/morkRowCellCursor.cpp
--- a/db/mork/src/morkRowCellCursor.cpp
+++ b/db/mork/src/morkRowCellCursor.cpp
@@ -306,7 +306,6 @@ morkRowCellCursor::NextCell( // get next
    *outColumn = col;
      
   mRowCellCursor_Col = pos;
-  *outPos = pos;
   return NS_OK;
 }
   
presumably this is to avoid the duplicate assignment and possible crash?

 // Item is |[dialogField, cardProperty]|.
 const kVcardFields =
         [ // Contact > Name
-         ["FirstName", "firstName"],
-         ["LastName", "lastName"],
-         ["DisplayName", "displayName"],
-         ["NickName", "nickName"],
+         ["FirstName", "FirstName"],
+         ["LastName", "LastName"],
+         ["DisplayName", "DisplayName"],
+         ["NickName", "NickName"],

...

So now these will all match apart from ScreenName? If so it would seem to make sense that we should rename the id of the screen name field, and then have just one list of properties. Best do the renaming in a separate bug (please file, we could do it before or after, lets see how the comments go).

     if ("aimScreenName" in window.arguments[0])
-      gEditCard.card.aimScreenName = window.arguments[0].aimScreenName;
+      gEditCard.card.setProperty("_AimScreenName", window.arguments[0].aimScreenName);

It'd be nice to match the case of the window arguments with the actual cards, but again, I think we need a separate bug for that.

+  nsIVariant getProperty(in AString name);

Do mork/sqlite use property/column names as AString or ACString? Just a thought that we should be using whichever is more appropriate.

Comments still in progress... I've only looked at a bit of the patch so far.
Depends on: 417065
Comment on attachment 302667 [details] [diff] [review]
Patch ready for review

@@ -433,18 +433,25 @@ nsresult nsAbAutoCompleteSession::Search
           // Now, retrieve the user name and nickname
           rv = card->GetDisplayName(pDisplayNameStr);
           if (NS_FAILED(rv))
-              continue;
+              pDisplayNameStr = NS_LITERAL_STRING("");
+

EmptyString() is what you want here. Though I'm not sure you need to change them in the failure as we're re-initialising them each time round the loop. However having said that, it may be better to move them outside the loop and set them to EmptyString() if they fail.

+#include "nsHashPropertyBag.h"

This may not be compatible with the frozen API that we're moving over to. I'll have to check it out.

+  // Should this not be PR_Now?
+  SetPropertyAsUint32(NS_LITERAL_STRING("LastModifiedDate"), 0);

I think its best left as zero = "uninitialised". Please put a comment in to that effect.

+NS_IMETHODIMP nsAbCardProperty::GetFirstName(nsAString &aString)
+{
+  nsresult rv = GetPropertyAsAString(NS_LITERAL_STRING("FirstName"), aString);
+  if (rv == NS_ERROR_NOT_AVAILABLE)
+  {
+    aString.Truncate();
+    rv = NS_OK;
   }
-
-  // don't assert here, as failure is expected in certain cases
-  // we call GetCardValue() from nsAbView::Init() to determine if the
-  // saved sortColumn is valid or not.
   return rv;
 }

I think its neater just to return NS_OK early. ditto in the other places.

@@ -816,125 +312,32 @@ NS_IMETHODIMP nsAbCardProperty::Copy(nsI
...
+  nsresult rv = NS_OK;
...
+  nsCOMPtr<nsISimpleEnumerator> properties;
+  rv = srcCard->GetProperties(getter_AddRefs(properties));
+  NS_ENSURE_SUCCESS(rv, rv);

no need to do the separate initialisation here, just do nsresult rv = srcCard...

+  PRBool hasMore;
+  rv = properties->HasMoreElements(&hasMore);
+  while (NS_SUCCEEDED(rv) && hasMore)
+  {
+    nsCOMPtr<nsISupports> result;
+    rv = properties->GetNext(getter_AddRefs(result));
+    NS_ENSURE_SUCCESS(rv, rv);

please put the nsCOMPtr outside the loop.

The loop statement can just be:
while (NS_SUCCEEDED(properties->HasMoreElements(&hasMore) && hasMore)

saves on checking the value each time.
+    nsCOMPtr<nsIProperty> property = do_QueryInterface(result);

I think I'd prefer to keep the rv check in here just in case someone gives us a bogus iterator.

@@ -999,7 +402,7 @@ NS_IMETHODIMP nsAbCardProperty::ConvertT
         myAddPropValue(t, VCGivenNameProp, str.get(), &vCardHasData);
     }
 
-    (void)GetCompany(str);
+    (void)GetPropertyAsAString(NS_LITERAL_STRING("Company"), str);
     if (!str.IsEmpty())
     {

This won't work, GetPropertyAsAString will fail if the str doesn't exist, it doesn't alter str. If the previous Get succeeded in getting a value, then we'll duplicate it on the next one (bug 417065 should help with testing this ;-) ).

     nsCString escResult;
     MsgEscapeString(nsDependentCString(vCard), nsINetUtil::ESCAPE_URL_PATH, escResult);
-    *aResult = strdup(escResult.get());
-    return (*aResult ? NS_OK : NS_ERROR_OUT_OF_MEMORY);
+    aResult = escResult.get();
+    return NS_OK;

I think its safe enough here just to pass aResult to MsgEscapeString.

-  *result = PL_Base64Encode(NS_ConvertUTF16toUTF8(xmlStr).get(), 0, nsnull);
-  return (*result ? NS_OK : NS_ERROR_OUT_OF_MEMORY);
+  result = PL_Base64Encode(NS_ConvertUTF16toUTF8(xmlStr).get(), 0, nsnull);

Use result.Adopt(PL...) otherwise we'll leak strings.

@@ -1344,7 +747,7 @@ nsresult nsAbCardProperty::AppendData(co
 nsresult nsAbCardProperty::AppendData(const char *aAttrName, mozITXTToHTMLConv *aConv, nsString &aResult)
 {
   nsString attrValue;
-  nsresult rv = GetCardValue(aAttrName, attrValue);
+  nsresult rv = GetPropertyAsAString(NS_ConvertASCIItoUTF16(aAttrName), attrValue);
   NS_ENSURE_SUCCESS(rv,rv);

Not sure why you're converting from ascii here and UTF8 in the other Append functions.

+  else if (lastName.IsEmpty())
+    GetFirstName(aResult);
+  else if (firstName.IsEmpty())
+    GetLastName(aResult);

You can just do aResult = firstName or aResult = lastName here seeing as you've cached them already.

-  {
-    aResult = m_PhoneticLastName;
-    aResult += m_PhoneticFirstName;
-  }
+    aResult = lastName + firstName;

Please do it in the manner we had before. The former way does it in a fashion that the frozen API supports (it doesn't support (string + string).

 public: 
-
 	NS_DECL_ISUPPORTS
 	NS_DECL_NSIABCARD
+  NS_DECL_NSIABITEM
 
-	nsAbCardProperty(void);
+	nsAbCardProperty(const char * uuid = "Uuid");
 	virtual ~nsAbCardProperty(void);

As you're there and its a big rewrite, could you drop the tabs please?

@@ -173,7 +173,7 @@ NS_IMETHODIMP nsAbLDAPCard::GetLDAPMessa
     if (attr.IsEmpty())
       continue;
  
-    rv = GetCardValue(props[i], propvalue);
+    rv = GetPropertyAsAString(NS_ConvertASCIItoUTF16(props[i]), propvalue);
     NS_ENSURE_SUCCESS(rv, rv);
 
     nsCOMPtr<nsILDAPModification> mod =
@@ -246,7 +246,7 @@ NS_IMETHODIMP nsAbLDAPCard::BuildRdn(nsI
     NS_ENSURE_SUCCESS(rv, rv);
 
     // Get the property value
-    rv = GetCardValue(prop.get(), propvalue);
+    rv = GetPropertyAsAString(NS_ConvertUTF8toUTF16(prop), propvalue);
     NS_ENSURE_SUCCESS(rv, rv);
 
     // XXX The case where an attribute needed to build the Relative

Inconsistency again, maybe we need to look at these in more detail.

-	nsAbCardProperty(void);
+	nsAbCardProperty(const char * uuid = "Uuid");

I'm not sure if defaulting this is a good idea. For instance, if we keep equals as it is, I could create two cards, give them totally different fields, then compare them, and they'd come out the same. Ok, we can do that with ones we get out a database, but I think I'd prefer for it to return a failure or something if uuid hasn't been set (only when accessing uuid of course). What do you think?

I've still got more to look at yet, but that'll do for tonight.

Comment 9

9 years ago
Comment on attachment 302667 [details] [diff] [review]
Patch ready for review

+  nsCOMPtr<nsIMdbRowCellCursor> iterator;
+  nsIMdbCell *cell;

You should use COMPtrs for all the nsIMdb pointers. In general, all of Mozilla would leak like a sieve if people didn't use comptrs for nsISupports pointers - it's not particular to Mork - it's particular to XPCOM.
(Assignee)

Comment 10

9 years ago
(In reply to comment #7)
> (From update of attachment 302667 [details] [diff] [review])
> diff --git a/db/mork/src/morkRowCellCursor.cpp
> b/db/mork/src/morkRowCellCursor.cpp
> --- a/db/mork/src/morkRowCellCursor.cpp
> +++ b/db/mork/src/morkRowCellCursor.cpp
> @@ -306,7 +306,6 @@ morkRowCellCursor::NextCell( // get next
> 
> presumably this is to avoid the duplicate assignment and possible crash?

Yes, it stops mork from crashing when reading in a database.

> So now these will all match apart from ScreenName? If so it would seem to make
> sense that we should rename the id of the screen name field, and then have just
> one list of properties. Best do the renaming in a separate bug (please file, we
> could do it before or after, lets see how the comments go).

If we're changing property names, WebPage1 and WebPage2 could also be renamed to be more descriptive. Will create a new bug.

> +  nsIVariant getProperty(in AString name);
> 
> Do mork/sqlite use property/column names as AString or ACString? Just a thought
> that we should be using whichever is more appropriate.

SQLite uses AUTF8String (= ACString). However, the property bag uses AString, which means that using ACString here would probably cause a lot more UTF-8/UTF-16 conversion than necessary.

One thing to look into is using ACString, or even string, for the getPropertyAs*** strings because their most prevalent uses seem to be for getting a specific property with a literal string.

> Comments still in progress... I've only looked at a bit of the patch so far.

Replies also to come later, hoping that traffic isn't bad today...
(In reply to comment #10)
> (In reply to comment #7)
> > (From update of attachment 302667 [details] [diff] [review] [details])
> > diff --git a/db/mork/src/morkRowCellCursor.cpp
> > b/db/mork/src/morkRowCellCursor.cpp
> > --- a/db/mork/src/morkRowCellCursor.cpp
> > +++ b/db/mork/src/morkRowCellCursor.cpp
> > @@ -306,7 +306,6 @@ morkRowCellCursor::NextCell( // get next
> > 
> > presumably this is to avoid the duplicate assignment and possible crash?
> Yes, it stops mork from crashing when reading in a database.

ok, let's hive that off to another bug and it'll be easier to track.

> > +  nsIVariant getProperty(in AString name);
> > 
> > Do mork/sqlite use property/column names as AString or ACString? Just a thought
> > that we should be using whichever is more appropriate.
> SQLite uses AUTF8String (= ACString). However, the property bag uses AString,
> which means that using ACString here would probably cause a lot more
> UTF-8/UTF-16 conversion than necessary.

Ok, AString is fine then.

> One thing to look into is using ACString, or even string, for the
> getPropertyAs*** strings because their most prevalent uses seem to be for
> getting a specific property with a literal string.

I'd prefer to keep them as ACString or AString whichever is more appropriate.
(Assignee)

Updated

9 years ago
Depends on: 418006
Comment on attachment 302667 [details] [diff] [review]
Patch ready for review

@@ -173,7 +173,7 @@ NS_IMETHODIMP nsAbLDAPCard::GetLDAPMessa
     if (attr.IsEmpty())
       continue;
  
-    rv = GetCardValue(props[i], propvalue);
+    rv = GetPropertyAsAString(NS_ConvertASCIItoUTF16(props[i]), propvalue);
     NS_ENSURE_SUCCESS(rv, rv);
 
     nsCOMPtr<nsILDAPModification> mod =
@@ -246,7 +246,7 @@ NS_IMETHODIMP nsAbLDAPCard::BuildRdn(nsI
     NS_ENSURE_SUCCESS(rv, rv);
 
     // Get the property value
-    rv = GetCardValue(prop.get(), propvalue);
+    rv = GetPropertyAsAString(NS_ConvertUTF8toUTF16(prop), propvalue);
     NS_ENSURE_SUCCESS(rv, rv);
 
 
This is inconsistent, I think both should probably be UTF-8, not only that, why not get as CString anyway?

@@ -738,32 +732,13 @@ NS_IMETHODIMP nsAbMDBDirectory::AddCard(
   if (NS_FAILED(rv) || !mDatabase)
     return NS_ERROR_FAILURE;
 
-  nsCOMPtr<nsIAbCard> newCard;
-  nsCOMPtr<nsIAbMDBCard> dbcard;
-
...
-  NS_IF_ADDREF(*addedCard = newCard);
+  *addedCard = card;
   return NS_OK;
 }

This is a bit of a note to self, but does this mean we won't need to return a "new" card because it doesn't get changed during the modifications?

-              rv = card->GetCardValue(EXPORT_ATTRIBUTES_TABLE[i].abColName,
-                                      value);
+              nsAutoString column = NS_ConvertUTF8toUTF16(nsCString(EXPORT_ATTRIBUTES_TABLE[i].abColName));
+              rv = card->GetPropertyAsAString(column, value);

This looks wrong. I'd prefer it without the column temp var, but do we really need the extra nsCString around the export attributes table? Would it be better to reformat that table into a PRUnichar* or something?

+SetStringProperty(nsAbOSXCard *aCard, nsString &aValue, const char *aMemberName,
+                  PRBool aNotify, nsIAbManager *aAbManager)
 {
+  NS_ConvertASCIItoUTF16 aMember(aMemberName);

ASCII or UTF-8?

diff --git a/mailnews/addrbook/src/nsAbOutlookCard.cpp b/mailnews/addrbook/src/nsAbOutlookCard.cpp
--- a/mailnews/addrbook/src/nsAbOutlookCard.cpp
+++ b/mailnews/addrbook/src/nsAbOutlookCard.cpp
...
+        SetPropertyAsAString(NS_LITERAL_STRING("NickName"), *unichars[index_NickName]);

Not using the constants? Ditto in nsAbOutlookDirectory.cpp

+        SetPropertyAsAString(NS_LITERAL_STRING("HomeAddress"), unichar) ;
+        SetPropertyAsAString(NS_LITERAL_STRING("HomeAddress2"), unicharBis) ;

nit: no space before ;

Note to self: reviewed up to nsAddrDatabase.cpp.
Comment on attachment 302667 [details] [diff] [review]
Patch ready for review

+
+#include "nsIProperty.h"
+#include "nsIVariant.h"

No need for the blank line here

 static const char kRecordKeyColumn[] = "RecordKey";
+#define RECORDKEYCOLUMN NS_LITERAL_STRING("RecordKey")
 static const char kLastRecordKeyColumn[] = "LastRecordKey";

Why not get rid of kRecordKeyColumn?

-      card->GetFirstName(unicodeStr);
+      card->GetPropertyAsAString(NS_LITERAL_STRING("FirstName"), unicodeStr);

Constants?

+#define NS_MDB_ROWID NS_LITERAL_STRING("DbRowID")
 nsresult nsAddrDatabase::AddAttributeColumnsToRow(nsIAbCard *card, nsIMdbRow *cardRow)

#define at top of the file, below the includes please.

-  mdbOid rowOid, tableOid;
-  m_mdbPabTable->GetOid(m_mdbEnv, &tableOid);
+  mdbOid rowOid;
   cardRow->GetOid(m_mdbEnv, &rowOid);
 
-  nsCOMPtr<nsIAbMDBCard> dbcard(do_QueryInterface(card, &err));
-  if(NS_SUCCEEDED(err) && dbcard)
-  {
-    dbcard->SetDbTableID(tableOid.mOid_Id);
-    dbcard->SetDbRowID(rowOid.mOid_Id);
-  }
+  card->SetPropertyAsUint32(NS_MDB_ROWID, rowOid.mOid_Id);

So are we assuming all cards are in the pab table now?

+      // We can't get as a char * because that messes up UTF8 stuff

I may have already asked this, why?

+      const char * realValue = NS_ConvertUTF16toUTF8(value).get();
 
-    card->GetDisplayName(unicodeStr);
-    AddDisplayName(cardRow, NS_ConvertUTF16toUTF8(unicodeStr).get());
+      mdb_token token;
+      rv = m_mdbStore->StringToToken(m_mdbEnv, NS_ConvertUTF16toUTF8(name).get(), &token);
+      NS_ENSURE_SUCCESS(rv, rv);
+ 
+      rv = AddCharStringColumn(cardRow, token, realValue);
+      NS_ENSURE_SUCCESS(rv, rv);

Just do the value conversion in-line in the  AddCharStringColumn call.

+    // Primary email is special
+    nsAutoString primaryEmail;
+    card->GetPrimaryEmail(primaryEmail);
+    AddPrimaryEmail(cardRow, NS_ConvertUTF16toUTF8(primaryEmail).get());

Hmm, this is unclear, so we've just called getProperties, but primaryEmail isn't one of those properties? What about first/last/displayName? The interface definition implies I could call getCardProperty to get these attributes, rather than just calling the easy function.

-  nsCOMPtr<nsIAbMDBCard> dbcard(do_QueryInterface(card, &err));
+  err =card->GetPropertyAsUint32(NS_MDB_ROWID, (PRUint32*)&rowOid.mOid_Id);

nit: missing space

+      nsString name = NS_ConvertUTF8toUTF16(static_cast<char *>(colYarn.mYarn_Buf),
+          colYarn.mYarn_Fill);
...
+  NS_ConvertUTF8toUTF16 value(static_cast<const char*>(newYarn.mYarn_Buf),
+      newYarn.mYarn_Fill);

Inconsistent, the second one is better.


I've been through it all once now, in general it is looking reasonable. I haven't tested it yet, I think I'll do that with the next version.
Attachment #302667 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 14

9 years ago
(In reply to comment #13)

(Sorry, haven't had a chance to go through comment #12 yet)

>  static const char kRecordKeyColumn[] = "RecordKey";
> +#define RECORDKEYCOLUMN NS_LITERAL_STRING("RecordKey")
>  static const char kLastRecordKeyColumn[] = "LastRecordKey";
> 
> Why not get rid of kRecordKeyColumn?

The constructor needs it for some internal mork bookkeeping stuff.

> -      card->GetFirstName(unicodeStr);
> +      card->GetPropertyAsAString(NS_LITERAL_STRING("FirstName"), unicodeStr);
> 
> Constants?

I wanted to define these in nsIAbCard.idl (as a replacement for the property list in nsIAddrDatabase.idl), but the requisite header for NS_LITERAL_STRING wasn't there...

> -  mdbOid rowOid, tableOid;
> -  m_mdbPabTable->GetOid(m_mdbEnv, &tableOid);
> +  mdbOid rowOid;
>    cardRow->GetOid(m_mdbEnv, &rowOid);
> 
> -  nsCOMPtr<nsIAbMDBCard> dbcard(do_QueryInterface(card, &err));
> -  if(NS_SUCCEEDED(err) && dbcard)
> -  {
> -    dbcard->SetDbTableID(tableOid.mOid_Id);
> -    dbcard->SetDbRowID(rowOid.mOid_Id);
> -  }
> +  card->SetPropertyAsUint32(NS_MDB_ROWID, rowOid.mOid_Id);
> 
> So are we assuming all cards are in the pab table now?

The only time when table IDs are used are for equality purposes. The only other table is the deleted cards table, so the only time when the table ID matters may be when comparing cards that have been deleted with those that have not been deleted. Looking through the code, it appears that this does not happen...

Furthermore, my ad-hoc tests seem to indicate that mork doesn't reuse the rowids across tables (looks at bienvenu)? If so, the rowid is sufficient to test; if not, the question of whether or not we are comparing deleted/non-deleted (MDB) cards anyway needs to be looked into more.

> +      // We can't get as a char * because that messes up UTF8 stuff
> 
> I may have already asked this, why?

Stuff was mangled the last time, IIRC, but it may have been due to other concurrent problems.

> +    // Primary email is special
> +    nsAutoString primaryEmail;
> +    card->GetPrimaryEmail(primaryEmail);
> +    AddPrimaryEmail(cardRow, NS_ConvertUTF16toUTF8(primaryEmail).get());
> 
> Hmm, this is unclear, so we've just called getProperties, but primaryEmail
> isn't one of those properties? What about first/last/displayName? The interface
> definition implies I could call getCardProperty to get these attributes, rather
> than just calling the easy function.

PrimaryEmail is stored both lowercase and as originally specified (apparently for mailing list stuff); I have added a note to this effect in the code.

(Deleted the responses to which I have no response...)
(Assignee)

Updated

9 years ago
Flags: wanted-thunderbird3.0a1?
Flags: blocking-thunderbird3?
Depends on: 422800
(Assignee)

Comment 15

9 years ago
I'll hopefully be getting an updated patch up tomorrow (I'm still processing the changes, takes quite a while), but I want to point out some issues I have first.

1. The new tests, unfortunately, didn't catch the VCard failure, since the mork database contains all the properties, just with an empty value. After doing some manual mork-file modification (recommendation: avoid at all costs), I successfully confirmed the failure and my fix. The tests should probably be changed.

2. Reading nsIAbCard::generateFormattedEmail carefully, it seems that it should go on nsIAbGroup or nsIAbItem (IIUC).

3. Shouldn't nsIAbCollection::cardForProperty return an nsIAbCard and not an nsIAbItem? At the same time, wouldn't it be just as easy to return null if it can't be found and not error out?

4. nsIAbMDBDirectory::cardForEmailAddress is equivalent to two calls to cardForProperty (PrimaryEmail and SecondaryEmail). Should nsIAbCollection have this more advanced semantics as well (slightly generified?)?

5. It might be useful to have nsIAbCollection or nsIAbDirectory be able to batch changes. I'm thinking that only nsIAbDirectory should have the ability to modify cards, etc.

6. I'm starting to flesh out changes to nsIAb{MDB}Directory. For example, I've refactored cardForEmailAddress upwards and removed operations in lieu of readOnly. I'd appreciate some more flesh out in this area, especially with respect to which methods of nsIAbDirectory get refactored into nsIAbCollection.

I've probably got more to say, but that's all I can think of for now...
(Assignee)

Comment 16

9 years ago
Created attachment 309796 [details] [diff] [review]
nsIAbCard refactor, patch v1.5

Somewhat of a WIP and somewhat not. I would still like comments as if this was requesting review in areas that I've changed.

It's not quite a reviewable patch because it doesn't take into account Standard8's concurrent modification to the spec.

I've also modified what I'm calling the names of patches to explain better.

Oh yes, one last thing, I forgot to modify the UUID of nsIAbCard last time...
Attachment #302667 - Attachment is obsolete: true
(Assignee)

Comment 17

9 years ago
Created attachment 309799 [details] [diff] [review]
nsIAbDirectory refactor, patch WIP #1

This is the start of my work on sanifying directory stuff in addrbook. I don't quite remember all the changes I've made, but this is a brief summary:

* nsIAddrDatabase and nsIAbMDBDirectory usage limited to addrbook, palmsync, and import. More needs to be done on cleaning up usages in these places.
* cardForEmail is refactored upwards, and cardForProperty has been added.
* nsIAbDirectory.operations has been removed and replaced with nsIAbDirectory.readOnly where applicable. Everyone only ever cared about opWrite anyways...
* nsIAbCollection is defined but not used yet.
* More attributions probably need to be changed.
* Some of the newer accessors are neither defined nor implemented.
* Not tested on non-Linux builds. Not guaranteed to compile with palmsync or on Windows or Mac.

Re my last patch, ignore those client.mk changes. I have no idea how they ever got there.

This patch probably requires the other patch to work correctly or even apply correctly.
(Assignee)

Comment 18

9 years ago
Created attachment 310337 [details] [diff] [review]
nsIAbCard refactor, patch v2

Updated patch. Looking back over my nsAbOSXCard.mm changes drove me crazy for a while...
Attachment #309796 - Attachment is obsolete: true
Attachment #310337 - Flags: review?(bugzilla)
Comment on attachment 310337 [details] [diff] [review]
nsIAbCard refactor, patch v2

 function CreatePrintCardUrl(card)
 {
-  var url = "data:application/xml;base64," + card.convertToBase64EncodedXML();
+  var url = "data:application/xml;base64," + card.translateTo("base64");
   return url;
 }

nit: please drop the temporary variable.

+  /////////////////////////////////////////////////////////////////////////////
+  // DEPRECATED FOOLS
+  /////////////////////////////////////////////////////////////////////////////
+
+// Card properties

nit: Card properties needs indenting or dropping.

-              continue;
+              pDisplayNameStr = NS_LITERAL_STRING("");
+

pDisplayNameStr = EmptyString();

 struct AppendItem {
-  const char *mColumn;
-  const char *mLabel;
+  nsString mColumn;
+  const PRUnichar* mLabel;
   EAppendType mAppendType;
 };
 
+#define APPEND_ENTRY(column, literal, type) \
+  {NS_LITERAL_STRING(column), NS_ConvertASCIItoUTF16(literal).get(), type}
+
 static const AppendItem NAME_ATTRS_ARRAY[] = {

static nsString declarations aren't allowed especially because on mac it can cause crashes due to the order it unloads dylibs.

I've only had a brief look down to this level. I'll try and comment more later if I get chance.
Comment on attachment 310337 [details] [diff] [review]
nsIAbCard refactor, patch v2

+#define APPEND_ENTRY(column, literal, type) \
+  {NS_LITERAL_STRING(column), NS_ConvertASCIItoUTF16(literal).get(), type}
+

Oh, and I don't like the NS_ConvertASCIItoUTF16(...).get(); So if there is another way around it, that would be good.

+  attribute AUTF8String uuid;
...
+nsAbCardProperty::nsAbCardProperty(const char * uuid)
+  : m_IsMailList(PR_FALSE),
+    m_UuidProperty(NS_ConvertASCIItoUTF16(uuid))

uuids are UTF8, not ASCII

+NS_IMETHODIMP nsAbCardProperty::TranslateTo(const nsACString &type, nsACString &result)

Please add an XXX comment saying that this needs re-implementing using the category manager to provide generic access. Please don't do that in this patch.

   if (aLastNameFirst)
-  {
-    aResult = m_PhoneticLastName;
-    aResult += m_PhoneticFirstName;
-  }
+    aResult = lastName + firstName;
   else
-  {
-    aResult = m_PhoneticFirstName;
-    aResult += m_PhoneticLastName;
-  }
+    aResult = firstName + lastName;

Please don't do this, it doesn't work with the frozen API.

+  if (NS_FAILED(GetPropertyAsAUTF8String(DNCOLUMN, aDN)))
+      aDN.Truncate();

nit: wrong indentation

+    nsCOMPtr<nsIAbCard> newCard(do_CreateInstance(NS_ABMDBCARD_CONTRACTID,
                                                     &rv));

nit: indentation

+        SetPropertyAsAString(NS_LITERAL_STRING("NickName"), *unichars[index_NickName]);
+        SetPropertyAsAString(NS_LITERAL_STRING("WorkPhone"), *unichars[index_WorkPhoneNumber]);

I think I'd still prefer these ("NickName") etc, to be #define or const char* in a header file somewhere (without the NS_LITERAL_STRING I think).

-        listCard->SetNickName(tempString);
+        listCard->SetPropertyAsAString(NS_LITERAL_STRING("NickName"),
+            tempString);

nit: tempString should be aligned with NS_L...

This is looking much better now. Still r- because of the static nsString issue though.
Attachment #310337 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 21

9 years ago
(In reply to comment #19)

> static nsString declarations aren't allowed especially because on mac it can
> cause crashes due to the order it unloads dylibs.

Does this prohibit static NS_Convert{ASCII,UTF8}ToUTF16(...).get() as well?

(In reply to comment #20)
> Oh, and I don't like the NS_ConvertASCIItoUTF16(...).get(); So if there is
> another way around it, that would be good.

Unfortunately, that's the only way to convert a char * to a PRUnichar *.

> +  attribute AUTF8String uuid;
> ...
> +nsAbCardProperty::nsAbCardProperty(const char * uuid)
> +  : m_IsMailList(PR_FALSE),
> +    m_UuidProperty(NS_ConvertASCIItoUTF16(uuid))
> 
> uuids are UTF8, not ASCII

The uuid here is the property name (e.g., DBRowId); I'll probably change the argument name to uuidProperty to make this clearer.

> +NS_IMETHODIMP nsAbCardProperty::TranslateTo(const nsACString &type, 
> +        SetPropertyAsAString(NS_LITERAL_STRING("NickName"),
> *unichars[index_NickName]);
> +        SetPropertyAsAString(NS_LITERAL_STRING("WorkPhone"),
> *unichars[index_WorkPhoneNumber]);
> 
> I think I'd still prefer these ("NickName") etc, to be #define or const char*
> in a header file somewhere (without the NS_LITERAL_STRING I think).

E.g. #define kNickNameProperty "NickName" in nsIAbCard.idl? My personal preference is for the NS_LITERAL_STRING to be included in the define because that is by far the majority of the times it is used...
Blocks: 424570
Depends on: 406921
No longer blocks: 424570
(Assignee)

Comment 22

9 years ago
Created attachment 313751 [details] [diff] [review]
nsIAbCard refactor, patch v2.5

Version 2.5 of the patch. Not requesting review since I know it breaks palmsync for a fact (it won't even compile).

That said, I would like to see where it breaks on OS X address books, as well as on Outlook ones. Other compile-time errors in palmsync also appreciated (the "m_XYZ not defined" is a known failure).

Updated features:
* Removed DefaultAddress, Category as they were unused.
* Moved property #defines over to nsIAbCard (I can probably drop a few #include "nsIAddrDatabase.h", but that'll wait until the nsIAbDirectory stuff).
* Stopped using literal strings and used the #defines everywhere in C++, hopefully
* Used UTF-8 as the internal storage map instead of UTF-16.
Attachment #310337 - Attachment is obsolete: true
(Assignee)

Comment 23

9 years ago
Created attachment 313833 [details] [diff] [review]
nsIAbDirectory refactor, patch WIP #2

Updated nsIAbDirectory refactoring for your benefit.

nsIAddrDatabase removal is proceeding along smoothly, breaking almost all of import in the process. I have now fixed text-import and LDIF-import to not use this heinous interface, but I have a suspicion that the code is broken (although compilable) and will continue to be broken until I fix the mailing list issues.

Some of the usages can be shifted around to accommodate batching. Once I implement the batching procedure, that is.
Attachment #309799 - Attachment is obsolete: true

Comment 24

9 years ago
I tried building with attachment 313751 [details] [diff] [review] and it fails:

c++ -o nsAbOSXCard.o -c -I../../../dist/include/system_wrappers -include /Volumes/MoeFonzo/applemail2/config/gcc_hidden.h -DMOZ_LDAP_XPCOM -DMOZILLA_INTERNAL_API -DOSTYPE=\"Darwin9.2.2\" -DOSARCH=Darwin  -I/Volumes/MoeFonzo/applemail2/mailnews/addrbook/src -I. -I../../../dist/include/xpcom -I../../../dist/include/string -I../../../dist/include/rdf -I../../../dist/include/rdfutil -I../../../dist/include/toolkitcomps -I../../../dist/include/appcomps -I../../../dist/include/dom -I../../../dist/include/layout -I../../../dist/include/widget -I../../../dist/include/mork -I../../../dist/include/pref -I../../../dist/include/necko -I../../../dist/include/locale -I../../../dist/include/unicharutil -I../../../dist/include/uconv -I../../../dist/include/mailnews -I../../../dist/include/msgbase -I../../../dist/include/msgbaseutil -I../../../dist/include/mime -I../../../dist/include/intl -I../../../dist/include/windowwatcher -I../../../dist/include/uriloader -I../../../dist/include/embed_base -I../../../dist/include/mozldap -I../../../dist/include   -I../../../dist/include/addrbook -I../../../dist/include/nspr     -I/usr/X11/include   -fPIC  -I/usr/X11/include -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-long-long -fno-strict-aliasing -fpascal-strings -fno-common -fshort-wchar -pthread -I/Developer/Headers/FlatCarbon -pipe  -DDEBUG -D_DEBUG -DDEBUG_hakan -DTRACING -g  -I/usr/X11/include -DMOZILLA_CLIENT -include ../../../mozilla-config.h -Wp,-MD,.deps/nsAbOSXCard.pp -fobjc-exceptions /Volumes/MoeFonzo/applemail2/mailnews/addrbook/src/nsAbOSXCard.mm
/Volumes/MoeFonzo/applemail2/mailnews/addrbook/src/nsAbOSXCard.mm: In function ‘void SetStringProperty(nsAbOSXCard*, nsString&, const char*, PRBool, nsIAbManager*)’:
/Volumes/MoeFonzo/applemail2/mailnews/addrbook/src/nsAbOSXCard.mm:89: error: ‘aMember’ was not declared in this scope
/Volumes/MoeFonzo/applemail2/mailnews/addrbook/src/nsAbOSXCard.mm: In member function ‘virtual nsresult nsAbOSXCard::Update(PRBool)’:
/Volumes/MoeFonzo/applemail2/mailnews/addrbook/src/nsAbOSXCard.mm:246: error: ‘kCompanyColumn’ was not declared in this scope
make[3]: *** [nsAbOSXCard.o] Error 1
make[2]: *** [libs] Error 2
make[1]: *** [libs] Error 2
make: *** [default] Error 2
(Assignee)

Comment 25

9 years ago
Created attachment 313951 [details] [diff] [review]
nsIAbCard refactor, patch v2.75

Fixed hwaara's problem in nsAbOSXCard.mm, and also got palmsync theoretically working again. If no one has problems building on Mac or Windows, I'll request r on this patch.
Attachment #313751 - Attachment is obsolete: true
Thanks Joshua and all, for your continuing care where this important work potentially impacts palmsync code. 

Note - Anyone with windows can test the AB patches impact on palmsync via emulator in a virtual test test environment. Instructions at wsm.wetpaint.com (getting this in wiki.mozilla.com is a future project).  IOW, you do not need a palm device.
(Assignee)

Comment 27

9 years ago
Created attachment 314110 [details] [diff] [review]
nsIAbCard refactor, patch v3

Updated so it is now known to build on OS X.
Attachment #313951 - Attachment is obsolete: true
Attachment #314110 - Flags: review?(bugzilla)
Comment on attachment 314110 [details] [diff] [review]
nsIAbCard refactor, patch v3

Unfortunately, I managed to bitrot some of this with my last mailnews check-in. I've fixed it by hand though, but please update it soon.

>diff --git a/mailnews/addrbook/public/nsIAbCard.idl b/mailnews/addrbook/public/nsIAbCard.idl
+  nsIVariant getProperty(in AString name);
+  void setProperty(in AString name, in nsIVariant value);
+  void deleteProperty(in AString name);

Make these take ACString for the name and let JS worry about the conversion.

>diff --git a/mailnews/addrbook/resources/content/abCardOverlay.js b/mailnews/addrbook/resources/content/abCardOverlay.js
>+         ["NickName", "nNckName"],

That's not quite right... (the mail/ version of this is correct though)

>diff --git a/mailnews/addrbook/src/nsAbCardProperty.cpp b/mailnews/addrbook/src/nsAbCardProperty.cpp
>--- a/mailnews/addrbook/src/nsAbCardProperty.cpp
>+++ b/mailnews/addrbook/src/nsAbCardProperty.cpp
>+PR_STATIC_CALLBACK(PLDHashOperator)
>+PropertyHashToArrayFunc (const nsACString &aKey, nsIVariant* aData, void *userArg)
>+{
>+  nsIMutableArray *propertyArray = static_cast<nsIMutableArray *>(userArg);
>+  nsSimpleProperty *sprop = new nsSimpleProperty(aKey, aData);
>+  propertyArray->AppendElement(sprop, PR_FALSE);
>+  return PL_DHASH_NEXT;
>+}
>+
>+NS_IMETHODIMP nsAbCardProperty::GetProperties(nsISimpleEnumerator **props)
>+{
>+  nsCOMPtr<nsIMutableArray> propertyArray = do_CreateInstance(NS_ARRAY_CONTRACTID);
>+  if (!propertyArray)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  m_properties.EnumerateRead(PropertyHashToArrayFunc, propertyArray.get());
>+  return propertyArray->Enumerate(props);
>+}

I think you can use an nsCOMArray<nsIProperty> here (and change to NS_NewArrayEnumerator for the return). As an added bonus, you can initialise the nsCOMArray to the right size with m_properties.Count().

>+NS_IMETHODIMP nsAbCardProperty::GetProperty(const nsAString &name, nsIVariant **value)
>+NS_IMETHODIMP nsAbCardProperty::GetPropertyAsAString(const char *name, nsAString &value) 
>+NS_IMETHODIMP nsAbCardProperty::GetPropertyAsAUTF8String(const char *name, nsACString &value) 
>+NS_IMETHODIMP nsAbCardProperty::GetPropertyAsUint32(const char *name, PRUint32 *value) 
>+NS_IMETHODIMP nsAbCardProperty::GetPropertyAsBool(const char *name, PRBool *value) 

Please drop the temporary isFound variables in these functions. The result may not be too pretty but I'd prefer them not to be there.

I'm currently setting up on windows to test this with palm sync, hopefully I'll have something by the end of the day.
Comment on attachment 314110 [details] [diff] [review]
nsIAbCard refactor, patch v3

Apart from comment 28, the only problem I see with this now is that the preview pane doesn't work properly - the home/work addresses aren't displayed (and maybe some others). Problem is probably around the second email or screen name.
Attachment #314110 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 30

9 years ago
Created attachment 316927 [details] [diff] [review]
nsIAbCard refactor, patch v4

I've made the card view overlay a little more resistant to exceptions.
Attachment #314110 - Attachment is obsolete: true
Attachment #316927 - Flags: review?(bugzilla)
Comment on attachment 316927 [details] [diff] [review]
nsIAbCard refactor, patch v4

This seems to work and I'm happy with the code changes. r=me.
Attachment #316927 - Flags: review?(bugzilla) → review+
(Assignee)

Updated

9 years ago
Attachment #316927 - Flags: superreview?(dmose)
Moving from wanted‑thunderbird3.0a1? flag to wanted‑thunderbird3.0a2? flag since code for Thunderbird 3 Alpha 1 has been frozen.
Flags: wanted-thunderbird3.0a1? → wanted-thunderbird3.0a2?
Blocks: 434014
Comment on attachment 316927 [details] [diff] [review]
nsIAbCard refactor, patch v4

Here's a first batch of review comments; lots more code read still.

>diff --git a/mail/components/addrbook/content/abCardOverlay.js b/mail/components/addrbook/content/abCardOverlay.js
>
> var gEditCard;
> var gOnSaveListeners = new Array();
>@@ -137,10 +137,10 @@ function OnLoadNewCard()
>         gEditCard.generateDisplayName = false;
>     }
>     if ("aimScreenName" in window.arguments[0])
>-      gEditCard.card.aimScreenName = window.arguments[0].aimScreenName;
>+      gEditCard.card.setProperty("_AimScreenName", window.arguments[0].aimScreenName);
>     
>     if ("allowRemoteContent" in window.arguments[0]) {
>-      gEditCard.card.allowRemoteContent = window.arguments[0].allowRemoteContent;
>+      gEditCard.card.setProperty("AllowRemoteContent", window.arguments[0].allowRemoteContent);
>       window.arguments[0].allowRemoteContent = false;
>     }

window.arguments[0] is an nsIAbCard, which means that both of the above statements won't work, because they're using old-style getters rather than calls to getProperty.  Also need to update that last setter.

>@@ -409,8 +409,14 @@ function NewCardOKButton()
>       // the card that got created.
>       gEditCard.card = GetDirectoryFromURI(uri).addCard(gEditCard.card);
>       NotifySaveListeners();
>-      if ("arguments" in window && window.arguments[0])
>-        window.arguments[0].allowRemoteContent = gEditCard.card.allowRemoteContent;
>+      if ("arguments" in window && window.arguments[0]) {
>+        try {
>+          window.arguments[0].allowRemoteContent =
>+            gEditCard.card.getProperty("AllowRemoteContent");
>+        } catch (ex) {
>+          window.arguments[0].allowRemoteContent = false;
>+        }
>+      }

Similar problems here.

>@@ -423,22 +429,32 @@ function GetCardValues(cardproperty, doc
>   if (!cardproperty)
>     return;
> 
>-  for (var i = kVcardFields.length; i-- > 0; )
>-    doc.getElementById(kVcardFields[i][0]).value =
>-      cardproperty[kVcardFields[i][1]];
>+  for (var i = kVcardFields.length; i-- > 0; ) {
>+    try {
>+      doc.getElementById(kVcardFields[i][0]).value =
>+        cardproperty.getProperty(kVcardFields[i][1]);
>+    } catch (ex) {
>+      doc.getElementById(kVcardFields[i][0]).value = "";
>+    }
>+  }

Why is it necessary to do anything in the catch clause at all?

>@@ -267,13 +290,15 @@ function DisplayCardViewPane(card)
>     cvSetVisible(data.cvbHomeMapItBox, false);
>         }
> 
>-  visible = HandleLink(data.cvHomeWebPage, "", card.webPage2, data.cvHomeWebPageBox, card.webPage2) || visible;
>+  visible = HandleLink(data.cvHomeWebPage, "", card.getProperty("WebPage2"),
>+                       data.cvHomeWebPageBox, card.getProperty("WebPage2")) ||
>+                       visible;

The indention above makes it easy to mis-read |visible|
as being an argument to HandleLink.

>diff --git a/mailnews/addrbook/public/nsIAbCard.idl b/mailnews/addrbook/public/nsIAbCard.idl
>+   * The non-variant functions are marked [noscript] since xptconnect uses

xpconnect, here and in the comment above the setters

>+  /**
>+   * Deletes the property with the given name.
>+   *
>+   * Some properties may not be deleted. However, the implementation will not
>+   * check this constraint at this method.
>+   *
>+   * @param name             The case-sensitive name of the property to set.
>+   */
>+  void deleteProperty(in AUTF8String name);

It'd be good to define this function a bit more precisely: what happens if someone attempts to delete a read-only property? a non-existent property?  Use @exception as appropriate.

>   /**
>-   * This function will copy all values from one card to another.
>+   * Translates a card into a specific format.
>+   * The following types are supported:
>+   * - base64
>+   * - xml
>+   * - vcard

base64 is an encoding, not a format.  Maybe call this one base64xml?

/////////////////////////////////////////////////////////////////////////////
>+  // DEPRECATED FOOLS
>+  /////////////////////////////////////////////////////////////////////////////

I don't think it makes sense to mark something as deprecated without documenting what it's deprecated in favor of.   And it's not clear to me that most of the things that follow have any alternatives...
(Assignee)

Comment 34

9 years ago
(In reply to comment #33)
> (From update of attachment 316927 [details] [diff] [review])
> >@@ -423,22 +429,32 @@ function GetCardValues(cardproperty, doc
> >+  for (var i = kVcardFields.length; i-- > 0; ) {
> >+    try {
> >+      doc.getElementById(kVcardFields[i][0]).value =
> >+        cardproperty.getProperty(kVcardFields[i][1]);
> >+    } catch (ex) {
> >+      doc.getElementById(kVcardFields[i][0]).value = "";
> >+    }
> >+  }
> 
> Why is it necessary to do anything in the catch clause at all?

In retrospect, probably nothing. The catch clause is needed because getProperty throughs for non-existent properties but the value should be, by default, empty already, so it doesn't matter.

> /////////////////////////////////////////////////////////////////////////////
> >+  // DEPRECATED FOOLS
> >+  /////////////////////////////////////////////////////////////////////////////
> 
> I don't think it makes sense to mark something as deprecated without
> documenting what it's deprecated in favor of.   And it's not clear to me that
> most of the things that follow have any alternatives...

A relic from when I wrote the first patches and moved all the old stuff not in the final IDL below this line. copy and equals subsequently survived in the interface, so they're not deprecated any more. isMailList and mailListURI, OTOH, will be removed when mailing list sanity occurs.

Comment 35

9 years ago
blocking‑thunderbird3.0a2+ so the other addressbook improvements doesn't get held up.
Flags: wanted-thunderbird3.0a2?
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3.0a2+
Comment on attachment 316927 [details] [diff] [review]
nsIAbCard refactor, patch v4

>+[scriptable, uuid(f5646fba-3d20-4fdf-90fb-9f2ee9899dbb)]
>+interface nsIAbCard : nsIAbItem {
>
> [...]
>
>+  /**
>+   * This function will copy all values from one card to another.
>+   *
>+   * @param  srcCard         The source card to copy values from.
>+   */
>+  void copy(in nsIAbCard srcCard);

The argument name here wants to be aSourceCard, I bet.

>+  boolean equals(in nsIAbCard card);

The arg here should be aCard, and this needs doxygen comments.  In those comments, please definite what happens if the caller passes in null (return value of false, or exception -- I suspect the former is more appropriate, but I haven't given it a lot of thought).

>+%{C++
>+// A nice list of properties for the benefit of C++ clients

Too bad XPIDL doesn't support string constants so that we could get cross-language argument checking here.

>diff --git a/mailnews/addrbook/public/nsIAbDirectory.idl b/mailnews/addrbook/public/nsIAbDirectory.idl
>--- a/mailnews/addrbook/public/nsIAbDirectory.idl
>+++ b/mailnews/addrbook/public/nsIAbDirectory.idl
>@@ -141,7 +141,7 @@ interface nsIAbDirectory : nsISupports {
>   /**
>    * Adds a card to the database. card may be an abstract card.
>    *
>-   * @return "Real" card (eg nsIAbMDBCard) that can be used for some
>+   * @return "Real" card (eg nsIAbLDAPCard) that can be used for some
>    *         extra functions.
>    */
>   nsIAbCard addCard(in nsIAbCard card);

This comment doesn't really make sense.  Can you rephrase in order to make it more clear?

>diff --git a/mailnews/addrbook/public/nsIAbItem.idl b/mailnews/addrbook
/public/nsIAbItem.idl
>
> [...]
>
>+  /**
>+   * Unique identifier for this item.
>+   * Note that different implementations may impose other semantic requirements
>+   * on this value. This identifier is also only guaranteed to be unique within
>+   * the context of its source.
>+   *
>+   * This item SHOULD remain the same throughout the life of the item. If the
>+   * item is moved to a different source context, the uuid should remain the
>+   * same whenever possible. A copied card MUST have a new uuid.

I'd be interested in understanding more about the above design decisions.  Can you elaborate?  I'm unsure whether documentation for that belongs in the IDL or not.  

>+  /**
>+   * @{
>+   * These constants reflect the possible values of the
>+   * mail.addr_book.lastnamefirst preferences. They are intended to be used in
>+   * generateName, defined below.
>+   */
>+   const unsigned long kGenerateDisplayName = 0;
>+   const unsigned long kGenerateLastFirstOrder = 1;
>+   const unsigned long kGenerateFirstLastOrder = 2;
>+   /** @} */

XPIDL convention is generally that constants are ALL_CAPS rather than hUngarian.

>diff --git a/mailnews/addrbook/resources/content/addressbook.js b/mailnews/addrbook/resources/content/addressbook.js
>--- a/mailnews/addrbook/resources/content/addressbook.js
>+++ b/mailnews/addrbook/resources/content/addressbook.js
>@@ -371,8 +371,7 @@ function AbPrintPreviewCard()

>+    } catch (ex) {
>     }

I think this would be slightly more readable if those two lines were combined:

  } catch (ex) {}

>@@ -432,19 +432,17 @@ nsresult nsAbAutoCompleteSession::Search
> 
>           // Now, retrieve the user name and nickname
>           rv = card->GetDisplayName(pDisplayNameStr);
>+          rv = card->GetFirstName(pFirstNameStr);
>+          rv = card->GetLastName(pLastNameStr);

Please check each of the above return values rather than just overwriting them with the next call.

>+          rv = card->GetPropertyAsAString(kNicknameProperty, pNickNameStr);
>           if (NS_FAILED(rv))
>-              continue;
>-          rv = card->GetFirstName(pFirstNameStr);
>+              pNickNameStr = EmptyString();
>+

Is the explicit setter above necessary?  I would expect C++ to construct a new pNickName object for each loop iteration.

>+          rv = card->GetPropertyAsUint32(kPopularityIndexProperty,
>+                                         &popularityIndex);
>           if (NS_FAILED(rv))
>-              continue;
>-          rv = card->GetLastName(pLastNameStr);
>-          if (NS_FAILED(rv))
>-              continue;
>-          rv = card->GetNickName(pNickNameStr);
>-          if (NS_FAILED(rv))
>-              continue;
>-
>-          (void) card->GetPopularityIndex(&popularityIndex);
>+            popularityIndex = 0;

Analogous question re popularityIndex.

More comments to come...
Blocks: 437908
Comment on attachment 316927 [details] [diff] [review]
nsIAbCard refactor, patch v4

>@@ -816,125 +378,27 @@ NS_IMETHODIMP nsAbCardProperty::Copy(nsI

The whole copy method thing is weird.  A much more normal OO way to do this would be to have a clone method instead.  File a spinoff bug for this?  One could even imagine creating a mozICloneable interface, though that's not strictly necessary.

> [...]
>
>+  PRBool hasMore;
>+  nsCOMPtr<nsISupports> result;
>+  while (NS_SUCCEEDED(properties->HasMoreElements(&hasMore)) && hasMore)

If HasMoreElements fails, we should abort, not pretend that everything went well.

>+    nsAutoString name;
>+    property->GetName(name);
>+    nsCOMPtr<nsIVariant> value;
>+    property->GetValue(getter_AddRefs(value));
>+    SetProperty(NS_ConvertUTF16toUTF8(name), value);

Storing everything COMified internally feels pretty heavyweight to me.  If we can really tolerate this level of heaviness (which I suspect we can), the entire nsAbProperty class probably really belongs in JavaScript, for memory-safety, code conciseness, and approachability reasons.  That said, this patch is still a giant leap forward from where we were, so we can deal with that some other day.

Have you done any performance testing on this patch?  One thing that seems likely to require reasonable performance is scrolling through autocomplete on large LDAP directories.  There are probably other things as well; maybe Standard8 has some thoughts on what...

>     nsCString escResult;
>     MsgEscapeString(nsDependentCString(vCard), nsINetUtil::ESCAPE_URL_PATH, escResult);
>-    *aResult = strdup(escResult.get());
>-    return (*aResult ? NS_OK : NS_ERROR_OUT_OF_MEMORY);
>+    aResult = escResult.get();

Shouldn't need the .get() here.

More comments to come.
blocking tb3 based on the fact that it blocks 437908
Flags: blocking-thunderbird3+

Updated

9 years ago
Whiteboard: [needs sr
Comment on attachment 316927 [details] [diff] [review]
nsIAbCard refactor, patch v4

> nsresult nsAbCardProperty::AppendSection(const AppendItem *aArray, PRInt16 aCount, const nsString& aHeading,
>                                          nsIStringBundle *aBundle,
>                                          mozITXTToHTMLConv *aConv,
>@@ -1381,9 +841,9 @@ nsresult nsAbCardProperty::AppendSection
> 
>   PRInt16 i = 0;
>   for (i=0;i<aCount;i++) {
>-    rv = GetCardValue(aArray[i].mColumn, attrValue);
>-    NS_ENSURE_SUCCESS(rv,rv);
>-    sectionIsEmpty &= attrValue.IsEmpty();
>+    rv = GetPropertyAsAString(aArray[i].mColumn, attrValue);
>+    if (NS_SUCCEEDED(rv) && !attrValue.IsEmpty())
>+      sectionIsEmpty = PR_FALSE;
>   }

Throughout the ConvertTo* and Append* code, you've changing the semantics w.r.t. what happens on failure to get the property.  Is it correct that the current code in the tree would abort conversion/export if an error was encountered here, but now we'll complete while silently losing data?  If so, I don't think that's the right choice; if data is being lost, we need to notify the user somehow.  In which case, I would suggest reverting to the existing semantics, and filing a spinoff bug to change the behvaior from abort -> continue with notification.

>@@ -1498,13 +953,13 @@ nsresult nsAbCardProperty::AppendCitySta
>   AppendItem item;
>   const char *stateCol, *zipCol;

Do the above two variable names want to change to (e.g.) statePropertyName or some such for clarity?

> NS_IMETHODIMP nsAbLDAPCard::GetDn(nsACString &aDN)
> {
>-  aDN = m_dn;
>+  if (NS_FAILED(GetPropertyAsAUTF8String(kDNColumn, aDN)))
>+    aDN.Truncate();
>   return NS_OK;

Why not propagate the error up the stack here?

>@@ -120,33 +58,17 @@ NS_IMETHODIMP nsAbMDBCard::Equals(nsIAbC
>     return NS_OK;
>   }
> 
>+  // If we have the same directory, we will equal the other card merely given
>+  // the UIDs. If not, we are never equal. But we don't know when we have the
>+  // same directories. Assume that we do.

In what cases will having the same directory happen?  What are the consequences of this assumption being wrong then?  Please add this info to the commentary.

>   if (m_IsMailList)
>-    mDatabase->CreateNewListCardAndAddToDB(this, m_dbRowID, newCard, PR_TRUE /* notify */);
>+    mDatabase->CreateNewListCardAndAddToDB(this, m_dbRowID, card, PR_TRUE /* notify */);
>   else
>-    mDatabase->CreateNewCardAndAddToDB(newCard, PR_TRUE);
>+    mDatabase->CreateNewCardAndAddToDB(card, PR_TRUE);
>   mDatabase->Commit(nsAddrDBCommitType::kLargeCommit);
> 
>-  NS_IF_ADDREF(*addedCard = newCard);
>+  NS_IF_ADDREF(*addedCard = card);

I know this code was already this way (i.e. not your fault), but it seems scary to me to not be checking error codes calls that store data to the database so that the user has a chance of knowing if something goes wrong.  If it's straightforward, I'd suggest fixing this now.  If not, please spin off a bug to do it later.

>@@ -619,9 +617,9 @@ nsAbManager::ExportDirectoryToDelimitedT
> 
>           for (i = 0; i < NS_ARRAY_LENGTH(EXPORT_ATTRIBUTES_TABLE); i++) {
>             if (EXPORT_ATTRIBUTES_TABLE[i].plainTextStringID != 0) {
>-              rv = card->GetCardValue(EXPORT_ATTRIBUTES_TABLE[i].abColName,
>-                                      value);
>-              NS_ENSURE_SUCCESS(rv,rv);
>+              rv = card->GetPropertyAsAString(EXPORT_ATTRIBUTES_TABLE[i].abColName, value);
>+              if (NS_FAILED(rv))
>+                value.Truncate();

The exporter code seems to have the same silent dataloss mentioned previously.  Also does .abColName want to become .abPropertyName for readability?

>@@ -1051,52 +1045,52 @@ static void convertNameValue(VObject *vO
>   // a good example of this is VCTelephoneProp, this prop has four objects underneath it:
>   // fax, work and home and cellular.
>   if (PL_strcasecmp(VCCityProp, vObjectName(vObj)) == 0)
>-      cardColName = kWorkCityColumn;
>+      cardColName = kWorkCityProperty;

cardColName wants a name change too, I'm guessing.

>+MapDate(nsAbOSXCard *aCard, NSDate *aDate, const char *aYearName,
>+        const char *aMonthName, const char *aDayName, PRBool aNotify,
>         nsIAbManager *aAbManager)

Given that the "aFooName" parameters are names of the propertries, not names of the Foo in question, how about renaming to aFooPropertyName for clarity?

>@@ -1121,62 +1124,60 @@ nsresult nsAddrDatabase::InitMDBInfo()

The changes to this method appeare to have lost the notes property and the last modified date property.

More to come...
Comment on attachment 316927 [details] [diff] [review]
nsIAbCard refactor, patch v4

>@@ -1778,10 +1635,8 @@ NS_IMETHODIMP nsAddrDatabase::DeleteCard
> 
>   rowOid.mOid_Scope = bIsMailList ? m_ListRowScopeToken : m_CardRowScopeToken;
> 
>-  nsCOMPtr<nsIAbMDBCard> dbcard(do_QueryInterface(card, &err));
>+  err = card->GetPropertyAsUint32(kRowIDColumn, (PRUint32*)&rowOid.mOid_Id);

Prefer C++-style casts here and elsewhere in this file; they convey intent more precisely, allowing the compiler to sometimes catch errors that they otherwise wouldn't.

>+  err = card->GetPropertyAsUint32(kRowIDColumn, &cardRowID);

kRowIDColumn -> kRowIDProperty?

More tomorrow.
Comment on attachment 316927 [details] [diff] [review]
nsIAbCard refactor, patch v4


> NS_IMETHODIMP nsAddrDatabase::InitCardFromRow(nsIAbCard *newCard, nsIMdbRow* cardRow)
> {
>-    nsresult    err = NS_OK;
>-    if (!newCard || !cardRow)
>-        return NS_ERROR_NULL_POINTER;
>+  nsresult rv = NS_OK;
>+  if (!newCard || !cardRow || !m_mdbEnv)
>+    return NS_ERROR_NULL_POINTER;
> 
>-  nsAutoString tempString;
>+  nsCOMPtr<nsIMdbRowCellCursor> iterator;

It seems confusing to call the above variable "iterator" rather than "cursor".

>+  do
>+  {
>+    rv = iterator->NextCell(m_mdbEnv, &cell, &columnNumber, nsnull);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    if (cell == nsnull)

!cell is a more typical way to write this.

>+      break;
>+
>+    // Get the value of the cell
>+    cell->AliasYarn(m_mdbEnv, &cellYarn);
>+    NS_ConvertUTF8toUTF16 value(static_cast<const char*>(cellYarn.mYarn_Buf),
>+        cellYarn.mYarn_Fill);
>+
>+    if (!value.IsEmpty())
>     {
>-        newCard->SetFirstName(tempString);
>+      // Get the column of the cell
>+      // Mork makes this so hard...
>+      rv = m_mdbStore->TokenToString(m_mdbEnv, columnNumber, &colYarn);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      const char *name = PL_strndup(static_cast<char *>(colYarn.mYarn_Buf),
>+          colYarn.mYarn_Fill);

Why the strndup?  I don't think it's necessary, and it's also missing a corresponding PL_strfree().  Same points about the strndup later on in this same method.

>+      newCard->SetPropertyAsAString(name, value);
>     }
>+    cell->Release(); // always release ref
>+  } while (true);
>+ 
>+  // WARNING: UBER HACK BAD CODE DUE TO MORK CRAPINESS
>+  // The cursor seems to mice the first element, so we manually get it

"mice"?  :-)

> nsAbIPCCard::nsAbIPCCard()
> {
>@@ -86,34 +85,21 @@ NS_IMETHODIMP nsAbIPCCard::Copy(nsIAbCar
> {
>     NS_ENSURE_ARG_POINTER(srcCard);
> 
>-    nsresult rv;
>-    nsCOMPtr<nsIAbMDBCard> dbCard;
>-    dbCard = do_QueryInterface(srcCard, &rv);
>-    if(NS_SUCCEEDED(rv) && dbCard) {
>-        nsString palmIDStr;
>-        nsresult rv = dbCard->GetStringAttribute(CARD_ATTRIB_PALMID, getter_Copies(palmIDStr));
>-        if(NS_SUCCEEDED(rv) && !palmIDStr.IsEmpty()) {
>-            PRFloat64 f = PR_strtod(NS_LossyConvertUTF16toASCII(palmIDStr).get(), nsnull);
>-            PRInt64 l;
>-            LL_D2L(l, f);
>-            LL_L2UI(mRecordId, l);
>-        }
>-        else
>-            mRecordId = 0;
>-        // set tableID, RowID and Key for the card
>-        PRUint32 tableID=0;
>-        dbCard->GetDbTableID(&tableID);
>-        SetDbTableID(tableID);
>-        PRUint32 rowID=0;
>-        dbCard->GetDbRowID(&rowID);
>-        SetDbRowID(rowID);
>-        PRUint32 key;
>-        dbCard->GetKey(&key);
>-        SetKey(key);
>-    }
>-    PRUint32 lastModifiedDate = 0;
>-    srcCard->GetLastModifiedDate(&lastModifiedDate);
>-    mStatus = (lastModifiedDate) ? ATTR_MODIFIED : ATTR_NEW;

Why are the lines that cause mStatus to be set going away?

>+    nsString palmIDStr;
>+    nsresult rv = card->GetPropertyAsAString(CARD_ATTRIB_PALMID,
>+        getter_Copies(palmIDStr));

Why is getter_Copies necessary here?  Same question for nsAbIPCCard::Same.

Last comment: in the future, when you need to make a change as big as the hardcoded-setters -> {get,set}Property* functions, please try to avoid making any other changes in the same patch.  I'm sure I've missed stuff doing this review just because the patch is so large that it's really difficult to keep all of the pieces in my head at once.

That said, this is a giant step forward for the addrbook APIs; thanks for the hard work!
Attachment #316927 - Flags: superreview?(dmose) → superreview-
(Assignee)

Comment 42

9 years ago
Belated responses to comments (most of these have been addressed in IRC, just want to post them here for those not present):

(In reply to comment #36)
> >+%{C++
> >+// A nice list of properties for the benefit of C++ clients
> 
> Too bad XPIDL doesn't support string constants so that we could get
> cross-language argument checking here.

I'll file a bug on that shortly, but it looks like WebIDL doesn't define string constants...

> >diff --git a/mailnews/addrbook/public/nsIAbItem.idl b/mailnews/addrbook
> /public/nsIAbItem.idl
> >
> > [...]
> >
> >+  /**
> >+   * Unique identifier for this item.
> >+   * Note that different implementations may impose other semantic requirements
> >+   * on this value. This identifier is also only guaranteed to be unique within
> >+   * the context of its source.
> >+   *
> >+   * This item SHOULD remain the same throughout the life of the item. If the
> >+   * item is moved to a different source context, the uuid should remain the
> >+   * same whenever possible. A copied card MUST have a new uuid.
> 
> I'd be interested in understanding more about the above design decisions.  Can
> you elaborate?  I'm unsure whether documentation for that belongs in the IDL or
> not.  

This was a result of a discussion in IRC. The documentation for the latter part about UUIDs staying the same comes from this conversation (03/16):
<Standard8> Joshua, do you think we can support uuid for cards and mailing lists remaining the same if they are moved?
<Joshua> what do you mean?
<Standard8> well, will we be able to maintain a uuid for a card across different address book types do you think?
<Standard8> so if I move it from one to another, will I get a new uuid?
<Joshua> probably
<Standard8> I'll say it'll stay the same where possible then.

The other requirements were dictated by the necessities of using the UUID, at least for LDAP and MDB cards.

It is possible that the UUID requirements could change as I dig through the other interfaces, as effects outside of nsIAbCard have not been heavily considered wrt nsIAbItem.

> >@@ -432,19 +432,17 @@ nsresult nsAbAutoCompleteSession::Search
> > 
> >           // Now, retrieve the user name and nickname
> >           rv = card->GetDisplayName(pDisplayNameStr);
> >+          rv = card->GetFirstName(pFirstNameStr);
> >+          rv = card->GetLastName(pLastNameStr);
> 
> Please check each of the above return values rather than just overwriting them
> with the next call.

These shouldn't throw.

> Throughout the ConvertTo* and Append* code, you've changing the semantics
> w.r.t. what happens on failure to get the property.  Is it correct that the
> current code in the tree would abort conversion/export if an error was
> encountered here, but now we'll complete while silently losing data?  If so, I
> don't think that's the right choice; if data is being lost, we need to notify
> the user somehow.  In which case, I would suggest reverting to the existing
> semantics, and filing a spinoff bug to change the behvaior from abort ->
> continue with notification.

This has been discussed in depth on IRC. In summary:
* These methods don't really fail, except for the NOT_AVAILABLE error, which we want to ignore.
* The exception to the above are some properties on nsIVariant not convertible to strings. I've slapped a notice on nsIAbCard noting that code should not use these property types, and that bad things will happen if such a type is used.
* The API for getProperty* needs to be reviewed again. Suggestions:
** Add a default value param (optional?), and don't throw
** An out param to indicate whether or not the property exists
** Add a [optional] param to indicate whether or not it should throw

> >@@ -120,33 +58,17 @@ NS_IMETHODIMP nsAbMDBCard::Equals(nsIAbC
> >     return NS_OK;
> >   }
> > 
> >+  // If we have the same directory, we will equal the other card merely given
> >+  // the UIDs. If not, we are never equal. But we don't know when we have the
> >+  // same directories. Assume that we do.
> 
> In what cases will having the same directory happen?  What are the consequences
> of this assumption being wrong then?  Please add this info to the commentary.

Equals seems to be only used for testing if a card in an array is the same as this one. Docs added.

> >@@ -1121,62 +1124,60 @@ nsresult nsAddrDatabase::InitMDBInfo()
> 
> The changes to this method appeare to have lost the notes property and the last
> modified date property.

Not quite. DefaultAddress and Category were the ones removed, as they weren't used.

(In reply to comment #41)
> >+      break;
> >+
> >+    // Get the value of the cell
> >+    cell->AliasYarn(m_mdbEnv, &cellYarn);
> >+    NS_ConvertUTF8toUTF16 value(static_cast<const char*>(cellYarn.mYarn_Buf),
> >+        cellYarn.mYarn_Fill);
> >+
> >+    if (!value.IsEmpty())
> >     {
> >-        newCard->SetFirstName(tempString);
> >+      // Get the column of the cell
> >+      // Mork makes this so hard...
> >+      rv = m_mdbStore->TokenToString(m_mdbEnv, columnNumber, &colYarn);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+      const char *name = PL_strndup(static_cast<char *>(colYarn.mYarn_Buf),
> >+          colYarn.mYarn_Fill);
> 
> Why the strndup?  I don't think it's necessary, and it's also missing a
> corresponding PL_strfree().  Same points about the strndup later on in this
> same method.

char * + nsDependentCString + some of the earliest code I wrote + rewriting nsAbCardProperty two or three different ways in the meantime + scared-to-death of string interactions. I wrote something that works and changed it in the wrong way after an API changed a few iterations.

> >+  // The cursor seems to mice the first element, so we manually get it
> 
> "mice"?  :-)

Hmm. Most interesting typo I've written, I'd say.

> >-    PRUint32 lastModifiedDate = 0;
> >-    srcCard->GetLastModifiedDate(&lastModifiedDate);
> >-    mStatus = (lastModifiedDate) ? ATTR_MODIFIED : ATTR_NEW;
> 
> Why are the lines that cause mStatus to be set going away?

Overzealous deleting?

> >+    nsString palmIDStr;
> >+    nsresult rv = card->GetPropertyAsAString(CARD_ATTRIB_PALMID,
> >+        getter_Copies(palmIDStr));
> 
> Why is getter_Copies necessary here?  Same question for nsAbIPCCard::Same.

Overzealous s/// command? I must have forgotten that GetStringAttribute used char * and not ns(C)String...

> Last comment: in the future, when you need to make a change as big as the
> hardcoded-setters -> {get,set}Property* functions, please try to avoid making
> any other changes in the same patch.  I'm sure I've missed stuff doing this
> review just because the patch is so large that it's really difficult to keep
> all of the pieces in my head at once.

To be honest, for the most part, I considered this an atomic change, especially because of how I implemented this:
1. Add generic property support to nsIAbCard (requiring the list->loop changes).
2. Get rid of the flat list of attributes.

The last step flows naturally as an atomic extension of the first step (to me, at least), while the other way around is different.

> That said, this is a giant step forward for the addrbook APIs; thanks for the
> hard work!

Only the first of at least 5 major patches: still to come are the nsIAbCollection introduction, cardForEmail refactoring, mailing list sanity, and de-nsIAddrDatabase-ification (don't worry, this will be several patches in itself) patches. And, naturally, UI deforking and beautification slides in at some point, and everything is finished in the end with demorkification.

I should get a fixed patch tonight, but I'll need to ensure that the Windows stuff builds, and do as much regression testing as feasible there.
(Assignee)

Comment 43

9 years ago
Created attachment 328015 [details] [diff] [review]
nsIAbCard refactor, patch v5

I guarantee that this builds without error on both Windows and Linux, and that it passes all tests on Linux, including my basic usability tests (new/edit/delete card).
Attachment #316927 - Attachment is obsolete: true
Attachment #328015 - Flags: superreview?(dmose)
Attachment #328015 - Flags: review+
(Assignee)

Comment 44

9 years ago
Oops, to addendums:

1. Ignore the first file... I was doing some mucking from windows
2. r+ carried over from past patch.
(In reply to comment #33)
> (From update of attachment 316927 [details] [diff] [review])
> Here's a first batch of review comments; lots more code read still.
> 
> >diff --git a/mail/components/addrbook/content/abCardOverlay.js b/mail/components/addrbook/content/abCardOverlay.js
> >
> > var gEditCard;
> > var gOnSaveListeners = new Array();
> >@@ -137,10 +137,10 @@ function OnLoadNewCard()
> >         gEditCard.generateDisplayName = false;
> >     }
> >     if ("aimScreenName" in window.arguments[0])
> >-      gEditCard.card.aimScreenName = window.arguments[0].aimScreenName;
> >+      gEditCard.card.setProperty("_AimScreenName", window.arguments[0].aimScreenName);
> >     
> >     if ("allowRemoteContent" in window.arguments[0]) {
> >-      gEditCard.card.allowRemoteContent = window.arguments[0].allowRemoteContent;
> >+      gEditCard.card.setProperty("AllowRemoteContent", window.arguments[0].allowRemoteContent);
> >       window.arguments[0].allowRemoteContent = false;
> >     }
> 
> window.arguments[0] is an nsIAbCard, which means that both of the above
> statements won't work, because they're using old-style getters rather than
> calls to getProperty.  Also need to update that last setter.

This comment was not addressed in patch v5.

> 
> >@@ -409,8 +409,14 @@ function NewCardOKButton()
> >       // the card that got created.
> >       gEditCard.card = GetDirectoryFromURI(uri).addCard(gEditCard.card);
> >       NotifySaveListeners();
> >-      if ("arguments" in window && window.arguments[0])
> >-        window.arguments[0].allowRemoteContent = gEditCard.card.allowRemoteContent;
> >+      if ("arguments" in window && window.arguments[0]) {
> >+        try {
> >+          window.arguments[0].allowRemoteContent =
> >+            gEditCard.card.getProperty("AllowRemoteContent");
> >+        } catch (ex) {
> >+          window.arguments[0].allowRemoteContent = false;
> >+        }
> >+      }
> 
> Similar problems here.

Nor was this.
Comment on attachment 328015 [details] [diff] [review]
nsIAbCard refactor, patch v5

>+
>+  /**
>+   * Returns true if this card is equal to the other card.
>+   *
>+   * The default implementation defines equal as this card pointing to the
>+   * same object as @arg aCard; another implementation defines it as equality of
>+   * properties and values.
>+   *
>+   * @return                 Equality, as defined above.
>+   * @param  aCard           The card to compare against.
>+   */
>+  boolean equals(in nsIAbCard aCard);

Having equals sometimes mean "is the same object" and sometimes mean "is an equivalent object" strikes me as really likely to lead bugs.  If somebody really wants to test for "is the same object", it's possible to test for COM identity by comparing nsISupports pointers (albeit in a slightly more complex way from JS).  Is there some reason .equals shouldn't be defined to always compare by value and never by reference?
(Assignee)

Comment 47

9 years ago
(In reply to comment #45)
> > window.arguments[0] is an nsIAbCard, which means that both of the above
> > statements won't work, because they're using old-style getters rather than
> > calls to getProperty.  Also need to update that last setter.
> 
> This comment was not addressed in patch v5.

I originally addressed this, only to find out that it is not, in fact, a card, but a generic JS object. I discovered this after my last comment, so I reverted the changes but neglected to mention otherwise. My bad.

(In reply to comment #46)
> Having equals sometimes mean "is the same object" and sometimes mean "is an
> equivalent object" strikes me as really likely to lead bugs.  If somebody
> really wants to test for "is the same object", it's possible to test for COM
> identity by comparing nsISupports pointers (albeit in a slightly more complex
> way from JS).  Is there some reason .equals shouldn't be defined to always
> compare by value and never by reference?

At present, I count exactly 3 usages of this method, all 3 of which used to find a card in a list of cards. Given the current usage, I believe that UUID comparisons is probably sufficient for equality.
(In reply to comment #42)
> > 
> > I'd be interested in understanding more about the above design decisions.  Can
> > you elaborate?  I'm unsure whether documentation for that belongs in the IDL or
> > not.  
> 
> This was a result of a discussion in IRC. The documentation for the latter part
> about UUIDs staying the same comes from this conversation (03/16):
>
> [...]

There are still some pieces missing from this puzzle.  To wit:

> > >+  /**
> > >+   * Unique identifier for this item.
> > >+   * Note that different implementations may impose other semantic requirements
> > >+   * on this value. This identifier is also only guaranteed to be unique within
> > >+   * the context of its source.

Why is it only guaranteed to be unique with the context of its source?  The first U in UUID typically stands for "Universally".

> > >+   *
> > >+   * This item SHOULD remain the same throughout the life of the item. If the

Why is this a SHOULD and not a MUST?

> > >+   * item is moved to a different source context, the uuid should remain the
> > >+   * same whenever possible. A copied card MUST have a new uuid.
>
> The other requirements were dictated by the necessities of using the UUID, at
> least for LDAP and MDB cards.

And those necessities are what?  What's not clear to me here is exactly what problem(s) this UUID is aimed at solving (and what problems does it specifically not solve?).  Without knowing those things, it's not really possible to critique the specifics of the API.

As an example, LDAP already has a DN to identify itself.   Is the UUID of an LDAP object likely to be the DN?  Or is there some reason it needs to be separate from that?

(In reply to comment #37)
> (From update of attachment 316927 [details] [diff] [review])
> >@@ -816,125 +378,27 @@ NS_IMETHODIMP nsAbCardProperty::Copy(nsI
> 
> The whole copy method thing is weird.  A much more normal OO way to do this
> would be to have a clone method instead.  File a spinoff bug for this?  One
> could even imagine creating a mozICloneable interface, though that's not
> strictly necessary.

Still need a spinoff bug for this.
> * The exception to the above are some properties on nsIVariant not convertible
> to strings. I've slapped a notice on nsIAbCard noting that code should not use
> these property types, and that bad things will happen if such a type is used.

Great; thanks!  Might be worth adding a bit of code so that setProperty asserts in debug builds if someone tries to do this.

> * The API for getProperty* needs to be reviewed again. Suggestions:
> ** Add a default value param (optional?), and don't throw
> ** An out param to indicate whether or not the property exists
> ** Add a [optional] param to indicate whether or not it should throw

Sounds good, please file a spinoff bug with this info.

I have to go meet a friend for dinner shortly; will pick up again later on or tomorrow.
(Assignee)

Comment 51

9 years ago
(In reply to comment #48)
> As an example, LDAP already has a DN to identify itself.   Is the UUID of an
> LDAP object likely to be the DN?  Or is there some reason it needs to be
> separate from that?

The UUID is the DN for LDAP. It is the mork row ID for MDB cards. The problem is that a consumer of nsIAbCard may not specifically know whether or not the card is an LDAP card, MDB card, OS X card, etc. Thus, the point of the UUID is to provide a reference such that the card can be referred to uniquely without knowing which property makes it unique.

> And those necessities are what?  What's not clear to me here is exactly what
> problem(s) this UUID is aimed at solving (and what problems does it
> specifically not solve?).  Without knowing those things, it's not really
> possible to critique the specifics of the API.

To me, at least, the UUID is primarily solving the puzzle of how to refer to an address book item (directory or card) generically. *gets the feeling that isn't the best explanation.

>+   *
>+   * This item SHOULD remain the same throughout the life of the item. If the
>+   * item is moved to a different source context, the uuid should remain the
>+   * same whenever possible. A copied card MUST have a new uuid.

> Why is this a SHOULD and not a MUST?

The item is a SHOULD because there is an overriding exception in the following sentence, i.e., when moved to different source context.

> Why is it only guaranteed to be unique with the context of its source?  The
> first U in UUID typically stands for "Universally".

This is mostly in reference to cards' UUIDs only being unique wrt the parent directories. First off, the generation of UUIDs was simpler merely by using existing properties. Secondly, I expected that the only way to turn a UUID into a card would be through the parent directory, which means making it verifiably unique across all directories would be redundant at best.

I hope this demystifies UUIDs somewhat; the truth of the matter is that I believe more work needs to be done in the rewrite before I can pin down the edge cases.

Comment 52

9 years ago
The identifier under discussion here isn't a UUID:
http://en.wikipedia.org/wiki/UUID

As an alternative, how about LUID: "Locally Unique Identifier"?

Or even better:
- DUID: "Directory Unique Identifier"
- IDUD: "Identifier Unique to a Directory"
(In reply to comment #51)
> > And those necessities are what?  What's not clear to me here is exactly what
> > problem(s) this UUID is aimed at solving (and what problems does it
> > specifically not solve?).  Without knowing those things, it's not really
> > possible to critique the specifics of the API.
> 
> To me, at least, the UUID is primarily solving the puzzle of how to refer to an
> address book item (directory or card) generically. *gets the feeling that isn't
> the best explanation.

One of the problems we're trying to solve (that hasn't been mentioned) is for synchronisation - when synchronising to/from an address book, having a consistent ID for a card (and easily accessible) should make tracking changes a lot easier.

Updated

9 years ago
Blocks: 443099

Comment 54

9 years ago
Instead of using this "UUID" for the purpose of tracking changes, wouldn't sync software be better off assigning a unique id to a property on the card?  The card can then be tracked when it moves from one (local) directory to another.

A related issue: it would be nice for different bits of software to have a way of knowing that they're talking about the same Thunderbird contact.  To the extent that this "UUID" is a step in the direction of solving that problem, so much the better.
eg: http://thunderbird-instance/directory-UUID/card-UUID or http://thunderbird-instance/card-UUID
Comment on attachment 328015 [details] [diff] [review]
nsIAbCard refactor, patch v5

> NS_IMETHODIMP nsAbMDBCard::Equals(nsIAbCard *card, PRBool *result)
>
> [...]
>
>+  // If we have the same directory, we will equal the other card merely given
>+  // the UIDs. If not, we are never equal.

Meaning that the thing that is actually guaranteed to be unique is the tuple (directory, UUID), right?  So if someone copies a card from one directory to another, the UUID stays the same?

>+  // But we are dumb in that we don't know
>+  // who our directory is. However, the only known users of this method are for
>+  // locating us in a list of cards, most commonly mailing lists. In this
>+  // respect, it is safe to assume that the directory portion is satisified when
>+  // making this call.

This is a public API likely to be used by extensions, so we can't make assumptions like this about what callers might or might not do.

>+  // However, if we make the wrong assumption, it is quite likely that little
>+  // ill will happen, as the URIs are of different syntaxes across the board.

How are the URI syntaxes involved here?

>+  // And, since different cards implement this method differently, it is very
>+  // likely that the incorrect assumption will cause the symmetric property to
>+  // fail, which is a problem in and of itself.

So if comparing UUIDs really is a good enough test of identity, the above statement shouldn't be true.  nsAbCardProperty should implement Equals as a UID comparison, and none of the subclasses should need to override it at all.

> [...]
>
>+  *result = (uuid == ourUuid);

Looking at nsStringAPI.h suggests that operator== isn't overridden here, so I think Compare() is really what's needed.
(In reply to comment #51)
> (In reply to comment #48)
> > As an example, LDAP already has a DN to identify itself.   Is the UUID of an
> > LDAP object likely to be the DN?  Or is there some reason it needs to be
> > separate from that?
> 
> The UUID is the DN for LDAP. It is the mork row ID for MDB cards. The problem
> is that a consumer of nsIAbCard may not specifically know whether or not the
> card is an LDAP card, MDB card, OS X card, etc. Thus, the point of the UUID is
> to provide a reference such that the card can be referred to uniquely without
> knowing which property makes it unique.

As far as I can tell, noone needs to do that in the way that this patch provides.  That is, all the callers are either blindly copying this, or are in places where they can and do have specific knowledge of what this particular local primary key means.  e.g. all of the nsAbMDBCard stuff knows that this is the row ID, and having an extra level of abstraction pretending that it's somehow opaque just unnecessarily complicates things.

> > And those necessities are what?  What's not clear to me here is exactly what
> > problem(s) this UUID is aimed at solving (and what problems does it
> > specifically not solve?).  Without knowing those things, it's not really
> > possible to critique the specifics of the API.
> 
> To me, at least, the UUID is primarily solving the puzzle of how to refer to an
> address book item (directory or card) generically. *gets the feeling that isn't
> the best explanation.

As per my previous comment, I think that in this patch, publicly exposing the UUID on nsIAbCard as it is in the current patch adds complexity without solving any specific problems.  So my inclination is that it doesn't make sense to add it to the interface until later, once we actually have a specific problem that it will solve.  

> >+   *
> >+   * This item SHOULD remain the same throughout the life of the item. If the
> >+   * item is moved to a different source context, the uuid should remain the
> >+   * same whenever possible. A copied card MUST have a new uuid.
> 
> > Why is this a SHOULD and not a MUST?
> 
> The item is a SHOULD because there is an overriding exception in the following
> sentence, i.e., when moved to different source context.

Heh.  What I was trying to ask is, "why is it ok (from an API point of view) for the UUID to change when moved to another source context?".  It kinda feels like it's just a special exception whose rationale is related to the current implementation.
> > >@@ -120,33 +58,17 @@ NS_IMETHODIMP nsAbMDBCard::Equals(nsIAbC
> > >     return NS_OK;
> > >   }
> > > 
> > >+  // If we have the same directory, we will equal the other card merely given
> > >+  // the UIDs. If not, we are never equal. But we don't know when we have the
> > >+  // same directories. Assume that we do.
> > 
> > In what cases will having the same directory happen?  What are the consequences
> > of this assumption being wrong then?  Please add this info to the commentary.
> 
> Equals seems to be only used for testing if a card in an array is the same as
> this one. Docs added.

Equals is a really basic operation; I disbelieve that we can say with any confidence that the in-tree and extra-tree usage is likely to stay where it is now.  This is a bug that needs to be fixed before checkin.

> To be honest, for the most part, I considered this an atomic change, especially
> because of how I implemented this:
> 1. Add generic property support to nsIAbCard (requiring the list->loop
> changes).
> 2. Get rid of the flat list of attributes.

I agree that the vast majority here does belong together.  I was mostly thinking about the non-generic-property changes, especially the UUID / Equality stuff.

(In reply to comment #51)
> > Why is it only guaranteed to be unique with the context of its source?  The
> > first U in UUID typically stands for "Universally".
> 
> This is mostly in reference to cards' UUIDs only being unique wrt the parent
> directories. First off, the generation of UUIDs was simpler merely by using
> existing properties. Secondly, I expected that the only way to turn a UUID into
> a card would be through the parent directory, which means making it verifiably
> unique across all directories would be redundant at best.

I'll be really surprised if the second thing is true.  I would expect one of the main use cases is likely to be in sync code, which is going to want to find the card based on only the UUID.  In short, I don't think it really makes sense to expose the current thing, which is not a true UUID.  I do think we will want a true UUID at some point, though.

So I think the way forward here is to excise the UUID code from this patch, and move to using the non-abstracted local-IDs internally.
Comment on attachment 328015 [details] [diff] [review]
nsIAbCard refactor, patch v5

Almost there; thanks for bearing with me!
Attachment #328015 - Flags: superreview?(dmose) → superreview-

Comment 59

9 years ago
(In reply to comment #57)

> (In reply to comment #51)
> > > Why is it only guaranteed to be unique with the context of its source?  The
> > > first U in UUID typically stands for "Universally".
> > 
> > This is mostly in reference to cards' UUIDs only being unique wrt the parent
> > directories. First off, the generation of UUIDs was simpler merely by using
> > existing properties. Secondly, I expected that the only way to turn a UUID into
> > a card would be through the parent directory, which means making it verifiably
> > unique across all directories would be redundant at best.
> 
> I'll be really surprised if the second thing is true.  I would expect one of
> the main use cases is likely to be in sync code, which is going to want to find
> the card based on only the UUID.  In short, I don't think it really makes sense
> to expose the current thing, which is not a true UUID.  I do think we will want
> a true UUID at some point, though.
> 
> So I think the way forward here is to excise the UUID code from this patch, and
> move to using the non-abstracted local-IDs internally.
> 

As said within this bug thread a UUID has an advantage across different usages, just to name two:
- sync
- accessing a specific card from an extension.

Also the arguments have been named, I don't understand Dan's attempt to postpone the implementation of a UUID.
The 'outside' handling of a TB/AB card with applications related to the two arguments would be much easier.

To better support AB-Sync/-Access is a MUST for the next TB/AB release, so I would like to vote the "abCard-UUID" to be a blocker!

Comment 60

9 years ago
If what's to be implemented here among a big number of other things is not universally unique ID, then it shouldn't be named UUID and if there's no need to expose it publically, keeping it private sounds the best thing to do, as well as filing a followup bug on implementing a real UUID.
Günter, this is a really really huge patch, it doesn't make sense to add yet another functionality to it when that can be done as a separate patch, as it will only make testing and review more complex and therefore it will take even longer until anything gets into the code.
Sorry for repeating much of what Dan said, but I think there's a need to say this more clearly for people who aren't core developers and listening here.

All that said, I'm very much for still implementing that followup, i.e. the real UUID, for Thunderbird 3 and SeaMonkey 2, as it's quite important for any sync puposes.

Comment 61

9 years ago
(In reply to comment #60)
> All that said, I'm very much for still implementing that followup, i.e. the
> real UUID, for Thunderbird 3 and SeaMonkey 2, as it's quite important for any
> sync puposes.
> 
Thanks Robert, as long as the UUID makes it for TB3/SM2 I'm fine ;-)
And it's good to hear the SYNC is important and paving the floor for it is also on the radar.
(Assignee)

Comment 62

9 years ago
In summary of all the comments so far (and a defense of my actions, in part):
1. UUID, as it stands, is probably poorly named. A better name for what it is now is probably "key" (especially as its usages right now tend to be a replacement for nsIAbMDBCard's key property).

2. The equals method (and the copy method as well) was not included in Standard8's original API design. Equals as it stands has several different implementations, but whether or not it should be a soft equality test (equivalent keys, like nsAbMDBCard does) or a hard equality (equal properties, like nsAbIPCCard) was never formally decided, and I to this day still don't know what I prefer. Hence my decision to document in the IDL that the current situation was unstable.

3. I implemented UUID the way I did because I thought it would be best to reuse an existing property on the card (e.g., DN or DbRowID).

4. In retrospect, knowing what I do now, I probably should have left the implementation of UUID and similar latter requirements until after handling more directory stuff. In any case, it was the last part of the patch to be added (look at the WIPs and you can see that); IIRC, there were some in-depth discussions over IRC about what to do between myself and Standard8, but that is before I started keeping personal logs, so I have no records of what transpired.
Looks like a reasonable summary to me.  None of my comments should be construed as needing defense, FWIW.  Big changes like this are hard, and it's often difficult to understand what's likely to work and what's likely to have fallout until you've written a bunch of code (ah, the iterative life :-).  I think you're doing great stuff and am just trying to provide guidance on how to move forward with this patch and how to think about future big patches.
(Assignee)

Comment 64

9 years ago
Created attachment 328348 [details] [diff] [review]
nsIAbCard refactor, patch v6

This addresses the UUID problem by not including it, as much more thought needs to be given to it on multiple levels then what we can provide now.

It addresses the equals problem by leaving what we have now with a note to document what occurs and a bug to fix this later. Current thought is that we may provide two equals functions (one of which can't be implemented without the UUID stuff) to satisfy two of three different implementations. By the way, all three different theoretical implementations are defined by some implementation of cards. Whee!

r+ carried over from previous patches.
Attachment #328015 - Attachment is obsolete: true
Attachment #328348 - Flags: superreview?(dmose)
Attachment #328348 - Flags: review+

Comment 65

9 years ago
I' missing AB card item "Category" .. see "interface nsIAbCard" definition at:

%{C++
// A nice list of properties for the benefit of C++ clients

"Category" should be a natural attribute (list) to the addresses!
Please recheck.
Comment on attachment 328348 [details] [diff] [review]
nsIAbCard refactor, patch v6

sr=dmose, conditional on some basic performance testing on autocomplete with large addressbooks and filing the various spinoff bugs we've discussed.  Thanks for all the hard work!
Attachment #328348 - Flags: superreview?(dmose) → superreview+
Guenter: since Category isn't exposed in the UI yet, and we're not entirely sure what our strategy w.r.t. tags should be, not having a C++ shortcut here seems reasonable to me.  If an extension _really_ wants to use that field despite all the uncertainty, it's easy enough to just supply that string manually.

Comment 68

9 years ago
(In reply to comment #67)
> Guenter: since Category isn't exposed in the UI yet, and we're not entirely
> sure what our strategy w.r.t. tags should be, not having a C++ shortcut here
> seems reasonable to me.  If an extension _really_ wants to use that field
> despite all the uncertainty, it's easy enough to just supply that string
> manually.
> 
Sorry Dan to insist on this. From various discussion about TB/AB and other productivity tools I expected TB will support CATEGORY with future update/versions. Also it's not in the UI yet, CATEGORY (or maybe TAGs) is a widely used item with office productivity tools. 
So how to solve this? For a general attempt!
(Assignee)

Comment 69

9 years ago
(In reply to comment #68)
> Sorry Dan to insist on this. From various discussion about TB/AB and other
> productivity tools I expected TB will support CATEGORY with future
> update/versions. Also it's not in the UI yet, CATEGORY (or maybe TAGs) is a
> widely used item with office productivity tools. 
> So how to solve this? For a general attempt!

This issue will be addressed again if/when tagged cards is implemented. Especially because syntax, etc. has to be decided.

Comment 70

9 years ago
OK! Would be a good idea to have a timetable/roadmap. Can't remember I saw anything like that. Please give any reference available how the AB will be evolved "Refactor" to meet the user's requirements to replace other misliked apps in the office place!! And to help extension developers to get their code ready with TB3 
Current status is that we're seeing non-trivial performance regression in autocomplete, so this patch hasn't landed yet.  jcranmer is investigating.  My inclination is to not block 3.0a2 on this unless the performance stuff looks like it'll be very quick to address.

Guenter: <http://wiki.mozilla.org/MailNews:Address_Book#Roadmap_.26_Work_In_Progress> has some info, but we don't currently have any timetables.
Whiteboard: [needs sr → [needs profiling]
(Assignee)

Updated

9 years ago
Depends on: 444091
(Assignee)

Updated

9 years ago
Depends on: 444093
(Assignee)

Updated

9 years ago
Depends on: 444095
(Assignee)

Updated

9 years ago
Depends on: 444164
Because of the performance regression, I don't think we can block 3.0a2 on this.
Flags: blocking-thunderbird3.0a2+ → blocking-thunderbird3.0a2-
(Assignee)

Comment 73

9 years ago
Summarizing results from testing....

It seems that the JS implementation of autocomplete we currently have produces a regression of about 360μs per card; optimizing the code somewhat to cache values betters produces a regression of about 250μs per card. A change to the API of cards gives me about 220μs per card. After that, I tested an extremely rough version of the autocomplete card that utilized nsIAbDirectoryQuery, which brought the regression down to 60μs per card (give or take 5μs).

It looks like, therefore, that autocomplete should ultimately rely on the directory query; it also seems a good idea to rewrite mork's implementation to not create cards where necessary. I personally would also like to see some API improvements to the querying as well, such as specifying a value to sort by, for future SQLite leverage.

The only other improvements that could really be made if we don't change autocomplete would involve hacking nsInterfaceHashtable (allowing us to manually specify hashed keys).
On a build with --enable-static-mail, the latest patch (and I expect some older ones) fails with:

ld: duplicate symbol nsSimpleProperty::AddRef()      in nsAbCardProperty.o and nsMsgTxn.o

Building with --disable-static-mail works.

From the performance analysis, I've got a few questions/comments:

- On the cards you tested, did you fill in data to all the fields, or just some of them?

- How long does it take switching to/from an address book with lots of cards in it? (I have one with about 5000 I can give you).

- From your analysis, it seems that GetChildCards is the function where we'll take most hit (hence my question above).

- Do we know what is causing the most hurt - is it nsIVariant, is it getting things in and out of the hashtable or memory reallocation?

- I count approx 53 entries that we'll fill in, the default minimum size appears to be 16, it is probably worth increasing the default to 53, and seeing what effect it has.

- It makes me wonder if rewriting nsAbCardProperty into javascript could be an advantage because of the easy hash functions.
(Assignee)

Comment 75

9 years ago
(In reply to comment #74)
> ld: duplicate symbol nsSimpleProperty::AddRef()      in nsAbCardProperty.o and
> nsMsgTxn.o

I have this fixed locally, but haven't posted a new patch yet.

> - On the cards you tested, did you fill in data to all the fields, or just some
> of them?

There are about 2000 cards in 4 address books, almost 1900 of whom are in a single address book imported via an LDIF. The data in the address book varies a little between cards, but only 7 properties on average are defined: display, first, and last names, primary email, with three other lesser fields defined.

> - How long does it take switching to/from an address book with lots of cards in
> it? (I have one with about 5000 I can give you).

My large import is on the cusp of notability, so I'd say around 100-200ms.

> - From your analysis, it seems that GetChildCards is the function where we'll
> take most hit (hence my question above).

My statement above supports this theory.

> - Do we know what is causing the most hurt - is it nsIVariant, is it getting
> things in and out of the hashtable or memory reallocation?

I have several jprof-based reports (the only profiler I can reliably get on Linux); I have tried to get JS profilings with Venkman but failed. On the C++ side, the following actions are where the most hurt is being felt:
* UTF-16 to UTF-8 conversion from a JS string to an nsACString for the hashtable.
* Lowercase conversions inside the autocomplete. Cutting out but a few of them dropped the time roughly 100ms by my tests; the others still register among the most time-consuming functions.

> - It makes me wonder if rewriting nsAbCardProperty into javascript could be an
> advantage because of the easy hash functions.

I'm not entirely sure of the performance impact, but we'll be hitting xpconnect boundaries hard one way or the other, since most of our card producers are C++, and most of our card consumers are JS.
(Assignee)

Comment 76

9 years ago
Throwing more statistics in the mix...

My efforts to do JS profiling have all come to naught so far, so I don't know where time is being wasted on the autocomplete end. However, in a last attempt to get something to work, I followed the instructions in <http://www.mozilla.org/performance/jsprofiler.html>. Needless to say, I failed to get any output whatsoever in just rebuilding, so I did a clobber build and tried again. Again I failed to get any results, but I suddenly noticed something else change.

The 5-run average changed from about 615.6ms on my previous best run to 371.0ms; the only other changes in my tree were related to bug 418551. I haven't had enough time to try to track down where the 244.6ms savings came from, though...
(Assignee)

Comment 77

9 years ago
Created attachment 330567 [details] [diff] [review]
nsIAbCard refactor, patch v6.5 [checked-in]

Updated patch version. This is based on some profiling done and discussions held outside of bugzilla.

To summarize:
Some of the autocomplete stuff was tweaked to give better results, mostly in caching values to avoid some performance hits (like toLocaleLowerCase).
After some discussion, a decision was made to make nsIAbCard::getProperty not throw for performance reasons (throwing is expensive!). That said, some sort of throwing getProperty or feedback on the non-existence of a property is useful; I think such discussion will be deferred to bug 444164, if no one minds.

This patch has both of these enhancements.
Attachment #328348 - Attachment is obsolete: true
Attachment #330567 - Flags: superreview?(dmose)
Attachment #330567 - Flags: review?(bugzilla)
Product: Core → MailNews Core
Comment on attachment 330567 [details] [diff] [review]
nsIAbCard refactor, patch v6.5 [checked-in]


>+  /**
>+   * @{
>+   * Returns a property for the given name.
>+   *
>+   * These functions interact with each other as according to nsIVariant.
>+   *
>+   * The non-variant functions are marked [noscript] since xpconnect uses
>+   * magic with nsIVariant such that the other functions are not needed,
>+   * although C++ does need them. The non-variant functions also do not have
>+   * default values, since the outparam is not set on failure.
>+   *
>+   * @param name             The case-sensitive name of the property to get.
>+   * @param defaultValue     The value to return if the property does not exist.
>+   * @exception NS_ERROR_NOT_AVAILABLE if the named property does not exist.
>+   * @exception NS_ERROR_CANNOT_CONVERT_DATA if the property cannot be converted
>+   *                                         to the desired type.
>+   */
>+  nsIVariant getProperty(in AUTF8String name, in nsIVariant defaultValue);
>+  [noscript] AString getPropertyAsAString(in string name);
>+  [noscript] AUTF8String getPropertyAsAUTF8String(in string name);
>+  [noscript] PRUint32 getPropertyAsUint32(in string name);
>+  [noscript] boolean getPropertyAsBool(in string name);
>+  /** @} */

This comment should now be split into two - one for the scripted function, one for the noscript functions



>diff --git a/mailnews/addrbook/test/unit/test_collection.js b/mailnews/addrbook/test/unit/test_collection.js
>--- a/mailnews/addrbook/test/unit/test_collection.js
>+++ b/mailnews/addrbook/test/unit/test_collection.js
>@@ -227,19 +227,19 @@ var collectChecker = {
>       do_check_true(card != null);
> 
>       if ("secondEmail" in aDetails)
>-        do_check_eq(card.secondEmail, aDetails.secondEmail);
>+        do_check_eq(card.getProperty("SecondEmail", ""), aDetails.secondEmail);
> 
I think you should make the default "BAD" here.

r=me with those fixed (and any current bitrot)
Attachment #330567 - Flags: review?(bugzilla) → review+

Updated

9 years ago
No longer blocks: 443099
>+   * These functions interact with each other as according to nsIVariant.

This isn't a very clear comment.  Can you revise so as to make it's meaning more obvious? 

With that, sr=dmose.
Comment on attachment 330567 [details] [diff] [review]
nsIAbCard refactor, patch v6.5 [checked-in]

sr=dmose
Attachment #330567 - Flags: superreview?(dmose) → superreview+
(Assignee)

Comment 81

9 years ago
Comment on attachment 330567 [details] [diff] [review]
nsIAbCard refactor, patch v6.5 [checked-in]

Checked in:
http://hg.mozilla.org/index.cgi/comm-central/rev/b9967a04b9bd

(A few doc comments were updated)
Attachment #330567 - Attachment description: nsIAbCard refactor, patch v6.5 → nsIAbCard refactor, patch v6.5 [checked-in]
(Assignee)

Comment 82

9 years ago
Created attachment 332720 [details] [diff] [review]
Bustage fix [checked-in]

This is the bustage fix for the two failing tests on Windows and Mac, and also managed to kick the Linux tbox into working again.
Joshua, which works is left now? Only profiling or even more? 
(Assignee)

Comment 84

9 years ago
Only the card refactors have been checked in. Most of the other interfaces need to be checked in still. Those will be tracked in blocking bugs.
Whiteboard: [needs profiling] → [branch @ http://hg.mozilla.org/users/Pidgeot18_gmail.com/ab_rewrite]
(Assignee)

Updated

9 years ago
Depends on: 449618
(In reply to comment #84)
> Only the card refactors have been checked in. Most of the other interfaces need
> to be checked in still. Those will be tracked in blocking bugs.

So we can mark this one as fixed? Or will you explicitly wait for all the other fixes before closing this one?
(Assignee)

Comment 86

9 years ago
(In reply to comment #85)
> So we can mark this one as fixed? Or will you explicitly wait for all the other
> fixes before closing this one?

I personally prefer keeping this bug open until the other fixes are finished, at least to the point where we can say "there will be no more significant API churn."
(Assignee)

Updated

9 years ago
No longer blocks: 437908
Depends on: 450134
Blocks: 450149
(Assignee)

Updated

9 years ago
Blocks: 450905
(Assignee)

Updated

9 years ago
Blocks: 170265
(Assignee)

Updated

9 years ago
No longer blocks: 450149
Depends on: 450149
(Assignee)

Updated

9 years ago
No longer blocks: 450905
(Assignee)

Updated

9 years ago
Depends on: 450917
(Assignee)

Updated

9 years ago
Depends on: 451844
Depends on: 454073

Updated

9 years ago
Priority: -- → P3
Target Milestone: --- → Thunderbird 3.0b2
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
(Assignee)

Updated

9 years ago
Depends on: 464831
(Assignee)

Updated

9 years ago
Depends on: 464833
(Assignee)

Updated

9 years ago
No longer depends on: 464831
We're not going to block the Thunderbird releases on this, but more patches will be accepted as/when they come.
Flags: blocking-thunderbird3+ → blocking-thunderbird3-

Comment 88

7 years ago
What sort of stuff still needs work here? I'd love to see Mork go away, and I could probably work on some small(-ish) patches to help get closer to that day.
(In reply to comment #88)
> What sort of stuff still needs work here? I'd love to see Mork go away, and I
> could probably work on some small(-ish) patches to help get closer to that day.

We're currently thinking about rewriting the address book code, although I haven't started any big discussions on that - keep asking me for plans ;-) . The likely-hood would be that we'd just not have it in the rewritten code (except for a little bit of read-only import). Hence this would save us a) trying to fit something new within the existing limitations of our interfaces or b) trying to progressively get our interfaces to be where we want them to be.

Comment 90

7 years ago
Hm. I'll keep an eye out for that, then. I can already think of a few cool things I could do with an SQLite-based address book (tie it in with contacts on an IM program, for instance).

More generally, address books seem like one of the things where bug reports crop up a lot, and I always hesitate to volunteer too much of my time on them when I know that there are plans to change the interfaces.
Blocks: 758969

Comment 91

5 years ago
Hello Joshua, 

I have seen you have done already much of the work regarding the case.  Could you please say what is still left to be completed in order to remove Mork completely? Referring to your comment from 86. Do we have a actual target milestone for freezing API? Thank you in advance.
(Assignee)

Comment 92

5 years ago
(In reply to konstantin from comment #91)
> Hello Joshua, 
> 
> I have seen you have done already much of the work regarding the case. 
> Could you please say what is still left to be completed in order to remove
> Mork completely? Referring to your comment from 86. Do we have a actual
> target milestone for freezing API? Thank you in advance.

I know Mike Conley is currently working a new address book. I don't know what his plans are in terms of interfaces, though.
You need to log in before you can comment on or make changes to this bug.