441437 - Provide a non-rdf way to go from a URI to a msgFolder

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Joey Minta]

Attachment: patch v1

This is #1 in http://wiki.mozilla.org/Thunderbird:Thoughts_on_Removing_RDF

The attached patch provides a folder-lookup service that handles going from a uri to folder without touching rdf.  I've also changed two callers (1 js and 1 c++ so I could convince myself I could do it.)  Because the js front-end work, this one change means the vast majority of front-side calls will now avoid the rdf service.  There are a few more odds and ends we'll need to track down (but I'm trying to avoid bitrotting any other pending patches).  There are a ton of c++ callers that should be switched over as well, but this can be done piecemeal once this service lands.

Comment 1

[David :Bienvenu]

Comment on attachment 326418 [details] [diff] [review]
patch v1

> Returns a folder with the given id, or null if the folder does not exist.

that's not quite right, since we create the folder if it doesn't exist (I'd be interested in exploring changing that). For now, perhaps say returns null if we fail to create a non-existing folder.

I worry a bit about c++ code calling this js service frequently since it will be significantly slower to call from c++ to js - maybe there are no performance critical callers of RDF in the c++ code, but it concerns me a little.

I'm also a little worried about the folder discovery process, e.g., nsMsgLocalMailFolder::CreateSubFolders - this uses rdf currently, and if you have 100 local folders, it will make 100 synchronous calls at startup to RDF.

Similarly, I don't think you want to listen for all events:

session.AddFolderListener(this, Ci.nsIFolderListener.all);

because we'll call into js from c++ for all sorts of operations that you aren't interested in. You're only interested in "added" and "removed".  Even so, you'll get notified about every header that's added or removed as well. "added" could be naturally taken care of because we always notify rdf when a folder is added, so we could call getFolderById to do the add.  I wouldn't be against adding a method to the folder lookup service to tell it to remove folders as well.

Comment 2

[Joey Minta]

Turns out this patch breaks because we don't get notified of "folder" creation when new accounts are created.  Nonetheless, selecting a server in the folder pane expects to be able to get the root folder for that account.  This is annoying, but I think we're going to need another listener?

Comment 3

[Joey Minta]

Attachment: patch v2

This patch fixes the above problem by adding a listener for account creation.  It *really* makes me wish STEEL were available.

Comment 4

[Joey Minta]

Reminder to self: This patch probably also needs a packaging change to not break windows installers.

Comment 5

[David :Bienvenu]

+    session.AddFolderListener(this, Ci.nsIFolderListener.all);

this really should just be 
+    session.AddFolderListener(this, Ci.nsIFolderListener.added|Ci.nsIFolderListener.removed);

since you're not doing anything with the other notifications; no sense asking for the other notifications.

Seems like creating an account should cause one or more itemAdded notifications. I can look at that.

Comment 6

[Joey Minta]

Attachment: patch v3

This fixes the listener arguments, and I've confirmed that no itemAdded or itemEvent is fired during account creation.

Comment 7

[David :Bienvenu]

I believe this new js component needs to get added to all the relevant package files in order not to break builds - see http://mxr.mozilla.org/comm-central/search?string=nsLDAPPrefsService.js for an example. Mark will correct me if I'm wrong :-)

Comment 8

[David :Bienvenu]

Comment on attachment 336840 [details] [diff] [review]
patch v3

this patch is also missing the idl file.

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #7)
> I believe this new js component needs to get added to all the relevant package
> files in order not to break builds - see
> http://mxr.mozilla.org/comm-central/search?string=nsLDAPPrefsService.js for an
> example. Mark will correct me if I'm wrong :-)

That is correct.

+EXTRA_COMPONENTS = folderLookupService.js

Please can you make this:

EXTRA_COMPONENTS = \
<tab><tab>folderLookupService.js \
<tab><tab>$(NULL)

so that when we make more files into javascript components we can just add new lines rather than re-arranging existing ones.

My preference is also to put the xpcom registration stuff at the top of the class, because then if you're looking for the contractID it can be found easily.

Comment 10

[David :Bienvenu]

Comment on attachment 336840 [details] [diff] [review]
patch v3

minusing based on review comments.

Comment 11

[Joey Minta]

Attachment: patch v4 [Checkin: See comment 29]

I'm not a fan of having the xpcom stuff at the top, since I'm more often opening files to see what they do, not to look for contracts, but I've gone ahead and moved it anyway. This patch also includes the idl, and changes the package files.

Comment 12

[David :Bienvenu]

Comment on attachment 337044 [details] [diff] [review]
patch v4
[Checkin: See comment 29]

thx, jminta.

Comment 13

[Joey Minta]

Checked in changeset 265:d6e5d976103e. There will be follow-up bugs to get more people to use this, instead of the rdf service.

Comment 14

[[e-mail]]

So, supposing I create an IMAP account using the new account wizard, this will mean that it will do folder discovery almost straight away before I've had a chance to switch IMAP delete model...

Comment 15

[David :Bienvenu]

Neil, can you elaborate on how this change is related to that statement? 

In theory, with the new account wizard, you could go straight to advanced settings from the wizard to turn on imap delete model before we ever select the inbox and go through folder discovery.

Comment 16

[Joshua Cranmer [:jcranmer]]

Per discussion on IRC, this was backed out.

Comment 17

[David :Bienvenu]

I've relanded the idl part of this patch, since it doesn't actually affect anything unless the folder lookup service is used. I'm going to try to land the lookup service itself next, once I can get the tweak to allow SM to use RDF reviewed.

Comment 18

[David :Bienvenu]

Attachment: make the folder lookup service use rdf for SM, for now

Neil, are you ok with doing this for SM, while TB moves on to a non-rdf folder pane, and SM decides what it wants to do in this area? This way I can start landing backend code that uses the folder lookup service, and it will be functionally equivalent, for SM, to what we have today.

I ifdeffed only a small part of the folder lookup service, because none of the other code will get called for SM, but I could ifdef more if that makes more sense.

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 344953 [details] [diff] [review]
make the folder lookup service use rdf for SM, for now

>diff --git a/mail/installer/windows/packages-static b/mail/installer/windows/packages-static
>--- a/mail/installer/windows/packages-static
>+++ b/mail/installer/windows/packages-static
...
>+bin\components\folderLookupService.js

You need to do the same for the suite files.

>+  _map: null,
>+  _sessionAdded: false,
>+
>+  _buildMap: function fls_buildmap() {
...
>+    var acctMgr = Cc["@mozilla.org/messenger/account-manager;1"].
>+                  getService(Ci.nsIMsgAccountManager);
>+    var count = acctMgr.accounts.Count();
>+
>+    var accounts = new Array();
>+    for (var i = 0; i < count; i++) {
>+      var acct = acctMgr.accounts.GetElementAt(i).QueryInterface(Ci.nsIMsgAccount);

This is going to cause us to go across the xpcom boundary for each loop. Although the number of accounts is small, I'm not sure that's a good idea.

Isn't there also a queryElementAt that can be used here?

>+    if (!this._sessionAdded) {
>+      var session = Cc["@mozilla.org/messenger/services/session;1"].
>+                    getService(Ci.nsIMsgMailSession);
>+      session.AddFolderListener(this, Ci.nsIFolderListener.added|Ci.nsIFolderListener.removed);
>+      this._sessionAdded = true;
>+    }
>+
>+    // See the notes below on the observe method
>+    var ps = Cc["@mozilla.org/preferences-service;1"].
>+             getService(Components.interfaces.nsIPrefService);
>+    var branch = ps.getBranch("").QueryInterface(Ci.nsIPrefBranch2);
>+    branch.addObserver("mail.accountmanager.", this, false);

There's some inconsistency here - _sessionAdded seems to be protecting against multiple calls, but the pref branch observer doesn't care. 

I'm also concerned about leakage - we're not removing these listeners on shutdown (and they aren't weak). However I don't see a leak in the tests that I did (which was to load a folder through the server. Maybe Neil has an idea here.

>diff --git a/mailnews/makefiles.sh b/mailnews/makefiles.sh
>--- a/mailnews/makefiles.sh
>+++ b/mailnews/makefiles.sh
>@@ -48,16 +48,17 @@ mailnews/base/public/Makefile
> mailnews/base/public/Makefile
> mailnews/base/src/Makefile
> mailnews/base/util/Makefile
> mailnews/base/search/Makefile
> mailnews/base/search/public/Makefile
> mailnews/base/search/src/Makefile
> mailnews/build/Makefile
> mailnews/db/Makefile
>+mailnews/db/gloda/Makefile

This doesn't belong here.

Comment 20

[David :Bienvenu]

Attachment: patch addressing Standard8's comments

this unregisters our listeners on xpcom shutdown, and addresses your other comments as well.

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 345140 [details] [diff] [review]
patch addressing Standard8's comments

>+    var acctMgr = Cc["@mozilla.org/messenger/account-manager;1"].
>+                  getService(Ci.nsIMsgAccountManager);

Please put the . on the start of the following line, not the end of the line.

>+    let acctMgrAccounts = acctMgr.accounts;
>+    var count = acctMgrAccounts.Count();
>+
>+    var accounts = new Array();

This seems to be unused.

>+    if (!this._sessionAdded) {
>+      var session = Cc["@mozilla.org/messenger/services/session;1"].
>+                    getService(Ci.nsIMsgMailSession);
>+      session.AddFolderListener(this, Ci.nsIFolderListener.added|Ci.nsIFolderListener.removed);
>+      // See the notes below on the observe method
>+      var ps = Cc["@mozilla.org/preferences-service;1"].
>+               getService(Components.interfaces.nsIPrefService);
>+      var branch = ps.getBranch("").QueryInterface(Ci.nsIPrefBranch2);

This can just be Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch2);

no need for the intermediate step.

>+      branch.addObserver("mail.accountmanager.", this, false);
>+      var obsSvc = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>+      obsSvc.addObserver(this, "xpcom-shutdown", false);
>+      this._sessionAdded = true;
>+    }

nit: the dots need moving again.

nit: Please add a blank line in between the session/prefs sections and the prefs/observer sections

nit: I think we could drop the intermediate variables and just do the steps all in one go.

>+
>+  },
>+  cleanup: function() {
>+    if (this._sessionAdded)
>+    {
>+      var session = Cc["@mozilla.org/messenger/services/session;1"].
>+                    getService(Ci.nsIMsgMailSession);
>+      session.RemoveFolderListener(this);
>+      var ps = Cc["@mozilla.org/preferences-service;1"].
>+               getService(Components.interfaces.nsIPrefService);
>+      var branch = ps.getBranch("").QueryInterface(Ci.nsIPrefBranch2);
>+      branch.removeObserver("mail.accountmanager.", this);
>+      var obsSvc = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>+      obsSvc.removeObserver(this, "xpcom-shutdown");
>+    }

Similar comments apply to this section.

Comment 22

[David :Bienvenu]

Attachment: patch addressing Standard8's comments

carrying forward Standard8's r+, address comments, and request sr from Neil.

Comment 23

[[e-mail]]

Comment on attachment 345293 [details] [diff] [review]
patch addressing Standard8's comments

>+var RDF = Components.classes["@mozilla.org/rdf/rdf-service;1"]
>+                          .getService(Components.interfaces.nsIRDFService);
Nit: .getService should line up with .classes

>+    // This additional creation functionality is provided to replicate legacy
>+    // rdf functionality.  Callers of this may expect that if the folder does
>+    // not exist, that we will create it for them.  Do so here.
Actually it doesn't replicate legacy RDF functionality; RDF doesn't add the "created" folder to the parent's child list. (Wasn't there a patch that I reviewed recently that achieved that?)

Speaking of functionality, can you achieve the functionality of this service via nsIMsgAccountManager::FindServerByURI and nsIMsgFolder::GetChildWithURI?

>+      // This is a HACK to work around bug 41133. If we have one of the
>+      // dummy "news" accounts there, that account won't have an
>+      // incomingServer attached to it, and everything will blow up.
>+      if (acct.incomingServer)
>+        addFolder(acct.incomingServer.rootMsgFolder);
Why not enumerate the incomingServer objects in the first place?

>+  // XXX Believe it or not, the simplest way to watch for new accounts is to
>+  // throw an observer on the pref-system.
<Meldrew>I don't believe it!</Meldrew> [this is a UK joke]
Has nobody heard of nsIIncomingServerListener?

Comment 24

[Joey Minta]

(In reply to comment #23)
> Has nobody heard of nsIIncomingServerListener?
The IDL says addIncomingServerListener is going away?

Comment 25

[[e-mail]]

(In reply to comment #24)
> (In reply to comment #23)
> > Has nobody heard of nsIIncomingServerListener?
> The IDL says addIncomingServerListener is going away?
I was thinking that root folder listeners don't work with deferred to accounts yet, but on second thoughts I don't think it matters in this case.

Comment 26

[David :Bienvenu]

It doesn't matter for deferred accounts, because they shouldn't show up in the folder pane.

Comment 27

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 345293 [details] [diff] [review]
patch addressing Standard8's comments

bienvenu's patch has unfortunately bitrotted:

$ patch -p1 --dry-run < ~/Desktop/attachment-6.cgi.txt 
patching file mail/installer/windows/packages-static
Hunk #1 FAILED at 96.
1 out of 1 hunk FAILED -- saving rejects to file mail/installer/windows/packages-static.rej
patching file mailnews/base/src/Makefile.in
Hunk #1 succeeded at 88 (offset 2 lines).
patching file mailnews/base/src/folderLookupService.js
patching file suite/installer/unix/packages
Hunk #1 FAILED at 494.
1 out of 1 hunk FAILED -- saving rejects to file suite/installer/unix/packages.rej
patching file suite/installer/windows/packages
Hunk #1 FAILED at 481.
1 out of 1 hunk FAILED -- saving rejects to file suite/installer/windows/packages.rej

Comment 28

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Nominating wanted-thunderbird3 since there are already patches that are r+/sr+/checked in/a bit more before they get approved.

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 337044 [details] [diff] [review]
patch v4
[Checkin: See comment 29]


(In reply to comment #13)
> Checked in changeset 265:d6e5d976103e.

http://hg.mozilla.org/comm-central/rev/d6e5d976103e

(In reply to comment #16)
> this was backed out.

http://hg.mozilla.org/comm-central/rev/e9e7311aaae4

(In reply to comment #17)
> I've relanded the idl part of this patch

http://hg.mozilla.org/comm-central/rev/746364a208e1
(Though hg log for the idl file reports it as d6e5d976103e only :-/)

Comment 30

[Serge Gautherie (:sgautherie)]

(In reply to comment #27)
> (From update of attachment 345293 [details] [diff] [review])
> bienvenu's patch has unfortunately bitrotted:

Fixing the "packages" files should be trivial:
David, can you do it?

Comment 31

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #30)
> (In reply to comment #27)
> > (From update of attachment 345293 [details] [diff] [review])
> > bienvenu's patch has unfortunately bitrotted:
> 
> Fixing the "packages" files should be trivial:
> David, can you do it?

Some files, like suite/installer/windows/packages and mail/installer/windows/packages-static, no longer exist.

Comment 32

[David :Bienvenu]

Comment on attachment 345293 [details] [diff] [review]
patch addressing Standard8's comments

unmarking obsolete - this should be revived

Comment 33

[Joshua Cranmer [:jcranmer]]

(I presume jminta is no longer working on this :-P).

For what it's worth, shoehorning folder lookup via this service should cut out ~50% of the remaining uses of the RDF service.

One thing which is almost certainly worth doing is to modify the interface to have a function which acts like the old rdf service did (create it if it didn't exist) and another function which returns it only if it already existed, and try to convince people to use the latter.

It's also worth asking if SeaMonkey would be acceptable having the same implementation that Thunderbird does (especially useful for the "don't create it!" case).

Comment 34

[Joshua Cranmer [:jcranmer]]

Attachment: Updated implementation

This is an updated version of attachment 345293 [details] [diff] [review] that uses Services.jsm and mailServices.js, works with the new JS component registration services.

I've also updated the patch to fix three other issues. The first is making the map hold weak references instead of strong references to folders, which brings it more in line with what RDF does (RDF actually uses raw references, but JS can't really do that; this uses weak references instead). I've also tweaked the call to addSubfolder to make it work with escaped folder names correctly. Finally, it turns out that addSubfolder doesn't necessarily force a call to OnItemAdded (that only happens when creating a new folder, not a new folder object for an extant folder).

In addition, I've implemented the "mayCreate" attribute as I mentioned in the previous comment.

As a test for this patch, I replaced the call to getResource in nsNewsFolder.cpp with the folder lookup service, and I found a few issues just by running NNTP tests (in particular, the escaped name issue).

Okay, now for the hard part: do we actually want to do things in this manner? The underlying model of how and when folders get constructed has always been mildly fuzzy, and magic RDF abilities let us sweep a lot of it under the mess. I started poking at some things to try switching over to the folder lookup service and immediately ran into critical questions which I feel need technical discussion first.

One option is to build a literal replacement of RDF's magic under the hood (i.e., build a weak map and, if not present, call createInstance + Init), but I don't think the RDF magic is particularly desirable.

If we don't want to go that route, we need to be explicit about several things:
1. Who actually creates the folder object for a given folder?
2. When are folder objects created--aggressively at startup(/new account creation), or lazily (as they are used)?
3. Is there an explicit format to the URIs of folders?
4. When we do "magic" creation of folders, are we allowed to create new folders or are we limiting ourselves to the creation of new folder objects?
5. Can we create folder objects that don't correspond to actual folders? If so, what operations can we do on them?

Comment 35

[[e-mail]]

Comment on attachment 653443 [details] [diff] [review]
Updated implementation

Seems reasonable, apart from the ifdefs.

What is the intent of the mayCreate parameter? (I can't remember whether addSubfolder creates it permanently, especially on IMAP.) In particular, I can't remember what happens with the copies & folders pickers, where it's possible to choose a standard folder (e.g. Templates) for an account which doesn't have a folder of that name yet.

Comment 36

[Kent James (:rkent)]

# HG changeset patch
# Parent 8e502ee509afd6ba6c7cb5dea67807f4a85988ef
mailnews/base/public/nsIFolderLookupService.idl
* When looking up folders by URL, note that the URL must be encoded to be a
* valid URL. Escaping can be done via nsINetUtil::escapeString(name,
* nsINetUtil::ESCAPE_URL_PATH).
*
* The contractid for this service is "@mozilla.org/mail/folder-lookup;1".
	
This should be added instead as a C++ section in the idl with a C++ constant

* Returns a folder with the given URL. If the folder does not exist, but an
* identifiable parent does, we'll create it and then return that folder. If
* no parent exists, this will return null.
*
* @param aUrl The URL of the folder to create.
	
An example here would be really helpful to demonstrate the expected format.
mailnews/base/src/folderLookupService.js

let parentURL = aId.substr(0, aId.lastIndexOf('/'));
let childName = aId.substr(parentURL.length + 1);
 
// If there isn't even a parent, just bail
if (!(parentURL in this._map)) {
	
This is a good example of the problems inherent in trying to do this function separate from the ownership of the folders themselves. Ideally, callers should not have to worry about the status of the parent folders, as long as the parents are valid. If this function was integrated into the folder ownership structure, you would simply create the parent recursively here. But as it is, you are forcing all of the callers to concern themselves with the status of the parents. I think this is going to be the root of a lot of future bugs.

// nsIMsgFolder::AddSubfolder escapes the child's name. The URL we have here
// uses the escaped version, so unescape the string so that we don't get
// double-escaped strings.
let nu = Cc["@mozilla.org/network/util;1"].getService(Ci.nsINetUtil);
let folder = parent.addSubfolder(nu.unescapeString(childName, 0));
	
Certain folders have URLs that have standardized forms in the URL independent of a possibly localized name. Junk is a good example of that. How do you handle those?

//
// XXX Believe it or not, the simplest way to watch for new accounts is to
// throw an observer on the pref-system. I wish STEEL existed so I could just
// watch that
	
Bug 592710 is trying to allow accounts to exist in the preferences, but not be loaded. This is to accommodate new account type extensions, where the account might appear invalid at startup because its server cannot be loaded. In that case, at each extension startup, there would be a startup sequence that has accountmanager.UnloadAccounts; accountmanager.LoadAccounts; which effectively adds the new account. We need a way to either interact with that method through this bug, or have a more explicit way to manage individual account loading. The account unloading/loading with its implicit adding of an account would occur without any change to mail.accountmanager.accounts

Comment 37

[Kent James (:rkent)]

I also intended to add the following overall comment. (Tried to do a splinter review but it had issues)

The horse may have already left the barn on this one, but I have mixed feelings about this being done in js instead of C++, and for it being done as a global service with its own map.

It seems to me that the natural ownership for the map would be that the server owns the folders (and their map), so that the lookup map would be added to nsIMsgIncomingServer.cpp  with a lookup method added to nsIMsgIncomingServer.idl (or as part of getMsgFolderFromURI) Then the general lookup method is added to nsIMsgAccountManager which parses the URL to find the server, then asks the server for the folder.

There is an implicit assumption that somehow there is a future Thunderbird code base that is all done in js. I think that is a pipe dream myself, and attempts to "improve" the code by implementing methods in js that more naturally belong in the C++ base only serve to complicate matters.

Comment 38

[Kent James (:rkent)]

(In reply to Joshua Cranmer [:jcranmer] from comment #34)
> If we don't want to go that route, we need to be explicit about several
> things:
> 1. Who actually creates the folder object for a given folder?

The incoming server object. This object created is determined by the type attribute in the URL for the server. This code could be added to nsMsgIncomingServer.cpp

> 2. When are folder objects created--aggressively at startup(/new account
> creation), or lazily (as they are used)?

Lazily if possible - which your map does not really allow, since you create all subfolders when you create the map

> 3. Is there an explicit format to the URIs of folders?

There needs to be, and it needs to be well documented. This has been the source of endless grief for me. Still have not fully solved the problem of mapping standardized URLs that differ from the name such as Junk.

> 4. When we do "magic" creation of folders, are we allowed to create new
> folders or are we limiting ourselves to the creation of new folder objects?

In my own code, every time I have allowed folder lookup to magically create folders, I have regretted it and it caused many bugs.

> 5. Can we create folder objects that don't correspond to actual folders? If
> so, what operations can we do on them?

I would 100% support the splitting of the nsIMsgFolder interface into a minimal version that would work in the UI but not derive from msMsgDBFolder with all of its complexity. But if you do this without formally defining the minimal interface you are creating new problems.

Comment 39

[Joshua Cranmer [:jcranmer]]

(In reply to [e-mail] from comment #35)
> What is the intent of the mayCreate parameter? (I can't remember whether
> addSubfolder creates it permanently, especially on IMAP.) In particular, I
> can't remember what happens with the copies & folders pickers, where it's
> possible to choose a standard folder (e.g. Templates) for an account which
> doesn't have a folder of that name yet.

The intent of mayCreate is to make it explicit whether or not the magical autocreation functionality of RDF is desired. The problem, of course, is that what exactly the magical autocreation functionality is meant to do isn't very clear. Which is why I asked the questions in comment 34.

I want to be really clear on one thing: the patch I posted is an updated version of the original patch. I don't particularly agree with the approach the patch takes. The problem is that to truly provide a good solution for this bug, we need to figure out the hand-waved details.

Comment 40

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #36)
> This is a good example of the problems inherent in trying to do this
> function separate from the ownership of the folders themselves. Ideally,
> callers should not have to worry about the status of the parent folders, as
> long as the parents are valid. If this function was integrated into the
> folder ownership structure, you would simply create the parent recursively
> here. But as it is, you are forcing all of the callers to concern themselves
> with the status of the parents. I think this is going to be the root of a
> lot of future bugs.

The entire thing about what happens in the case that mayCreate is true and the folder isn't in the map is extremely troubling to me. Only going one level deep is also troubling me.

The reason this patch was backed out when it first landed 4 years ago was because it broke several things. I'm unpersuaded that this architecture is even the one we want; since having the fix for this allows/requires us to change several uses of RDF getResource, I'd rather spend a lot of time finding the architecture that we want to live with in 10 years time rather than just closing our eyes and trying to patch up the current one.

> // nsIMsgFolder::AddSubfolder escapes the child's name. The URL we have here
> // uses the escaped version, so unescape the string so that we don't get
> // double-escaped strings.
> let nu = Cc["@mozilla.org/network/util;1"].getService(Ci.nsINetUtil);
> let folder = parent.addSubfolder(nu.unescapeString(childName, 0));
> 	
> Certain folders have URLs that have standardized forms in the URL
> independent of a possibly localized name. Junk is a good example of that.
> How do you handle those?

It's not an issue here. This is trying to make sure that we don't turn, e.g., news://news.example.com/foo%25bar into news://news.example.com/foo%2625bar.

> Bug 592710 is trying to allow accounts to exist in the preferences, but not
> be loaded. This is to accommodate new account type extensions, where the
> account might appear invalid at startup because its server cannot be loaded.
> In that case, at each extension startup, there would be a startup sequence
> that has accountmanager.UnloadAccounts; accountmanager.LoadAccounts; which
> effectively adds the new account. We need a way to either interact with that
> method through this bug, or have a more explicit way to manage individual
> account loading. The account unloading/loading with its implicit adding of
> an account would occur without any change to mail.accountmanager.accounts

My recollection of the issues here was that the account manager would happily clear all the extension accounts only when the extension failed to load, so I don't know that this is necessary to worry about yet.

(In reply to Kent James (:rkent) from comment #37)
> The horse may have already left the barn on this one, but I have mixed
> feelings about this being done in js instead of C++, and for it being done
> as a global service with its own map.

The need at the core of the issue is to be able to persistently refer to a folder. The unique identifier is currently the URL, and changing that is effectively impossible at this point.

> It seems to me that the natural ownership for the map would be that the
> server owns the folders (and their map), so that the lookup map would be
> added to nsIMsgIncomingServer.cpp  with a lookup method added to
> nsIMsgIncomingServer.idl (or as part of getMsgFolderFromURI) Then the
> general lookup method is added to nsIMsgAccountManager which parses the URL
> to find the server, then asks the server for the folder.

I would agree that the server needs to own the folders. I don't agree that the servers should also own the map.

> There is an implicit assumption that somehow there is a future Thunderbird
> code base that is all done in js. I think that is a pipe dream myself, and
> attempts to "improve" the code by implementing methods in js that more
> naturally belong in the C++ base only serve to complicate matters.

It's easier to build and use hashtables in JS than it is in C++. I believe that it's better to implement in JS if we're adding a new interface. And there are some things which probably work much better in JS--MIME and compose code would be simpler if parameters were just JS objects instead of compose's present 20-argument methods.


Let me also give my own answers to the questions I asked:
> 1. Who actually creates the folder object for a given folder?

The server.

> 2. When are folder objects created--aggressively at startup(/new account
> creation), or lazily (as they are used)?

Folder objects should be aggressively created, but automatically creating folders as a result (like IMAP's magic creation during discovery) is forbidden.

> 3. Is there an explicit format to the URIs of folders?

Yes and no. The URI follows the generic format, with the authority component dictating the server who uses the folder and the path component being specific to the server.

> 4. When we do "magic" creation of folders, are we allowed to create new
> folders or are we limiting ourselves to the creation of new folder objects?

Since folder objects are aggressively created, there should be no magic creation. People who want magic creation should go through the effort of explicitly creating the folder.

> 5. Can we create folder objects that don't correspond to actual folders? If
> so, what operations can we do on them?

No.

Comment 41

[Joshua Cranmer [:jcranmer]]

Attachment: Updated prototype

This is a prototype which basically has three methods:
1. Get the folder if it exists else null (nsMsgUtils's GetExistingFolder basically).
2. Create the folder asynchronously (GetOrCreateFolder)
3. Open the database asynchronously (no equivalent yet)

The second method isn't actually implemented yet because, well, that's very much a pain. This patch doesn't actually fulfill the title of the bug, because someone needs to use RDF and it's easier to stick it in the folder lookup method for now.

Comment 42

[Kent James (:rkent)]

I've spent more time than I should have getting up to speed on these issues again, but it is still hard to give a definitive response. Rather than ignore the request for feedback though, I'll just say what I can and move on.

I'm not enthusiastic about starting to add in async issues into this interface, as there are simple non-sync issues that need solving, and the async issues just complicate that. In a general sense, we need a way to ensure that only one copy of folder objects exist at a time, and a method to find those folders using a string URL. There are not async issues, and can be logically separated from methods that need async.

Shouldn't we be trying to solve the simple problem first?

In the existing folder ownership structure, the singleton accountManager owns the accounts, an account owns a server, a server owns its root folder, and the root folder (and its subfolders) own the subfolders (and their subfolders). So isn't the mission here to just provide a lookup service for this, that is a url map and method to keep it up to date? That structure is providing a unique copy of each folder instance, we just have to find it quickly.

So isn't the right approach just to provide a lookup hash structure, keeping it up to date with the existing folders?

I suppose that the complication is coming from what happens when getFolderByUrl is asked to return a folder object that does not exist. The long-term solution should be to simply disallow that, that is it returns null when the folder does not exist as part of the existing accountmanager/account/server/rootfolder/subfolders/(..subfolders) hierarchy of ownership. Actual folder creation should be done through an existing folder, with the server creating the root folder. We should locate any places that are prematurely getting folders before initialization has completed, and force them to wait, rather than complicate getFolderByUrl in a way that it can be called without proper initialization of a server.

In my code, these issues have occurred when either the calendar, address book, or an extension tries to get a folder by URL before server initialization is complete.

Comment 43

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #42)
> I've spent more time than I should have getting up to speed on these issues
> again, but it is still hard to give a definitive response. Rather than
> ignore the request for feedback though, I'll just say what I can and move on.
> 
> I'm not enthusiastic about starting to add in async issues into this
> interface, as there are simple non-sync issues that need solving, and the
> async issues just complicate that. In a general sense, we need a way to
> ensure that only one copy of folder objects exist at a time, and a method to
> find those folders using a string URL. There are not async issues, and can
> be logically separated from methods that need async.
> 
> Shouldn't we be trying to solve the simple problem first?

This was driven by the last conversation we had in tb-planning about this; part of this was attempting to design the API we wanted here.

> So isn't the right approach just to provide a lookup hash structure, keeping
> it up to date with the existing folders?
> 
> I suppose that the complication is coming from what happens when
> getFolderByUrl is asked to return a folder object that does not exist.

There's really two complications to me, which are kind of the same thing. One is what you ask, and the other is how do we fill the map in the first place--do we have folder creation callback to the API to initialize it or do we have the API fallback to trying to construct the folder itself. They are kind of the same in that both really boil down to the fact that folders can be in three states:

1. The folder exists, and we have an object for that folder.
2. The folder exists (on the disk), but we don't have an object for that folder.
3. The folder doesn't exist, but we we want an object for that folder.

The first state is by far the most common state (excluding account creation, all folders should be in that state once the folder tree is loaded). However, most of the attempts to implement this patch have gotten hung up on "what do we do for #2?", which results in some discussion, vacillation, and then dropping the patch on the floor.

> The long-term solution should be to simply disallow that, that is it returns
> null when the folder does not exist as part of the existing
> accountmanager/account/server/rootfolder/subfolders/(..subfolders) hierarchy
> of ownership.

When I wrote the latest patch, my basic goal was to provide the API we want but use an implementation that worked within the realities of our current codebase. At this point, that is the only approach that I think is feasible for making any sort of progress forward on these issues...

Comment 44

[Kent James (:rkent)]

(In reply to Joshua Cranmer [:jcranmer] from comment #43)

> folders can be in three states:
> 
> 1. The folder exists, and we have an object for that folder.

Here the goal is an efficient hash-based lookup based on URL.

> 2. The folder exists (on the disk), but we don't have an object for that
> folder.

If the folder exists on the disk, then shouldn't it get picked up by GetSubfolders in folder initialization? Maybe what I have not made explicit is that the operation to actually create the folder object should exist IMHO in the folder (or server) C++, and is done as part of folder initialization. So the only time that I am aware of that folder objects do not exist for existing real folders occurs in the subfolder creation in the parent object (either the server for the root folder, or the parent folder for all other folders) and those are where explicit folder object creation should occur.

> 3. The folder doesn't exist, but we we want an object for that folder.

There are cases where we try to access a folder by URL that does not exist. For example, if you specified an fcc folder by url, then deleted that folder, then attempts to copy a sent message to that folder would encounter the issue.

In the current system, RDF creates a folder object without creating the underlying storage, and that is a source of grief. The change I would propose for the long run is for the lookup to return null in this case, and force the caller to deal with the folder creation through possible async calls to the parent folder. As an interim step, we could create a naked folder for backwards compatibility, perhaps with a warning to let us know it needs fixing.

> When I wrote the latest patch, my basic goal was to provide the API we want
> but use an implementation that worked within the realities of our current
> codebase. At this point, that is the only approach that I think is feasible
> for making any sort of progress forward on these issues...

I really think that the easiest way forward is to replace RDF with this proposed lookup and folder creation scheme. I think this is the correct long-term solution, as well as being relatively painless in the short term.

On the issue of the map, it would be populated by some sort of notification from the initial folder object creation process in the server or folder C++. The map would need explicit XPCOM methods to add and remove folders from the map, as well as a lookup function.

Comment 45

[Joshua Cranmer [:jcranmer]]

So I make sure we're on the same page here:

In this bug, I'll implement nsIFolderLookupService::getFolderByUrl by a hash map of URL -> weak reference to folder. This method will return the correct result only for folders in the first state. For folders in states 2 and 3, this method will return null--users who don't want this feature should go use other APIs. The API we'll want, to be implemented in a future patch, is a createFolderFromUrl that asynchronously returns the folder creation.

That's what the users see about the lookup service from the API. From the implementation perspective, it sounds like you disagree with my "use RDF for now" approach. My previous patch had folders register themselves explicitly in nsMsgDBFolder::Init and implicitly unregister themselves when they lose all references (by the weak reference going null). This scheme does have some consequences:
1. If someone uses RDF.getResource() on a folder URL that doesn't exist, it will end up getting registered in the map, since nsMsgDBFolder::Init is still called. However, if the folder doesn't end up being created, its refcount should go back down to 0 and get killed off immediately (if called from C++) or by the next GC (if called from JS).
2. Leaking folders that are deleted don't get removed. This is also the case with RDF.getResource right now, but I haven't seen evidence that it's a problem.

Does this sound like an acceptable solution?

Comment 46

[Kent James (:rkent)]

(In reply to Joshua Cranmer [:jcranmer] from comment #45)
> So I make sure we're on the same page here:
> 
> In this bug, I'll implement nsIFolderLookupService::getFolderByUrl by a hash
> map of URL -> weak reference to folder. This method will return the correct
> result only for folders in the first state. For folders in states 2 and 3,
> this method will return null--users who don't want this feature should go
> use other APIs. The API we'll want, to be implemented in a future patch, is
> a createFolderFromUrl that asynchronously returns the folder creation.
>

Correct in the long term, but I think that we have the intermediate goal of landing something that does not break, and letting fixes be done in smaller patches. So for an initial goal, perhaps it would be better to: 1) use RDF but centralize its use to a one or two places in the server and folder C++, 2) for your cases 2) and 3) return for now the RDF-created bare folder object, with a glaring error message (console error?) so that we can know what places needed fixing. Eventually we would replace RDF in the server and folder with a simple C++ "folderObject = new nsMsg*Folder()" creation.

> That's what the users see about the lookup service from the API. From the
> implementation perspective, it sounds like you disagree with my "use RDF for
> now" approach. My previous patch had folders register themselves explicitly
> in nsMsgDBFolder::Init and implicitly unregister themselves when they lose
> all references (by the weak reference going null). This scheme does have
> some consequences:
> 1. If someone uses RDF.getResource() on a folder URL that doesn't exist, it
> will end up getting registered in the map, since nsMsgDBFolder::Init is
> still called. However, if the folder doesn't end up being created, its
> refcount should go back down to 0 and get killed off immediately (if called
> from C++) or by the next GC (if called from JS).
> 2. Leaking folders that are deleted don't get removed. This is also the case
> with RDF.getResource right now, but I haven't seen evidence that it's a
> problem.
>

I think that what you did there (using RDF sparingly) is what I would like to see in the short run, so I actually agree with sparingly using RDF now. My main objection has been to complicating the lookup to also try to solve the async create problem.

Comment 47

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #46)
> 2) for your cases 2) and 3) return for now the RDF-created bare folder
> object, with a glaring error message (console error?) so that we can know
> what places needed fixing.

There are places where we lookup a folder URI but want it to return null for cases 2 and 3 (e.g., MsgUtils's GetExistingFolder function). My plan was to keep RDF.getResource for cases where we're not sure and audit all uses one-at-a-time.

> Eventually we would replace RDF in the server and
> folder with a simple C++ "folderObject = new nsMsg*Folder()" creation.

That's a long ways off, since we still have some RDF-driven UI that I think expects folders to be able to QI to nsIRDFResource.

> I think that what you did there (using RDF sparingly) is what I would like
> to see in the short run, so I actually agree with sparingly using RDF now.
> My main objection has been to complicating the lookup to also try to solve
> the async create problem.

Duly noted. I'll prepare a new patch and upload it after 24 branches so you can work on your get-it-into-24 patches. :-)

Comment 48

[Joshua Cranmer [:jcranmer]]

Attachment: Add RDF-backed folder lookup

Comment 49

[Joshua Cranmer [:jcranmer]]

Attachment: Add RDF-backed folder lookup

Forgot to qref first.

Comment 50

[:aceman]

Is the patch name right? Wasn't the summary of the bug to create a non-RDF lookup?

Comment 51

[Kent James (:rkent)]

Just a brief request for comments:

In my own code lately, I've been implementing js-based components by first implementing the equivalent js module, then writing a small XPCOM glue component that implements the component for C++ callers. That way, the js callers can simply use the module form. This function would be a good use for that as well. Any comments about whether this is a good or bad idea?

The component code is then very simple, here is one example:

function nativeMachineComponent()
{
  Cu.import("resource://exquilla/nativeMachine.js");
  NativeMachine.call(this);
  this.__proto__ = NativeMachine.prototype;
}

nativeMachineComponent.prototype = {
  classID:          Components.ID("{35EDFFE3-8533-4bb1-81D2-DE2A6AC24892}"),
}

Comment 52

[Kent James (:rkent)]

Comment on attachment 774455 [details] [diff] [review]
Add RDF-backed folder lookup

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

I assume you will need a mail and suite reviewer to be official, which is a real pain for this kind of change.

::: mailnews/base/public/nsIFolderLookupService.idl
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsISupports.idl"
>  
> +interface nsIMsgDatabase;

Why do you need this? Please remove. I see no nsIMsgDatabase here, so this is unexpected.

@@ +12,5 @@
> + *
> + * When looking up folders by URL, note that the URL must be encoded to be a
> + * valid URL, particularly including percent-encoding if necessary.
> + *
> + * The contractid for this service is "@mozilla.org/mail/folder-lookup;1".

Can you include this as a C++ define instead, which is the more typical way this is documented? AFAIK there is no plan to eliminate that convention.

::: mailnews/base/util/nsMsgUtils.cpp
@@ +821,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  return fls->GetFolderForURL(aFolderURI, aFolder);

If you look at the callers for GetExistingFolder, you will see that they are very inconsistent in how they check for errors. There are several (for example http://mxr.mozilla.org/comm-aurora/source/mailnews/compose/src/nsMsgComposeService.cpp#1116) where the return rv is checked, and if OK then the folder is assumed to be valid and is then used. We have this inconsistent convention that a valid rv means a valid object is returned. The new folder lookup does not follow this convention, returning NS_OK with null folder. This is good for javascript, but bad for C++. By changing that here, you are rendering the anti-crash error checking of downstream code ineffective.

Since this is a C++ only routine, please put back in the previous code that returns an error if the folder is null.

I am doing the r- primarily for this reason, the others issues are nits.

Comment 53

[Joshua Cranmer [:jcranmer]]

Attachment: RDF-backed folder lookup

This should fix all the identified nits. Since the package manifests are only adding a new file, I don't think it needs mail/suite review, and I'm not really changing any APIs, so I don't think it needs superreview either.

Comment 54

[Kent James (:rkent)]

I made the following changes to test_imapPump.js so that I could test this thing, but it might be good to have a test that specifically tested this:

function loadImapMessage()
{
  let fls = Cc["@mozilla.org/mail/folder-lookup;1"]
              .getService(Ci.nsIFolderLookupService);
  do_check_true(fls instanceof Ci.nsIFolderLookupService);

  do_check_true(IMAPPump.inbox === fls.getFolderForURL(IMAPPump.inbox.URI));

  IMAPPump.mailbox.addMessage(new imapMessage(specForFileName(gMessage),
                          IMAPPump.mailbox.uidnext++, []));

Comment 55

[Kent James (:rkent)]

Comment on attachment 815232 [details] [diff] [review]
RDF-backed folder lookup

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

Treat this as a request for information so we can discuss.

::: mailnews/base/src/folderLookupService.js
@@ +40,5 @@
> +    let rdf = Cc["@mozilla.org/rdf/rdf-service;1"]
> +                .getService(Ci.nsIRDFService);
> +    let folder = rdf.GetResource(aUrl)
> +                    .QueryInterface(Ci.nsIMsgFolder);
> +    if (folder.parent == null)

As we mentioned on IRC, this will return null for a root folder. Even if existing code is unlikely to call this for a root folder, isn't this a trap for the unwary?
We need a plan for this to work for the root folder. I did not see any straightforward way to do this unfortunately. Any suggestions? One possibility would be to detect if this is a server URL, and if so ask account manager for the server, and ask the server for its root folder.

@@ +44,5 @@
> +    if (folder.parent == null)
> +      return null;
> +
> +    // Add the new folder to our map.
> +    this._map.set(aUrl, folder);

This needs to create a weak reference, as we discussed on IRC

Comment 56

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #55)
> As we mentioned on IRC, this will return null for a root folder. Even if
> existing code is unlikely to call this for a root folder, isn't this a trap
> for the unwary?

After some thought, I think that letting this call return a root folder is an even bigger trap. Root folders are not real folders in the sense that you can't do anything useful with them save traversal-ish functions. The primary purpose of URLs for folders are to be able to persistently refer to them (e.g., folder targets) without explicitly having an object on hand. For these use cases, server or account keys are better proxies than root folder URLs.

Comment 57

[Kent James (:rkent)]

Comment on attachment 815232 [details] [diff] [review]
RDF-backed folder lookup

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

Let me finish the review.

In reply to comment 56, I am not convinced. Inconsistencies in the code behavior are traps for the future, and need to be minimized particularly in a core routine like this. "This service provides a way to lookup any nsIMsgFolder" is the correct spec and should be implemented. If there was some important reason why this routine should not return a root folder, then it should be documented clearly. But root folders play an important role in mailnews code, and I cannot think of a good reason to disallow their use here. If you want to disagree further, you'll need a second opinion to back you.

As to how to add in the search for root folders, my first thought was to use AccountManager's FindServerByURI, but looking briefly at that code, it looks to me like the code relies on having the username in the URI, which we cannot assume here. So the method I would suggest would be to enumerate over all servers using the AccountManager, and try === of the returned RDF folder and the server's root folder.

As to testing, this is a really core routine, and it seems to me that a simple XPCSHELL test would be worthwhile, and not that difficult to do. You would need to test 1) rejection of folders created that do not exist, 2) successful lookup from the cache and not using RDF, 3) deletion from cache of a folder that has been deleted, and 4) lookup of the root folder to a server.

Also, the patch needs the weak reference put in that was left out in the current version.

Comment 58

[Joshua Cranmer [:jcranmer]]

Attachment: RDF-backed folder lookup

Comment 59

[Kent James (:rkent)]

Comment on attachment 8356817 [details] [diff] [review]
RDF-backed folder lookup

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

Although I am happy to look at this again, but issues reported in this review are striving for perfection rather than absolutely necessary. So I'll r+ this, but suggest that you ask for feedback before you land it with the suggested changes.

::: mailnews/base/public/nsIFolderLookupService.idl
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsISupports.idl"
>  
> +interface nsIMsgDatabase;

You don't need this, do you (nsIMsgDatabase)?

@@ +11,5 @@
> + * This service provides a way to lookup any nsIMsgFolder.
> + *
> + * When looking up folders by URL, note that the URL must be encoded to be a
> + * valid URL, particularly including percent-encoding if necessary.
> + *

This issue is actually really tricky, and perhaps an example here would be good. It is not as simple as "including percent-encoding".

Like here is what I am using, and I am still not sure if I am following the standard or not:

exquilla://[e-mail]/Inbox

That is, no trailing slash, encoding of "." in username but not in server DNS name.

::: mailnews/base/src/folderLookupService.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +

At the beginning of the module you need a brief description of what the module is supposed to do.

@@ +24,5 @@
> +  }
> +}
> +
> +function folderLookupService() {
> +  this._map = new Map();

It seems to me that _map should not be a property of an instance, but should instead be a global. If someone uses createInstance to create this (which I have accidentally done for services in the past) then they will get a unique map. In a non-RDF future, we really don't want that to happen.

@@ +29,5 @@
> +}
> +folderLookupService.prototype = {
> +  // XPCOM registration stuff
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIFolderLookupService]),
> +  classDescription: "Folder Lookup Service",

XPCOMUtils.jsm says of classDescription and contractID: "The following properties were used prior to Mozilla 2, but are no longer supported". The modern way to do this is with classInfo.

@@ +44,5 @@
> +        folder = this._map.get(aUrl).QueryReferent(Ci.nsIMsgFolder);
> +        valid = isValidFolder(folder);
> +      } catch (e) {
> +        // The object was deleted, so it's not valid
> +        valid = false;

This second "valid = false" is not needed, because you set a default value.

@@ +46,5 @@
> +      } catch (e) {
> +        // The object was deleted, so it's not valid
> +        valid = false;
> +      }
> +      dump("Lookup for " + aUrl + " found " + folder + " (" + valid + ")\n");

Please remove these 2 dumps. I don't think you want to be printing anything in this routine. Even an error should not generate a message IMHO, as it could be routine to check for an invalid folder prior to trying to create something.

@@ +68,5 @@
> +      folder = rdf.GetResource(aUrl)
> +                  .QueryInterface(Ci.nsIMsgFolder);
> +    }
> +    if (!isValidFolder(folder))
> +      return null;

In my own code at this point of failure, I log and remove the resource:
      EWS_LOG_INFO("QI to root folder failed, maybe this was incorrectly loaded earlier");
      rdf->UnregisterResource(serverResource);

Letting the folder stay in RDF at this point is problematic. Your method will report that the folder does not exist, but attempts to create it will fail since it is already defined in RDF. This can happen at startup when the order of initialization is off. I would suggest that you do the same thing here, Components.utils.reportError() and unregister.

::: mailnews/base/test/unit/test_folderLookupService.js
@@ +40,5 @@
> +  // Now delete the folder; we should be unable to find it
> +  root.propagateDelete(root.getChildNamed("Child"), true, null);
> +  do_check_eq(fls.getFolderForURL(kRootURI + "/Child"), null);
> +
> +  do_check_eq(fls.getFolderForURL(kRootURI + "/"), null);

Nice, checking for trailing slash!

Comment 60

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #59)
> It seems to me that _map should not be a property of an instance, but should
> instead be a global. If someone uses createInstance to create this (which I
> have accidentally done for services in the past) then they will get a unique
> map. In a non-RDF future, we really don't want that to happen.

There is code in the service manager that warns on creating instances that are services (although it's commented out, how typical). I'd usually argue that it's better to make people who accidentally createInstance a service fail much faster instead of silently working.

Hmm, would keeping a gInstanceCount of folderLookupService objects and throwing in the constructor if it's greater than 1 work better? That would make createInstance fail much faster, and it's easier to retrofit on our other services to catch the same issues.

> @@ +68,5 @@
> > +      folder = rdf.GetResource(aUrl)
> > +                  .QueryInterface(Ci.nsIMsgFolder);
> > +    }
> > +    if (!isValidFolder(folder))
> > +      return null;
> 
> In my own code at this point of failure, I log and remove the resource:
>       EWS_LOG_INFO("QI to root folder failed, maybe this was incorrectly
> loaded earlier");
>       rdf->UnregisterResource(serverResource);
>  
> Letting the folder stay in RDF at this point is problematic. Your method
> will report that the folder does not exist, but attempts to create it will
> fail since it is already defined in RDF. This can happen at startup when the
> order of initialization is off. I would suggest that you do the same thing
> here, Components.utils.reportError() and unregister.

For this method, a non-existent folder isn't necessarily an error, so I'm opposed to reporting it in the error console (that place is already noisy, and I don't want to increase noise).

The RDF service is complicated--it doesn't actually hold a reference to the resource. Upon completion of the method, the resource will lose all references to it and will die out on next gc anyways, whereupon it will be unregistered from RDF. In either case, it doesn't prevent folder creation (the test_folderLookupService.js test actually contains a case where the folder is looked, doesn't exist, and is then created).

> Nice, checking for trailing slash!

I only added the test because I started with trailing slashes and then discovered that was wrong (== failing tests).

Comment 61

[Joshua Cranmer [:jcranmer]]

Attachment: RDF-backed folder lookup

Comment 62

[Kent James (:rkent)]

Comment on attachment 8384185 [details] [diff] [review]
RDF-backed folder lookup

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

I only had a couple of issues in error handling. Unless you object to that comments, r+ with those items fixed.

::: mailnews/base/src/folderLookupService.js
@@ +72,5 @@
> +    if (folder == null) {
> +      let rdf = Cc["@mozilla.org/rdf/rdf-service;1"]
> +                  .getService(Ci.nsIRDFService);
> +      folder = rdf.GetResource(aUrl)
> +                  .QueryInterface(Ci.nsIMsgFolder);

If the folder was created by RDF with certain errors, it creates a resource but not an nsIMsgFolder. I think in that case your QI will throw an error. But in keeping with the philosophy of the rest of this, you should return null in that case and not propagate the error. So please enclose this in a try block with null catch.

::: mailnews/base/util/nsMsgUtils.cpp
@@ +819,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  return fls->GetFolderForURL(aFolderURI, aFolder);

Way back in comment 52 I complained that there are existing users of this method that only check rv, others that only check for non-null folder, and others that check both. So as written you will return NS_OK but null folder if the folder is missing, and allow possible crashes for users that are only checking rv. Rather than fix those users, can you just check for null and return an error in that case?

Comment 63

[Joshua Cranmer [:jcranmer]]

Pushed with the fixes mentioned in the last comment:
https://hg.mozilla.org/comm-central/rev/7c838d41bce0

Comment 64

[[e-mail]]

Comment on attachment 8384185 [details] [diff] [review]
RDF-backed folder lookup

>-  nsIMsgFolder getFolderById(in ACString id);
>+  nsIMsgFolder getFolderForURL(in ACString aUrl);
Bad news: getFolderFromUri in folderUtils.jsm calls this.
Good news: getFolderFromUri has no callers.