Comment on attachment 9028858 [details] [diff] [review] freebusy-day.patch Review of attachment 9028858 [details] [diff] [review]: ----------------------------------------------------------------- Could lose the mNotation for member variables while you're touching all of this ::: calendar/base/content/dialogs/calendar-event-dialog.css @@ -36,5 @@ > -moz-user-focus: normal; > } > > freebusy-day { > - -moz-binding: url(chrome://calendar/content/calendar-event-dialog-freebusy.xml#freebusy-day); you didn't include the freebusy-day removal in the patch, so it's not easy to compare

I get a bunch of JavaScript error: chrome://calendar/content/calendar-event-dialog-freebusy.xml, line 1302: TypeError: this.getListItem(...) is undefined; can't access its "getElementsByTagName" property ... that would be in the "getFreeBusyElement" method

(In reply to undefined from comment #undefined) > they are not caused by this patch. if you see the logs without applying this patch, you ll even then find them there.

(In reply to Magnus Melin [:mkmelin] from comment #3) > I get a bunch of > JavaScript error: > chrome://calendar/content/calendar-event-dialog-freebusy.xml, line 1302: > TypeError: this.getListItem(...) is undefined; can't access its > "getElementsByTagName" property > > ... that would be in the "getFreeBusyElement" method they are not caused by this patch. if you see the logs without applying this patch, you ll even then find them there.

This patch will be applied on top of Bug 1509837.

This patch applies on top of Bug 1509837.

This patch applies on top of Bug 1509837.

Comment on attachment 9030074 [details] [diff] [review] freebusy-day.patch Review of attachment 9030074 [details] [diff] [review]: ----------------------------------------------------------------- LGTM ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-bindings.js @@ +177,5 @@ > + } > + > + // First set the formatted date string as title > + let dateValue = this.zoomFactor > 100 ? this.dateFormatter.formatDateShort(date) > + : this.dateFormatter.formatDateLong(date); : goes on the previous line

(In reply to undefined from comment #undefined) > https://searchfox.org/comm-central/source/calendar/.eslintrc.js#372

according to this rule it should be at after the line break..

Ok, looks like a calendar only thing. It's an non-default configuration not used elsewhere in mozilla eslint rules either.

Comment on attachment 9050608 [details] [diff] [review] freebusy-day.patch Review of attachment 9050608 [details] [diff] [review]: ----------------------------------------------------------------- I haven't got to tested the patch, so some comments mainly on the doc blocks and some style nits for now. Pleaese check especially the doc blocks, since the comments apply on all of them. ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +264,5 @@ > > customElements.define("calendar-event-freebusy-timebar", MozCalendarEventFreebusyTimebar); > + > + > +class MozFreebusyDay extends MozXULElement { Please add a doc block for the class itself describing its purpose. @@ +303,5 @@ > + /** > + * Zooms time slots and removes children. > + * > + * @param {Number} val new mZoomFactor value > + * @returns {Number} new mZoomFactor value The documentation should not expose details of the implementation like internal variable names but give a comprehensible description of the purpose of the function. So, |new zoom factor| seems suitable here for the parameter and the return value description, like |Sets the zoom factor for the free busy grid| does for the setter description. This comment applies in general for all doc blocks in this file, so please revisit them. @@ +348,5 @@ > + /** > + * Sets start date of the event and make it immutable. > + * > + * @param {calDateTime} val mStartDate value > + * @returns {calDateTime} mStartDate value calIDateTime @@ +409,5 @@ > + /** > + * Updates the date object passed according to startHour and endHour. > + * > + * @param {calDateTime} val date object to be modified > + * @returns {calDateTime} modified date object calIDateTime @@ +423,5 @@ > + date.isDate = false; > + > + if (!this.dateFormatter) { > + this.dateFormatter = Cc["@mozilla.org/calendar/datetime-formatter;1"] > + .getService(Ci.calIDateTimeFormatter); Please add some indentation here, so that the leading dot is vertically aligned with the [ above @@ +428,5 @@ > + } > + > + // First set the formatted date string as title > + let dateValue = this.zoomFactor > 100 ? this.dateFormatter.formatDateShort(date) > + : this.dateFormatter.formatDateLong(date); Please add some indentation here, so that the : is vertically aligned with the ? above @@ +439,5 @@ > + if (hours.childNodes.length <= 0) { > + let template = document.createXULElement("text"); > + template.className = "freebusy-timebar-hour"; > + let count = Math.ceil( > + (this.endHour - this.startHour) * 60 / step_in_minutes); Doesn't this fit into a single line not exceeding 100 chracters?

Comment on attachment 9056288 [details] [diff] [review] freebusy-day.patch Review of attachment 9056288 [details] [diff] [review]: ----------------------------------------------------------------- Please re-check documentation for the issues I've brought up before.