Comment on attachment 9150656 [details] [diff] [review] 1639763-attendees-dialog-1.diff Review of attachment 9150656 [details] [diff] [review]: ----------------------------------------------------------------- Wow, so glad to see this getting a rewrite! Thanks for working on it. It would be ready for r+ but I got failures when I ran the test locally (see below). I also ran into an error when using a calendar without an email address set for it. (That may already exist?) The header sections for the days and times don't seem to align with the grid lines below it. (The lines between the days in the header fall in between the grid column lines.) Other than that just a few minor comments, questions, and nits. Edit: I updated my m-c and got some different test failures (which I've updated here): Unexpected Results ------------------ comm/calendar/test/browser/eventDialog/browser_attendeesDialog.js FAIL 0 == 6 - JS frame :: chrome://mochitests/content/browser/comm/calendar/test/browser/eventDialog/browser_attendeesDialog.js :: checkAttendeeCells :: line 180 Stack trace: chrome://mochitests/content/browser/comm/calendar/test/browser/eventDialog/browser_attendeesDialog.js:checkAttendeeCells:180 chrome://mochitests/content/browser/comm/calendar/test/browser/eventDialog/browser_attendeesDialog.js:null:186 chrome://mochikit/content/browser-test.js:Tester_execTest/<:1064 chrome://mochikit/content/browser-test.js:Tester_execTest:1104 chrome://mochikit/content/browser-test.js:nextTest/<:927 chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:918 FAIL Uncaught exception - at chrome://mochitests/content/browser/comm/calendar/test/browser/eventDialog/browser_attendeesDialog.js:182 - TypeError: can't access property "getAttribute", cells[i] is undefined Stack trace: checkAttendeeCells@chrome://mochitests/content/browser/comm/calendar/test/browser/eventDialog/browser_attendeesDialog.js:182:7 @chrome://mochitests/content/browser/comm/calendar/test/browser/eventDialog/browser_attendeesDialog.js:186:21 Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1064:34 Tester_execTest@chrome://mochikit/content/browser-test.js:1104:11 nextTest/<@chrome://mochikit/content/browser-test.js:927:14 SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:918:23 FAIL Found an unexpected Calendar:EventDialog at the end of test run - ::: calendar/base/content/dialogs/calendar-event-dialog-attendees.js @@ +164,5 @@ > + dayHeaderOuter.scrollTo(scrollPoint, 0); > + freebusyGrid.scrollTo(scrollPoint, freebusyGrid.scrollTop); > + } > + }, > +}; You really don't like empty lines, do you? :-p @@ +190,5 @@ > + return cal.dtz.jsDateToDateTime(this.start.value, this.startZone._zone); > + }, > + set startValue(value) { > + // Set the zone first, because the change in time will trigger an update. > + this.startZone._zone = value.timezone; I try to avoid saving data on DOM elements to maintain more of a Model/View separation (as with MVC), but that doesn't seem to be your approach here. What do you think? @@ +279,2 @@ > > +addEventListener( Huh, I'm unfamiliar with `addEventListener` at the top level like this. I assume it adds the listener to the `window`. Would it make sense to do `window.addEventListener` to make that explicit? @@ +381,5 @@ > + let organizerElement = attendeeList.appendChild(document.createXULElement("event-attendee")); > + organizerElement.attendee = organizer; > + } else { > + let organizerElement = attendeeList.appendChild(document.createXULElement("event-attendee")); > + organizerElement.value = calendar.getProperty("organizerId").replace(/^mailto:/, ""); I happened to run into an error here when I used a calendar without an email address set for it. Apparently, the `calendar.getProperty("organizerId)` is null in that case. The dialog looked a bit broken in that case. @@ +457,5 @@ > } > > +// Wrap in a block to prevent leaking to window scope. > +{ > + class EventAttendee extends MozXULElement { Would be good to include docstrings for these CEs (and probably some other functions too). @@ +711,5 @@ > + EventAttendee.statusCycle = ["ACCEPTED", "DECLINED", "TENTATIVE"]; > + EventAttendee.userTypeCycle = ["INDIVIDUAL", "GROUP", "RESOURCE", "ROOM"]; > + customElements.define("event-attendee", EventAttendee); > + > + class CalendarDay extends MozXULElement { Docstring for CE.

Comment on attachment 9152649 [details] [diff] [review] 1639763-attendees-dialog-2.diff Review of attachment 9152649 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the updates. Looks good. Confirmed that the test now passes locally.

Comment on attachment 9152649 [details] [diff] [review] 1639763-attendees-dialog-2.diff I suppose we should actually ship this. :-)