1626167 - Reset mail.collect_addressbook pref when the address book it points to is deleted

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Geoff Lankow (:darktrojan)]

From bug 1625650 comment 2:

Side note: for this crash to happen the user would have to have an invalid value for mail.collect_addressbook, probably an address book that's been deleted. Should we reset that pref if the directory it points to is deleted?

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1626167-addrbook-manager-improvements-1.diff

This patch suffered from scope creep. I fixed the original problem, then I realised that the two built-in directories could be deleted which should never happen. When writing a test for these I remembered that it's possible to throw more user-friendly exceptions than just Cr.NS_ERROR_FAILURE, so I changed some of those.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9137672 [details] [diff] [review]
1626167-addrbook-manager-improvements-1.diff

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

That is indeed nicer exceptions. r=mkmelin

::: mailnews/addrbook/jsaddrbook/AddrBookManager.jsm
@@ +178,5 @@
>    },
>    getDirectory(uri) {
>      if (uri.startsWith("moz-abdirectory://")) {
> +      throw new Components.Exception(
> +        "The root address book no longer exists",

It think it's nice to also add the uri to the message like

`The address book does not exist; uri=${uri}`

@@ +226,5 @@
>      }
>  
>      if (!dirName) {
> +      throw new Components.Exception(
> +        "Name must be specified",

maybe dirName instead of Name?

@@ +233,1 @@
>      }

I would move this whole if clause up.

Comment 3

[Geoff Lankow (:darktrojan)]

(In reply to Magnus Melin [:mkmelin] from comment #2)

It think it's nice to also add the uri to the message like

The address book does not exist; uri=${uri}

I don't think that's helpful here, uri would always be either "moz-abdirectory://" or the same with a search query on the end. That is, if we ever hit this code, and I hope we don't.

I would move this whole if clause up.

It's already at the top of the function it's in, logically at least. There's only a function definition above it.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f30adb6e9f15
Several improvements in address book manager. r=mkmelin