Last Comment Bug 1142261 - Don't split interfaces between libical and ical.js
: Don't split interfaces between libical and ical.js
Product: Calendar
Classification: Client Software
Component: Build Config (show other bugs)
: Trunk
: x86 Mac OS X
-- normal (vote)
Assigned To: Philipp Kewisch [:Fallen]
Depends on:
Blocks: 1130854
  Show dependency treegraph
Reported: 2015-03-11 14:39 PDT by Philipp Kewisch [:Fallen]
Modified: 2015-03-12 16:42 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Fix - v1 (44.23 KB, patch)
2015-03-11 14:41 PDT, Philipp Kewisch [:Fallen]
geoff: review+
Details | Diff | Splinter Review
Fix - v2 (46.46 KB, patch)
2015-03-12 03:42 PDT, Philipp Kewisch [:Fallen]
geoff: review+
philipp: approval‑calendar‑aurora+
Details | Diff | Splinter Review

Description User image Philipp Kewisch [:Fallen] 2015-03-11 14:39:20 PDT
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.
Comment 1 User image Philipp Kewisch [:Fallen] 2015-03-11 14:41:35 PDT
Created attachment 8576268 [details] [diff] [review]
Fix - v1
Comment 2 User image Geoff Lankow (:darktrojan) 2015-03-11 16:14:27 PDT
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?
Comment 3 User image Philipp Kewisch [:Fallen] 2015-03-12 03:42:05 PDT
Created attachment 8576593 [details] [diff] [review]
Fix - v2

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.

Note You need to log in before you can comment on or make changes to this bug.