Mass replace the Components.interface/Components.utils uses with Ci/Cu in Calendar

RESOLVED FIXED in 6.8

Status

defect
P3
normal
RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: jorgk, Assigned: martinschroeder)

Tracking

(Blocks 1 bug)

Trunk
Dependency tree / graph

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

a year ago
+++ This bug was initially created as a clone of Bug #1436605 +++

In bug 1436605 were replaced everything in C-C apart from Calendar. Calendar was doing some refactoring at the time, the replacement will have to happen there in due course.
Reporter

Updated

a year ago
Summary: Mass replace the Components.interface/Components.utils uses with Ci/Cu in Calendad → Mass replace the Components.interface/Components.utils uses with Ci/Cu in Calendar
Assignee

Updated

11 months ago
Assignee: nobody → mschroeder
Status: NEW → ASSIGNED
Please wait until bug 1469459 is fixed as it'll save time cleaning up bitrot.
Depends on: 1469459
(In reply to Geoff Lankow (:darktrojan) from comment #1)
> Please wait until bug 1469459 is fixed as it'll save time cleaning up bitrot.

Oh, would it have been filed in the correct "Product"...
Reporter

Comment 3

11 months ago
OK, when you're done here, I'd finally like to fix all spelling mistakes in Calendar, there are heaps! Prior attempt, never landed, see attachment 8960495 [details] [diff] [review].
Is this likely to happen soon, Martin? If you'd like me to do it instead I can.
Blocks: 1442047
The patch is already prepared, I just have to fix some cosmetic things. I will try to wrap it up tonight.
Posted patch Patch v1 (obsolete) — Splinter Review
Attachment #9011254 - Flags: review?(makemyday)

Comment 7

8 months ago
Comment on attachment 9011254 [details] [diff] [review]
Patch v1

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

Thank you for this conversion. Looks like you got your build environment back :-)

r+wc - mainly nits, but for the gdata code, we need a polyfill of the new shortcuts with the previous notion if not available to retain backwards compatibility.

One thing I haven't commented on is the line length of some of the touched code. We currently haven't the linter rule for that enabled, but it would be apprciated if we don't exceed a line length of 100 characters when touching code.

When you fixed everything for checkin, a try run would be nice before landing, just to be on the safe side.

::: calendar/.eslintrc.js
@@ +476,5 @@
>  
>          // Enforce consistent indentation (4-space)
>          "indent-legacy": [2, 4, { SwitchCase: 1, }],
>  
> +        "mozilla/use-cc-etc": 2,

Please add a comment to explain the scope of the rule

::: calendar/base/src/calAlarmMonitor.js
@@ +29,1 @@
>  ];

Can you fold this into one line?

::: calendar/base/src/calAlarmService.js
@@ +103,1 @@
>  ];

Same here

::: calendar/base/src/calCalendarManager.js
@@ +799,1 @@
>      ]),

Here again.

::: calendar/base/src/calRecurrenceDate.js
@@ +15,1 @@
>  ];

Here as well

::: calendar/providers/caldav/calDavCalendar.js
@@ +75,5 @@
>  // some shorthand
> +var calICalendar = Ci.calICalendar;
> +var calIErrors = Ci.calIErrors;
> +var calIFreeBusyInterval = Ci.calIFreeBusyInterval;
> +var calICalDavCalendar = Ci.calICalDavCalendar;

Since the var name are not really shorter here anymore, we could get rid of the extra var alltogether here.

::: calendar/providers/gdata/components/calGoogleCalendar.js
@@ +26,5 @@
>      migrateItemMetadata,
>      JSONToAlarm
>  } = ChromeUtils.import("resource://gdata-provider/modules/gdataUtils.jsm", null);
>  
> +var cIOL = Ci.calIOperationListener;

The Provider for Google needs probably a special treatment since this change is not backward compatible, the provider is build from c-c and currently supports versions back to 45 (even if that would be raised to 52, that problem still would exist).

Can you add a Ci/Cc/Cu/Cr (as applicable) var in the files if these are not already available globally to make that change backwards compatible? Please add a comment that this special handling could be removed once the minimum requirement is raised to TB60.

@@ +104,1 @@
>              throw new Components.Exception("", cIE.CAL_IS_READONLY);

No need to assign a cont here.

::: calendar/providers/wcap/public/calIWcapCalendar.idl
@@ +252,4 @@
>  //     calIOperation defineAccessControl(
>  //         in string userId, in unsigned long accessControlBits,
>  //         in calIGenericOperationListener listener);
>      

whitespce
Attachment #9011254 - Flags: review?(makemyday) → review+
Reporter

Comment 8

6 months ago
Stalled for two months now (and likely with bit rot)?
Reporter

Comment 9

4 months ago

(In reply to Geoff Lankow (:darktrojan) from comment #4)

Is this likely to happen soon, Martin? If you'd like me to do it instead I
can.

How much longer do we want to wait here?

Flags: needinfo?(mschroeder)
Flags: needinfo?(geoff)

I've updated the patch and addressed the review comments, but this probably needs reviewing again given how much time has passed.

Attachment #9011254 - Attachment is obsolete: true
Flags: needinfo?(geoff)
Attachment #9036454 - Flags: review?(makemyday)
Reporter

Comment 11

4 months ago

You think MMD wants to look at 700 KB again? That's torture. Do a try run and ship it.

Flags: needinfo?(mschroeder)

Comment 12

4 months ago

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/4ae0c5713dfe
Mass replace the Components.interface/Components.utils uses with Ci/Cu in Calendar. r=MakeMyDay

Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Attachment #9036454 - Flags: review?(makemyday)
Target Milestone: --- → 6.8
You need to log in before you can comment on or make changes to this bug.