Closed Bug 1142261 Opened 5 years ago Closed 5 years ago

Don't split interfaces between libical and ical.js

Categories

(Calendar :: Build Config, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
4.0.0.1

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

Attachments

(1 file, 1 obsolete file)

Dynamically loading the xpt file like we do makes the packager cry when it attempts to link xpts. We don't actually have to do that if we just have an extra interface that extends the base and adds the [notxpcom] methods.
Attached patch Fix - v1 (obsolete) β€” β€” Splinter Review
Attachment #8576268 - Flags: review?(geoff)
Comment on attachment 8576268 [details] [diff] [review]
Fix - v1

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

I'm no expert on the C++ side of things, but this looks good to me. FWIW more lines of context would've been really useful here.

::: calendar/base/backend/libical/calRecurrenceRule.cpp
@@ +211,1 @@
>              aRecurEnd->GetInTimezone(cal::UTC(), getter_AddRefs(dt));

What does |dt| do in this method now? Did you mean to use icaldt here?
Attachment #8576268 - Flags: review?(geoff) → review+
Attached patch Fix - v2 β€” β€” Splinter Review
What a great find. Not only was the C++ code wrong, but also it uncovered a bug in ical.js. Here is a new patch with more context and a unit test.
Attachment #8576268 - Attachment is obsolete: true
Attachment #8576593 - Flags: review?(geoff)
Attachment #8576593 - Flags: review?(geoff) → review+
Attachment #8576593 - Flags: approval-calendar-aurora+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.1
Target Milestone: 4.1 → 4.0
You need to log in before you can comment on or make changes to this bug.