Over to Paul for this one.

This is ready for review. There's a lot here. I considered doing the gdata tree separately but it uses the same CSS, which I'd already changed, so I went ahead and did it with the rest.

Tests are passing locally (linux), and I manually tested:

• the main (full) tree (the main calendar list in the sidebar in calendar/tasks tabs)
• gdata tree (in the dialog for creating a new google calendar)

I wasn't able to manually test the 3rd usage of this tree, which (I think) should appear when you uninstall the google (gdata) provider add-on. For some reason there is no uninstall button.

Note that I'm handling attribute inheritance manually for now (see bug 1545824).

A patch to remove (de-overlay) the calendar-calendars-list.xul overlay file by merging it into messenger-overlay-sidebar.xul. This overlay was only used in this one place so removing it simplifies things. This patch should be applied after the de-xbl patch.

Try server run looks fine:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bba475e9aa5c815bbf0f0f1406be4dc2c73bbb18

Thanks for the review. I'll make those changes. One question below...

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

@@ +967,5 @@

•         *
  

•         * @implements calICalendarManagerObserver
  

•         */
  

•        this.calMgrObserver = {
  

•            listTree: this,
  

Remove this, and in stead use array as I'll write below. Then this will be
correct. same for the others below

Can you say more here? I'm not sure what you'd like me to remove (the @implements part?) and what you mean about using array instead.

I meant the part about listTree: this

When you use onCalendarRegistered: (calendar) => {

.... then the this will be properly defined inside and you don't need to set listTree: this just to access the proper this

Ah, I see, using arrow functions for different 'this' behavior. Thanks for clarifying. I made that change all through the file, but then I got JS errors in the console like:

JavaScript error: chrome://calendar/content/widgets/calendar-list-tree.js, line 234: TypeError: this.onPropertyChanged is not a function

Not sure why... So I changed it back to using the listTree property. Although that's less concise, on the plus side, it has the advantage of being more explicit.

I've made the other changes and also switched to using the shorthand method syntax without "function" keyword. Rebased on current trunk. Mozmill tests all pass locally.

::: calendar/base/content/widgets/calendar-list-tree.js
@@ +45,2 @@

•        // The first class of each treecol is used to identify it, add additional classes after.
  

let's not rely on the first class being the "id" (and then remove this
comment)

Ok, makes sense. I've used ids instead. Often in this code we have a reference to the element and need to find out what kind of element it is, and classes don't work well for that (since there might be more than one).

@@ +414,5 @@

•     *
  

•     * @param {string} className    The class to query by.
  

•     * @return                      The column.
  

•     */
  

•    getColumn(className) {
  

I suggest to inline this method, it's a oneliner in the new situation, and
used only twice

I can see pros and cons to either way of doing this. I've inlined it, as suggested.

Regarding using ids: you can't do that, since the document might contain many widgets and these "internal" ids shine through. The emphasis was on first class.

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

Regarding using ids: you can't do that, since the document might contain many widgets and these "internal" ids shine through. The emphasis was on first class.

Ok, I see. I'm aware of the duplicate id problem, and I've been using classes to avoid it. In this case I think there's (currently...) only one instance of the widget per document (it's used in dialog windows), so that's why I thought it would be okay (though not ideal...) to use ids.

But it would be better to avoid them. To use classes, in any order, I'll just have to do something like this:

function getElementType(element) {
  const classesUsedAsIds = ['one', 'two', 'three'];

  for (const className of element.classList) {
    if (classesUsedAsIds.contains(className) {
      return className;
    }
  }
  return "";
}

I'll do that and upload another patch.

If you really need to know the type, you can always store it as a type attribute.
I don't like using custom attributes when it should be css, but there are of course usecases for it too.

Ah, that's a good solution. I'll do that.

Ok, this time using a custom 'type' attribute in place of 'id' for identifying columns. Mozmill tests pass locally.

Good idea about a diff to capture what changes are the XBL converter and which are after the converter run. I'll plan on doing that.

Here's the first of the two patches, rebased on current trunk. Mozmill calendar tests pass locally (linux). Ready for checkin. (Let me know if another try server run is needed.)

And here's the second of the two patches, rebased on trunk.

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

Thes patches are in conflict with bug 1544914, see comment there. It would be nice to indicate which bug goes first.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ebe4366fb0a3
[de-xbl] Migrate calendar-list-tree bindings to custom elements. r=philipp
https://hg.mozilla.org/comm-central/rev/844558b1ed2e
De-overlay calendar-calendars-list.xul. r=mkmelin