Comment on attachment 9039845 [details] [diff] [review] freebusy-grid.patch ui-r+ because it looks like with the binding. But two things I observed: - the rows haven't always the same height as the time grid rows when changing the height of the attendee dialog. This can be because the time grid isn't yet converted to CE. This needs to be checked when it will be moved to CE too. - When I make the dialog tall and then shrink it again, the freebusy-grid paints into the box below. I'll attach a screenshot. This happens already with the pre-patch version, so it's not an issue by this patch but if it's easily fixable then do it in this patch. I only tested it on Linux but this should look on all platforms the same.

Comment on attachment 9040521 [details] [diff] [review] freebusy-grid.patch The list isn't correctly aligned. The attendee-spacer-bottom is on the wrong position on the top and maybe because of this you added the -5px to the attendee-spacer-top. The patch does also not apply on tip.

Comment on attachment 9040549 [details] [diff] [review] freebusy-grid.patch Thanks, this works now much better.

Comment on attachment 9040549 [details] [diff] [review] freebusy-grid.patch Review of attachment 9040549 [details] [diff] [review]: ----------------------------------------------------------------- Adding an invitee, I get JavaScript error: chrome://calendar/content/calendar-event-dialog-attendees-custom-elements.js, line 75: TypeError: this.scrollToIndex is not a function Clicking the right area, and the left area in the dialog, I also get JavaScript error: chrome://messenger/content/richlistbox.xml, line 857: TypeError: this.currentItem._fireEvent is not a function ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +ChromeUtils.import("resource://gre/modules/Preferences.jsm"); this needs updating now @@ +5,5 @@ > +ChromeUtils.import("resource://gre/modules/Preferences.jsm"); > + > +/* global MozXULElement */ > + > +class MozFreebusyGrid extends MozXULElement { please add jsdoc documentation. also describe the typical usage @@ +155,5 @@ > + > + onChangeCalendar(calendar) { } > + > + /** > + * appends a new empty row Please capitalize + . @@ +222,5 @@ > + } > + > + /** > + * updateFreeBusy(), implementation of the core functionality of this binding > + */ . But, binding? Maybe figure out what this means and clarify? @@ +395,5 @@ > + this.mMaxFreeBusy--; > + } > + > + /** > + * gets the next row from the top down Capitalize + . @@ +471,5 @@ > + } > + } > +} > + > +customElements.define("freebusy-grid", MozFreebusyGrid); calendar-event-freebusy-listing perhaps?

Comment on attachment 9044502 [details] [diff] [review] freebusy-grid.patch Review of attachment 9044502 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +6,5 @@ > + > +/* global MozXULElement */ > + > +/** > + * MozCalendarEventFreebusyGrid MozCalendarEventFreebusyGrid is the container element holding rows for each persons' free and busy time slots. It is typically used in the dialog where you can select attendees whom to invite to an event. @@ +8,5 @@ > + > +/** > + * MozCalendarEventFreebusyGrid > + * @augments {MozElements.RichListBox} > + */ I used @autgments somewhere, but perhaps @extends is preferable (at least more common in mozilla code)

Comment on attachment 9044513 [details] [diff] [review] freebusy-grid.patch Review of attachment 9044513 [details] [diff] [review]: ----------------------------------------------------------------- Seems to be working, but you didn't remove the binding

Comment on attachment 9047992 [details] [diff] [review] freebusy-grid.patch Review of attachment 9047992 [details] [diff] [review]: ----------------------------------------------------------------- Can you take my comments from bug 1522473 and apply them here as well? This would make the review easier.

Comment on attachment 9050251 [details] [diff] [review] freebusy-grid.patch Review of attachment 9050251 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +302,5 @@ > + * > + * @param {Number} val new mZoomFactor value > + * @returns {Number} new mZoomFactor value > + */ > + set zoomFactor(val) { I am not sure what should be the proper comment here. Please suggest. @@ +443,5 @@ > + * > + * @param {Number} val Scroll offset value > + * @return {Number} Scroll offset value > + */ > + set scroll(val) { Also here.

Comment on attachment 9050251 [details] [diff] [review] freebusy-grid.patch Review of attachment 9050251 [details] [diff] [review]: ----------------------------------------------------------------- If you don't know what a method does, check the code. Sometimes you'll have to go and see what the code that method is calling does. I've left a few comments, please take them into account for the whole patch. ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +297,5 @@ > + window.addEventListener("unload", this.onUnload.bind(this), true); > + } > + > + /** > + * Zooms time slots and container element accordingly. This one is ok @@ +299,5 @@ > + > + /** > + * Zooms time slots and container element accordingly. > + * > + * @param {Number} val new mZoomFactor value Don't mention mMembers in comments. the lowercase m stands for member, and internal members should be considered a black box w.r.t. comments. @@ +313,5 @@ > + return val; > + } > + > + /** > + * Returns mZoomFactor property. While you are here can you move the getter before the setter? You can just write "Gets the zoom factor for the freebusy grid" here. Normally it shouldn't be that blunt, but in this case it makes sense because that is easily understandable. Everyone knows what a zoom factor w.r.t. a HTML ui is. @@ +323,5 @@ > + } > + > + /** > + * Sets mForce24Hours property and all freebusy-row's mForce24Hours to a new value, updates > + * endHour, startHour to a new value. You are describing the code to closely here. If you describe every single line then you'll have trouble when code is added or removed. Maybe something like "Forces the freebusy grid into 24 hours mode, which <does thing with UI>" @@ +438,5 @@ > + return this.getFreeBusyElement(1).documentSize; > + } > + > + /** > + * Sets mScrollOffset value and and scroll property of all freebusy-row elements, which So to find out what the best comment here is, figure out what setting the scroll property does on a high level, and what mScrollOffset is used for. I don't know off hand, but I am sure this is easy to find out. @@ +491,5 @@ > + > + /** > + * Appends a new empty row. > + * > + * @param {Node} templateNode Node to be cloned Node or Element? @@ +530,5 @@ > + * This event handler is executed when modify event is emitted. This event is emitted when > + * attendees-list element is modified. freebusy-grid remains synced with the modified > + * attendees-list using this modify event. > + * > + * @param {object} event Event object for the element on which event was triggered Object, and needs to be more specific if possible, as noted in other review comments. if this is a custom object with a few properties, you can also document it like: ``` @param {Object} event the thing @param {string} event.member a thing within the thing @param {number} event.othermem another thing within the thing ``` @@ +565,5 @@ > + this.updateFreeBusy(); > + } > + > + /** > + * updateFreeBusy(), implementation of the core functionality of this custom element. I don't know what this means. @@ +725,5 @@ > + this.updateFreeBusy(); > + } > + > + /** > + * This method returns the <xul:richlistitem> at row number 'row'. If you use `row` instead it may show as a code block in generated docs. @@ +767,5 @@ > + > + /** > + * Gets the next row from the top down. > + * > + * @returns {?Object} Next dummy row or null if there isn't any This looks like an Element not a generic Object.

Comment on attachment 9056276 [details] [diff] [review] freebusy-grid.patch Review of attachment 9056276 [details] [diff] [review]: ----------------------------------------------------------------- what exactly is the diff between node and element? Every element is node but all nodes are not element, I guess. ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +439,5 @@ > + } > + > + /** > + * Sets scroll offset value of freebusy grid element and and scroll property of all > + * freebusy-row elements. I changed scroll offset value of the freebusy-grid element and I couldn't find any difference in UI atleast on mac, that's why I couldn't write a better comment than this.

Comment on attachment 9058057 [details] [diff] [review] freebusy-grid.patch Review of attachment 9058057 [details] [diff] [review]: ----------------------------------------------------------------- Similar documentation issues as flagged in other previous reviews, please fix.

Comment on attachment 9059749 [details] [diff] [review] freebusy-grid.patch Review of attachment 9059749 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-event-dialog-attendees.js @@ +973,5 @@ > } > } > > /** > + * This listener is used in calendar-event-dialog-attendees-custom-elements.js inside the Seems there is a whitespace at eol, can you run the linter as well?