I would like to format our code with eslint. This will mess up blame for many files, but experience has shown that waiting for all lines to be changed during other patches will never happen.

Based on my experience with automated code and style checking tools in other projects I work on, I think this is a great idea. There will probably be a little pain in the beginning, but it is totally work it in the long run.

I don't expect a full code review as the amount of changes in this bug will not be easy to be reviewed. I will post a try run with all patches once I have everything uploaded.

Missing .eslintrc added

Comment on attachment 8763516 [details] [diff] [review] Initial Rules and minimal automatic fixes - v2 Actually, due to the vast number of patches I think I am going to use mozreview for this bug.

Review commit: https://reviewboard.mozilla.org/r/59694/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59694/

Review commit: https://reviewboard.mozilla.org/r/59696/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59696/

Review commit: https://reviewboard.mozilla.org/r/59698/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59698/

Review commit: https://reviewboard.mozilla.org/r/59700/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59700/

Review commit: https://reviewboard.mozilla.org/r/59702/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59702/

Review commit: https://reviewboard.mozilla.org/r/59704/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59704/

Review commit: https://reviewboard.mozilla.org/r/59706/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59706/

Review commit: https://reviewboard.mozilla.org/r/59708/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59708/

Review commit: https://reviewboard.mozilla.org/r/59710/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59710/

Review commit: https://reviewboard.mozilla.org/r/59712/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59712/

Review commit: https://reviewboard.mozilla.org/r/59714/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59714/

Review commit: https://reviewboard.mozilla.org/r/59716/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59716/

Review commit: https://reviewboard.mozilla.org/r/59718/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59718/

Review commit: https://reviewboard.mozilla.org/r/59720/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59720/

Review commit: https://reviewboard.mozilla.org/r/59722/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59722/

Review commit: https://reviewboard.mozilla.org/r/59724/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59724/

Review commit: https://reviewboard.mozilla.org/r/59726/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59726/

Review commit: https://reviewboard.mozilla.org/r/59728/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59728/

Review commit: https://reviewboard.mozilla.org/r/59730/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59730/

Review commit: https://reviewboard.mozilla.org/r/59732/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59732/

Review commit: https://reviewboard.mozilla.org/r/59734/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59734/

Review commit: https://reviewboard.mozilla.org/r/59736/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59736/

Review commit: https://reviewboard.mozilla.org/r/59738/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59738/

Review commit: https://reviewboard.mozilla.org/r/59740/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59740/

Review commit: https://reviewboard.mozilla.org/r/59742/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59742/

Review commit: https://reviewboard.mozilla.org/r/59744/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59744/

Review commit: https://reviewboard.mozilla.org/r/59746/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59746/

Review commit: https://reviewboard.mozilla.org/r/59748/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59748/

Review commit: https://reviewboard.mozilla.org/r/59750/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59750/

Review commit: https://reviewboard.mozilla.org/r/59752/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59752/

Phew, what a rush! I have created a try run here: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fd2e033596d4 I'm also adding bug 1274728 as a dependency, because I had this one applied when working on the patches and removing it causes bitrot.

Ok, a few test failures. I'll take a look and keep you posted.

(In reply to MakeMyDay from bug 1274728 comment #11) > I haven't taken a look at the patches yet, but there are massive test > failures for that try build, including a timeout of the gdata xpcshell test > for bug 1280898. > > Apart from that, is there an option to exclude such mass changes from blame? > It will be probably useless after landing these patches for a longer time, > especially for that part of the code base that doesn't receive patches > frequently (so nearly everything...). I've got the test failures covered, hopefully this works better: <https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5c0b78f4f13c>. It passes locally for me. There is no such option to exclude from blame. While it may be more annoying to skip through the changeset in blame, it is not impossible to get to the parent changeset and check blame again from there. I've set the user to "eslint <eslint@bugzilla.kewis.ch>" so it becomes more apparent when viewing annotations, and so that not everyone blames me :) I think the benefit outweighs the pain, personally I don't look at blame all too often but I want to fix style for every written patch. If some of the suggestions in https://www.mercurial-scm.org/wiki/BlamePlan are implemented (there is a MOSS grant for this afaik) then maybe we can retroactively skip blame for the final changeset. I can also squash the commits before pushing, this way there is only one revision to parent when browsing blame.

Thanks. Hopefully some if the blame feature will get implemented one day. There still seem some Mozmill failures around. Will the eslint checks be integrated in review (splinter/reviewboard), test, (try) builds or patch landing processes in any way? Or do we need that to run locally once in a while for auditing purposes?

(In reply to MakeMyDay from comment #39) > Thanks. Hopefully some if the blame feature will get implemented one day. > > There still seem some Mozmill failures around. Found them. For some reason using <field name="foo">{}</field> doesn't work, while <field name="foo">new Object()</field> does. I moved it into the constructor instead. There was also another issue with the occurrence prompt. Getting mozmill to run locally was pretty painful, but I think it will be good now: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0095274932cf Due to the nature of this patch there may of course be some fallout not covered by tests. On the bright side, when we catch them through testing, we can write test to make sure it doesn't happen again. > Will the eslint checks be integrated in review (splinter/reviewboard), test, > (try) builds or patch landing processes in any way? Or do we need that to > run locally once in a while for auditing purposes? If we can get this to work, perfect. I fear it will not work out of the box, because our automation doesn't run using mach yet. See also bug 1280896. We'll have to run this ourselves, but the good thing is we won't have to specify each nit for new contributors during review but can ask them to run mach eslint calendar.

Review commit: https://reviewboard.mozilla.org/r/59962/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59962/

Review commit: https://reviewboard.mozilla.org/r/59964/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59964/

Review commit: https://reviewboard.mozilla.org/r/59966/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59966/

Review commit: https://reviewboard.mozilla.org/r/59968/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59968/

Review commit: https://reviewboard.mozilla.org/r/59970/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59970/

Review commit: https://reviewboard.mozilla.org/r/59972/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59972/

Review commit: https://reviewboard.mozilla.org/r/59974/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59974/

Review commit: https://reviewboard.mozilla.org/r/59976/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59976/

Review commit: https://reviewboard.mozilla.org/r/59978/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59978/

Review commit: https://reviewboard.mozilla.org/r/59980/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59980/

* * * [mq]: noobj.diff Review commit: https://reviewboard.mozilla.org/r/59982/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59982/

Review commit: https://reviewboard.mozilla.org/r/59984/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59984/

Review commit: https://reviewboard.mozilla.org/r/59986/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59986/

Review commit: https://reviewboard.mozilla.org/r/59988/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59988/

Review commit: https://reviewboard.mozilla.org/r/59990/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59990/

Review commit: https://reviewboard.mozilla.org/r/59992/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59992/

Review commit: https://reviewboard.mozilla.org/r/59994/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59994/

Review commit: https://reviewboard.mozilla.org/r/59996/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59996/

Review commit: https://reviewboard.mozilla.org/r/59998/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59998/

Review commit: https://reviewboard.mozilla.org/r/60000/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60000/

Review commit: https://reviewboard.mozilla.org/r/60002/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60002/

Review commit: https://reviewboard.mozilla.org/r/60004/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60004/

Review commit: https://reviewboard.mozilla.org/r/60006/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60006/

Review commit: https://reviewboard.mozilla.org/r/60008/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60008/

Review commit: https://reviewboard.mozilla.org/r/60010/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60010/

Review commit: https://reviewboard.mozilla.org/r/60012/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60012/

Review commit: https://reviewboard.mozilla.org/r/60014/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60014/

Review commit: https://reviewboard.mozilla.org/r/60016/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60016/

Review commit: https://reviewboard.mozilla.org/r/60018/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60018/

Review commit: https://reviewboard.mozilla.org/r/60020/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60020/

mozreview commenting needs some UI work :-P Try run passes now, at least with the same failures as on c-c. Let me know if I can do something to make review easier. You may also want to update your reviewboard profile and possibly set your bugzilla displayname to [:MakeMyDay], I had to enter you as a reviewer for each changeset manually because it didn't find your nickname.

Drive by comment, please forgive me for not using the mozreview tool as I'm not used to it. > diff --git a/calendar/lightning/content/messenger-overlay-sidebar.js b/calendar/lightning/content/messenger-overlay-sidebar.js > - var id = null; > - try { id = deck.selectedPanel.id } catch (e) { } > + let id = deck.selectedPanel && deck.selectedPanel.id; Does this has the same meaning as before, i.e. id holds the string value of deck.selectedPanel.id? With C++ background this looks like id is now a boolean variable. > diff --git a/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml b/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml > let attendees = []; > let inputField; > > - for (let i = 1; inputField = this.getInputElement(i); i++) { > - if (inputField && inputField.value != "") { > - // the inputfield already has a reference to the attendee > - // object, we just need to fill in the name. > - let attendee = inputField.attendee.clone(); > - if (attendee.isOrganizer) { > - continue; > + for (let i = 1; true; i++) { > + let inputField = this.getInputElement(i); Problem if inputField is defined twice now? Once before and once inside the for loop?

(In reply to Stefan Sitter from comment #72) > Drive by comment, please forgive me for not using the mozreview tool as I'm > not used to it. No worries, I haven't used it more than 1-2 times myself and prefer the normal tools :) > > > diff --git a/calendar/lightning/content/messenger-overlay-sidebar.js b/calendar/lightning/content/messenger-overlay-sidebar.js > > - var id = null; > > - try { id = deck.selectedPanel.id } catch (e) { } > > + let id = deck.selectedPanel && deck.selectedPanel.id; > > Does this has the same meaning as before, i.e. id holds the string value of > deck.selectedPanel.id? With C++ background this looks like id is now a > boolean variable. It does, if the first variable is truthy, then it evaluates to the second. If not, then it evaluates to the falsy value of the first. It is similar to the let foo = maybeNull || "default"; syntax. > > > diff --git a/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml b/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml > > let attendees = []; > > let inputField; >... > > + let inputField = this.getInputElement(i); > > Problem if inputField is defined twice now? Once before and once inside the > for loop? Ah yes. Not a big problem, the second variable shadows the first, but I should get rid of one. Will fix that in the next round of commits. Thanks for the comment!

Comment on attachment 8763867 [details] Bug 1280898 - Set up eslint for calendar files - initial rules and minimal automatic fixes. https://reviewboard.mozilla.org/r/59962/#review57278 Ok, here's the review for the first patch. If the remaining reviews don't take that long again, I would appreciate if you could merge all the patches before landing to keep blame usable. Hmm, the reviewboard seems not to provide as much context for comments as splinter - at least in the preview here. But there seems to be no option to move the anchor of the comment from with the preview. Let's see what bugzilla makes of it. ::: .eslintignore:45 (Diff revision 1) > +# preprocessed files > +calendar/base/content/dialogs/calendar-migration-dialog.js > +calendar/base/content/calendar-dnd-listener.js > + > +# temporary ignore until gsoc 2016 work is complete > +calendar/base/content/dialogs/calendar-event-dialog.js You also should exclude calendar-item-editing.js and calendar-summary-dialog.js (and maybe more depending on bug 1277972) for the same reason for now. ::: calendar/base/content/agenda-listbox.js:1080 (Diff revision 1) > .getInTimezone(agendaListbox.kDefaultTimezone) > .subtractDate(anow).inSeconds; > if (msuntillstart <= 0) { > - var msuntillend = complistItem.occurrence.endDate > + msuntillend = complistItem.occurrence.endDate > - .getInTimezone(agendaListbox.kDefaultTimezone) > + .getInTimezone(agendaListbox.kDefaultTimezone) > - .subtractDate(anow).inSeconds; > + .subtractDate(anow).inSeconds; can you fix the inconsitent indention here and for the msuntilstart block above? ::: calendar/base/content/dialogs/calendar-summary-dialog.js (Diff revision 1) > */ > function onLoad() { > var args = window.arguments[0]; > var item = args.calendarEvent; > item = item.clone(); // use an own copy of the passed item > - var calendar = item.calendar; as mentioned above it might make sense to exclude calendar-summary-dialog.js here due to GSoC. ::: calendar/base/src/calTimezoneService.js:795 (Diff revision 1) > ((standardStart < daylightStart > ? standardText + daylightText > : daylightText + standardText)+ > - (calProperties.GetStringFromName > - ("TZAlmostMatchesOSDifferAtMostAWeek"))); > + (calProperties.GetStringFromName( > + "TZAlmostMatchesOSDifferAtMostAWeek"))); > } else { you can remove the outer brackets here as well. ::: calendar/base/src/calTimezoneService.js (Diff revision 1) > } > var offsetString = (standardTZOffset+ > (!daylightTZOffset? "": "/"+daylightTZOffset)); > - var warningMsg = (calProperties.formatStringFromName > - ("WarningUsingGuessedTZ", > + var warningMsg = (calProperties.formatStringFromName("WarningUsingGuessedTZ", > + [tzId, offsetString, warningDetail, probableTZSource], 4)); > - [tzId, offsetString, warningDetail, again: outer brackets can be removed. ::: calendar/lightning/components/calItipProtocolHandler.js:13 (Diff revision 1) > var CI = Components.interfaces; > > var ITIP_HANDLER_MIMETYPE = "application/x-itip-internal"; > var ITIP_HANDLER_PROTOCOL = "moz-cal-handle-itip"; > +var NS_ERROR_WONT_HANDLE_CONTENT = 0x805d0001; > Why do you need to assign this here? Later you use Components.results.NS_ERROR_WONT_HANDLE_CONTENT anyway. ::: calendar/resources/content/publish.js:57 (Diff revision 1) > // Ask user to select the calendar that should be published. > // publishEntireCalendar() will be called again if OK is pressed > // in the dialog and the selected calendar will be passed in. > // Therefore return after openDialog(). > - var args = new Object(); > + let args = new Object(); > args.onOk = publishEntireCalendar; ou can switch to {} here, too.

Comment on attachment 8763868 [details] Bug 1280898 - Set up eslint for calendar files - enable block-scoped-var rule. https://reviewboard.mozilla.org/r/59964/#review57324 ::: calendar/base/content/calendar-daypicker.xml:89 (Diff revision 1) > var mainbox = > document.getAnonymousElementByAttribute( > this, "anonid", "mainbox"); > var numChilds = mainbox.childNodes.length; > - for (var i = 0; i < numChilds; i++) { > + for (let i = 0; i < numChilds; i++) { > var child = mainbox.childNodes[i]; Use let here. As mentioned on irc, please add a seperate changeset for the complete conversion of non-toplevel var to let. ::: calendar/base/content/calendar-daypicker.xml:93 (Diff revision 1) > - for (var i = 0; i < numChilds; i++) { > + for (let i = 0; i < numChilds; i++) { > var child = mainbox.childNodes[i]; > child.removeAttribute("checked"); > } > - for (i = 0; i < val.length; i++) { > + for (let i = 0; i < val.length; i++) { > var index = val[i] - 1 - this.weekStartOffset; let again.

Comment on attachment 8763869 [details] Bug 1280898 - Set up eslint for calendar files - enable comma-dangle,comma-spacing,comma-style rules. https://reviewboard.mozilla.org/r/59966/#review57328

Comment on attachment 8763870 [details] Bug 1280898 - Set up eslint for calendar files - enable curly rule. https://reviewboard.mozilla.org/r/59968/#review57332 Is there also a check to detect all missing let keywords in a for() like in most of the comments below? ::: calendar/base/content/calendar-multiday-view.xml:76 (Diff revision 1) > <parameter name="aAttr"/> > <parameter name="aVal"/> > <body><![CDATA[ > var needsrelayout = false; > if (aAttr == "orient") { > - if (this.getAttribute("orient") != aVal) > + if (this.getAttribute("orient") != aVal) { Just collapse this to let needsrelayout = (aAttr == "orien" && this.getAttribute("orient") != aVal); ::: calendar/base/content/calendar-multiday-view.xml:2336 (Diff revision 1) > <parameter name="aVal"/> > <body><![CDATA[ > var needsrelayout = false; > if (aAttr == "orient") { > - if (this.getAttribute("orient") != aVal) > + if (this.getAttribute("orient") != aVal) { > needsrelayout = true; once again ::: calendar/base/content/widgets/minimonth.xml:764 (Diff revision 1) > for (var endPrefix = 0; endPrefix < minLength; endPrefix++) { > var c = dayList[0][endPrefix]; > if (dayList.some(function(dayAbbr) { > return dayAbbr[endPrefix] != c; })) { > if (endPrefix > 0) { > - for (i = 0; i < dayList.length; i++) // trim prefix chars. > + for (i = 0; i < dayList.length; i++) { // trim prefix chars. for (let i = 0;.... ::: calendar/base/content/widgets/minimonth.xml:773 (Diff revision 1) > break; > } > } > } > // 3. trim each day abbreviation to 1 char if unique, else 2 chars. > for (i = 0; i < dayList.length; i++) { for (let i = 0; ... ::: calendar/base/content/widgets/minimonth.xml:775 (Diff revision 1) > } > } > // 3. trim each day abbreviation to 1 char if unique, else 2 chars. > for (i = 0; i < dayList.length; i++) { > var foundMatch = 1; > for (j = 0; j < dayList.length; j++) { for (let j = 0; ... ::: calendar/resources/content/publish.js (Diff revision 1) > } > > > -var publishingListener = > -{ > +var publishingListener = { > + QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIStreamListener]), > - QueryInterface: function(aIId, instance) There are some more occurrences of such QI definitions - see https://dxr.mozilla.org/comm-central/search?q=QueryInterface%3A+fun+path%3Acalendar%2F*&redirect=false Can you include the clean up for the others here, too, or should we file a separate bug for it? ::: calendar/test/mozmill/eventDialog/testEventDialog.js:219 (Diff revision 1) > eventBox.replace("rowNumber", "0").replace("columnNumber", "5"))); > controller.assertNodeNotExist(new elementslib.Lookup(controller.window.document, > eventBox.replace("rowNumber", "0").replace("columnNumber", "6"))); > > - for(row = 1; row < 5; row++) > - for(col = 0; col < 7; col++) > + for (row = 1; row < 5; row++) { > + for (col = 0; col < 7; col++) { for (let row = 1;... for (let col = 1;... ::: calendar/test/mozmill/shared-modules/calendar-utils.js:331 (Diff revision 1) > let calendarTree = (new elementslib.Lookup(controller.window.document, > '/id("messengerWindow")/id("tabmail-container")/id("tabmail")/id("tabpanelcontainer")/' > + 'id("calendarTabPanel")/id("calendarContent")/id("ltnSidebar")/id("calendar-panel")/' > + 'id("calendar-list-pane")/id("calendar-listtree-pane")/id("calendar-list-tree-widget")')) > .getNode(); > - for(i = 0; i < calendarTree.mCalendarList.length; i++) > + for (i = 0; i < calendarTree.mCalendarList.length; i++) { for (let i = 0;... ::: calendar/test/mozmill/views/testTaskView.js:43 (Diff revision 1) > let calendarTree = (new elementslib.Lookup(controller.window.document, > '/id("messengerWindow")/id("tabmail-container")/id("tabmail")/id("tabpanelcontainer")/' > + 'id("calendarTabPanel")/id("calendarContent")/id("ltnSidebar")/id("calendar-panel")/' > + 'id("calendar-list-pane")/id("calendar-listtree-pane")/id("calendar-list-tree-widget")')) > .getNode(); > - for(i = 0; i < calendarTree.mCalendarList.length; i++) > + for (i = 0; i < calendarTree.mCalendarList.length; i++) { for (let i = 0;...

Comment on attachment 8763871 [details] Bug 1280898 - Set up eslint for calendar files - enable key-spacing rule. https://reviewboard.mozilla.org/r/59970/#review57480 r- for now to get an updated patch. As discussed on irc the rule should be relaxed to only note about { foo : "bar"} but allow { foo: "bar" }

Comment on attachment 8763872 [details] Bug 1280898 - Set up eslint for calendar files - enable new-parens,no-array-constructor rule. https://reviewboard.mozilla.org/r/59972/#review57482

Comment on attachment 8763873 [details] Bug 1280898 - Set up eslint for calendar files - enable no-catch-shadow rule. https://reviewboard.mozilla.org/r/59974/#review57484

Comment on attachment 8763874 [details] Bug 1280898 - Set up eslint for calendar files - enable no-cond-assign rule. https://reviewboard.mozilla.org/r/59976/#review57486 ::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xml:383 (Diff revision 1) > <getter><![CDATA[ > let attendees = []; > let inputField; > > - for (let i = 1; inputField = this.getInputElement(i); i++) { > - if (inputField && inputField.value != "") { > + for (let i = 1; true; i++) { > + let inputField = this.getInputElement(i); No let here. inputFied has already been declared outside of the for loop. ::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xml:430 (Diff revision 1) > > <property name="organizer"> > <getter><![CDATA[ > let inputField; > - for (let i = 1; inputField = this.getInputElement(i); i++) { > - if (inputField && inputField.value != "") { > + for (let i = 1; true; i++) { > + let inputField = this.getInputElement(i); The same here.

Comment on attachment 8763875 [details] Bug 1280898 - Set up eslint for calendar files - enable no-debugger rule. https://reviewboard.mozilla.org/r/59978/#review57488

Comment on attachment 8763876 [details] Bug 1280898 - Set up eslint for calendar files - enable yoda rule. https://reviewboard.mozilla.org/r/59980/#review57490

Comment on attachment 8763877 [details] Bug 1280898 - Set up eslint for calendar files - enable no-new-object rule. https://reviewboard.mozilla.org/r/59982/#review57492

https://reviewboard.mozilla.org/r/59968/#review57332 Yes, this is no-undef. I will take care of that separately, it is a big one. > Just collapse this to > > let needsrelayout = (aAttr == "orien" && this.getAttribute("orient") != aVal); There is a separate rule for this (lonely-if), I will take care there. > for (let i = 0; ... > > ::: calendar/base/content/widgets/minimonth.xml:764 > (Diff revision 1) > > for (var endPrefix = 0; endPrefix < minLength; endPrefix++) { > > var c = dayList[0][endPrefix]; > > if (dayList.some(function(dayAbbr) { > > return dayAbbr[endPrefix] != c; })) { > > if (endPrefix > 0) { > > - for (i = 0; i < dayList.length; i++) // trim prefix chars. > > + for (i = 0; i < dayList.length; i++) { // trim prefix chars. > > for (let i = 0;.... i and j are defined further up, but for other issues with missing let you mentioned I will eventually introduce the no-undef rule. > There are some more occurrences of such QI definitions - see https://dxr.mozilla.org/comm-central/search?q=QueryInterface%3A+fun+path%3Acalendar%2F*&redirect=false > > Can you include the clean up for the others here, too, or should we file a separate bug for it? I'll take care of this in another bug

https://reviewboard.mozilla.org/r/59964/#review57324 > Use let here. As mentioned on irc, please add a seperate changeset for the complete conversion of non-toplevel var to let. Yes, will do this in a separate changeset

https://reviewboard.mozilla.org/r/59962/#review57278 > You also should exclude calendar-item-editing.js and calendar-summary-dialog.js (and maybe more depending on bug 1277972) for the same reason for now. I kind of hoped we could land the first gsoc patch before this one one and I would rebase, but if needed I can also ignore those files and keep those for later. > Why do you need to assign this here? Later you use Components.results.NS_ERROR_WONT_HANDLE_CONTENT anyway. Unfinished, good catch. Components.results.NS_ERROR_WONT_HANDLE_CONTENT is actually undefined, hence I introduce the variable, but looks like I didn't actually use it. > ou can switch to {} here, too. Will be taken care of in a later patch

https://reviewboard.mozilla.org/r/59976/#review57486 > No let here. inputFied has already been declared outside of the for loop. This is the issue ssitter mentioned, fixed locally.

Comment on attachment 8763878 [details] Bug 1280898 - Set up eslint for calendar files - enable spaced-comment rule. https://reviewboard.mozilla.org/r/59984/#review57500 ::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xml:347 (Diff revision 1) > input.removeAttribute("value"); > input.attendee = newAttendee; > > // set role and participation status > - //status.setAttribute("status", newAttendee.participationStatus); > - //role.setAttribute("role", newAttendee.role); > + // status.setAttribute("status", newAttendee.participationStatus); > + // role.setAttribute("role", newAttendee.role); These comments seem to be obsolete. ::: calendar/base/content/widgets/calendar-widgets.xml:688 (Diff revision 1) > let flavor = {}; > let data = {}; > // nsITransferable sucks when it comes to trying to add extra flavors. > // This will throw NS_ERROR_FAILURE, so as a workaround, we use the > // sourceNode property and get the event that way > - //transfer.getAnyTransferData(flavor, data, {}); > + // transfer.getAnyTransferData(flavor, data, {}); Can we make the extra indention 2 whitespaces instead of one in such cases? ::: calendar/base/src/calCalendarManager.js:335 (Diff revision 1) > db.beginTransactionAs(Components.interfaces.mozIStorageConnection.TRANSACTION_EXCLUSIVE); > try { > if (db.tableExists("cal_calendars_prefs")) { > // Check if we need to upgrade: > let version = this.getSchemaVersion(db); > - //cal.LOG("*** Calendar schema version is: " + version); > + // cal.LOG("*** Calendar schema version is: " + version); This seems to be a remainder of some debug code. Either remove the // or get rid of that line altogether. ::: calendar/base/src/calItemBase.js:463 (Diff revision 1) > (paramName in this.mPropertyParams[propName])); > }, > > - //void setPropertyParameter(in AString aPropertyName, > - // in AString aParameterName, > - // in AUTF8String aParameterValue); > + // void setPropertyParameter(in AString aPropertyName, > + // in AString aParameterName, > + // in AUTF8String aParameterValue); We should convert this kind of comments to /** * Sets a paramter for a given parameter * * @param AString aPropertyName * @param AString aParameterName * @param AUTF8String aParameterValue */ But maybe this is something for a separate bug. ::: calendar/base/src/calUtils.js:97 (Diff revision 1) > function getDateFormatter() { > return Components.classes["@mozilla.org/calendar/datetime-formatter;1"] > .getService(Components.interfaces.calIDateTimeFormatter); > } > > -/// @return the UTC timezone. > +// @return the UTC timezone. Only one whitespace after // ::: calendar/base/src/calUtils.js:105 (Diff revision 1) > UTC.mObject = getTimezoneService().UTC; > } > return UTC.mObject; > } > > -/// @return the floating timezone. > +// @return the floating timezone. Same here. ::: calendar/base/src/calUtils.js:1174 (Diff revision 1) > mInterfaces: null, > > // Iterating the inteface bag iterates the interfaces it contains > [Symbol.iterator]: function() { return this.mInterfaces[Symbol.iterator](); }, > > - /// internal: > + // internal: Only one whitespace after // ::: calendar/base/src/calUtils.js:1180 (Diff revision 1) > init: function calInterfaceBag_init(iid) { > this.mIid = iid; > this.mInterfaces = []; > }, > > - /// external: > + // external: here, too. ::: calendar/providers/ics/calICSCalendar.js:877 (Diff revision 1) > this.mCalendar.readOnly = true; > this.mCalendar.notifyError(aErrNo, aMessage); > } > }; > > -/*************************** > +/* Use /** here ::: calendar/providers/storage/calStorageCalendar.js:673 (Diff revision 1) > }); > }, > getItems_: function cSC_getItems_(aItemFilter, aCount, > aRangeStart, aRangeEnd, aListener) > { > - //var profStartTime = Date.now(); > + // var profStartTime = Date.now(); Debug code once again (further occurences down until line 766). If we think this is useful to have it in the code base, we probably should wrap it in an if (a.perf.debug.pref) {} block, otherwise we should consider to remove it. Maybe worth a separate bug. ::: calendar/providers/wcap/calWcapCalendar.js:8 (Diff revision 1) > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > Components.utils.import("resource://calendar/modules/calUtils.jsm"); > Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > > -function calWcapCalendar(/*optional*/session, /*optional*/calProps) { > +function calWcapCalendar(/* optional */ session, /* optional */ calProps) { Can't we add a documentation block above mentioning that the params are optional? ::: calendar/test/mozmill/eventDialog/testEventDialogModificationPrompt.js:139 (Diff revision 1) > > // delete it > // XXX somehow the event is selected at this point, this didn't use to be the case > // and can't be reproduced manually > - /*controller.click(new elementslib.Lookup(controller.window.document, > - calUtils.getEventBoxPath(controller, "day", calUtils.EVENT_BOX, undefined, 1, 8)));*/ > + /* controller.click(new elementslib.Lookup(controller.window.document, > + calUtils.getEventBoxPath(controller, "day", calUtils.EVENT_BOX, undefined, 1, 8))); */ It looks this is a candidate for removal. ::: calendar/test/unit/test_items.js:262 (Diff revision 1) > let e = cal.createEvent(); > > e.alarmLastAck = cal.createDateTime("20120101T010101"); > > // Our items don't support this yet > - //equal(e.getProperty("X-MOZ-LASTACK"), "20120101T010101"); > + // equal(e.getProperty("X-MOZ-LASTACK"), "20120101T010101"); The same here. ::: calendar/test/unit/test_recur.js:36 (Diff revision 1) > let occurrences = event.recurrenceInfo.getOccurrences(start, end, 0, {}); > > // Check number of items > dump("Expected " + expected.length + " occurrences\n"); > dump("Got: " + recdates.map(x => x.toString()) + "\n"); > - //equal(recdates.length, expected.length); > + // equal(recdates.length, expected.length); ...and here

https://reviewboard.mozilla.org/r/59984/#review57500 > We should convert this kind of comments to > > /** > * Sets a paramter for a given parameter > * > * @param AString aPropertyName > * @param AString aParameterName > * @param AUTF8String aParameterValue > */ > > But maybe this is something for a separate bug. Yes, probably better in a separate bug > Use /** here /** is used for jsdoc-style comments specific to the identifier following it, which is not the case here. We use this comment to separate code into a new section. Therefore I will stick with /* > The same here. This actually looks like it is something to be supported in the future, might as well keep it in. > ...and here This one actually looks like it should be uncommented. Tests run through with it, so that is what I am doing.

Comment on attachment 8763879 [details] Bug 1280898 - Set up eslint for calendar files - enable radix rule. https://reviewboard.mozilla.org/r/59986/#review57506 ::: calendar/base/content/calendar-statusbar.js:97 (Diff revision 1) > } > if (this.spinning != Components.interfaces.calIStatusObserver.NO_PROGRESS) { > if (this.spinning == Components.interfaces.calIStatusObserver.DETERMINED_PROGRESS) { > if (!this.mCalendars[aCalendar.id] || this.mCalendars[aCalendar.id] === undefined) { > this.mCalendars[aCalendar.id] = true; > - this.mStatusBar.value = (parseInt(this.mStatusBar.value) + this.mCalendarStep); > + this.mStatusBar.value = (parseInt(this.mStatusBar.value, 10) + this.mCalendarStep); The outer brackets shouldn't be needed here.

Comment on attachment 8763871 [details] Bug 1280898 - Set up eslint for calendar files - enable key-spacing rule. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59970/diff/1-2/