1576525 - Use JS address book provider for the default address books in new profiles

Status: RESOLVED FIXED

(MailNews Core :: Address Book, task)

Description

[Geoff Lankow (:darktrojan)]

A new profile has abook.mab and history.mab created by default. That needs to be changed to the new provider.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1576525-jsaddrbook-default-1.diff

Almost there. I've got it down to one failing test. Outstanding issues:

• Mailing lists inside mailing lists. (This is the failing test.) I don't know what I'm going to do about this. It seems like an odd thing to want to do but you can be some of our users do it.
• I've just ignored the import code for now. Too messy.
• If we land this, existing profiles get messed up. I've only just realised this so I'm unsure of the extent, but I think what will happen is new default address books will be created and the existing ones will disappear. I need to somehow work out if the default books already exist and adjust the prefs before the code reads them.

Comment 2

[Geoff Lankow (:darktrojan)]

• Modifying the addressLists array does nothing. I don't intend to fix this, because it was a stupid idea in the first place. This property will be removed as soon as I start modifying interfaces.

I might break off some of the bits of this patch and land them separately because they should be fixed even while other issues remain unresolved. Someone out there might actually be using this stuff!

Comment 3

[Jorg K (CEST = GMT+2)]

Mailing lists inside mailing lists.

That's an enterprise feature contributed by the folks from the French government in bug 1287726.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9090573 [details] [diff] [review]
1576525-jsaddrbook-default-1.diff

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

It might be easier to get so far that you can just do the one time migration of existing data before landing.

::: mail/components/addrbook/content/abTrees.js
@@ +44,5 @@
>      if (aDir._directory instanceof Ci.nsIAbMDBDirectory) {
>        return "mork";
>      }
> +    if (aDir._directory.dirType == DIRTYPE_JS) {
> +      return "js";

related to this, maybe it should be some other string, like sqlite?
Any future addrbooks are likely to be js but storage could be different

::: mailnews/addrbook/jsaddrbook/AddrBookDirectory.jsm
@@ +107,5 @@
>        }
> +      // Make sure we always have a file.
> +      let file = FileUtils.getFile("ProfD", [filename]);
> +      if (!file.exists()) {
> +        this._dbConnection;

Why is this needed?

@@ +114,1 @@
>        this.UID;

I suppose that triggers the generation. But is it needed here, can't it happen when it's needed?. Good to have a comment about it.

::: mailnews/addrbook/src/nsAbAddressCollector.h
@@ +26,5 @@
>    nsresult Init();
>  
>   private:
>    virtual ~nsAbAddressCollector();
> +  already_AddRefed<nsIAbCard> GetCardForAddress(const char *aProperty,

const nsCString & maybe?

::: mailnews/test/resources/abSetup.js
@@ +48,5 @@
>  // This currently applies to all address books of local type.
>  var kNormalPropertiesURI =
>    "chrome://messenger/content/addressbook/abAddressBookNameDialog.xul";
> +
> +function loadABFile(source, dest) {

please add jsdoc

Comment 5

[Geoff Lankow (:darktrojan)]

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

• if (aDir._directory.dirType == DIRTYPE_JS) {

•  return "js";
  

related to this, maybe it should be some other string, like sqlite?
Any future addrbooks are likely to be js but storage could be different

It really doesn't matter, this is only used by abSort for sorting and can be changed at any time as long as the value in AB_ORDER is also changed.

•  // Make sure we always have a file.
  

•  let file = FileUtils.getFile("ProfD", [filename]);
  

•  if (!file.exists()) {
  

•    this._dbConnection;
  

Why is this needed?

The file must be present or the next created address book gets the same file name, and bad things happen. For users that's unlikely but in tests it's terrible.

@@ +114,1 @@

   this.UID;

I suppose that triggers the generation. But is it needed here, can't it
happen when it's needed?. Good to have a comment about it.

It was put there to solve a problem, but I can't remember exactly what the problem was. I think it was something to do with the notification sent when the book is created, and needing a UID before the notification happened. I'll check if it's still needed next time I'm working on this patch.

• already_AddRefed<nsIAbCard> GetCardForAddress(const char *aProperty,

const nsCString & maybe?

I just went with the same type as the function I'm calling uses. And the argument I'm passing, for that matter.

Comment 6

[Magnus Melin [:mkmelin]]

(In reply to Geoff Lankow (:darktrojan) from comment #5)

Why is this needed?

I was tinking more about the "this._dbConnection". When it does setup, maybe have a method to do that instead if the getter, so it's not so confusing. You don't expect a getter to set stuff up really.

Anway, please add some code comments about these.

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: 1576525-jsaddrbook-default-2.diff

Comment 8

[Geoff Lankow (:darktrojan)]

SeaMonkey people, this will affect you.

The most important bit is that the default address book will go from being moz-abmdbdirectory://abook.mab to jsaddrbook://abook.sqlite (similar for history.mab) but both are still possible, so you should check your code for places the old value is hard-coded.

To prevent upsetting existing profiles I'm temporarily reverting the pref changes in mail/base/modules/MailMigrator.jsm. That will go away once migration is ready. Whether you do the same now or wait is up to you.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9093192 [details] [diff] [review]
1576525-jsaddrbook-default-2.diff

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

For the sql files, add a comment on top of the file to explain what the file is.

::: mailnews/addrbook/jsaddrbook/AddrBookCard.jsm
@@ +169,5 @@
>    },
>    getPropertyAsUint32(name) {
> +    let value = this.getProperty(name);
> +    if (isNaN(parseInt(value, 10))) {
> +      throw Cr.NS_ERROR_NOT_AVAILABLE;

Cr.NS_ERROR_ERROR_UNEXPECTED for these?

@@ +215,5 @@
>      }
>      return false;
>    },
>    translateTo(type) {
> +    return "NS_ERROR_NOT_IMPLEMENTED";

why stop throwing here?

Comment 10

[Geoff Lankow (:darktrojan)]

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

Cr.NS_ERROR_ERROR_UNEXPECTED for these?

The interface expects NS_ERROR_NOT_AVAILABLE.

why stop throwing here?

That appears to be a mistake.

Comment 11

[Geoff Lankow (:darktrojan)]

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

For the sql files, add a comment on top of the file to explain what the file
is.

-- This file is converted to xyz.sqlite for use in tests.

Or did you have something else in mind?

Comment 12

[Magnus Melin [:mkmelin]]

Depends on the file, but perhaps something like "This file is used to add test data used especially for address book list expansion."

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/23362b85a4fa
Use JS address book provider for the default address books in new profiles; r=mkmelin

Comment 14

[Jorg K (CEST = GMT+2)]

This asserts in debug mode now:
TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/xpcshell/test_ext_addressBook.js | xpcshell return code: 1
PID 2547 | Assertion failure: threadOpenedOn == NS_GetCurrentThread(), at /builds/worker/workspace/build/src/storage/mozStorageConnection.cpp:993
PROCESS-CRASH | comm/mail/components/extensions/test/xpcshell/test_ext_addressBook.js | application crashed [@ mozilla::storage::Connection::synchronousClose()]
TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/xpcshell/test_ext_messages_query.js | xpcshell return code: 1
PID 2550 | Assertion failure: threadOpenedOn == NS_GetCurrentThread(), at /builds/worker/workspace/build/src/storage/mozStorageConnection.cpp:993
PROCESS-CRASH | comm/mail/components/extensions/test/xpcshell/test_ext_messages_query.js | application crashed [@ mozilla::storage::Connection::synchronousClose()]

Empty M-C range
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange3d02a4c69a81b5e64cb05b053975d35554=&tochange=3d02a4c69a81b5e64cb05b053975d35554
so we broke it ourselves in this push:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=14db01bcabd2197e6cdb86b3bcde3b7246a7ab22

Since I already backed out bug 1581909, it must come from here.

Comment 15

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/fc23924872465f081c982627b664258eed5537ec
Backed out changeset 23362b85a4fa (bug 1576525) for test failures. a=backout

Sorry, I didn't find any joy in backing out a 100KB patch.

Comment 16

[Geoff Lankow (:darktrojan)]

I really should've seen that coming. I'll push a fixed version at the next merge.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/aa8107b682ba
Use JS address book provider for the default address books in new profiles; r=mkmelin

Comment 18

[Frank-Rainer Grahl (:frg)]

Geoff, thanks for the notification. Filed bug 1582988 for suite.