1049591 - Fix lots of strict warnings

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Philipp Kewisch [:Fallen][📱📆]]

I don't know if something in the platform changed or if its just my profile, but I now see a lot more strict warnings. We should fix a few of them.

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

This fixes all of them aside from the storage calendar, I'm waiting on the async storage patch before I look into those.

Comment 2

[Stefan Sitter [:ssitter]]

Comment on attachment 8468491 [details] [diff] [review]
Fix - v1

Review of attachment 8468491 [details] [diff] [review]:
-----------------------------------------------------------------

Because my Windows build still doesn't work again I was not able to test any of you changes and review is based on just reading.
r- because of the possible error in calendar-migration-dialog.js

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.js
@@ +995,4 @@
>  
>  calFreeBusyListener.prototype = {
>      onResult: function cFBL_onResult(aRequest, aEntries) {
> +        function neq(aOp) aRequest.id != aOp.id;

maybe this could be written as an anonymous function inside the filter() call as done on other places in this patch?

::: calendar/base/content/dialogs/calendar-migration-dialog.js
@@ +475,2 @@
>  
> +                let calfile = maildir.clone();

I think you removed too much code here, e.g. where is |maildir| created? I assume this would cause undefined reference error.

::: calendar/base/modules/calExtract.jsm
@@ +1194,5 @@
>          let prefixSuffix = {start: res.index, end: res.index + res[0].length,
>                              pattern: pattern, relation: relation};
>          let ch = "\\s*";
> +
> +        let psres;

The methods limitChars() and limitsNum() above call this local variable |result|. I therefore suggest to use the same for better readability. Or something more understandable like |match|.

::: calendar/base/modules/calIteratorUtils.jsm
@@ +120,5 @@
>          if (aComponent && aComponent.componentType == "VCALENDAR") {
>              return cal.ical.subcomponentIterator(aComponent, compType);
>          } else if (aComponent && aComponent.componentType == "XROOT") {
> +            return {
> +                __iterator__: function(aWantKeys) {

I'd like to keep the name for this local function for better error reporting.

::: calendar/base/modules/calRecurrenceUtils.jsm
@@ +18,5 @@
>   */
>  function recurrenceRule2String(recurrenceInfo, startDate, endDate, allDay) {
>      function getRString(name, args) cal.calGetString("calendar-event-dialog", name, args);
> +    function day_of_week(day) Math.abs(day) % 8;
> +    function day_position(day) (Math.abs(day) - day_of_week(day)) / 8 * (day < 0 ? -1 : 1);

Doesn't our style guides states to always use {} brackets, i.e. also for one lines?

::: calendar/base/src/calTimezoneService.js
@@ +571,5 @@
> +    }
> +
> +    function symbolicLinkTarget(filepath) {
> +        try {
> +            var file = (CC["@mozilla.org/file/local;1"]

Is CC and CI defined in this scope? Maybe just use Components.classes and Components.interfaces to avoid problems?

@@ +589,5 @@
> +    function fileFirstZoneLineString(filepath) {
> +        // return first line of file that matches tzRegex (ZoneInfo id),
> +        // or "" if no file or no matching line.
> +        try {
> +            var file = (CC["@mozilla.org/file/local;1"]

CC and CI?

@@ +621,5 @@
> +    }
> +
> +    function weekday(icsDate) {
> +        let calDate = cal.createDateTime(icsDate);
> +        calDate.timezone = tz;

Where does |tz| comes from? Should it be a function argument?

::: calendar/providers/wcap/calWcapCalendarItems.js
@@ +575,4 @@
>              if (err) {
>                  throw err;
>              }
>              var items = this_.parseItems(icalRootComp, calICalendar.ITEM_FILTER_ALL_ITEMS,

In the lines below you changed |this_| to |this|, why note here? And other touched methods still use |this_|.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

Thanks for the review. I dug this up again a week ago when warning were annoying me by creating a separate patch. I've now merged it with this one which had some issues and I still need to do some testing.

Anything I've not commented on below I have fixed, I will upload a new patch shortly.

(In reply to Stefan Sitter from comment #2)
> The methods limitChars() and limitsNum() above call this local variable
> |result|. I therefore suggest to use the same for better readability. Or
> something more understandable like |match|.
I'm not touching calExtract.jsm in the new patch, aside from some extra parenthesis.

> >  function recurrenceRule2String(recurrenceInfo, startDate, endDate, allDay) {
> >      function getRString(name, args) cal.calGetString("calendar-event-dialog", name, args);
> > +    function day_of_week(day) Math.abs(day) % 8;
> > +    function day_position(day) (Math.abs(day) - day_of_week(day)) / 8 * (day < 0 ? -1 : 1);
> 
> Doesn't our style guides states to always use {} brackets, i.e. also for one
> lines?
Yes, for if/while/for/etc. I've been using one-line function expressions when the lines are sufficiently small, but given there are more methods at the top now I went for brackets as you suggested.


> ::: calendar/providers/wcap/calWcapCalendarItems.js
> @@ +575,4 @@
> >              if (err) {
> >                  throw err;
> >              }
> >              var items = this_.parseItems(icalRootComp, calICalendar.ITEM_FILTER_ALL_ITEMS,
> 
> In the lines below you changed |this_| to |this|, why note here? And other
> touched methods still use |this_|.
I fixed the occurrence you mentioned, I just missed that one. For other touched methods, I'd like to avoid making too many changes in the wcap provider since I don't have cycles to test it extensively.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v2

Here we go. Tests pass for me, hope I didn't break anything.

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

Stefan, I've set r? for Mohit since I know you don't have a lot of time for calendar at the moment. You are of course invited to comment on the patch if you like.

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

Mohit, can you take a look at this patch? I'd love to get rid of the warnings and see if this fixes bug 1159633.

Comment 10

[Mohit Kanwal [:redDragon]]

Comment on attachment 8561122 [details] [diff] [review]
Fix - v2

Review of attachment 8561122 [details] [diff] [review]:
-----------------------------------------------------------------

Some minor comments. I think calTransactionManager.js old logic does not match the new one.

::: calendar/base/backend/icaljs/calICSService.js
@@ +49,5 @@
>      },
>  
>      get valueAsIcalString() {
> +        return this.innerObject.getValues().map(v => {
> +            return ICAL.stringify.value(v.toString(), this.innerObject.type);

Arrow functions are still experimental?

::: calendar/base/content/calendar-month-view.xml
@@ +766,3 @@
>                var boxClass;
>                if (this.showFullMonth) {
>                    boxClass = "calendar-month-day-box-" + 

oops extra space.

::: calendar/base/content/dialogs/calendar-migration-dialog.js
@@ +498,3 @@
>          let maildir = this.dirService.get("LocalAppData",
>                                            Components.interfaces.nsILocalFile);
> +        if (!maildir) {

Is omitting the `!maildir.exists()` check intentional?

@@ +505,5 @@
>          maildir.append("Microsoft");
>          maildir.append("Windows Calendar");
>          maildir.append("Calendars");
>  
>          let settingsxml = maildir.clone();

Probably need to move `!settingsxml` check here?

::: calendar/base/src/calTransactionManager.js
@@ +53,5 @@
>      checkWritable: function cTM_checkWritable(transaction) {
> +        transaction = transaction && transaction.wrappedJSObject;
> +        return transaction && item && item.calendar &&
> +               isCalendarWritable(item.calendar) &&
> +               userCanAddItemsToCalendar(item.calendar);

This isn't the same logic as the previous one. Need to check against `transaction.mItem` and `transaction.mOldItem`

::: calendar/providers/wcap/calWcapCalendarItems.js
@@ +404,3 @@
>              }
>          }
>          

style nit. extra tab.

Comment 11

[:aceman]

Yes, please move this patch forward. Now that Calendar is bundled with TB, it spams the console with many of these warnings.

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Mohit Kanwal [:redDragon] from comment #10)
> Arrow functions are still experimental?

Nope, safe for use. I already love them!

> ::: calendar/base/content/dialogs/calendar-migration-dialog.js
> @@ +498,3 @@
> >          let maildir = this.dirService.get("LocalAppData",
> >                                            Components.interfaces.nsILocalFile);
> > +        if (!maildir) {
> 
> Is omitting the `!maildir.exists()` check intentional?
Yes, it will be checked with settings.xml later on anyway. Actually .get() won't return null anyway, it will just throw if the key doesn't exist or return the file object if the key exists.


> 
> @@ +505,5 @@
> >          maildir.append("Microsoft");
> >          maildir.append("Windows Calendar");
> >          maildir.append("Calendars");
> >  
> >          let settingsxml = maildir.clone();
> 
> Probably need to move `!settingsxml` check here?
I don't believe that maildir.clone() will return null, so I think its safe to drop it.

> ::: calendar/base/src/calTransactionManager.js
> @@ +53,5 @@
> >      checkWritable: function cTM_checkWritable(transaction) {
> > +        transaction = transaction && transaction.wrappedJSObject;
> > +        return transaction && item && item.calendar &&
> > +               isCalendarWritable(item.calendar) &&
> > +               userCanAddItemsToCalendar(item.calendar);
> 
> This isn't the same logic as the previous one. Need to check against
> `transaction.mItem` and `transaction.mOldItem`
Thanks for catching this, will fix it.

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v3

Here we go. I've rebased this on top of bug 1159638 which is already checkin-needed.

Comment 14

[Mohit Kanwal [:redDragon]]

Comment on attachment 8603725 [details] [diff] [review]
Fix - v3

Review of attachment 8603725 [details] [diff] [review]:
-----------------------------------------------------------------

All Good. Ship it :-)

Comment 15

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/ee0620ac1185