De-RDF the address book

RESOLVED FIXED in Thunderbird 7.0

Status

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 7.0
x86
All
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 19 obsolete attachments)

5.80 KB, patch
Details | Diff | Splinter Review
4.01 KB, patch
standard8
: review+
Details | Diff | Splinter Review
164.88 KB, patch
standard8
: review+
Details | Diff | Splinter Review
The address book currently uses the RDF service to look up and instantiate different address book types.

We want to eliminate our need for the RDF service within the address book, and replace it with a simpler look-up table managed by AbManager.
Assignee: nobody → mconley
Blocks: mail-killrdf
Depends on: 422845
Assignee: mconley → nobody
Product: Thunderbird → MailNews Core
QA Contact: address-book → address-book
Assignee: nobody → mconley
Posted patch Work-in-progress patch - v1 (obsolete) — Splinter Review
This is my first WIP patch for de-RDFing the address book.

I took a wrecking ball to this thing, and I guess I just was hoping for another pair of eyes to make sure I didn't wipe anything critical out.
Attachment #528904 - Flags: feedback?(mbanner)
Attachment #528904 - Flags: feedback?(dbienvenu)
I should also point out:

1)  All address book xpcshell tests are passing
2)  I haven't de-RDF'd any of the Outlook or OSX address book code, simply because I'm yet to set up a Windows / Mac machine to compile and test that stuff
Status: NEW → ASSIGNED
cool - I've looked through some of this. It occurs to me that perhaps CreateDirectory wants to take a URI, which might get rid of some of the need for Init().

I might have a whack at the Outlook stuff because I'd need that fixed in order to run the patch. If I get a chance to start on that, I'll let you know.
Comment on attachment 528904 [details] [diff] [review]
Work-in-progress patch - v1

this patch didn't come close to applying - is it against the comm-central trunk?
If I may add a few comments:
1. As far as I can tell, nsIAbDirFactory::createDirectory is essentially a wrapper around Cc.createInstance(<magic instance>)? If there is no particular reason for anyone to implement it differently, I would just recommend that you replace it with the createInstance directly.

2. I'm concerned about the nsAddrDatabase::AddListNode implementation. The original code made the rdfService a proxied instance, so you should probably also set up a proxied service call to nsIAbManager, especially because nsInterfaceHashtable is not thread-safe.
Posted patch Work-in-progress patch-v2 (obsolete) — Splinter Review
Whoops - looks like I forgot to merge upstream comm-central.  Sorry about that.
Attachment #528904 - Attachment is obsolete: true
Attachment #528904 - Flags: feedback?(mbanner)
Attachment #528904 - Flags: feedback?(dbienvenu)
Attachment #529141 - Flags: feedback?(mbanner)
Attachment #529141 - Flags: feedback?(dbienvenu)
thx, the patch applies now.  I'm having a bit of fun with nsAbOutlookDirFactory::CreateDirectory because CreateDirectory conflicts with some built-in Windows function/define, so I have to remember how to work around that...
Mike, you'll want to incorporate the changes to the outlook source files into your patch going forward. I haven't tested that they work, but they do build. I'll look at OS/X next.
Posted patch get abmanager outside loop (obsolete) — Splinter Review
actually, this is better, because it gets the abmanager outside the for loop.
Attachment #529190 - Attachment is obsolete: true
ah, sorry, libxul doesn't link w/ this patch - update coming, sorry for the bugspam.
Posted patch fix link issues (obsolete) — Splinter Review
ok, this should build on windows.
Comment on attachment 529141 [details] [diff] [review]
Work-in-progress patch-v2

You should get abManager from MailServices. I know this patch is going to evolve some more, but marking feedback+.
Attachment #529141 - Flags: feedback?(dbienvenu) → feedback+
These are the backend changes for de-RDFing the address book.  These changes also integrate bienvenu's work with the Outlook address book type.
Attachment #529141 - Attachment is obsolete: true
Attachment #529195 - Attachment is obsolete: true
Attachment #529207 - Attachment is obsolete: true
Attachment #529141 - Flags: feedback?(mbanner)
These are the front-end Javascript changes for de-RDFing the address book.
Sorry for the bugspam - I'm breaking this patch down even further...
Attachment #529723 - Attachment is obsolete: true
This patch holds the simpler changes to the backend.
Attachment #529732 - Flags: review?(mbanner)
Attachment #529724 - Flags: review?(mbanner)
I forgot the bump the uuid on nsIAbDirectory.
Attachment #529732 - Attachment is obsolete: true
Attachment #529732 - Flags: review?(mbanner)
Attachment #529744 - Flags: review?(mbanner)
Comment on attachment 529724 [details] [diff] [review]
Work-in-progress patch-v3 - Javascript changes

>+let Cu = Components.utils;
>+

Please drop this in both files. We don't define these globally as it actually causes namespace issues, and no-one has resolved them yet.

>diff -r a3de94be0dcc mailnews/addrbook/prefs/content/pref-directory-add.js
>+let Cu = Components.utils;

as above

>+Cu.import("resource:///gre/modules/mailServices.js");

This shouldn't have the gre in it as it breaks adding an LDAP dir.
Attachment #529724 - Flags: review?(mbanner) → review-
Not quite what I meant with the C++ split. This patch is more what I meant - just a few of the RDF -> AB manager call changes, and some of the include-only changes.
Attachment #529731 - Attachment is obsolete: true
Attachment #529744 - Attachment is obsolete: true
Attachment #529744 - Flags: review?(mbanner)
Attachment #529758 - Flags: review?(mbanner)
and this is the rest of the changes that aren't in the basic c++ one.
Comment on attachment 529758 [details] [diff] [review]
Work-in-progress patch v3 - basic c++ changes

>diff --git a/mailnews/addrbook/src/nsAbCardProperty.cpp b/mailnews/addrbook/src/nsAbCardProperty.cpp
>+    nsCOMPtr <nsIAbDirectory> mailList = nsnull;

nit: No need to initialise to nsnull, as nsCOMPtr does that for us.
nit: No space in-between nsCOMPtr and <

The rest of this seems to compile on Mac at least, so r=Standard8 with those fixed.
Attachment #529758 - Flags: review?(mbanner) → review+
Posted patch Patch-v4 - Javascript changes (obsolete) — Splinter Review
Thanks for the review, Standard8.  I've implemented your changes.
Attachment #529724 - Attachment is obsolete: true
Attachment #529768 - Flags: review?(mbanner)
Fixed those tiny nits that Standard8 found - thanks for the review!
Attachment #529758 - Attachment is obsolete: true
I accidentally based this patch off of trunk, when it relies on bug 422845.
Attachment #529768 - Attachment is obsolete: true
Attachment #529768 - Flags: review?(mbanner)
Attachment #529815 - Flags: review?(mbanner)
Comment on attachment 529815 [details] [diff] [review]
[checked in] Patch-v5 - Javascript changes

Looks good. r=Standard8
Attachment #529815 - Flags: review?(mbanner) → review+
Comment on attachment 529786 [details] [diff] [review]
[checked in] Basic c++ groundwork changes for de-RDFing address book (r+'d by Standard8)

I checked this in with the js changes merged as one.
Attachment #529786 - Attachment description: For check-in: Basic c++ groundwork changes for de-RDFing address book (r+'d by Standard8) → [checked in] Basic c++ groundwork changes for de-RDFing address book (r+'d by Standard8)
Comment on attachment 529815 [details] [diff] [review]
[checked in] Patch-v5 - Javascript changes

Checked in: http://hg.mozilla.org/comm-central/rev/548d10bdab86
Attachment #529815 - Attachment description: Patch-v5 - Javascript changes → [checked in] Patch-v5 - Javascript changes
Should I create a new bug for making the changes to the suite/ equivalent of abCommon.js, add the patch here (already have it locally) or leave it to you guys?
(In reply to comment #28)
> Should I create a new bug for making the changes to the suite/ equivalent of
> abCommon.js, add the patch here (already have it locally) or leave it to you
> guys?

I'd suggest making the suite/ port separate - there's already a lot going on here.
No longer blocks: 654864
Depends on: 654864
Attachment #529759 - Flags: feedback?(mbanner)
This work-in-progress patch de-RDF's the Mork, LDAP, OSX and Outlook address books.

I'm going to clean it up some on Monday and have a patch up for review soon after.
Attachment #529759 - Attachment is obsolete: true
Attachment #529759 - Flags: feedback?(mbanner)
Posted patch De-RDF Patch - V1 (obsolete) — Splinter Review
Ok, this is it.  Fire away.
Attachment #537211 - Attachment is obsolete: true
Attachment #537834 - Flags: review?(dbienvenu)
Comment on attachment 537834 [details] [diff] [review]
De-RDF Patch - V1

Whoops, put down the wrong reviewer.
Attachment #537834 - Flags: review?(dbienvenu) → review?(mbanner)
I should also point out that my EDS Contacts integration work I've been doing relies on this patch, and so this needs to land in TB 7.0 if we're going to make the Ubuntu O release on time.

No pressure. :)
Comment on attachment 537834 [details] [diff] [review]
De-RDF Patch - V1

jcranmer mentioned that I'll probably need superreview for this - I'll assign bienvenu for now, but I'm open to suggestions.
Attachment #537834 - Flags: superreview?(dbienvenu)
Attachment #537834 - Flags: review?(Pidgeot18)
Comment on attachment 537834 [details] [diff] [review]
De-RDF Patch - V1

Review of attachment 537834 [details] [diff] [review]:
-----------------------------------------------------------------

Whee, my first review comment got swallowed :-( .

I'll start my review with some nits I've found from browsing source; I'll save a more thorough review of stuff for a bit later.

::: mailnews/addrbook/public/nsAbBaseCID.h
@@ +51,5 @@
>  //
> +// The start of the contract ID for address book directory types
> +//
> +#define NS_AB_DIRECTORY_TYPE_CONTRACTID_PREFIX \
> +  "@mozilla.org/addressbook/directory;1?type="

Since I presume this is being required as a contract-ID for directories, it would probably help to note this in the documentation for nsIAbDirectory.

@@ +66,4 @@
>  #define NS_ABMANAGER_CID \
>  { /* {ad81b321-8a8a-42ca-a508-fe659de84586} */ \
>    0xad81b321, 0x8a8a, 0x42ca, \
> +	{0xa5, 0x08, 0xfe, 0x65, 0x9d, 0x8e, 0x45, 0x86}	\

Nit: remove the tabs, if you're already changing these lines

::: mailnews/addrbook/public/nsIAbDirFactory.idl
@@ -43,4 +43,4 @@
> >  
> >  interface nsIAbDirectory;
> >  
> > -[scriptable, uuid(ad61b4fc-d8d8-40b2-b924-4c10f28a8a17)]
> > +[scriptable, uuid(3b1cf12e-2a7d-4719-8e51-23d2f6b6a680)]

Doesn't look like nsIAbDirFactory actually needs a UUID revision.

::: mailnews/addrbook/public/nsIAbDirectory.idl
@@ +46,5 @@
>  interface nsIMutableArray;
>  
>  %{C++
> +/* moz-abdirectory:// is the URI to access nsAbBSDirectory,
> + * which is the root directory for all types of address books */

If you're already touching this line, you might as well make it a regular documentation comment.

::: mailnews/addrbook/public/nsIAbMDBDirectory.idl
@@ +51,4 @@
>  [scriptable, uuid(744072be-1ba0-46bc-af24-46e22567a2ea)]
>  interface nsIAbMDBDirectory : nsISupports {
>  
> +	// Creates an directory component from the

Nit: s/an/a/

::: mailnews/addrbook/src/nsAbBSDirectory.cpp
@@ +69,5 @@
> +  mURI = aURI;
> +  return NS_OK;
> +}
> +
> +NS_IMPL_ISUPPORTS_INHERITED1(nsAbBSDirectory, nsAbDirProperty, nsIAbDirectory)

Since nsAbDirProperty already declares the QI for nsIAbDirectory, you don't need to do it again here. This comment also applies to the other ns*Directory implementations.
One complicated comment:

nsIAbManager is listed as THREADSAFE, which means any method can be called from any thread in theory. The hashtable for the URI->directory map is not threadsafe, which means adding new entries into the database might not be seen by other threads. Since acquiring a reference to nsIAbManager in the first place acts as a memory barrier (via the atomic ref count), I'm more concerned that someone might accidentally add a new directory off the main thread, which I'm not sure enforces memory visibility guarantees.

Now, judging from the fact that the RDF code appears to be blissfully unconcerned with the vagaries of multithreaded I/O, it could just be that I'm being extra-paranoid here...
Some more information: there apparently remains a use of address books-as-rdf in import code (if you set the ab import to import into a specific address book). Grepping indicates that this code does not appear to be used, and code coverage confirms that it's not being tested. Could you be so kind as to fix nsImportAddressBook's GetAddressBookFromUri method as well?
And I wish I would stop finding these:
* Spurious include of rdf.h in nsAbDirProperty.cpp
* mail/base/content/ABSearchDialog.js: nsIAbDirectory as RDF resource
  (equivalent one in suite's copy of this file)

I think that's all of them... most of the other nsIRDFService uses I've seen are either obviously folders or I've verified to not refer to the address book (at 347 instances according to MXR, I'm not going to go dumpster diving into every single nsIRDFService definition).
Posted patch De-RDF Patch - V2 (obsolete) — Splinter Review
> Since I presume this is being required as a contract-ID for directories, it 
> would probably help to note this in the documentation for nsIAbDirectory.

Done.

> Nit: remove the tabs, if you're already changing these lines

Done.

> Doesn't look like nsIAbDirFactory actually needs a UUID revision.

Reverted.

> If you're already touching this line, you might as well make it a regular 
> documentation comment.

Done.

> Nit: s/an/a/

Fixed.

> Since nsAbDirProperty already declares the QI for nsIAbDirectory,
> you don't need to do it again here. This comment also applies to
> the other ns*Directory implementations.

Hm.  On removing that line, linking produces the following error:

/media/Projects/mozilla/thunderbird/mailnews/addrbook/src/nsAbBSDirectory.cpp:58: undefined reference to `vtable for nsAbBSDirectory'
/usr/bin/ld: ../../staticlib/components/libmail.a(nsAbBSDirectory.o): relocation R_386_GOTOFF against undefined hidden symbol `vtable for nsAbBSDirectory' can not be used when making a shared object
/usr/bin/ld: final link failed: Bad value
collect2: ld returned 1 exit status

Thoughts on that?

> nsIAbManager is listed as THREADSAFE, which means any method can be called 
> from any thread in theory. The hashtable for the URI->directory map is not 
> threadsafe, which means adding new entries into the database might not be 
> seen by other threads. Since acquiring a reference to nsIAbManager in the 
> first place acts as a memory barrier (via the atomic ref count), I'm more 
> concerned that someone might accidentally add a new directory off the main 
> thread, which I'm not sure enforces memory visibility guarantees.
>
> Now, judging from the fact that the RDF code appears to be blissfully 
> unconcerned with the vagaries of multithreaded I/O, it could just be that I'm 
> being extra-paranoid here...

Hm.  From the sounds of it, this patch doesn't make this problem any worse than it already is.  While we might want to clean this up down the road, considering the time pressure I'm under to get this landed, we might want to push that off for later.

> Could you be so kind as to fix nsImportAddressBook's GetAddressBookFromUri 
> method as well?

Done

> And I wish I would stop finding these:
> * Spurious include of rdf.h in nsAbDirProperty.cpp
> * mail/base/content/ABSearchDialog.js: nsIAbDirectory as RDF resource
>  (equivalent one in suite's copy of this file)

Done
Attachment #537834 - Attachment is obsolete: true
Attachment #537834 - Flags: superreview?(dbienvenu)
Attachment #537834 - Flags: review?(mbanner)
Attachment #537834 - Flags: review?(Pidgeot18)
Attachment #538918 - Flags: superreview?(dbienvenu)
Attachment #538918 - Flags: review?(mbanner)
Attachment #538918 - Flags: review?(Pidgeot18)
Comment on attachment 538918 [details] [diff] [review]
De-RDF Patch - V2

the "lucky 13" size initialization of the ab store and osx card store hash tables look a bit odd. I'm not sure it's worthwhile for the ab store, and it seems a bit small for the os/x card store. In any case, a meaningfully named constant (or two) would be more appropriate. Other than that, this continues to looks reasonable...
Attachment #538918 - Flags: superreview?(dbienvenu) → superreview+
(In reply to comment #39)
> > Since nsAbDirProperty already declares the QI for nsIAbDirectory,
> > you don't need to do it again here. This comment also applies to
> > the other ns*Directory implementations.
> 
> Hm.  On removing that line, linking produces the following error:
> 
> /media/Projects/mozilla/thunderbird/mailnews/addrbook/src/nsAbBSDirectory.
> cpp:58: undefined reference to `vtable for nsAbBSDirectory'
> /usr/bin/ld: ../../staticlib/components/libmail.a(nsAbBSDirectory.o):
> relocation R_386_GOTOFF against undefined hidden symbol `vtable for
> nsAbBSDirectory' can not be used when making a shared object
> /usr/bin/ld: final link failed: Bad value
> collect2: ld returned 1 exit status
> 
> Thoughts on that?

It's not that you want to remove it, you can replace it with NS_IMPL_ISUPPORTS_INHERITED0(nsAbBSDirectory, nsAbDirProperty).
Comment on attachment 538918 [details] [diff] [review]
De-RDF Patch - V2

Review of attachment 538918 [details] [diff] [review]:
-----------------------------------------------------------------

So I've looked at pretty much every file except nsAbOSXDirectory.*, as my familiarity with that stuff is very low.

The nsAbOutlookDirectory code looks fairly wrong, but as you're not really changing anything that wasn't already the case, I'll leave fixing that to another bug.

Modulo these issues and the NS_IMPL_ISUPPORTS issues, r+.

::: mailnews/addrbook/src/nsAbManager.cpp
@@ +279,5 @@
> +    NS_ENSURE_SUCCESS (rv, rv);
> +
> +    nsCAutoString scheme;
> +    rv = nsService->ExtractScheme(aURI, scheme);
> +    NS_ENSURE_SUCCESS (rv, rv);

I'm dubious that it's worth using nsIIOService just to get the scheme; why not just use

PRInt32 colon = aURI.FindChar(':');
if (colon <= 0)
  return NS_ERROR_MALFORMED_URI;
scheme = Substring(aURI, 0, colon);

?

::: mailnews/addrbook/src/nsAbOSXCard.mm
@@ +88,4 @@
>    else if (!oldValue.Equals(aValue)) {
>      aCard->SetPropertyAsAString(aMemberName, aValue);
>      
> +    nsISupports *supports = NS_ISUPPORTS_CAST(nsIAbCard*, aCard);

nsAbCardProperty, not nsIAbCard...

Actually, is NS_ISUPPORTS_CAST even necessary? Try this code without it and see if it compiles.

::: mailnews/addrbook/src/nsAbOutlookDirFactory.cpp
@@ +48,4 @@
>  
>  #include "prlog.h"
>  
> +#undef CreateDirectory

Why do you need this #undef here?

::: mailnews/addrbook/src/nsAbOutlookDirectory.cpp
@@ +192,2 @@
>  
> +  if (NS_SUCCEEDED(rv)) {

As long as you're already touching the code here, you might as well go all the way and fix up style issues in this code.
For example, this if block could just be replaced with `NS_ENSURE_SUCCESS(rv, rv);' to allow you to reindent the others.

In addition, you can move the `{'s to the next line per style guidelines, and remove spaces from before semicolons.

@@ +242,5 @@
> +              NotifyItemAddition(card) ;
> +          }
> +      }
> +      else
> +        NS_WARNING("Card wasn't stored");

Since this event is apparently expected (judging from the comment at line 219), I'm not sure this ought to be an NS_WARNING.

@@ +303,5 @@
> +
> +	nsCAutoString stub;
> +    nsAbWinType objType = getAbWinType(kOutlookDirectoryScheme, uri.get(), stub, aEntry) ;
> +
> +    return NS_OK ;

Nit: you're touching these lines as well, so fix indentation, extra whitespace issues...

::: mailnews/addrbook/src/nsAddrDatabase.cpp
@@ +3222,5 @@
> +    rv = proxyObjMgr->GetProxyForObject( NS_PROXY_TO_MAIN_THREAD,
> +                               NS_GET_IID( nsIAbDirectory),
> +                               parentDir,
> +                               NS_PROXY_SYNC | NS_PROXY_ALWAYS,
> +                               getter_AddRefs(proxiedParentDir));

As long as you're changing this line, can you fix the whitespace?
Attachment #538918 - Flags: review?(Pidgeot18) → review+
Blocks: 664726
Hey - thanks for the reviews!  There's a question for bienvenu below - that's why I've added him for feedback.

> the "lucky 13" size initialization of the ab store and osx card store hash
> tables look a bit odd. 

Removed for the address book store, and increased to 100 for the OSX card store.

> It's not that you want to remove it, you can replace it with
> NS_IMPL_ISUPPORTS_INHERITED0(nsAbBSDirectory, nsAbDirProperty).

Done.

> I'm dubious that it's worth using nsIIOService just to get the scheme

Fixed, thanks.

> nsAbCardProperty, not nsIAbCard...

Done.

> Actually, is NS_ISUPPORTS_CAST even necessary? Try this code without it 
> and see if it compiles.

Yeah, it failed out.  It seems to want the cast here.

> Why do you need this #undef here?

I think that got put in by bienvenu at somepoint during my WIP patches.  Bienvenu - is there a reason to keep this in?

> As long as you're already touching the code here, you might as well go
> all the way and fix up style issues in this code.

Done.

> Since this event is apparently expected (judging from the comment at line 
> 219), I'm not sure this ought to be an NS_WARNING.

Removed.

> Nit: you're touching these lines as well, so fix indentation, extra 
> whitespace issues...

Done.

> As long as you're changing this line, can you fix the whitespace?

Done.
Attachment #538918 - Attachment is obsolete: true
Attachment #538918 - Flags: review?(mbanner)
Attachment #540093 - Flags: review?(mbanner)
Attachment #540093 - Flags: feedback?(dbienvenu)
(In reply to comment #43)
> > Why do you need this #undef here?
> 
> I think that got put in by bienvenu at somepoint during my WIP patches. 
> Bienvenu - is there a reason to keep this in?

Yes, because it won't compile without it, or it least it didn't when I was using the patch. CreateDirectory is defined in a windows header file, and it conflicts with our method.  There may be other ways around it, but you'd want to make sure you still build on windows if you take this out.
> Yes, because it won't compile without it, or it least it didn't when 
> I was using the patch. 

Ah, gotcha.  Yeah, it compiles without it now, since the CreateDirectory method was removed from nsIAbDirFactory.
Attachment #540093 - Flags: feedback?(dbienvenu) → feedback+
Comment on attachment 540093 [details] [diff] [review]
De-RDF Patch - V3 (r+ from jcranmer, sr+ from bienvenu)

Review of attachment 540093 [details] [diff] [review]:
-----------------------------------------------------------------

Generally looking good, though I'm concerned by the thread safety stuff because we're declaring safety when we're not.

General note: it'd be useful if you could turn on -p for generating patches as this gives the function name which is helpful.

::: mailnews/addrbook/public/nsIAbDirectory.idl
@@ +136,5 @@
> +  /**
> +   * Initializes a directory, pointing to a particular
> +   * URI
> +   */
> +  void init (in string aURI);

nit: no space before (

::: mailnews/addrbook/src/nsAbDirProperty.cpp
@@ +76,4 @@
>  #endif
>  }
>  
> +NS_IMPL_THREADSAFE_ISUPPORTS4(nsAbDirProperty, nsIAbDirectory, nsISupportsWeakReference,

I'm not convinced making this threadsafe is the right thing to do. We're definitely not managing threads here, and really the derived classes should also be threadsafe.

This seems wrong, but I guess the question is really what is actually trying to access these as non-threadsafe? hence why do we need to change it now?

@@ +293,5 @@
> +NS_IMETHODIMP
> +nsAbDirProperty::Init(const char *aURI)
> +{
> +
> +  mURINoQuery = aURI;

nit: no blank line necessary.

::: mailnews/addrbook/src/nsAbMDBDirectory.cpp
@@ +476,4 @@
>      rv = database->AddListener(this);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    nsCOMPtr<nsIAbDirectory> directory;

nit: Generally prefer to create this variable just before it is used (at least two places in this file).

::: mailnews/addrbook/src/nsAbManager.cpp
@@ +296,5 @@
> +
> +    // If all went well, add the address book to the look-up table, and
> +    // return it.
> +
> +    mAbStore.Put(aURI, directory);

This isn't going to keep address books that are based on queries alive is it? iirc we RDF didn't keep those alive, and we generally need them to go away.

::: mailnews/addrbook/src/nsAbOSXDirFactory.cpp
@@ +82,4 @@
>  {
>    return NS_OK;
>  }
> +

nit: this change seems a bit pointless.

::: mailnews/addrbook/src/nsAbOSXDirectory.h
@@ +55,5 @@
>  class nsIAbBooleanExpression;
>  
> +#ifdef __OBJC__
> +@class NSString;
> +@class ABRecord;

I don't see why these need defining here.

::: mailnews/addrbook/src/nsAbOSXDirectory.mm
@@ +87,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NS_IF_ADDREF(*aResult = directory);
> +
> +  return NS_OK;

nit: no blank line necessary (several places in this file).

@@ +92,4 @@
>  }
>  
>  static nsresult
> +GetCard(ABRecord *aRecord, nsIAbCard **aResult)

Do we really want to get the OSX Directory each time? As we generally call this from loops, it seems like it may affect perf a bit more than necessary.

@@ +569,5 @@
> +    // to create were already created in the root nsAbOSXDirectory,
> +    if (mURINoQuery.Length() > sizeof(NS_ABOSXDIRECTORY_URI_PREFIX))
> +      rv = GetCard([cards objectAtIndex:i], getter_AddRefs(card));
> +    // If we're not a Group, that means we're the root nsAbOSXDirectory,
> +    // which means we have to create the cards from scratch.

Unusual(?) way of doing comments, I'd have expected this after the else.
Standard8:

Thanks for the review!

> General note: it'd be useful if you could turn on -p for generating patches as > this gives the function name which is helpful

Done.

> nit: no space before (

Done.

> This seems wrong, but I guess the question is really what is actually trying > to access these as non-threadsafe? hence why do we need to change it now?

Very strange / unsettling, is that without it, everything works just fine.  I may have had a reason for changing it, but unfortunately it escapes me at this point.  Perhaps it's an unfortunate leftover from one of my earlier iterations? Regardless, I've removed it, and everything seems to work just peachy.

> nit: no blank line necessary.

Removed.

> nit: Generally prefer to create this variable just before it is used (at
> least two places in this file).

Ack, fixed.

> This isn't going to keep address books that are based on queries alive is it? > iirc we RDF didn't keep those alive, and we generally need them to go away.

Hm.  If they're destroyed using RDF, I cannot seem to tell when.  At bienvenu's advice, I set a breakpoint in nsDirectoryDataSource::OnItemRemoved, made some autocomplete queries, sent a message, refreshed my inbox, and waited around for a bit for it to hit my breakpoint, and it never did.  So I'm forced to wonder if the query directories are cleared out at all...if not, then my patch doesn't make the situation better or worse.

> nit: this change seems a bit pointless.

Fixed.

> I don't see why these need defining here.

Removed.

> nit: no blank line necessary (several places in this file).

Removed.

> Do we really want to get the OSX Directory each time? As we generally call 
> this from loops, it seems like it may affect perf a bit more than necessary.

Ok, instead of querying for the root OSX directory each time, I have directories cache the root OSX directory, and pass it into the GetCard function.

> Unusual(?) way of doing comments, I'd have expected this after the else.

Fixed!

Thanks again for the review!  Appears to behave correctly, and all tests appear to pass:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=52559cd0fe79

Let me know if there's anything else I can do.
Attachment #540093 - Attachment is obsolete: true
Attachment #540093 - Flags: review?(mbanner)
Attachment #543047 - Flags: review?(mbanner)
(In reply to comment #47)
> > This isn't going to keep address books that are based on queries alive is it?
> > iirc we RDF didn't keep those alive, and we generally need them to go away.
> 
> Hm.  If they're destroyed using RDF, I cannot seem to tell when.  At
> bienvenu's advice, I set a breakpoint in
> nsDirectoryDataSource::OnItemRemoved, made some autocomplete queries, sent a
> message, refreshed my inbox, and waited around for a bit for it to hit my
> breakpoint, and it never did.  So I'm forced to wonder if the query
> directories are cleared out at all...if not, then my patch doesn't make the
> situation better or worse.

I experimented a bit more, in the process I found and fixed bug 669151, however, with a bit of digging, I found that if nsAbManager stores references to queries, then LDAP connections don't get dropped. We don't currently re-use the LDAP connections, hence keeping them open until shutdown isn't a good thing.

So I think we really should not have queries stored in the manager. This seems to work and pass tests, this is the bit of code I used:

    PRBool isQuery = PR_FALSE;
    rv = directory->GetIsQuery(&isQuery);
    NS_ENSURE_SUCCESS(rv, rv);

    if (!isQuery)
      mAbStore.Put(aURI, directory);
Comment on attachment 543047 [details] [diff] [review]
Patch v4 - r+ from jcranmer, sr+ from bienvenu

Review of attachment 543047 [details] [diff] [review]:
-----------------------------------------------------------------

General nit: There's a few places throughout the patch where you have NS_ENSURE_SUCCESS (rv, rv); this should just be NS_ENSURE_SUCCESS(rv, rv); i.e. no space.

I've now noticed that the import tests are failing with threadsafe errors - that's strange as they were ok without the patch, I'm currently looking into that.
Attachment #543047 - Flags: review?(mbanner) → review-
> So I think we really should not have queries stored in the manager. This seems
> to work and pass tests, this is the bit of code I used:

Excellent, thanks for the tip!  Will do.

> General nit: There's a few places throughout the patch where you have
> NS_ENSURE_SUCCESS (rv, rv); this should just be NS_ENSURE_SUCCESS(rv, rv); i.e.
> no space.

On it.

> I've now noticed that the import tests are failing with threadsafe errors - 
> that's strange as they were ok without the patch, I'm currently looking into 
> that.

Ah, yes!  That sounds familiar.  I have a feeling this is the explanation for my NS_IMPL_THREADSAFE_ISUPPORTS4 code you mentioned in Comment 46.  Yes, any and all assistance there is appreciated.

Thanks again for the review,

-Mike
Posted patch Patch v5 (obsolete) — Splinter Review
Thanks for the review!  This patch addresses most of the issues raised in Comment 48 and Comment 49.

The lone exception being the thread-safety problem, which Standard8 is currently investigating.
Attachment #543047 - Attachment is obsolete: true
Attachment #543782 - Flags: review?(mbanner)
Posted patch Patch v6Splinter Review
This patch incorporates Standard8's thread-safety fix in the address book importer.

This revision also patched a big where imported address books did not appear in the UI until TB restart.  This was because the AbManager was instantiating a new root address book directory, as opposed to returning it's cached copy.  Should probably have the root address book directory be a singleton.  Future plans.
Attachment #543782 - Attachment is obsolete: true
Attachment #543782 - Flags: review?(mbanner)
Attachment #543833 - Flags: review?(mbanner)
Comment on attachment 543833 [details] [diff] [review]
Patch v6

That looks better. I'll ask bienvenu to double-check the import changes tomorrow, but I think this can land as is for now given that the unit tests show up clear.
Attachment #543833 - Flags: review?(mbanner) → review+
Checked in: http://hg.mozilla.org/comm-central/rev/e3f147cb26aa
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 7.0
Blocks: 672830
No longer blocks: 672830
Depends on: 672830
Depends on: 680826
Blocks: 692392
You need to log in before you can comment on or make changes to this bug.