Closed Bug 1458367 Opened 4 years ago Closed 3 years ago

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

Categories

(Calendar :: General, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jorgk-bmo, Assigned: mschroeder)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

+++ 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.
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: 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"...
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.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #9011254 - Flags: review?(makemyday)
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+
Stalled for two months now (and likely with bit rot)?

(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)

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

Flags: needinfo?(mschroeder)

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
Closed: 3 years 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.