Closed Bug 1346808 Opened 4 years ago Closed 3 years ago

Make calUtils.js functions independent of window

Categories

(Calendar :: General, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

Attachments

(2 files, 1 obsolete file)

Some of the calUtils.js functions are meant to run in a scope that has a window. Moving to calUtils.jsm we should stop doing this.
Attached patch Fix - v1 (obsolete) — Splinter Review
Attachment #8846663 - Flags: review?(makemyday)
Comment on attachment 8846663 [details] [diff] [review]
Fix - v1

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

Thanks. r+ with the missing prefixes added.

::: calendar/base/content/dialogs/calendar-migration-dialog.js
@@ +559,3 @@
>                      break;
>                  default:
> +                    cal.showError(calGetString("calendar", "unableToRead") + icsFile.path + "\n"+ ex, window);

Please use cal.calGetString() instead (here an at some other places in this patch).
Attachment #8846663 - Flags: review?(makemyday) → review+
I have the cal.calGetString changes lined up in a different patch in my queue, I'd prefer to fix only the issue in the bug :) Will land this when I have my queue cleaned up.
Priority: -- → P1
Attached patch Fix v2Splinter Review
After talking to Philipp I updated the patch, editing an additional usage of showError() in calendar-alarm-dialog.js. Carrying over r+.
Attachment #8846663 - Attachment is obsolete: true
Attachment #8871837 - Flags: review+
Thanks for updating the patch! The next one in my queue is bug 1347149.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/a4458b9e5377a6cb415bedee8fddf7e677a2b7ce
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 5.7
I am getting:

> Timestamp: 5/29/2017, 1:44:05 PM
> Error: ReferenceError: gCalendarStatusFeedback is not defined
> Source File: resource://calendar/modules/calUtils.jsm -> file:///C:/Program%20Files/SeaMonkey
> /extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calUtils.js
> Line: 1871

Shouldn't 
 comp.setStatusObserver(gCalendarStatusFeedback, chromeWindow);
be 
 comp.setStatusObserver(aWindow.gCalendarStatusFeedback, chromeWindow);
Flags: needinfo?(mschroeder)
Attached patch Follow-up fixSplinter Review
Flags: needinfo?(mschroeder)
Attachment #8872355 - Flags: review?(ssitter)
Comment on attachment 8872355 [details] [diff] [review]
Follow-up fix

That looks right.
Attachment #8872355 - Flags: review?(ssitter) → review+
You need to log in before you can comment on or make changes to this bug.