869407 - Add ical.js backend wrapper

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

This bug takes care of adding the ical.js backend. These should be the remaining patches for the first round, I'm splitting them up a bit for easier review.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 1 - Add Backend and its JS Wrapper Objects - v1

This patch adds the backend to the build system, together with the js wrapper objects that glue together xpcom and ical.js

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 2 - Split calIDateTime into JS/C++ interfaces - v1

The C++ implementation requires the jsDate attribut to be [implicit_jscontext] to forward the context to the date constructor. This flag does not work with a js implementation though. Therefore this patch splits out the interface into two different ones, each loaded based on the pref again.

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 3 - Timezone Service changes - v1

The timezone service uses an intermediate object for certain timezones for the C++ implementation. A similar wrapper is needed for ical.js. Therefore we should be loading each based on the backend used. the calICALJSTimezone object is also being used in other parts of the ical.js wrapper.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 4 - Unit test changes and additions - v1

Some slight modifications are needed for the unit tests. Most cases where an if/else block is used, the tests are not general enough (i.e comparing a whole string when there is a paremeter that is optional. Libical adds it, ical.js doesn't). As the goal is to get rid of libical, I think these are OK.

Comment 5

[Martin Schröder [:mschroeder]]

Comment on attachment 746368 [details] [diff] [review]
Part 3 - Timezone Service changes - v1

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

::: calendar/base/src/calTimezone.js
@@ +72,5 @@
> +        return (this.icalComponent ? this.icalComponent.toString() : this.tzid);
> +    },
> +
> +    get isUTC() {
> +        dump("### testing is utc " + this.mUTC + "\n");

Debugging/Testing left-over ;)

Comment 6

[Matthew Mecca [:mmecca]]

Comment on attachment 746366 [details] [diff] [review]
Part 1 - Add Backend and its JS Wrapper Objects - v1

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

::: calendar/base/backend/icaljs/calRecurrenceRule.js
@@ +103,5 @@
> +        prop.setValue(this.innerObject);
> +        return new calIcalProperty(prop);
> +    },
> +    set icalProperty(val) unwrapSetter(ICAL.Property, val, function(val) {
> +        Components.utils.reportError("GOT RRULE ICALPROP VALUE: " + val.toSource());

When testing with a large ics file, this line caused TB to hang, it attempts to write the entire parsed content of the calendar out to the error console.

Comment 7

[Matthew Mecca [:mmecca]]

Comment on attachment 746370 [details] [diff] [review]
Part 4 - Unit test changes and additions - v1

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

Everything looks good overall. r=mmecca

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central changeset 1ad08ece2315

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central changeset 7062dbead024

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central changeset a9164d775349

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central changeset c5b0682b53eb

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I've removed the debug messages in these patches, I hope that improves the performance for now.

Comment 13

[Martin Schröder [:mschroeder]]

Build OS X 10.7 opt is burning 
https://tbpl.mozilla.org/php/getParsedLog.php?id=22878370&tree=Thunderbird-Trunk

It seems the removal of <em:targetPlatform> is problematic for the scripts preparing the Mac "universal" builds.

Comment 14

[Stefan Sitter [:ssitter]]

I think em:targetPlatform is still required as long as we include binary components. Otherwise you could install e.g. the linux package on windows and be confused because it doesn't work.

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

For now I've pushed a bustage fix that keeps it for merging under the name em:realTargetPlatform. I'm not really happy with this solution, but it should at least make the tree green again. I don't have a final solution for this yet, maybe you have an idea:

If we do specify em:targetPlatform, then it won't be possible to install Lightning on a different platform when calendar.backend = "icaljs". How can we restrict the targetPlatform only when the backend is libical?

OTOH maybe its just too early. Theming is not ready for cross-platform install, so it might not hurt to just keep the targetPlatform tag intact.