In bug 1425356 and/or deps, we'll remove support for XUL templates which are effectively unused in Firefox. Thunderbird (and SeaMonkey) still use these, per https://dxr.mozilla.org/comm-central/search?q=%27datasources%3D%27+-path%3Atest&redirect=false . It's not clear to me off-hand how easy (in terms of time/effort) it would be to replace some of these. What I *will* say is that from my almost-gone memory of working on CZ, uh, 10 years ago, you might get performance improvements from switching to a JS treeview anyway. See bug 315913. So if time is available, I'd suggest switching the implementation just because it'll make things better.

Talked about this with :Fallen on IRC. As far as I can tell, there are 4 consumers in Thunderbird (I could be wrong, I'm not super familiar with comm-central anymore): https://dxr.mozilla.org/comm-central/rev/b045d93bbd539474f6c9ded13bd993ba2f3e4c07/mail/base/content/msgSelectOffline.xul#31 #synchronizeTree is a XUL <tree> that displays folders/subfolders to make available offline for IMAP (I think). I think you would be able to replace this with a JS-based treeview. Seems the datasource will be requestable by just using the rdf service (while that lasts - I would expect us to remove that soon, too, but it might be easier to resurrect as a standalone-ish component than templates are because of the tie-in with XUL creation). Then you can provide access to the data source using the RDF principles and forward it into a tree view. Alternatively, you could use Thunderbird's own msg folder datastructures directly from a JS treeview. We probably will want to replace XUL <tree>s with something else at some point, too, but I'm less sure that'll happen before 61. If it does, I expect we'll need to rewrite a lot of stuff ourselves and we'll have tools (or a portability JS layer) that would be usable in TB. https://dxr.mozilla.org/comm-central/rev/b045d93bbd539474f6c9ded13bd993ba2f3e4c07/mail/base/content/subscribe.xul#91 the same, but for newsgroups, AFAICT. It'll take similar code, except here it seems the rdf stuff is already kinda in JS, maybe, sort of. Might be easier to adapt this first to whatever strategy you choose, and then copy that approach for the previous item. https://dxr.mozilla.org/comm-central/rev/b045d93bbd539474f6c9ded13bd993ba2f3e4c07/mailnews/base/content/virtualFolderListDialog.xul#33 honestly, not sure what this thing is and if it's exposed in TB. But the code looks like it's essentially using the same folder style DS that the above two cases are using, so maybe there's an opportunity to share (more) code between these 3 cases. https://dxr.mozilla.org/comm-central/rev/b045d93bbd539474f6c9ded13bd993ba2f3e4c07/mailnews/base/prefs/content/AccountWizard.xul#51 This might be the easiest of the lot. Just manually get the ISP datasource (or some other structure that's based on, and remove the rdf goop) and iterate over it, and generate <radio> elements. You can do this once the wizard pane is shown for the first time (there are events for this for sure) or on load, given it's the first pane.

Thanks for the analysis. Can you please see if the patch in bug 464710 actually removes some of the uses? For msgSelectOffline.xul it removes some <template> element. For virtualFolderListDialog.xul we create another virtualFolderListDialogJS.xul without any <template> and ignore the old file for Thunderbird (yes, Seamonkey may still use it). The subscribe.xul and accountwizard.xul may still be left to do.

(In reply to :aceman from comment #2) > Thanks for the analysis. Can you please see if the patch in bug 464710 > actually removes some of the uses? Yes, it does. > For msgSelectOffline.xul it removes some <template> element. > For virtualFolderListDialog.xul we create another > virtualFolderListDialogJS.xul without any <template> and ignore the old file > for Thunderbird (yes, Seamonkey may still use it). > > The subscribe.xul and accountwizard.xul may still be left to do. Yes, that looks right to me from a quick look at those patches. As noted in comment #1, I actually think accountwizard should be pretty easy to fix anyway. I don't know if the fixes to msgSelectOffline port easily to subscribe.xul, but it's plausible.

(In reply to :Gijs from comment #1) > https://dxr.mozilla.org/comm-central/rev/ > b045d93bbd539474f6c9ded13bd993ba2f3e4c07/mailnews/base/prefs/content/ > AccountWizard.xul#51 > > This might be the easiest of the lot. Just manually get the ISP datasource > (or some other structure that's based on, and remove the rdf goop) and > iterate over it, and generate <radio> elements. You can do this once the > wizard pane is shown for the first time (there are events for this for sure) > or on load, given it's the first pane. You mean keep the files (like https://dxr.mozilla.org/comm-central/source/mailnews/base/ispdata/movemail.rdf) in xml if we need that, but read them in using other mechanism than RDF parser?

(In reply to :aceman [mostly away 23-29 Dec] from comment #4) > (In reply to :Gijs from comment #1) > > https://dxr.mozilla.org/comm-central/rev/ > > b045d93bbd539474f6c9ded13bd993ba2f3e4c07/mailnews/base/prefs/content/ > > AccountWizard.xul#51 > > > > This might be the easiest of the lot. Just manually get the ISP datasource > > (or some other structure that's based on, and remove the rdf goop) and > > iterate over it, and generate <radio> elements. You can do this once the > > wizard pane is shown for the first time (there are events for this for sure) > > or on load, given it's the first pane. > > You mean keep the files (like > https://dxr.mozilla.org/comm-central/source/mailnews/base/ispdata/movemail. > rdf) in xml if we need that, but read them in using other mechanism than RDF > parser? Well, the simplest fix right now would be to just have some JS code that fetches the datasource that's currently in use, and manually creates the radio elements based on that datasource. The slightly harder fix would be, indeed, to store and/or parse the data differently. You could probably just `fetch` the files as xml, but if you were going to do that you might as well change the storage to use something more sane like JSON or plain XML.

How are we going here? Bug 1425356 has all its reviews and will land soon. What else is needed besides bug 464710?

Hey gijs, thanks for the help for Thunderbird, for notifying us, looking into each case, and making suggestions! Very kind of you.

AccountWizard.xul is old cruft that has been superceded by the Email Account Setup Wizard. There are only gmail, AOL and movement. Gmail and AOL are handled (in a much more generic way) by email account setup wirzard. That leaves only movemail. movemail can simply be hardcoded, like news is. That would simplify the code and remove the list for the template. I'm not sure whether anybody actually still uses movemail. Personally, I'd vote for removing it entirely.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/b9d14117cde4 Port bug 1425356 to TB/IB/SM: Remove xultmpl.xpt from package manifests. rs=bustage-fix

Looks like this cause only one test failure: https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=b2d833012dd9f871223e91d56e22eeec502c8636&selectedJob=154078832 TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515063002/build/tests/mozmill/subscribe/test-subscribe-news-filter.js | test-subscribe-news-filter.js::test_subscribe_newsgroup_filter So all the other stuff that stopped working isn't covered by tests. And subscribe.xul isn't even covered by bug 464710.

(In reply to Jorg K (GMT+1) from comment #10) > Looks like this cause only one test failure: > https://treeherder.mozilla.org/#/jobs?repo=comm- > central&revision=b2d833012dd9f871223e91d56e22eeec502c8636&selectedJob=1540788 > 32 > TEST-UNEXPECTED-FAIL | > /Users/cltbld/tasks/task_1515063002/build/tests/mozmill/subscribe/test- > subscribe-news-filter.js | > test-subscribe-news-filter.js::test_subscribe_newsgroup_filter > > So all the other stuff that stopped working isn't covered by tests. :-( > And > subscribe.xul isn't even covered by bug 464710. Right, but see my earlier comment: (In reply to :Gijs from comment #3) > > The subscribe.xul and accountwizard.xul may still be left to do. > > I don't know if the fixes to msgSelectOffline port easily to > subscribe.xul, but it's plausible. As far as I can tell, they're effectively the same type of dialog. They show a tree of folders/groups, with ticks to subscribe/sync-for-offline-use, right? So I imagine the same-ish JS treeview could handle both. It only needs per-case methods for: - getting the list of children of any node including getting the root (and perhaps 'tags' for those for the treestyle display (the things that used to be in nsIAtoms but now aren't anymore) - getting the state (synced or not) - toggling the state so that actually has effect when closing the dialog otherwise it's, as best I can tell, basically the same treeview that's been implemented twice.

(In reply to :Gijs from comment #11) > (In reply to :Gijs from comment #3) > > > The subscribe.xul and accountwizard.xul may still be left to do. > > > > I don't know if the fixes to msgSelectOffline port easily to > > subscribe.xul, but it's plausible. > > As far as I can tell, they're effectively the same type of dialog. They show > a tree of folders/groups, with ticks to subscribe/sync-for-offline-use, > right? So I imagine the same-ish JS treeview could handle both. It only > needs per-case methods for: > otherwise it's, as best I can tell, basically the same treeview that's been > implemented twice. Yes, I will try to adapt the code from bug 464710 to implement this for subscribe.xml. Frg, can you compare TB's and SM's /mail/base/content/synchronize.{xul,js} and tell if I should just implement a common version in /mailnews/base/content based on TB's version (even if it won't yet works for SM), as done in bug 464710 for the other dialogs?

(In reply to Ben Bucksch (:BenB) from comment #8) > AccountWizard.xul is old cruft that has been superceded by the Email Account > Setup Wizard. There are only gmail, AOL and movement. Gmail and AOL are > handled (in a much more generic way) by email account setup wirzard. That > leaves only movemail. So can all those /mailnews/base/ispdata files be removed after this? > movemail can simply be hardcoded, like news is. That would simplify the code > and remove the list for the template. > > I'm not sure whether anybody actually still uses movemail. Personally, I'd > vote for removing it entirely. Can you please implement this? But please do not remove movemail, just hardcode it as you say.

> Frg, can you compare TB's and SM's ... /mail/base/content/synchronize.{xul,js} I checked the actual dialogs in 56 / 2.53 and they are almost identical. Nothing which couldn't be fixed with some application specific css but not even needed I think. SeaMonkeys version seems just to be a little older imho. I would be fine with a common version.

(In reply to :aceman from comment #13) > So can all those /mailnews/base/ispdata files be removed after this? Yes. > Can you please implement this? No, sorry. (It's not in my code, either.) But it should be straight-forward to implement.

(In reply to :Gijs from comment #11) > As far as I can tell, they're effectively the same type of dialog. They show > a tree of folders/groups, with ticks to subscribe/sync-for-offline-use, > right? So I imagine the same-ish JS treeview could handle both. It only > needs per-case methods for: > - getting the list of children of any node including getting the root (and > perhaps 'tags' for those for the treestyle display (the things that used to > be in nsIAtoms but now aren't anymore) > - getting the state (synced or not) > - toggling the state so that actually has effect when closing the dialog > > otherwise it's, as best I can tell, basically the same treeview that's been > implemented twice. I tried to do this but found this isn't as trivial. This dialog works on folders/newsgroups that are on server only and not in our local database and aren't backed by nsIMsgFolders shich folderPane.js operates on. This dialog fetches these folders via nsSubcribableServer.cpp and nsSubscribableDataSource.cpp. So this dialog would need to open-code the whole tree listing (including expandable 'folder' nodes) or update the tree implementation in folderPane.js to also work on fictional folder objects, maybe something like the unified/smart folders we already have. But first we need to find out how to even get the list of folders on the server. It is somewhere in the mentioned files. But e.g. startPopulating() function only removes the existing list and does nothing. The real fetching is somewhere else and happens async with a progressbar in the dialog, etc. No fun.

(In reply to :aceman from comment #16) > No fun. And I thought volunteers work here for their enjoyment :-( So how did it work before? Or that's the ... > first we need to find out how to even get the list of folders on the server. We should have driven the debugger at it while it was still working, now it's a little hard to see the internals at work :-(

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/4cf5e206f3f7 temporarily disable failing test test-subscribe-news-filter::test_subscribe_newsgroup_filter. rs=bustage-fix DONTBUILD CLOSED TREE

This generates a whole heap of debug output. When used on news.mozilla.org I get: === nsSubscribableServer::AddTo(): mozilla.dev.flyweb === nsSubscribableServer::FindAndCreateNode(): mozilla.dev.flyweb === nsSubscribableServer::AddChildNode(): news://news.mozilla.org === nsSubscribableServer::AddChildNode(): mozilla === nsSubscribableServer::AddChildNode(): mozilla === nsSubscribableServer::AddChildNode(): dev === nsSubscribableServer::AddChildNode(): dev === nsSubscribableServer::AddChildNode(): flyweb === nsSubscribableServer::AddChildNode(): flyweb === nsSubscribableServer::AddTo(): netscape.public.mozilla.newsclips === nsSubscribableServer::FindAndCreateNode(): netscape.public.mozilla.newsclips === nsSubscribableServer::AddChildNode(): netscape === nsSubscribableServer::AddChildNode(): netscape === nsSubscribableServer::AddChildNode(): public === nsSubscribableServer::AddChildNode(): public === nsSubscribableServer::AddChildNode(): mozilla === nsSubscribableServer::AddChildNode(): mozilla === nsSubscribableServer::AddChildNode(): newsclips === nsSubscribableServer::AddChildNode(): newsclips === nsSubscribableServer::AddTo(): mozilla.tools.pulse ... and a ton more. So a bit of debugging should show where this is called. Would you like me to continue?

OK C++ stack: xul.dll!nsSubscribableServer::AddTo() Line 119 C++ xul.dll!nsNntpIncomingServer::AddTo() Line 1108 C++ xul.dll!nsNntpIncomingServer::HandleLine() Line 1231 C++ xul.dll!nsNntpIncomingServer::LoadHostInfoFile() Line 889 C++ xul.dll!nsNntpIncomingServer::StartPopulating() Line 949 C++ xul.dll!XPTC__InvokebyIndex() Line 99 Unknown And the JS stack is: 0 SetUpTree(forceToServer = false, getOnlyNew = false) ["chrome://messenger/content/subscribe.js":134] this = [object ChromeWindow] 1 ShowCurrentList() ["chrome://messenger/content/subscribe.js":451] this = [object ChromeWindow] 2 SubscribeOnLoad() ["chrome://messenger/content/subscribe.js":233] this = [object ChromeWindow] 3 onload(event = [object Event]) ["chrome://messenger/content/subscribe.xul":1] this = [object ChromeWindow] 4 Subscribe(preselectedMsgFolder = [xpconnect wrapped (nsISupports, nsIMsgFolder, nsIRDFResource) @ 0x17f6c904940 (native @ 0x17f6adcbc00)]) ["chrome://messenger/content/mailCommands.js":346] this = [object ChromeWindow] 5 MsgSubscribe() ["chrome://messenger/content/mailWindowOverlay.js":2103] this = [object ChromeWindow] 6 oncommand(event = [object XULCommandEvent]) ["chrome://messenger/content/messenger.xul":1] this = [object XULElement] Is that helpful? I'm not sure how it hangs together with the XUL template though.

This also affects IMAP subscribe, that's really bad then :-(

I received this patch from Aceman via e-mail who has trouble attaching patches. Let's see whether Richard likes it first, then worry about with how many reviews this will land ;-)

Comment on attachment 8942721 [details] [diff] [review] subscribe - part 1 (main) This is a first pass on the subscribe dialog. This replaces the <template> element with a normal tree that is manually populated with rows and cells via the frontend JS code, no RDF+template magic from backend. The sad thing is this still runs the RDF engine in the background to do its magic via the *datasource* attributes on the tree, I just extract the found server folders via a new event on the nsISubscribeListener . From those data the tree rows are built. The RDF tree is built in the background in parallel, but should not access the XUL elements anymore. What is not implemented yet is the nice collapsible folders on IMAP and News servers. For now, all folders/newsgroups are displayed in a long flat list. I'm not yet sure how to extract the folder hierarchy from the RDF backend (but it does have it). All the JS is for the <tree id="subscribeTree"> for now. The <tree="searchTree"> is driven by nsNntpIncomingServer.cpp (see the nsNntpIncomingServer::GetCell*() functions), which is crazy. You can see in nsNntpIncomingServer::GetCellProperties the server backend even has tree column names hardcoded in it. The UI should be totally separated, but isn't... Please play with this guys and then we can decide if we can land this as first bustage fix to not let users with broken IMAP and News for too long. I plan to make some more passes at the subscribe.js file (it has some very old syntax and could use some cleanup) and also look through the nsSubscribeDataSource.cpp and nsSubscribableServer.cpp whether we can drop something from it. We should kill RDF where possible. Paenglab, the css changes in this patch aren't strictly necessary, I just couldn't stand that 2 trees in the same dialog (even alternating each other in a <deck>) had different property names for the same thing. So while merging that, I aligned the naming and capitalization with the other trees, e.g. folderPane.css and msgSelectOffline.css. If you want I can split this into a separate followup patch. But then the first patch would have to use the old properties in new code and then would be switched over to new properties soon. Jorg, with this patch the disabled test passes for me locally (but I'm so back in the past that it isn't even disabled in my tree). Please check for yourself, if we can enable it again. Frg, this again merges the TB and SM implementation of the subscribe dialog, using the TB version. Please see if it is OK. I see that the SM version even used another template for the server picker on top of the dialog, referencing datasources="rdf:msgaccountmanager rdf:mailnewsfolders", which we just removed in the previous bug. TB has this picker already reimplemented as the JS folder picker from folderWidgets.xml (<menupopup type="folder">).

I've commented out some lines that gave JS errors and now it works as far as I can see: Line 96: // gSubscribeTree.database.AddDataSource(subscribeDS); Line 163: // gSubscribeTree.database.RemoveDataSource(subscribeDS); Line 192: // gSubscribeTree.database.RemoveDataSource(subscribeDS); I'm not at the very latest M-C, it's from the weekend.

I think rdf datasources are gone now too which might explain the errors. SeaMonkey is pretty much broken because of the latest xbl removals and I can test anything. Will take some time to get it going again so just go forward. We can always add aditional suite fixes later in followup bugs. FRG

Comment on attachment 8942729 [details] [diff] [review] 1425962-subscribe.patch - offending lines commented out This one works and it looks like the old subscribe dialog. Please add the folderMenus.css stylesheet to the dialog, then the server menulist shows icons. The old dialog didn't show them too.

(In reply to Richard Marti (:Paenglab) from comment #29) > This one works and it looks like the old subscribe dialog. Not quite. We now get a list when before we had a tree (see 2nd paragraph of comment #25). Please compare with TB 52 or 58 beta. > Please add the folderMenus.css stylesheet to the dialog, then the server > menulist shows icons. The old dialog didn't show them too. Done. I'm not sure where we're at with this patch. Something works, so it could serve as bustage fix. I don't know what the now disabled calls gSubscribeTree.database.{Add|Remove}DataSource(subscribeDS); are for. Does removing them open the door to more removals? Joshua also mentioned bug 457333 on IRC.

Yes, it is not a tree yet, which I mentioned. But it should work regardless, just less nice for now. From the hint of jcranmer on IRC, he has several implementations of a tree for this dialog, so I can look at those in the next patch. Yes, removal of gSubscribeTree.database may open the door for more cleanup, in the next patch. The goal is to just remove the whole nsSubscribeDataSource.* files, I'll see if we are at that point now. I would like to upload my version of the patch with some more touches soon which then can be landed as solving the user affecting bustage (but with proper reviews) and I'll continue on more cleanup in next patches here.

Latest input from Aceman.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/3d52812591b8 Convert subscribe.xul dialog from XUL templates to a manually populated tree list. rs=bustage-fix

I've landed this so we get at lease some subscribe functionality back. We should take a look at the patches in bug 457333 and also: https://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/patches-derdf/subscribe-invert and maybe https://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/patches-derdf/subscribe-tree and potentially more stuff from https://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/patches-derdf/ That the current solution doesn't offer a hierarchy/tree is suboptimal for - folders in IMAP trash folders - nested newsgroups.

Comment on attachment 8943770 [details] [diff] [review] 1425962-subscribe.patch - part 1 (main), v2 I'd like a proper review here (albeit PLR). While this fixes bustage (users aren't able to subscribe to newsgroups and IMAP folders in any way), it slightly changes the behaviour (hopefully temporarily) and it is not a straightforward replacement according to some already known pattern (e.g. m-c patch). I hacked this to make the dialog running using my own ideas and I'd like to know if I should continue in this direction for the further patches. E.g. the new event on the nsISubscribeListener is a quick hack to get at the discovered folders from the backend. Interestingly it also appears in jcranmer's patches so it may not be a bad idea :)

It seems we do not really need the RDF backend for the subscribe dialog so remove some more remnants from it. Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=30c8d6588f0de357a3373f34b492d1589c33fb63

Comment on attachment 8944166 [details] [diff] [review] 1425962-subscribe - part 2 Thanks for the clean-up.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/67727712f9b0 don't reference obsolete RDF service and subscribeDataSource in subscribe.js. r=jorgk

There is bug 732106 for removing ispdata (from comment 8) so we can do that there. Jcranmer even did all of the work. Let's keep the bug here for the proper finish of IMAP/NNTP subscribe dialog, which was landed with partial implementation and slow.

Let's call this a day. This bug here implemented a simple flat subscribe list. We'll fix this in bug 1466705.

Comment on attachment 8943770 [details] [diff] [review] 1425962-subscribe.patch - part 1 (main), v2 rs=jorgk. This has long landed and some of the stuff added here will be removed again in bug 1466705.