424567 - Fix nsIAbDirectory::URI and improve nsAbMDBDirectory's IsMailList member handling

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

Joey commented to me that nsIAbDirectory::URI returned null for mailing lists obtained via the expected route. This isn't good, turns out that we always try and get the URI from the preference value - which doesn't apply to mailing lists.

The good bit is, that for mailing lists, they are already correctly initialised with the appropriate URI, so all we have to do in the mailing list case is return the stored URI.

In digging around, I also noticed that nsAbMDBDirectory's implementation for handling "IsMailList" sucks. It uses a second variable, and tries to use that for working out if it is initialized or not. The better option is to just try and GetAbDatabase() and if that finds it hasn't got an mURI, error out there. This means we can drop nsAbMDBDirectory's mIsMailingList flag and use nsAbDirectory's version instead.

Comment 1

[Mark Banner (:standard8)]

Attachment: The fix

The fix, see also comment 0. Note that some of the xpcshell test wouldn't work properly (not related to this bug), so I raised bug 424570 to cover that.

Comment 2

[[e-mail]]

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

>+  nsCAutoString uri(aUri);
Could this be an nsDependentCString?

>+  m_IsMailList = (uri.Find("MailList") == -1) ? 0 : 1;
m_IsMailList = uri.Find("MailList") != -1;
or, as it defaults to false,
if (uri.Find("MailList") != -1)
  m_IsMailList = PR_TRUE;

>+  if (mURI.IsEmpty())
>+    return NS_ERROR_NOT_INITIALIZED;
>+
>+  if (m_IsMailList)
>+  {
>+    aURI = mURI;
>+    return NS_OK;
>+  }
When would mURI be incorrect?

>-  if (mIsMailingList == 1)
>+  if (m_IsMailList == 1)
Don't compare a PRBool to 1. Just use if (m_IsMailList) [twice]

>+    var rdf = Components.classes["@mozilla.org/rdf/rdf-service;1"]
>+                        .getService(Components.interfaces.nsIRDFService);
>+    do_check_true(rdf != null);
getService throws rather than returning null.

>+    var AB = rdf.GetResource(kPABData.URI)
>+                .QueryInterface(Components.interfaces.nsIAbDirectory);
>+    do_check_true(AB != null);
As does QueryInterface.

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> >+  if (mURI.IsEmpty())
> >+    return NS_ERROR_NOT_INITIALIZED;
> >+
> >+  if (m_IsMailList)
> >+  {
> >+    aURI = mURI;
> >+    return NS_OK;
> >+  }
> When would mURI be incorrect?

When you've created an instance of the directory not using rdf, and/or not called Init().

Comment 4

[Mark Banner (:standard8)]

Attachment: The fix v2

Addresses comments (but see also comment 3).

Comment 5

[[e-mail]]

(In reply to comment #3)
>(In reply to comment #2)
>>>+  if (mURI.IsEmpty())
>>>+    return NS_ERROR_NOT_INITIALIZED;
>>>+
>>>+  if (m_IsMailList)
>>>+  {
>>>+    aURI = mURI;
>>>+    return NS_OK;
>>>+  }
>>When would mURI be incorrect?
>When you've created an instance of the directory not using rdf, and/or not
>called Init().
Sorry, I'm not following - what would mURI be in this case, if not empty?

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> Sorry, I'm not following - what would mURI be in this case, if not empty?

I think I've got what you mean now. If mURI is not emtpy, and its a directory, then the mURI will be the rdf uri, so we therefore don't need to use the uri preference or construct it ourselves... hence looks like we can simplify the GetURI functions we currently have.

Comment 7

[Mark Banner (:standard8)]

Attachment: The fix v3

Revised patch. This one just returns mURI in either the mail list or non-mail list case. Unfortunately we can't make the GetURI function central, as the inheritance structure doesn't allow that.

Additionally, the GetURI job could be performed by nsIRDFResource::GetValue*, however I don't want to do that just yet (as it will probably involve extra QIs), and I want us to be able to hide RDF a bit until we finish the re-factoring of the AB interfaces.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 312897 [details] [diff] [review]
The fix v3

See comment 7. I've just had this checked on Mac and it seems to work fine, so I think its time to push this on through.

Comment 9

[[e-mail]]

Comment on attachment 312897 [details] [diff] [review]
The fix v3

Compiles on Windows too ;-)

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 312897 [details] [diff] [review]
The fix v3

This will help jminta's non-rdf tree implementation for address books...

Comment 11

[Mark Banner (:standard8)]

Attachment: The fix v4

Revised patch per Dan's comment on irc - dropped the testnum and try/check parts of the unit test.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 313361 [details] [diff] [review]
The fix v4

Mmmm, good stuff.  Nice cleanup, Mark!

>+  // XXX Bug 424570 means that these tests don't work correctly (crash on
>+  // exit due to cycle collector) - no actual problems when running with
>+  // full app.

Add a note to bug 424570 that parts 3 and 4 should be uncommented as part of the patch to that bug?
 
sr=dmose

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #12)
> (From update of attachment 313361 [details] [diff] [review])
> Mmmm, good stuff.  Nice cleanup, Mark!

Patch checked in

> Add a note to bug 424570 that parts 3 and 4 should be uncommented as part of
> the patch to that bug?

Comment added

Fixed.