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

RESOLVED FIXED

Status

MailNews Core
Address Book
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Updated

10 years ago
Blocks: 424570
(Assignee)

Comment 1

10 years ago
Created attachment 311180 [details] [diff] [review]
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.
Attachment #311180 - Flags: superreview?(neil)
Attachment #311180 - Flags: review?(neil)

Comment 2

10 years ago
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.
(Assignee)

Comment 3

10 years ago
(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().
(Assignee)

Comment 4

10 years ago
Created attachment 311193 [details] [diff] [review]
The fix v2

Addresses comments (but see also comment 3).
Attachment #311180 - Attachment is obsolete: true
Attachment #311193 - Flags: superreview?(neil)
Attachment #311193 - Flags: review?(neil)
Attachment #311180 - Flags: superreview?(neil)
Attachment #311180 - Flags: review?(neil)

Comment 5

10 years ago
(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?
(Assignee)

Comment 6

10 years ago
(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.
(Assignee)

Updated

10 years ago
Attachment #311193 - Attachment is obsolete: true
Attachment #311193 - Flags: superreview?(neil)
Attachment #311193 - Flags: review?(neil)
(Assignee)

Updated

10 years ago
Depends on: 426317
(Assignee)

Comment 7

10 years ago
Created attachment 312897 [details] [diff] [review]
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.
(Assignee)

Comment 8

10 years ago
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.
Attachment #312897 - Flags: review?(neil)

Comment 9

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

Compiles on Windows too ;-)
Attachment #312897 - Flags: review?(neil) → review+
(Assignee)

Comment 10

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

This will help jminta's non-rdf tree implementation for address books...
Attachment #312897 - Flags: superreview?(dmose)
(Assignee)

Comment 11

10 years ago
Created attachment 313361 [details] [diff] [review]
The fix v4

Revised patch per Dan's comment on irc - dropped the testnum and try/check parts of the unit test.
Attachment #312897 - Attachment is obsolete: true
Attachment #313361 - Flags: superreview?(dmose)
Attachment #313361 - Flags: review+
Attachment #312897 - Flags: superreview?(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
Attachment #313361 - Flags: superreview?(dmose) → superreview+
(Assignee)

Comment 13

10 years ago
(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.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.