1433802 - Move listeners and property bags into calDataUtils.jsm

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

This was a fully manual change, I'm modernizing calInterfaceBag, calPropertyBag, calObserverBag and sticking them into calDataUtils.jsm. A few more data related functions will go here in a future patch.

The classes used here are pretty much internal, and providing migrations seems a bit messy. I'm considering not migrating these, but instead providing a simple compat wrap that throws an exception with the deprecated message. Thoughts?

Comment 1

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

Attachment: Fix - v1

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0252aca9f4f60703c9ab7a50649c29b19c50f2a7

Comment 3

[[:MakeMyDay]]

Comment on attachment 8946159 [details] [diff] [review]
Fix - v1

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

Thanks, looks good. r+ with the nit fixed.

::: calendar/base/src/calAlarm.js
@@ +79,5 @@
>  
>          // Properties
> +        for (let propval of this.mProperties.values()) {
> +            if (propval instanceof Components.interfaces.calIDateTime) {
> +                if (propval.isMutable) {

Please consolidate this into a single if block.

Comment 4

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

Attachment: Fix - v2

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ed473b3278b7
Move listeners and property bags into calDataUtils.jsm. r=MakeMyDay

Comment 6

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

Reopening this bug to add a test case.

Comment 7

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

Attachment: Unit tests and additional fix - v1

This adds unit for both this bug and bug 1436557. It also fixes a regression where the batch observer calls were done incorrectly. I am not completely sure this will fix bug 1438452, but it needs to be fixed either way.

There is still a small discrepancy to the old code, the add method used to add in the item first, then run the missing onStartBatch calls. The code here first runs the onStartBatch calls, then adds in the item.

I think this would only be a problem if delete() or has() is called within onStartBatch, so for slightly prettier code I'm willing to risk it.

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to Philipp Kewisch [:Fallen]  from comment #7)
> I am not completely
> sure this will fix bug 1438452, but it needs to be fixed either way.
Still hangs. I also noticed that if you import an e-mail with an event where you are the person inviting, accepting the event hangs straight away and the "Do you want to send a confirmation" panel doesn't show (since you originated the event).

Comment 9

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

Comment on attachment 8951964 [details] [diff] [review]
Unit tests and additional fix - v1

Yep, this is not ready yet. Aside from finding some more discrepancies, I can also reproduce the hang. Thanks for testing!

Comment 10

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

Attachment: Unit tests and additional fix - v2

This is what I have now, not ready for review yet. It seems onAddItem still hangs, but deleting the invitation worked without issues. More this evening.

Comment 11

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

Attachment: Unit tests and additional fix - v3

This should do it. The notify func was using a live iterator, and our code removed and added a listener while the callback was being executed. This caused it to be executed in loop.

Comment 12

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

Attachment: Unit tests and additional fix - v4

Missing hg amend for unit test to make sure the hang doesn't come back.

Comment 13

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

Successful try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6bf2037ffe4e067c007a0e4fe0dd6eb94c986c84&selectedJob=164056900


(note that try may look like it doesn't have all bugs, but there was a previous failed try run, so the newest one isn't showing all changesets. See https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ae663d89c9caf8d2f0917f5c83168d8540100ea1&selectedJob=163954434 for the previous run)

Comment 14

[[:MakeMyDay]]

Comment on attachment 8952219 [details] [diff] [review]
Unit tests and additional fix - v4

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

Thanks looks good and works, r=me

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2450cc00c7be
Move listeners and property bags into calDataUtils.jsm - fix regression and add unit tests. r=MakeMyDay