I updated this morning to Daily. In the unified view, some folders in the Inbox virtual folder disappear (Inbox & Sent IMAP folders). They are indeed disabled from the list of selected folders (rclick on Inbox/properties/choose). When I try to re-enable them (check & click OK), the following error appears in the console: Error: 'JavaScript component does not have a method named: "setInVFEditSearchScope"' when calling method: [nsIMsgFolder::setInVFEditSearchScope] = NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED Source file: chrome://messenger/content/virtualFolderListDialog.js Line: 90

Daily should be version 14. So what is your exact version? And which one did you use before the update and it worked fine? Did you intentionally assign this to David Bienvenu? Is he content with it?

Changed to Daily. Assigned to bienvenu on purpose. But not response... yet :-)

this is all front end code, so I'm not very familiar with it. That error is very strange, since the folder is not a js object. Some sort of js weirdness, perhaps with the jit?

yeah, I can't reproduce this here. Perhaps you could send me your virtualfolders.dat and I can see if anything is weird there. Did you delete any accounts recently?

I now believe this is caused by having chat accounts. The js folder w/o the attribute is probably a chat account folder. I'll try to figure out why the saved search ui is getting chat accounts. I suspect that AllFolders needs to filter out chat folders/accounts.

I quickly looked at the code in the virtualFolderListDialog.js file and I don't see any obvious cause for the error. The two places in the code that use accountManager.allServers to then access all folders do so by calling ListDescendents on the rootFolder of each incoming server. ListDescendants is implemented as a no-op function at http://mxr.mozilla.org/comm-central/source/mail/components/im/imIncomingServer.js#233 so IM accounts should automatically be ignored without us adding any special case. I'll try to reproduce to see if I can understand what's going on.

Wait, ignore comment 6, there's actually a call to processSearchSettingForFolder before the rootFolder.ListDescendants call.

Here's a simple patch to add another IM-specific check. I tried to look at the other consumers of accountManager.allServers with MXR and haven't found any other problematic one, but given how many there are, it's possible I missed one.

(In reply to Florian Quèze from comment #8) > Created attachment 616511 [details] [diff] [review] > Patch > > Here's a simple patch to add another IM-specific check. Do you need accountManager.allServers to return im servers? If not, you could probably avoid a lot of trouble by not returning im servers here. I thought the primary reason for making im servers implement nsIMsgIncomingServer was so that you could be in the account settings dialog, not that you wanted to play fully in the rest of the mailnews handling of incoming servers...

(In reply to David :Bienvenu from comment #9) > Do you need accountManager.allServers to return im servers? No. For some reasons I thought the account settings dialog used it; it doesn't, the "accounts" getter is used instead. It seems I was confused between accountManager.allServers and accountManager.accounts, thinking they were the same. Now I just wish we found this earlier, before adding IM checks in lots of places. At least these 2 can be reverted after doing what you are suggesting: http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#2304 http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/amUtils.js#77 And possibly more if some .accounts calls that actually only use the .incomingServer property of each account can be switched to .allServers.

(In reply to Florian Quèze from comment #10) > Now I just wish we found this earlier, before adding IM checks in lots of > places. > At least these 2 can be reverted after doing what you are suggesting: > http://mxr.mozilla.org/comm-central/source/mail/base/content/ > mailWindowOverlay.js#2304 > http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/ > amUtils.js#77 That's good news. Probably want to annotate the .idl to say that allServers doesn't include im servers. > > And possibly more if some .accounts calls that actually only use the > .incomingServer property of each account can be switched to .allServers. That would be good too. If you link me to them, I can try to help figuure out if that switch can be made.

Comment on attachment 616511 [details] [diff] [review] Patch minusing based on the idea of doing this at a lower level.

Another attempt, taking into account comments 9-11. 5 tests for type == "im" removed :-). I'm not sure I've been able to fully test the code that this patch touches, so you may want to be careful while reviewing the .accounts -> .allServers changes.

Comment on attachment 616957 [details] [diff] [review] Patch v2 this looks good to me. I'm running the mozmill tests, but assuming they're ok, r=me.

Comment on attachment 616957 [details] [diff] [review] Patch v2 [Approval Request Comment] Even though we have decided to pref off IM for Tb 13, we may still want to fix this regressions on 13 for users who would flip the pref.

Comment on attachment 616957 [details] [diff] [review] Patch v2 I'd like to take this for our existing alpha and nightly users.

Looks like this was pushed to aurora earlier today: http://hg.mozilla.org/releases/comm-aurora/rev/ef0b70ed8c0e