Use cal prefix for all calUtils functions

RESOLVED FIXED in 5.7

Status

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

unspecified
Dependency tree / graph

Details

Attachments

(1 attachment, 6 obsolete attachments)

The upcoming patch will make sure that all current calUtils functions use the cal. prefix. While we may eventually decide differently based on bug 905097 comment 1, I think it would be a good idea to stay consistent and always use the cal. prefix (besides, I already had most of it written).
Posted patch Use cal prefix - v1 (obsolete) — Splinter Review
Attachment #8847100 - Flags: review?(makemyday)
Posted patch Use cal prefix - v1 (obsolete) — Splinter Review
Fixed commit message
Attachment #8847100 - Attachment is obsolete: true
Attachment #8847100 - Flags: review?(makemyday)
Attachment #8847103 - Flags: review?(makemyday)
Blocks: 1347211
Posted patch Use cal prefix - v2 (obsolete) — Splinter Review
And now without accidentally adding calendar-event-dialog back in
Attachment #8847102 - Attachment is obsolete: true
Attachment #8847166 - Flags: review?(makemyday)
Posted patch Use cal prefix - v3 (obsolete) — Splinter Review
Attachment #8847166 - Attachment is obsolete: true
Attachment #8847166 - Flags: review?(makemyday)
Attachment #8847178 - Flags: review?(makemyday)

Comment 6

2 years ago
Comment on attachment 8847178 [details] [diff] [review]
Use cal prefix - v3

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

r+ with the nits fixed.

As we now have several cal.calSomething names (like cal.calGetString), can we please consolidate the names when moving over from calUtils to calUtils.jsm to cal.something?

::: calendar/base/content/calendar-base-view.xml
@@ +245,2 @@
>          let alarmService = Components.classes["@mozilla.org/calendar/alarm-service;1"]
>                             .getService(Components.interfaces.calIAlarmService);

Can you enlarge the indentation here to align the dots?

@@ +552,5 @@
>        <method name="deleteItemsFromCalendar">
>          <parameter name="aCalendar"/>
>          <body><![CDATA[
>            /* This method must be implemented in subclasses. */
> +          throw Components.results.NS_cal.ERROR_NOT_IMPLEMENTED;

Let's leave it with NS_ERROR_NOT_IMPLEMENTED ;-)

::: calendar/base/content/calendar-statusbar.js
@@ +103,1 @@
>                                                   [this.mCurIndex, this.mCalendarCount]);

indentation.
Attachment #8847178 - Flags: review?(makemyday) → review+

Updated

2 years ago
Attachment #8847103 - Flags: review?(makemyday) → review+
(In reply to [:MakeMyDay] from comment #6)
> Comment on attachment 8847178 [details] [diff] [review]
> Use cal prefix - v3
> 
> Review of attachment 8847178 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the nits fixed.
> 
> As we now have several cal.calSomething names (like cal.calGetString), can
> we please consolidate the names when moving over from calUtils to
> calUtils.jsm to cal.something?
Yes, this will happen in another patch.


> > +          throw Components.results.NS_cal.ERROR_NOT_IMPLEMENTED;
> 
> Let's leave it with NS_ERROR_NOT_IMPLEMENTED ;-)
Ha! Great catch :)

Will fix these issues before checkin!
Priority: -- → P1
Posted patch Updated and Combined patch (obsolete) — Splinter Review
After talking to Philipp, I updated and combined the patches to a check-in ready version.

Philipp, please set the checkin-needed keyword if you think it can land.
Attachment #8847103 - Attachment is obsolete: true
Attachment #8847178 - Attachment is obsolete: true
Flags: needinfo?(philipp)
Attachment #8874551 - Flags: review+
Updated the patch after some try-server runs
Attachment #8874551 - Attachment is obsolete: true
Attachment #8875458 - Flags: review+
Duplicate of this bug: 905101
After talking to Philipp over IRC, I set the checkin-needed keyword myself.
Flags: needinfo?(philipp)
Keywords: checkin-needed

Comment 12

2 years ago
https://hg.mozilla.org/comm-central/rev/4bafb6c2ddaf96fce9afed6ad76793224927207e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 5.7

Comment 13

2 years ago
Add-on Lightning Calendar Tabs is now broken:
ReferenceError: getPrefSafe is not defined       multiweek_tabs.js:39:3
ReferenceError: getDateFormatter is not defined  month_tabs.js:44:7

Looks like they need to prepend cal.getDateFormatter, but getPrefSafe seems to have disappeared.

Comment 14

2 years ago
BTW, I fixed the add-on by adding "cal." at month_tabs.js. Multiweek tabs still don't work since getPrefSafe was removed ages ago, I didn't look where, but it's mentioned in bug 1032172.
Bug 923026 marked getPrefSafe() as deprecated in Lighting 3.2 about 3 years ago. Please report errors to the extension author.

Comment 16

2 years ago
I did ;-). As I said in comment #14, I've fixed the function I need myself. Sorry about the noise.
You need to log in before you can comment on or make changes to this bug.