Move listeners and property bags into calDataUtils.jsm

RESOLVED FIXED in 6.2

Status

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

Lightning 6.2
Dependency tree / graph

Details

Attachments

(2 attachments, 4 obsolete attachments)

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?
Posted patch Fix - v1 (obsolete) β€” β€” Splinter Review
Attachment #8946159 - Flags: review?(makemyday)

Comment 3

Last year
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.
Attachment #8946159 - Flags: review?(makemyday) → review+
Posted patch Fix - v2 β€” β€” Splinter Review
Attachment #8946159 - Attachment is obsolete: true
Attachment #8948288 - Flags: review+

Comment 5

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ed473b3278b7
Move listeners and property bags into calDataUtils.jsm. r=MakeMyDay
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 6.2

Updated

Last year
Depends on: 1438452
Reopening this bug to add a test case.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted patch Unit tests and additional fix - v1 (obsolete) β€” β€” Splinter Review
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.
Attachment #8951964 - Flags: review?(makemyday)

Comment 8

Last year
(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 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!
Attachment #8951964 - Attachment is obsolete: true
Attachment #8951964 - Flags: review?(makemyday)
Posted patch Unit tests and additional fix - v2 (obsolete) β€” β€” Splinter Review
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.
Posted patch Unit tests and additional fix - v3 (obsolete) β€” β€” Splinter Review
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.
Attachment #8951968 - Attachment is obsolete: true
Attachment #8952125 - Flags: review?(makemyday)
Missing hg amend for unit test to make sure the hang doesn't come back.
Attachment #8952125 - Attachment is obsolete: true
Attachment #8952125 - Flags: review?(makemyday)
Attachment #8952219 - Flags: review?(makemyday)
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

Last year
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
Attachment #8952219 - Flags: review?(makemyday) → review+

Comment 15

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2450cc00c7be
Move listeners and property bags into calDataUtils.jsm - fix regression and add unit tests. r=MakeMyDay
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.