Fix lots of strict warnings

RESOLVED FIXED in 4.3

Status

Calendar
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

unspecified

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8468491 [details] [diff] [review]
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.
Attachment #8468491 - Flags: review?(ssitter)

Comment 2

3 years ago
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_|.
Attachment #8468491 - Flags: review?(ssitter) → review-
(Assignee)

Updated

2 years ago
Flags: needinfo?(philipp)
(Assignee)

Comment 3

2 years ago
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.
(Assignee)

Comment 4

2 years ago
Created attachment 8561122 [details] [diff] [review]
Fix - v2

Here we go. Tests pass for me, hope I didn't break anything.
Attachment #8468491 - Attachment is obsolete: true
Flags: needinfo?(philipp)
(Assignee)

Updated

2 years ago
Attachment #8561122 - Flags: review?(mohit.kanwal)
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
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.
Flags: needinfo?(mohit.kanwal)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1159635
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1159636
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1159633
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.
Attachment #8561122 - Flags: review?(mohit.kanwal) → review-

Comment 11

2 years ago
Yes, please move this patch forward. Now that Calendar is bundled with TB, it spams the console with many of these warnings.
(Assignee)

Comment 12

2 years ago
(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.
Flags: needinfo?(mohit.kanwal)
(Assignee)

Comment 13

2 years ago
Created attachment 8603725 [details] [diff] [review]
Fix - v3

Here we go. I've rebased this on top of bug 1159638 which is already checkin-needed.
Attachment #8561122 - Attachment is obsolete: true
Attachment #8603725 - Flags: review?(mohit.kanwal)
Comment on attachment 8603725 [details] [diff] [review]
Fix - v3

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

All Good. Ship it :-)
Attachment #8603725 - Flags: review?(mohit.kanwal) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 15

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

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.1
Target Milestone: 4.1 → 4.3
You need to log in before you can comment on or make changes to this bug.