Comment on attachment 9031331 [details] [diff] [review]
cpp-use-fls-8.patch

Yes it seems there are 3 patches concatenated into that file. I'm not sure that works.
Sorry for taking so long that the patch does not apply cleanly now.
Please update it and split into 3 patches and describe what they do. E.g. you now disable the cache of the folders which is a new thing never mentioned before. Why is it needed?

Sorry the bitrot got you, aceman - it was bad timing, as I was just rebasing it as you posted!

This new one is a single changeset:

• I ditched the changeset removing the FLS cache (I thought the cache was causing some unit test failures, but it turned out to be something else)
• I merged the changeset with the GetOrCreateJunkFolder() rewrite. The old GetOrCreateJunkFolder() implementation was causing a test failure, so really it needed to be rolled in with the other changes anyway.
• I found a bunch more places where GetOrCreateFolder could be removed.

Try build running here:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=037b7bbebb4f29255bb65e78bb8bd2c4aaa9d11d

(In reply to Ben Campbell from comment #111)

• I found a bunch more places where GetOrCreateFolder could be removed.

Just to recap (it's been a while!): GetOrCreateFolder() is the old RDF-style behaviour which can return dangling folders. The aim is to ditch it completely. There are still a few places which rely upon dangling folders, and they need to be sorted out - but that's outside the scope of this bug - I'll open another issue for them when this stuff is landed.

Comment on attachment 9035213 [details] [diff] [review]
cpp-use-fls-9.patch

Gah. Looks like some of my GetOrCreateFolder() removals have caused some mozmill fails. I'll back some of them out and try again.

Dropped my set of seemingly-straightforward GetOrCreateFolder() removals. Sigh.

Try build looking much better now:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c3ae3bbcdb23ab6ed13d26986b4671f5404028d9

(In reply to :aceman from comment #115)

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +614,5 @@

         !uri.Equals(actionTargetFolderUri))
     {

•      nsCOMPtr<nsIMsgFolder> destIFolder;
  

•      rv = GetExistingFolder(actionTargetFolderUri, getter_AddRefs(destIFolder));
  

•      NS_ENSURE_SUCCESS(rv, rv);
  

Shouldn't this be FindFolder? We cope with folder not existing by disabling
the filter, so we must not return here.

Yes! Now changed.

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +1449,4 @@

   nsCOMPtr<nsIMsgFolder> folder;
   thisIdentity->GetFccFolder(folderUri);

•  if (!folderUri.IsEmpty() &&
  

•      NS_SUCCEEDED(GetOrCreateFolder(folderUri, getter_AddRefs(folder))))
  

This originally checked parent folder, so why does it not take
GetExistingFolder() ? And all the folders below.

I did switch to GetExistingFolder() but later switched back as it was causing some test fails.
I just forgot to put the parent check back in. However, I've left it out still. It sets the folder flags even on dangling folders. Rationale: can't see any reason why this'd be an issue, and there are other places in the code which do this.

::: mailnews/base/util/nsMsgUtils.cpp
@@ +875,4 @@

{
nsresult rv;

• nsCOMPtr<nsIMsgFolder> folder;
• GetExistingFolder(aURI, getter_AddRefs(folder));

Why does this not use FindFolder() ?

I suspect it predated FindFolder :-) Now changed.

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +1646,4 @@

 }

• nsString folderName;
• folder->GetPrettyName(folderName);

I wonder why we have to get the name here.

I was curious about this too (although haven't dug too deep - I just left the code as it was).
I suspect it's to do with massaging the names a little: "Inbox" vs "INBOX" etc.
There's also some localisation hacks scattered about the place. I'd like to take a look at the naming stuff some time and see if such policy can be factored into one place and documented properly. But for now, if it works I'll just leave it :-)

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ -3352,5 @@

• // .msf file when we don't want to.
• nsCOMPtr <nsIMsgFolder> parent;
• msgFolder->GetParent(getter_AddRefs(parent));
• if (!parent)
• return NS_ERROR_FAILURE;

Why is this check removed? FindSubFolder calls GetOrCreateFolder(), which
creates the folder, but it may be unparented (due to RDF). Or isn't it the
case anymore?

Ahh yes - I was originally using GetExistingFolder() but had to roll it back, and forgot to restore this bit! Restored now.

I sorted out a bunch of the spacing/lack-of-spacing nits in the diff hunks I touched. There are a bunch of other offending places in those files - I might see about doing a couple of mass fix-up patches just to tidy them up. But for now, I've tried to keep too much noise out of this patch. It's noisy enough as is :-)

Try build running here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6c8dbd8bd92f522d341fa9e058de39686d320158

So... I think that last revision of the patch brings us back to some mozmill failures. This patch is too flakey.
I've come to the conclusion that it's too much to bite off in one go, so I'm going to do what I should have done to begin with and break it up into small, discrete steps.
Specifically, I'm going to leave out my ham-fisted attempts to tackle the dangling folders issue, which turned out to be way more subtle and tricky than I initially thought.

So, for this bug I'm purely going to replace the direct RDF-service calls with folder-lookup-service calls.
(and I've got a folder-lookup-service patch which replaces RDF completely, after which the RDF code can be removed).
The dangling folders can be dealt with in another bug. (or at least properly defined/documented)

Nice easy one to start with - renames the old GetOrCreateFolder() to GetOrCreateJunkFolder() so we can repurpose the name.

Next up, this patch adds the FindFolder() and GetOrCreateFolder() utility functions, which use the folder-lookup-service (as does the GetExistingFolder() which is already in use).

This patch is independent to the rest of the C++ patches, and simply removes a couple of the leftover direct RDF calls on the javascript side.

This one just straightens out some of the nsresult usage on some folder lookups. A bunch of them were using GetExistingFolder() to test for folder existence, but now there's FindFolder() which doesn't treat missing folders as an error condition.
(requires the rename-getorcreatefolder and add-findfolder patches first)

And this is the biggest one - it replaces all the direct rdf GetResource() calls by going through GetOrCreateFolder().
(again, requires the rename-getorcreatefolder and add-findfolder patches first)

aceman: Sorry to lumber you with so many r? flags, but I figured it was clearer to separate each patch out into deliberate, single-stage steps that just make one change at a time.

There's a try build here (I've tidied up the commit messages, but the changesets otherwise include all these new patches exactly):
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=617eab8ff04996c9fb425c65cbb2fc6ad7a95f4b

Can we land some of this?

(In reply to Jorg K (GMT+1) from comment #124)

Can we land some of this?

Yes please! Attachment #9037881 [details] [diff] and Attachment #9037882 [details] [diff] are good to go (in that order).

What about the big patch "cpp-use-fls-11.patch"? That's superseded by "remove-direct-rdf-calls-1.patch" and more to come?

(In reply to Jorg K (GMT+1) from comment #127)

What about the big patch "cpp-use-fls-11.patch"? That's superseded by "remove-direct-rdf-calls-1.patch" and more to come?

Yes, ignore cpp-use-fls-11.patch (actually, I'll obsolete it now). I couldn't get it working right, so I split it up into these new discrete patches which just do a single thing at a time:

rename-getorcreatefolder-1.patch - r+ ready to land
add-findfolder-1.patch - r+ ready to land
mop-up-leftover-js-rdf-use-1.patch - landed
use-findfolder-1.patch - r? awaiting review
remove-direct-rdf-calls-1.patch - r? awaiting review

I think the commit message should be:
Bug 453908 - Add FindFolder() and GetOrCreateFolder() to nsMsgUtils. r=aceman

They're free-standing functions, and I used lower-case "msgutils" to avoid the impression there was a nsMsgUtils class (it's just the filename).
How about:

Bug 453908 - Add FindFolder() and GetOrCreateFolder() utility functions

Do I need to upload a new patch to do it? (and would that require another r?)

Just updated commit message. No code changes.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5d2d5584e944
rename GetOrCreateFolder() to GetOrCreateJunkFolder(). r=aceman
https://hg.mozilla.org/comm-central/rev/0b1d7c60056b
Add FindFolder() and GetOrCreateFolder() to nsMsgUtils. r=aceman

And to answer the questions: In general changing the commit messages doesn't need another patch, I can do it if I see it on time. And it also doesn't need another review. I change commit messages all the time, only this time I posted to the bug to let Geoff know since he also lands patches. It's not a big deal, however, it would be extremely useful being able to edit commit messages after the event to fix missing/wrong bug numbers, typos, wrong reviewers and other misc. mistakes. But we can't :-(

Aceman, any update on getting these reviewed and landed?

(In reply to :aceman from comment #137)

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +614,5 @@

         !uri.Equals(actionTargetFolderUri))
     {

•      nsCOMPtr<nsIMsgFolder> destIFolder;
  

•      rv = GetOrCreateFolder(actionTargetFolderUri, getter_AddRefs(destIFolder));
  

•      CONTINUE_IF_FAILURE(rv, "Could not get action folder");
  

Didn't you say you'll change this to FindFolder? We do not want to create
missing folders here. You could also drop the parentFolder check below.

My previous attempts at this stuff were much more aggressive about using FindFolder()/GetExistingFolder(), but I kept running into loads of subtle issues with tests failing, I spent ages running around in circles trying to figure them out and ended up just buried in the complexity.

So I stepped back and broke it all down into pure refactoring steps. So this patch was purely to replace all the old remaining GetResource() calls with GetOrCreateFolder(), which should be exactly equivalent.

So, yes, likely a whole bunch of lost opportunities to tighten up the dangling-folder issue, but I'd rather attack that separately. I had no idea how fiddly it'd end up being :-(

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +1454,4 @@

   {

•    nsCOMPtr <nsIMsgFolder> parent;
  

•    rv = folder->GetParent(getter_AddRefs(parent));
  

•    if (NS_SUCCEEDED(rv) && parent)
  

So we create the folder in any case, but only if it is real (with parent) we
set the flag on it? This is the same as just finding the folder if it exists
and setting the flag on it. If that caused failures that means we rely on
this creating dangling folders, maybe because we store messages in them
later and then the folders stop being dangling. In such a case, I wonder why
the folders would not need those flags. The flags may be added on restart in
some of our flag cleanup function.

So if we fix this place one day, we will need to choose whether to just
create all the folders and with flags, or no.

Again, I've tried to stick to the pure GetResource() -> GetOrCreateFolder() refactor, keeping all the surrounding code, warts and all.
(incidentally, there are a few places where flags are set before the folder is properly created, relying on the dangling-folders behaviour to pick up the correct flags when the folder is created for real...)

::: mailnews/base/util/nsMsgUtils.cpp
@@ +875,3 @@

• nsCOMPtr<nsIMsgFolder> folder;
• rv = GetOrCreateFolder(aURI, getter_AddRefs(folder));

Again, you promised to use FindFolder() here. Even though that was when the
function looked quite differently. Now you have again totally changed it
around (back to original), dropping the IMAP additions. So why is that?

Same again - just dropping the patch back to a pure refactoring step.
The IMAP changes were a kludge. There are so many places where IMAP is handled via special casing. It'd be really nice to unify/simplify the behaviors of the various folder types (eg lots of localmailfolder operations are synchronous, but the IMAP equivalent is async, because it requires the round-trip to the server. This duality leads to a lot of special-casing and complexity). I don't currently see any easy wins to be had, but I'm still thinking.

@@ +878,3 @@

NS_ENSURE_SUCCESS(rv, rv);

// don't check validity of folder - caller will handle creating it

Which 'caller' is meant here? We created it right above.

Old comment left in, warts and all :-) I've an idea that there are some circumstances where the folder can be left dangling (IMAP maybe), in which case it is up to the caller to sort out...
I'd rather like to nuke the entire GetOrCreateJunkFolder() function anyway - I don't see what's so special about Junk that doesn't apply to any of the other special folders.

So, how much RDF is left after landing this? Is this the last patch in this bug or is there more to come?

I haven't seen any recent try, so here is one:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a5e38f8815a488345f08345b5bf560382a781bd1

There are a couple of places out in javascript land where the RDF code is used for non-folder reasons (feed-parsing was one, off the top of my head). I'll take another look and report back.

On the folder side, I've got two more patches in the pipeline:

1. to remove RDF use from the folder-lookup-service (looking good, and I've done try builds with it which didn't raise any more unittest/mozmill fails, so I just need to tidy up all the debug dump()-spew).

2. to decouple the folders from RDF (remove the inheritance of nsIRDFResource). This was causing me no end of trouble, but I think I had a breakthrough yesterday - I'm just running tests locally, then I'll hit the try servers with it.

We could call this one off after the 140 comments;) After all, we converted all folder lookups to the lookup service (as the summary says). RDF is still hidden in that service, but we could file followups for the next steps. This one is NOT the meta bug for all RDF removal.

Try looks good, let's land.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5fa6fd01e555
Replace direct rdf-service calls in C++ with folder-lookup-service calls. r=aceman DONTBUILD

OK, please file follow-up bugs that depend on this one here.