I've been meaning to finalize this patch, the past few times I've looked I'm not actually sure what was missing. Before I keep moving this up I am going to dump it here. Maybe it makes sense to have someone with a fresh pair of eyes look at it. Magnus, can you find someone to work on this?

If you want to rebase this locally instead of applying it you can do:

hg up -r c7cda013b0b433778d7f9a64b12823ab6c40f01c
hg import http://...
hg rebase -d comm

Comments:

• The tests make sense to me, but Geoff had once noted the gdata tests are hard to read so maybe these tests need more documentation or changes.
• I'm not sure where it went, but I used to have the ics provider hooked up to the request classes as well. I can do some digging, but I think it shouldn't be too much work to adapt.

Sending over to Paul's backlog.

WIP 1 of 5. Just uploading some work in progress. The overview is I've rebased Philipp's original patch on current trunk and then made some changes, most per Magnus' comments. There's more to do, but it's not clear to me yet exactly how much. For example the xpcshell test fails at line 495. Known todos are tagged with "TODO DAV" comments.

WIP 2 of 5.

WIP 3 of 5.

WIP 4 of 5.

WIP 5 of 5.

Philipp's patch re-rebased, with Prettier formatting.

Some more further work on top of Philipp's patch, also rebased with Prettier formatting.

Philipp, when you have a chance I could use some feedback on next steps here. I've got the test running, but it has one failure (noted in comments), and if you comment out the failing part, it fails in two other places. I didn't get far trying to troubleshoot those failures.

I've added "TODO DAV" comments in a few places to mark areas that seem WIP, so you might want to take a look at those. (Some are just reminders to delete something if it ends up not being used.)

This is a summary of some of the changes:

• Use a single export for CalDavRequest
• Use a single export for CalDavUtils
• Capitalize names for the new jsm modules
• Fix the header iterator in test_caldav_requests.js
• Other small test fixes
• Do a basic checkRedirect function implementation, etc.
• Various edits and updates to satisfy eslint, etc.

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #11)

Hope these comments are helpful in finding those errors. Ping me if you'd
like me to apply and test.

Thanks for the helpful feedback.

Yeah if this isn't used anymore I think it can go. It was originally the
function that set up the OAuth keys.

Removed, and some globals along with it.

I'm wondering if we can move this file to a JSM and maybe just re-define
these consts?

Good idea. Flagging it as a TODO for now until we get the test passing.

I think it would be better to keep exporting the single symbols. This keeps
the constructors short, and when we move to es6 imports I'm thinking it
would be better to use this:

import { ItemRequest } from
"resource://calendar/modules/caldav/CalDavRequest.jsm";

It also makes the no-undef rule more useful, otherwise eslint won't know
which properties are defined and which are not. If in an import you'd really
want everything you could still use import * as CalDavRequest from ...

That makes sense to me. I'm fine with changing them back but will hold off for the moment, to let Magnus weigh in again if he wants to.

+class CalDavRequestBase {

Do we use this class on its own? If so I think we should drop the Base part.

It's not used on its own. I added the "Base" to free up "CalDavRequest" to be the single export.

It currently shows up in doc strings but probably shouldn't. I haven't yet found an established JSDoc way to indicate a param is "any subclass of this abstract base class".

Does this work? We are returning a traditional function here, so this
should be whatever that is called with, which I believe is the object that
is being proxied, so this.request._handler from line 472?

Good point, fixed now with "self" capturing the correct "this".

I think one of the reasons I didn't test this was that I'd like to get rid
of the SAX handler. See also bug 1514666.

OK.

What happened to IterSimpleEnumerator?

It went away in m-c, and was replaced in c-c with code like I used here.

I've seen this error a few times, it was happening when passing a date to a
method that expects a date-time or vice versa. Possibly we need to make the
wrapper call the right method, or you need to find out why a date is being
passed to a function that expects a date-time.

It might just be a matter of making sure that you use
DTSTART;VALUE=DATE:20180103 instead of DTSTART:20180103. Libical would
gracefull assume the right type.

I did some digging and was able to fix this error with some tweaks to icaljs. See more with next uploaded patch.

After some digging I was able to fix the "invalid date-time value" error in the xpcshell test by making a few changes to ical.js.

Basically ical.js assumes that a "period" involves date-times not dates, but the free-busy input in the test is a period that uses dates:

    FREEBUSY;FBTYPE=FREE:20180103/20180117
    FREEBUSY;FBTYPE=BUSY:20180118/P7D

This demonstration patch just makes a few changes to prevent the error in the test. More would need to be done to fully support this case, and of course in the actual ical.js repo.

After this error is fixed, two more errors appear, which I haven't really looked into yet. Here's the end of the log:

0:04.87 pid:5532 console.log: Lightning: CalDAV: Status 207 on initial PROPFIND for calendar xpcshell
 0:04.87 pid:5532 console.error: Lightning: 
 0:04.87 pid:5532   [calCachedCalendar] replay action failed: <unknown>, uri=http://localhost:40105/calendars/xpcshell/events, result=2147500037, operation=[xpconnect wrapped calIOperation]
 0:04.87 pid:5532 console.log: Lightning: [calCachedCalendar] replayChangesOn finished.
 0:04.87 pid:5532 console.log: Lightning: [calCachedCalendar] sync queue empty.
 0:04.87 ERROR Unexpected exception 2147500037
undefined
 0:04.87 INFO exiting test
 0:04.89 pid:5532 JavaScript error: resource://testing-common/PromiseTestUtils.jsm, line 112: uncaught exception: Object
 0:04.94 TEST_END: Test FAIL, expected PASS. Subtests passed 121/121. Unexpected 0 - xpcshell return code: 0
 0:04.95 INFO INFO | Result summary:
 0:04.95 INFO INFO | Passed: 0
 0:04.95 INFO INFO | Failed: 2
 0:04.95 INFO INFO | Todo: 0
 0:04.95 INFO INFO | Retried: 0
 0:04.95 SUITE_END

xpcshell
~~~~~~~~
Ran 244 checks (242 subtests, 2 tests)
Expected results: 242
Unexpected results: 2
  test: 2 (2 fail)

Unexpected Results
------------------
xpcshell-icaljs.ini:comm/calendar/test/unit/test_caldav_requests.js
  FAIL xpcshell-icaljs.ini:comm/calendar/test/unit/test_caldav_requests.js - xpcshell return code: 0
xpcshell-libical.ini:comm/calendar/test/unit/test_caldav_requests.js
  FAIL xpcshell-libical.ini:comm/calendar/test/unit/test_caldav_requests.js - xpcshell return code: 0
ERROR Unexpected exception 2147500037
undefined
ERROR Unexpected exception 2147500037
undefined

And if I revert line 347 in calCachedCalendar.js to (operation ? operation.id : "<unknown>") +
(https://searchfox.org/comm-central/source/calendar/base/src/calCachedCalendar.js#347)

Then you get this slight variation:

0:04.55 pid:7953 console.log: Lightning: CalDAV: Status 207 on initial PROPFIND for calendar xpcshell
 0:04.55 pid:7953 JavaScript strict warning: resource://calendar/calendar-js/calCachedCalendar.js, line 347: ReferenceError: reference to undefined property "id"
 0:04.55 pid:7953 console.error: Lightning: 

(In reply to Paul Morris [:pmorris] from comment #12)

I think it would be better to keep exporting the single symbols. This keeps
the constructors short, and when we move to es6 imports I'm thinking it
would be better to use this:

import { ItemRequest } from
"resource://calendar/modules/caldav/CalDavRequest.jsm";

It also makes the no-undef rule more useful, otherwise eslint won't know
which properties are defined and which are not. If in an import you'd really
want everything you could still use import * as CalDavRequest from ...

That makes sense to me. I'm fine with changing them back but will hold off for the moment, to let Magnus weigh in again if he wants to.

Agreed. Personally, I would rename some/most of the classes to be prefixed the names with CalDav though to make it more clear what they are/do. Like CalDavPropfindRequest.

I've moved this back to exporting multiple classes per module and following mkmelin's suggestion I've renamed the classes using a "CalDav" prefix.

Despite my efforts, I haven't had much success debugging the new test errors (ERROR Unexpected exception 2147500037 undefined). They seem fairly obscure. Philipp, let me know if you have any ideas on that.

2147500037 is unfortunately NS_ERROR_FAILURE so not very helpful. I'd suggest using add_task(...).only() to determine the exact test that is failing and then try to debug from there. You can also try using --jsdebugger.

Running test_freebusy_request I happened to see this, despite the test passing. We should make sure the test fails in this case and we fix the recursion error.

 0:05.17 INFO (xpcshell/head.js) | test test_freebusy_request finished (2)
 0:05.18 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "too much recursion" {file: "resource://calendar/modules/ical.js" line: 5764}]"
 0:05.18 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "too much recursion" {file: "resource://calendar/modules/ical.js" line: 5727}]"

Looking at the last test, which fails reliably for me, I added a try/catch with console.error, this explains the 2147500037 error a bit better:

 0:01.63 pid:11712 console.error: Lightning:
 0:01.63 pid:11712   [calCachedCalendar] replay action failed: <unknown>, uri=http://localhost:55773/calendars/xpcshell/events, result=2147500037, operation=[xpconnect wrapped calIOperation]
 0:01.63 pid:11712 console.log: Lightning: [calCachedCalendar] replayChangesOn finished.
 0:01.63 pid:11712 console.log: Lightning: [calCachedCalendar] sync queue empty.
 0:01.63 pid:11712 console.error: 2147500037

I attached the debugger and stepped through, and the issue is within the checkRedirect() function. You have a TODO to implement that function, and I think if you do then the tests should work better.

The function name might be a bit misleading. What the function should do is return true if the request was not redirected (which would be your quick fix for the tests). If it was redirected, prompt the user if they want to accept following that redirect, if they cancel then return false to disable the calendar. If they do accept the redirect, you may also need to see if the calendar URI needs to be adapted.

Doing this we get a bit further. I did notice this warning which looks like an underlying error, but possibly not related to the test. Definitely something to investigate:

 0:01.91 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "QueryInterface"" {file: "file:///Users/kewisch/mozilla/gecko/objdir-tb-artifact/dist/Thunderbird%20Daily.app/Contents/Resources/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calStorageCalendar.js" line: 2331}]"

Anyway, the caldav calendar doesn't seem to be returning the item that should be stored in the equal(items.length, 1); check, I'll leave this to you to figure out :)

As for your question if you can use CalDavResponseBase in the docstring, I'd say just do it. I don't think eslint checks for exported symbols, but if the eslint jsdoc plugin complains that is something to look into.

Note also I had to add the debugger keyword to the test to make attaching the js debugger work well. There seem to be a few things in devtools where Thunderbird might need some fixes down the line, but with that workaround you can at least debug.

Thanks again for the feedback. Getting back to this after working on other things for awhile. This patch continues fixing test related errors and failures. I've also rebased the other patches locally, will upload new versions of those.

test_caldav_client
I stubbed out the checkRedirect function correctly to solve that issue. Then using the debugger (which required using --flag icaljs or --flag libical to run only a single test), I looked into the next failure with getAllItems returning an empty array. With the debugger running the test did not fail, and I could see that the async call to the database in getItems (here: https://searchfox.org/comm-central/source/calendar/providers/storage/calStorageCalendar.js#737) was returning a result, and the callback function was called. Without the debugger that async call to the database was resolving but without returning a result, and the callback wasn't getting called. It appears that the test was checking for something that is not there yet. Also, if I made two calls to getAllItems in the test, the first would return an empty array and the second would return the expected result. So adding executeSoon got the test to pass. (And also eliminated that warning about QueryInterface being undefined that was apparently related.)

test_freebusy_request
Given what you wrote on this, I took a new approach and changed the input to be correct -- DATE-TIME rather than just DATE. (Maybe we want to test incorrect input too, eventually but first things first.) That solved the problem where ical.js was expecting a DATE-TIME but was getting a DATE. However, ical.js and libical produce different results for the last deepEqual in the test given the same input, so one or the other will fail. Do we want to tweak ical.js output to match libical? Or...?

test_freebusy_request - too much recursion issue
I looked into this briefly, but haven't gotten to the bottom of it. Will keep working on it (and the other TODOs that aren't test related).

Rebased on current trunk.

Rebased. I can squash these patches eventually, but keeping them separate for now for easier review/feedback.

The "too much recursion" error in test_freebusy_request happens when this assertion fails:
strictEqual(first, response.data["mailto:recipient1@example.com"]);
which causes JSON.stringify to be called on the arguments to that assertion (to log the error), and that causes the runaway recursion. You can also hit it by just calling JSON.stringify(first). Here's the trace of that (which is similar to the trace with strictEqual).

 0:09.82 pid:11878 resource://calendar/modules/ical.js 4950 fromData
 0:09.82 pid:11878 resource://calendar/modules/ical.js 4813 icaltime
 0:09.82 pid:11878 resource://calendar/modules/ical.js 4866 clone
 0:09.82 pid:11878 resource://calendar/modules/ical.js 5044 startOfWeek
 0:09.82 pid:11878 resource://calendar/calendar-js/calDateTime.js 165 get startOfWeek
 0:09.82 pid:11878 [...]/xpcshell/comm/calendar/test/unit/test_caldav_requests.js 899 test_freebusy_request

I don't yet know where the call to startOfWeek comes from. (The JS debugger connects but execution doesn't stop for debugging, even though I'm using the "debugger;" statement.) Maybe it's something with the timezone argument in the call to fromData , which is {wrappedJSObject:{}, expandedUntilYear:0, changes:[], tzid:"floating"}.

For figuring out where calls come from, often putting a console.trace() at the interesting place will help a bunch.

(In reply to Magnus Melin [:mkmelin] from comment #24)

For figuring out where calls come from, often putting a console.trace() at the interesting place will help a bunch.

Thanks, yeah, I've been using console.trace when I can't get the debugger working. It's how I got the log above. But if I put a console.trace() in the startOfWeek call I don't get any more info about how we get from JSON.stringify(first) in the test to the startOfWeek call:

console.trace: 
 0:12.96 pid:24864 resource://calendar/calendar-js/calDateTime.js 167 get startOfWeek
 0:12.96 pid:24864 [...]/xpcshell/comm/calendar/test/unit/test_caldav_requests.js 903 test_freebusy_request
 0:12.96 pid:24864 [...]/xpcshell/head.js 1567 run_next_test/_run_next_test/<
 0:12.96 pid:24864 [...]/xpcshell/head.js 1567 _run_next_test
 0:12.96 pid:24864 [...]/xpcshell/head.js 735 run
 0:12.96 pid:24864 [...]/xpcshell/head.js 246 _do_main
 0:12.96 pid:24864 [...]/xpcshell/head.js 573 _execute_test

I've spent enough time on this test that I think it may make sense to defer fixing it to a follow-up bug. Maybe just comment out this strictEqual assert for now. [EDIT: no need to comment it out, see next comment.] It succeeds in libical, so we have an icaljs issue and not a problem with the caldav refactor changes.

After backing out my WIP debugging commit and looking again, I see that the strictEqual(first, response.data["mailto:recipient1@example.com"]); assertion succeeds with ical.js. Apparently when logging a success or a failure, Assert.jsm tries to do JSON.stringify and if that fails it falls back to toString. The "too much recursion" happens in the JSON.stringify attempt but the assert still succeeds.

0:20.39 PASS test_freebusy_request - [test_freebusy_request : 898] [object Object] === [object Object]

I'll open a new bug for the recursion error and add a comment to the test about it, but keep the assertion.

I filed https://github.com/mozilla-comm/ical.js/issues/410 for the "too much recursion" issue, and made a comment in the test with this patch.

This patch also fixes the test_freebusy_request error where ical.js was expecting a DATE-TIME but was getting a DATE (see comment 20 above). By changing the FREEBUSY part of the response to a DATE-TIME and leaving the DTSTART and DTEND parts untouched... ical.js and libical produce the same results for this assertion, so it passes for both:

deepEqual(
    first.intervals.map(interval => interval.begin.icalString + ":" + interval.end.icalString),
    [
      "20180101:20180102",
      "20180103T010101Z:20180117T010101Z",
      "20180118T010101Z:20180125T010101Z",
      "20180126:20180201",
    ]
  );

With this patch all the new tests pass. Next is implementing the checkRedirect dialog, moving calDavRequestHandlers.js to a jsm, and the raiseForStatus error handling in CalDavRequest.jsm.

Moves calDavRequestHandlers.js to a .jsm.

Looking at this redirect dialog again, I realize I'm not sure about some of the details. Philipp you wrote:

If the URI was redirected, prompt the user if they want to accept following that redirect, if they cancel then return false to disable the calendar. If they do accept the redirect, you may also need to see if the calendar URI needs to be adapted.

Can you explain what you mean by "see if the calendar URI needs to be adapted"? Do you mean let the user click a button to update the calendar's URI to be the target URI of the redirect? Or do we just do that automatically on accepting a redirect? But only for some types of redirects and not others?

My sense is that the dialog should show the user the current URI and the URI where it is being redirected. There should be "cancel" and "accept redirect" buttons that either disables the calendar or accepts the redirect. On accept also do something about adapting the URI.

(cc'ing aleca since this will be a UI addition. This is a dialog that will appear when the URI of a remote CalDav calendar is being redirected.)

Adds a basic dialog to prompt the user when a calendar URI is being redirected.

Screenshot of a first pass at this URI redirect dialog.

This defines the missing classes for the various response errors.

The raiseForStatus function where these errors are thrown is still not called anywhere. These types of errors are handled in calDavCalendar.js (e.g. with response.serverError, response.clientError, response.authError, etc.). Should raiseForStatus be called there in the various places where these errors are handled?

There's a response.nsirequest property that's used in various places (like the getter for response.status), but I don't see it defined or set on the CalDavResponseBase class or its sub-classes. Is that one of the things that's missing or am I missing something?

Of the TODOs I identified, there are only two left: this error stuff and the redirect dialog above. I'll need some feedback in order to take this further.

(In reply to Magnus Melin [:mkmelin] from comment #33)

nit: for documentation, I think indention like this doesn't scale (you add
one param, and then everything is off).
I'd do it like this: (and for return, no -)
@return {boolean} true if the request was not redirected

If it wraps, the next line start would align with e under @return

I definitely agree on avoiding the style where the wrapping doesn't scale. In that regard I like this style found in some m-c files, which I find really readable, and would be simple to write consistently: Edit: here's the link: https://searchfox.org/mozilla-central/source/dom/tests/browser/browser_hasbeforeunload.js#12

I like using "-" for return, if/since we're using it for param, for greater consistency. It's simpler if you always use it, but I don't feel that strongly about it.

I see you put two spaces between {boolean} true. We should really document all these conventions and expectations somewhere, especially as they get tighter/more specific than the JSDoc docs, which are pretty loose. If only Prettier could auto-format JSDoc strings. :)

Personally I prefer the version that doesn't scale and just reindent if necessary. Blame for doc params isn't that interesting. What I have been doing in calendar normally is tabbing to the next tabstop after the longest param and then aligning the others. I don't particularly like the compact version because I feel it decreases readability, which is what docs are all about.

(In reply to Paul Morris [:pmorris] from comment #34)

I definitely agree on avoiding the style where the wrapping doesn't scale. In that regard I like this style found in some m-c files, which I find really readable, and would be simple to write consistently: Edit: here's the link: https://searchfox.org/mozilla-central/source/dom/tests/browser/browser_hasbeforeunload.js#12

That looks good too, but still cause inconsistency for the common case that you only need the one line.

I like using "-" for return, if/since we're using it for param

Well, JSDoc examples never use hypen for return.

I see you put two spaces between {boolean} true.

Typo ;) Just one space please.

For the record, I added a lenient mode to upstream ical.js that parses the dates better. Also a lot of other PRs are merged. We might want to spin a release and then pull from upstream. Paul, just checking in on this bug, I'm a bit lost what feedback you need from me and I don't want to block you on this. Anything I can help with?

(In reply to Paul Morris [:pmorris] from comment #29)

Looking at this redirect dialog again, I realize I'm not sure about some of the details. Philipp you wrote:

If the URI was redirected, prompt the user if they want to accept following that redirect, if they cancel then return false to disable the calendar. If they do accept the redirect, you may also need to see if the calendar URI needs to be adapted.

Can you explain what you mean by "see if the calendar URI needs to be adapted"? Do you mean let the user click a button to update the calendar's URI to be the target URI of the redirect? Or do we just do that automatically on accepting a redirect? But only for some types of redirects and not others?

The previous dialog always prompted the user in case of a redirect if they want to accept the target of the redirect as the new calendar uri. Just to avoid a misconfigured webserver changing the calendar uri.

My sense is that the dialog should show the user the current URI and the URI where it is being redirected. There should be "cancel" and "accept redirect" buttons that either disables the calendar or accepts the redirect. On accept also do something about adapting the URI.

(cc'ing aleca since this will be a UI addition. This is a dialog that will appear when the URI of a remote CalDav calendar is being redirected.)

I'm fine with changing how the dialog looks, maybe in a followup though?

Thanks Philipp. Sounds good on ical.js. This bug has been on the back burner while I've been focusing on calendar integration. From your comments it sounds like I'm on the right track with the dialog and what it should do. I think my question was basically: does clicking "accept redirect" in the dialog mean:

(a) Make the target URI of the redirect the new URI for the calendar, forget the old one, and use the new one going forward.

(b) Accept just a one-time redirect, the user will see the dialog again the next time the calendar data is synced, and also show a second dialog asking if the user wants to accept the new URI "permanently" as the new URI for the calendar, as a separate question.

I think (a) is better, because simpler, and seems to be what you have in mind. Let me know if that's not the case.

(I think I was uncertain about what you meant with the phrase "If they do accept the redirect, you may also need to see if the calendar URI needs to be adapted". Namely, the "may" which I read as you "may or may not" need to do anything more than just a one-time redirect, and then the "also need to see if", did that mean present a second dialog to ask the user if ("see if") the calendar URI should be updated to the target of the redirect. I think I've got it straight now, but that's where I got confused. Thanks again for clarifying.)

The previous dialog always prompted the user

Huh, there was a previous dialog? I hadn't seen one, must be missing it somehow.

(In reply to Paul Morris [:pmorris] from comment #38)

I think my question was basically: does clicking "accept redirect" in the dialog mean:

(a) Make the target URI of the redirect the new URI for the calendar, forget the old one, and use the new one going forward.

(b) Accept just a one-time redirect, the user will see the dialog again the next time the calendar data is synced, and also show a second dialog asking if the user wants to accept the new URI "permanently" as the new URI for the calendar, as a separate question.

Yes, I meant (a).

The previous dialog always prompted the user

Huh, there was a previous dialog? I hadn't seen one, must be missing it somehow.

The previous one is fairly simple, it is triggered here: https://searchfox.org/comm-central/source/calendar/providers/caldav/calDavCalendar.js#1821

0 of 5. This is Philipp's original patch; I've just been rebasing it. I've flagged darktrojan for review since it's a pretty significant change.

In the next patch there's follow-up work I've done to use current idioms and conventions, so no need to review for those things here if they are covered in that patch. Apologies if that's a pain. I usually prefer working with smaller, separate patches; happy to merge them if that makes more sense.

1 of 5. Follow-up to part0. Here's most of what's going on here, from the commit message:

• Capitalize names for the new jsm modules
• Prefix exported classes with CalDav
• Fix the header iterator in test_caldav_requests.js
• Other small test fixes
• Do a basic checkRedirected function implementation, etc.
• Various edits and updates to satisfy eslint, etc.

2 of 5. Fixes the new unit tests that were failing.

3 of 5. Converts the request handlers JS file to a JSM.

4 of 5. Adds a URI redirect dialog. Now using an xhtml instead of a xul file.

The previous one is fairly simple, it is triggered here: https://searchfox.org/comm-central/source/calendar/providers/caldav/calDavCalendar.js#1821

Now that I see this simpler way of implementing the dialog, it looks like the better way to do it. It looks like that method supports everything that I'm doing with this new dialog but with fewer moving parts.

Philipp, if you agree I'll just take some feedback and rework this to use that simpler method, otherwise feel free to review what I've done in this patch. :)

Screenshot of current state of the dialog from part4 patch in previous comment.

5 of 5. Adds error classes needed by the raiseForStatus function. This function is currently not called anywhere, but better to add the classes it needs so it's ready to go and avoid no-undef eslint exceptions.

With these patches applied I tested by subscribing to a read-only fastmail calendar via caldav. The unit tests pass locally, and I'll add a try run to this comment shortly.

Edit: try run just started: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=68161785182a29648fb8aa74660d95cae648a6c8

Looks like I should have done a try run before uploading the patches. I've now fixed the issues locally:

• eslint issue: converted to using method shorthand in three places
• build issue: removed the now non-existent calDavRequestHandlers.js from package-manifest.in

Here's another try run showing it's working now with those changes.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2f3cf7b822b02c1823e5b7cb3dff545496cbd2b2

I'll hold off on uploading the latest version of the patches for now since the changes are minor.

Apologies for dragging the chain on this, I was actually half-way through a review but I've not had a chance to finish it.

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #51)

public class fields are supported since Firefox 69 now:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Class_fields#Public_fields

I didn't know that! I'd gotten so used to them not being supported.

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #48)

New trick I learned if you like it, works since Gecko 74:

operation?.id || "<unknown>"

Oh nice, cool that that's in Gecko now. I tried it but unfortunately eslint doesn't support it yet, and it doesn't seem worth adding an eslint exception for that line. https://github.com/eslint/eslint/issues/12642

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #51)

Another new thing you could do, public class fields are supported since Firefox 69 now:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/
Class_fields#Public_fields

Good to know. Unfortunately, it looks like eslint is not supporting this yet either (still stage 3).

So I think the outstanding things for me to address here are:

• Convert the dialog to fluent.
• The race issue that comes up in the tests.

Now using Fluent. I couldn't get the custom button labels to work with Fluent, but the standard "Cancel"/"OK" is fine here, and arguably better, so let's just do that.

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #50)

I guess you can't easily due to system styling of the dialog element, but
this seems like a dialog that would be great to write in pure html without
xul.

Yeah, good point. Leaving it as xul for now, should be easy to convert when we convert dialogs to html.

::: calendar/providers/caldav/CalDavCalendar.jsm
@@ +1426,5 @@

if (args.returnValue) {
  this.uri = response.uri;
  // TODO: Is this line needed?  It was used in the previous dialog.
  // this.setProperty("uri", response.uri.spec);

This is what sets the calendar URI to the redirected URI, are you doing this
somewhere else?

Ah, okay, I was thinking the line above covered that (this.uri = response.uri). I should look into the difference between those two ways of setting a property. I've put both lines in in this patch.

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #49)

executeSoon(async () => {

I think this will cause the inner function to execute after the task
completes, wouldn't you need to await this? Not sure why we need to
executeSoon here though, if there is a race condition let's see if we can
fix it instead.

In the interest of landing these patches, I propose fixing the race condition in a follow-up bug. Below is the diff between this patch and the previous version. The change solves the need to await so the task doesn't complete before we're actually done.

Geoff, Philipp has r+'d the previous version of the patch. Can you review the change below? I'll open a follow-up bug to look further into this after these patches land.

-  executeSoon(async () => {
-    let items = await pclient.getAllItems();
-    equal(items.length, 1);
-  });
+  // TODO: Fix the problem that is causing this test to fail when the next line is removed.
+  await new Promise(resolve => executeSoon(resolve));
+
+  let items = await pclient.getAllItems();
+  equal(items.length, 1);

Fixes the eslint issue from the try run (introduced by new version of Prettier).

Rebased and addressing the following:

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #58)

::: calendar/providers/caldav/CalDavCalendar.jsm
@@ +1413,1 @@

 return response.redirected;

Isn't this mostly equivalent to return true;? If response.redirected is
falsy, return true. Otherwise return a truthy value.

You are right. This was just a temporary thing to help with getting the tests passing. I've changed it to just return true and added a comment. It is implemented for real in the later patch that adds the URI redirect dialog.

Just rebased.

Just rebased.

Just rebased.

Final rebases before landing.

Rebased.

Rebased.

Rebased.

Rebased.

Rebased.

One more try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=24b812b1ce3c47561a2d3487ef91f703896b99e8

This fixes the linting issue from the try run.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/eefb833d48f2
Refactor CalDAV request handling. r=pmorris
https://hg.mozilla.org/comm-central/rev/eb5fd4e3c650
Refactor CalDAV request handling some more. r=Fallen
https://hg.mozilla.org/comm-central/rev/9deaa698c34d
Fix the new CalDAV unit tests. r=Fallen
https://hg.mozilla.org/comm-central/rev/b5b3dc5fdf70
Move calDavRequestHandlers.js into a JSM. r=Fallen
https://hg.mozilla.org/comm-central/rev/1295426fff19
Add a CalDAV calendar URI redirect dialog. r=Fallen
https://hg.mozilla.org/comm-central/rev/11272b25eeab
Define classes for the various CalDAV response errors. r=Fallen