469477 - Move fromRFC3339 from calGoogleUtils.js to calProviderUtils.js

Status: RESOLVED FIXED

(Calendar :: Provider: GData, defect)

Description

[Patrick Cloke [:clokep]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20081201 Lightning/1.0pre Thunderbird/3.0b1

calGoogleUtils.js contains a function fromRFC3339 which can parse a RFC 3339 date string into a TimeDate object.  This would be useful for other providers to use besides GData.  Moving it to calProviderUtils.js would allow other providers access.

Reproducible: Always

Comment 1

[Patrick Cloke [:clokep]]

Just

Comment 2

[Patrick Cloke [:clokep]]

Attachment: Patch to Move functions

Patch to move fromRFC3339 and toRFC3339 from calGoogleUtils.js to calProviderUtils.js

Comment 3

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

Comment on attachment 352848 [details] [diff] [review]
Patch to Move functions

Codewise this looks good, checking in Monday or Tuesday to allow for comments.

r=philipp

Comment 4

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

Looking foward to your unit tests. Go ahead and attach it to this bug when you are done.

Comment 5

[Daniel Boelzle [:dbo]]

What further provider needs these functions? Please mind that every function added to calProviderUtils.js is nailed and cannot be removed later on without potentially breaking 3rd party code.
Finally, I suggest we introduce a js module file for provider utils, instead of adding more stuff to calProviderUtils.js.

Comment 6

[Patrick Cloke [:clokep]]

It actually came up when discussing how to produce a testsuite for the fromRFC3339 function.  It didn't seem there was an elegant way to have it run on build.  Since RFC 3339 dates are quite common on the internet it seemed that it would be useful to other providers moving it to calProviderUtils would allow this in addition to allowing unit tests to be ran on the functions.

I know the following use the fromRFC3339:
GData
hCalendar (https://addons.mozilla.org/en-US/thunderbird/addon/6702)
Remember the Milk (https://addons.mozilla.org/en-US/sunbird/addon/7125)
as well as a provider I've been working on
They're just copied from the GData extension (and renamed in the case of RtM provider)

I suppose it could also be part of the DateTime object, but that wouldn't address the issue of it being nailed. What benefits come from using a JS module as opposed to calProviderUtils.js?

Comment 7

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

Attachment: Patch to move functions - v2

Updated patch to use calProviderUtils.jsm

A module is the new way of including functions from other files, see https://developer.mozilla.org/En/JavaScript_modules

There is also a date utils module in core code, but it uses jsDate instead of calIDateTime.

Comment 8

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

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/abb3fe7271d8>

(and unbreak things I did wrong in rev d06f1cb6732c)

-> FIXED

Comment 9

[Patrick Cloke [:clokep]]

Attachment: Testsuite for fromRFC3339 and toRFC3339

Comment 10

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

Comment on attachment 359506 [details] [diff] [review]
Testsuite for fromRFC3339 and toRFC3339

>+	
>+	testRFC3339_();
Indentation is not quite correct here since you use tabs. Please expand tabs to spaces.

>+		var dateTime = cal.fromRFC3339(aRFC3339Date, aTimezone);
Please use let instead of var everywhere in your patch.


>+		do_check_eq(dateTime, aResult);
>+		var RFC3339 = cal.toRFC3339(dateTime);
JS convention is camelCase, please use rfc3339Date or such.

>+		// In theory this should be:
>+		// do_check_eq(RFC3339, aRFC3339Date);
Why only in theory? Please explain in the code comment.

>+	 * All tests are done under the default timezone and UTC (although both
>+	 * should give the same time)
Good start, although it would be great to also test a specific timezone that has DST boundaries and see if the function behaves well when crossing the boundary. Which timezone you use doesn't maktter.

>+	// This represents March 14, 2006 in the default timezone
        testRFC3339_("2006-03-14",
                     timezone,
                     "2006/03/14 00:00:00 " + timezone + " isDate=1",
                     "2006-03-14");
We tend to wrap parameters a bit differently, one parameter per line if the line would otherwise exceed 80 characters. The above is an example of how that could look like.


>+	testRFC3339_("2004-10-14T03:00:23-06:00", timezone, "2004/10/14 03:00:23 America/Belize isDate=0",
>+				 "2004-10-14T03:00:23-06:00");
You said you use the default timezone. Please note that the default timezone may not be America/Belize for everyone. Unless you are already doing so, please use it explicitly (on which occasion you could test the DST boundaries).

I'm a bit worried that using calIDateTime::toString() might not be very failsafe. You could consider testing the memers directly. you could pass the expected date like:

[2004,10,14,3,0,23,timezone] or [2004,10,14,-1,-1,-1,timezone] for allday dates.

then some magic like


let propmap = { year: expected[0],
                month: expected[1],
                ...
              };

for (let prop in propmap) {
    do_check_eq(theDate[prop], propmap[prop]);
}

I won't require this right now though, we can fix it if it fails.

Overall the patch looks very nice, only setting r- so you can consider the comments above. Please upload new patch and rerequest review.

Comment 11

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

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".

Comment 12

[Patrick Cloke [:clokep]]

Attachment: Testsuite for fromRFC3339 and toRFC3339 v2

It only took a bit over 5 years, but I got back to this. Thanks for the review. :)

Comment 13

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

Comment on attachment 8416478 [details] [diff] [review]
Testsuite for fromRFC3339 and toRFC3339 v2

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

Looks good, r=philipp with this minor nit:

::: calendar/test/unit/test_rfc3339_parser.js
@@ +59,5 @@
> +    let timezone = getTz("/mozilla.org/20071231_1/America/New_York");
> +    let utc = cal.UTC();
> +    // Timezones used in tests.
> +    belize = getTz("/mozilla.org/20071231_1/America/Belize");
> +    dawson = getTz("/mozilla.org/20071231_1/America/Dawson");

I think you can drop the prefix on these, i.e just America/New_York

Comment 14

[Patrick Cloke [:clokep]]

(In reply to Philipp Kewisch [:Fallen] from comment #13)
> Looks good, r=philipp with this minor nit:

> I think you can drop the prefix on these, i.e just America/New_York
Thought I tested that, but you're right: it works without those!

Thanks for the review. I'll push (with the nit and commit message fixed) once the tree is back open.

Comment 15

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/c9552299a4e7