Last Comment Bug 788004 - No email send after invitation creation if offline cache is enabled [Exception... "'TypeError: aItem.calendar.canNotify is not a function' when calling method: [calIOperationListener::onOperationComplete]"
: No email send after invitation creation if offline cache is enabled [Excepti...
Status: RESOLVED FIXED
: regression
Product: Calendar
Classification: Client Software
Component: Provider: CalDAV (show other bugs)
: Lightning 1.7
: All All
: -- major with 1 vote (vote)
: 2.6
Assigned To: Mohit Kanwal [:redDragon]
:
:
Mentors:
: 787350 792244 793993 796226 799055 (view as bug list)
Depends on: 902916 938455
Blocks: calcache
  Show dependency treegraph
 
Reported: 2012-09-03 12:53 PDT by Jan
Modified: 2013-11-13 17:44 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
[HACK] Patch to cleanly apply on beta (2.51 KB, patch)
2012-10-01 09:09 PDT, Mohit Kanwal [:redDragon]
no flags Details | Diff | Splinter Review
Patch to cleanly apply on beta [HACK] (1.31 KB, patch)
2012-10-01 09:12 PDT, Mohit Kanwal [:redDragon]
philipp: review+
philipp: checkin+
Details | Diff | Splinter Review
Patch that removes all calInstanceOf calls (28.84 KB, patch)
2012-10-02 08:07 PDT, Mohit Kanwal [:redDragon]
philipp: review+
philipp: approval‑calendar‑aurora+
Details | Diff | Splinter Review
Clean Patch for removing calInstance calls (29.76 KB, patch)
2012-12-16 10:22 PST, Mohit Kanwal [:redDragon]
no flags Details | Diff | Splinter Review
Warning message for calInstanceOf function (716 bytes, patch)
2013-01-10 07:33 PST, Mohit Kanwal [:redDragon]
philipp: review-
Details | Diff | Splinter Review
Cleaner Patch for removal of calInstanceOf calls (29.80 KB, patch)
2013-01-10 10:13 PST, Mohit Kanwal [:redDragon]
philipp: review+
Details | Diff | Splinter Review
Warning message for calInstanceOf function - v2 (994 bytes, patch)
2013-02-11 02:04 PST, Philipp Kewisch [:Fallen]
philipp: review+
Details | Diff | Splinter Review
Warning message for calInstanceOf function - v2 (1.01 KB, patch)
2013-02-11 02:05 PST, Philipp Kewisch [:Fallen]
philipp: review+
Details | Diff | Splinter Review
Correct the previous patch's behavior (22.29 KB, patch)
2013-02-14 03:54 PST, Mohit Kanwal [:redDragon]
philipp: review-
Details | Diff | Splinter Review
Fix for removal of calInstanceOf calls (31.01 KB, patch)
2013-05-31 02:58 PDT, Mohit Kanwal [:redDragon]
philipp: review+
Details | Diff | Splinter Review
Updated Fix-2 (31.08 KB, patch)
2013-06-01 11:10 PDT, Mohit Kanwal [:redDragon]
philipp: review+
Details | Diff | Splinter Review
Patch for checkin (45.10 KB, patch)
2013-07-24 17:33 PDT, Mohit Kanwal [:redDragon]
philipp: review+
philipp: approval‑calendar‑aurora+
philipp: approval‑calendar‑beta+
Details | Diff | Splinter Review

Description Jan 2012-09-03 12:53:28 PDT
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120209155602

Steps to reproduce:

Platform: Windows 7 - with enabled cache options in the Preferences of calendar create event in the network CalDav calendar and invite attendees 


Actual results:

No event invitation is send. Error console shows:

Fehler: [Exception... "'TypeError: aItem.calendar.canNotify is not a function' when calling method: [calIOperationListener::onOperationComplete]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "JS frame :: resource://calendar/modules/calUtils.jsm -> file:///C:/Users/jlange/AppData/Roaming/Thunderbird/Profiles/8umop2ul.default/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calCachedCalendar.js :: <TOP_LEVEL> :: line 711"  data: no] 

Workarround: disable the cache option and the email is send. But then you can't use the offline version of the calendar.


Expected results:

after creating the event the Outlook compatibility dialog is shown and afterwards the email is send.
Comment 1 alo 2012-09-09 14:41:57 PDT
Same on :
ubuntu 12.04
TB 15.0
Lightning 1.7 

The workaround... works!
Comment 2 Jan Ole Kastens 2012-09-14 06:06:33 PDT
I have the same bug with the same error message on
3.2.0-30-generic #48-Ubuntu SMP Fri Aug 24 16:52:48 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux
TB 15.0
Lightning 1.7
The workaround works for me too
Comment 3 Stefan Sitter 2012-09-14 08:34:05 PDT
Confirming per duplicated reports.
Comment 4 Stefan Sitter 2012-09-14 08:34:35 PDT
*** Bug 787350 has been marked as a duplicate of this bug. ***
Comment 5 Stefan Sitter 2012-09-15 08:06:42 PDT
The cached calendar provider probably needs a similar fix as the storage provider in Bug 762854, i.e. it needs to implement and announce Components.interfaces.calISchedulingSupport.

Mohit, would you have time to take a look at this as you were involved in the latest changes to the cached calendar provider?
Comment 6 Mohit Kanwal [:redDragon] 2012-09-17 07:09:55 PDT
Hi Stefan, I will take a look into this. Do I need to just announce the calISchedulingSupport in calCachedCalendar.js? 

CalDAV does seem to implement the calISchedulingSupport Interface [1]

[1] http://mxr.mozilla.org/comm-central/source/calendar/providers/caldav/calDavCalendar.js
Comment 7 Philipp Kewisch [:Fallen] 2012-09-17 08:12:11 PDT
You possibly need to forward the methods to the inner calendar, see the forward methods at the bottom of calCachedCalendar.js

While you are at it, could you check all other providers that implement the interface to make sure they are properly advertising the interface?
Comment 8 Stefan Sitter 2012-09-21 13:29:50 PDT
*** Bug 792244 has been marked as a duplicate of this bug. ***
Comment 9 Stefan Sitter 2012-09-25 10:30:06 PDT
*** Bug 793993 has been marked as a duplicate of this bug. ***
Comment 10 Mohit Kanwal [:redDragon] 2012-10-01 09:09:21 PDT
Created attachment 666576 [details] [diff] [review]
[HACK] Patch to cleanly apply on beta

This patch takes care of having a hackish fix for the beta branch. The wrappedJSObject apparently is not being cast into a calISchedulingSupport object. For example,
// works:

let foo = aItem.calendar;
(foo instanceof Ci.calISchedulingSupport);
// works:
let foo = aItem.calendar;
(foo instanceof Ci.calISchedulingSupport);
cal.calInstanceOf(aItem.calendar, Ci.calISchedulingSupport) && aItem.calendar.canNotify();

// fails:
cal.calInstanceOf(aItem.calendar, Ci.calISchedulingSupport) && aItem.calendar.canNotify();

Will upload another patch to fix all the calInstanceOf calls soon.
Comment 11 Mohit Kanwal [:redDragon] 2012-10-01 09:12:27 PDT
Created attachment 666578 [details] [diff] [review]
Patch to cleanly apply on beta [HACK]

Oops! uploaded a wrong copy.
Comment 12 info 2012-10-02 03:51:58 PDT
I introduced the patch and still no invites are sent out.
Maybe it's because I used the stable v.17 and not the beta.
Comment 13 Philipp Kewisch [:Fallen] 2012-10-02 04:53:53 PDT
hbarel, are you getting any error console messages?
Comment 14 info 2012-10-02 05:07:40 PDT
Warnings and errors included:

Timestamp: 02/10/12 14:04:37
Warning: XUL box for _moz_generated_content_before element contained an inline #text child, forcing all its children to be wrapped in a block.
Source File: chrome://messenger/content/messenger.xul
Line: 0

Timestamp: 02/10/12 14:04:38
Warning: XUL box for _moz_generated_content_before element contained an inline #text child, forcing all its children to be wrapped in a block.
Source File: chrome://global/content/bindings/toolbar.xml
Line: 276

Timestamp: 02/10/12 14:04:42
Warning: Use of Mutation Events is deprecated. Use MutationObserver instead.
Source File: chrome://calendar/content/calendar-event-dialog-attendees.js
Line: 29

Timestamp: 02/10/12 14:05:01
Error: calendarURI is null
Source File: file:///l:/thunderbird/profile/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calDavCalendar.js
Line: 1136

Timestamp: 02/10/12 14:05:01
Error: syntax error
Source File: moz-nullprincipal:{5f7d48dd-0040-40b2-aa5a-48ad5fad5e05}
Line: 1, Column: 1
Source Code:
Exception [0] Dodgy looking namespace from 'CALDAV:schedule-send-invite'!
Comment 15 Philipp Kewisch [:Fallen] 2012-10-02 05:11:25 PDT
Could you try this again with the beta and also enable calendar.debug.log and calendar.debug.log.verbose? This looks like a server issue with the namespaces.
Comment 16 Philipp Kewisch [:Fallen] 2012-10-02 06:01:00 PDT
Pushed to comm-central changeset b8a344f591b3
Comment 17 Philipp Kewisch [:Fallen] 2012-10-02 06:04:47 PDT
Backported to releases/comm-aurora changeset dc19a6076430
Comment 18 Philipp Kewisch [:Fallen] 2012-10-02 06:06:09 PDT
Backported to releases/comm-beta changeset df78399be401
Comment 19 Mohit Kanwal [:redDragon] 2012-10-02 08:07:09 PDT
Created attachment 667002 [details] [diff] [review]
Patch that removes all calInstanceOf calls

This patch takes care of removing all the calInstanceOf calls which was the source of issue for the bug. The instanceof method can be cryptic at times (as it was not casting calISchedulingSupport objects) and as such the function calInstanceOf has been removed in this patch. Instead a new function cal.wrapInstance has been added inside of calUtils module which only does QueryInterface and returns the wrapped object. Help me check if this does not break anything.
Comment 20 info 2012-10-02 08:18:18 PDT
Not on beta yet, but with verbose logging I get the following errors:

Timestamp: 02/10/12 17:08:58
Error: calendarURI is null
Source File: file:///l:/thunderbird/profile/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calDavCalendar.js
Line: 1136

CalDAV: Sending iTIP failed with status 500 for Dcal

Timestamp: 02/10/12 17:08:58
Error: syntax error
Source File: moz-nullprincipal:{0af0ad2c-83b0-400d-ac95-0a4661f609b4}
Line: 1, Column: 1
Source Code:
Exception [0] Dodgy looking namespace from 'CALDAV:schedule-send-invite'!

The CalDAV server is mine, and it has not changed since the previous version of lightning when it still worked, so I don't think it's a server issue.
Comment 21 info 2012-10-02 09:10:50 PDT
Found the source of the problem. It was not the server but another, unrelated bug which caused the calendar cache data to be corrupt. Completely unrelated to this issue. Sorry.
Comment 22 Stefan Sitter 2012-10-03 00:09:48 PDT
*** Bug 796226 has been marked as a duplicate of this bug. ***
Comment 23 Mohit Kanwal [:redDragon] 2012-10-03 01:43:42 PDT
So it works now after introducing the patch?
Comment 24 psmith 2012-10-03 06:05:40 PDT
Regarding bug 796226 that was marked as a duplicate of this bug, I disabled cache and now Lightning sends two meeting invites to attendees.  One has the reminder set and one does not.  It is a problem if attendee only accepts the invite with no reminder set.
Comment 25 Philipp Kewisch [:Fallen] 2012-10-03 07:16:38 PDT
This bug was only recently fixed, please retest with the release builds next week.
Comment 26 Stefan Sitter 2012-10-08 05:10:58 PDT
*** Bug 799055 has been marked as a duplicate of this bug. ***
Comment 27 Philipp Kewisch [:Fallen] 2012-10-08 09:55:49 PDT
Comment on attachment 667002 [details] [diff] [review]
Patch that removes all calInstanceOf calls

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

r=philipp with the following changes. Also, please push this to comm-aurora. Depending on when our beta builds run we might want to consider doing this on beta too, but lets see.

::: calendar/base/content/dialogs/calendar-dialog-utils.js
@@ +440,4 @@
>  
>          // Only show if its either an internal protcol handler, or its external
>          // and there is an external app for the scheme
> +        let handlerWrappedInstance = cal.wrapInstance(handler, Components.interfaces.nsIExternalProtocolHandler);  

Looks like there are some extra whitespaces here.

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +50,1 @@
>                      rule = rules[0];

You could just do this:

rule = calWrapInstance(rules[0], Components.interfaces.calIRecurrenceRule);

::: calendar/base/content/dialogs/calendar-invitations-list.xml
@@ +201,4 @@
>          <parameter name="aItem"/>
>          <parameter name="aStatus"/>
>          <body><![CDATA[
> +          aItem.calendar = cal.wrapInstance(aItem.calendar, Components.interfaces.calISchedulingSupport);

I'd avoid re-assigning the calendar here, but instead do 

let calendar = cal.wrapInstance(...);

if (calendar) {
  // and change all other aitem.calendar things here.

::: calendar/providers/composite/calCompositeCalendar.js
@@ +319,4 @@
>      mCompositeObservers: null,
>      mObservers: null,
>      addObserver: function (aObserver) {
> +        let aCompositeObserver = cal.wrapInstance(aObserver, Components.interfaces.calICompositeObserver);

the "a" prefix is usually meant for arguments. I'd suggest naming it compositeObserver

@@ +327,5 @@
>      },
>  
>      // void removeObserver( in calIObserver observer );
>      removeObserver: function (aObserver) {
> +        let aCompositeObserver = cal.wrapInstance(aObserver, Components.interfaces.calICompositeObserver);

Same here.

::: calendar/providers/storage/calStorageCalendar.js
@@ +2116,4 @@
>              this.prepareStatement(this.mInsertProperty);
>              var pp = this.mInsertProperty.params;
>              pp.key = propName;
> +            let propDateTimeVal = cal.wrapInstance(propValue, Components.interfaces.calIDateTime); 

Extra whitespace here.

::: calendar/providers/wcap/calWcapCalendarItems.js
@@ +502,4 @@
>              if (attachments) {
>                  var strings = [];
>                  for each (var att in attachements) {
> +                    let aAtt = cal.wrapInstance(att, Components.interfaces.calIAttachment); 

Extra whitespace here, also same comment about the "a" prefix. Use something like attendeeAtt

@@ +977,5 @@
>              for each (let recItem in recItems) {
>                  // cs bug: workaround missing COUNT
> +                if (cal.wrapInstance(recItem, Components.interfaces.calIRecurrenceRule)) {
> +                    // Possible performance issue double QueryInterface calls
> +                    recItem = recItem.QueryInterface(Components.interfaces.calIRecurrenceRule); 

Extra whitespace
Comment 28 Philipp Kewisch [:Fallen] 2012-11-29 08:03:46 PST
Comment on attachment 666578 [details] [diff] [review]
Patch to cleanly apply on beta [HACK]

This was already checked in.
Comment 29 Philipp Kewisch [:Fallen] 2012-11-29 08:08:13 PST
Comment on attachment 667002 [details] [diff] [review]
Patch that removes all calInstanceOf calls

Mohit, I think this patch was never actually checked in. Could you de-bitrot it and push it to comm-central and comm-aurora?
Comment 30 Philipp Kewisch [:Fallen] 2012-12-14 10:15:51 PST
Mohit, do you think you could take care of comment 29?
Comment 31 Mohit Kanwal [:redDragon] 2012-12-16 10:22:21 PST
Created attachment 692752 [details] [diff] [review]
Clean Patch for removing calInstance calls

Sorry about the oversight,I have updated the patch to apply cleanly on trunk. Phillip help me push this through, I think I don't have pushing rights =)
Comment 32 Mohit Kanwal [:redDragon] 2013-01-10 07:33:41 PST
Created attachment 700423 [details] [diff] [review]
Warning message for calInstanceOf function

Since the function calInstanceOf is being unsupported from next release onwards therefore adding a short Warning message for developers to see in the console, informing them about the change.
Comment 33 Mohit Kanwal [:redDragon] 2013-01-10 10:13:16 PST
Created attachment 700524 [details] [diff] [review]
Cleaner Patch for removal of calInstanceOf calls

The previous patch a bit of whitespaces around which I had ignored. Cleaned the patch to apply cleanly on current tree.
Comment 34 Philipp Kewisch [:Fallen] 2013-02-11 01:50:41 PST
Comment on attachment 700524 [details] [diff] [review]
Cleaner Patch for removal of calInstanceOf calls

Patch looks fine, r=philipp
Comment 35 Philipp Kewisch [:Fallen] 2013-02-11 01:57:44 PST
Comment on attachment 700423 [details] [diff] [review]
Warning message for calInstanceOf function

I was thinking that we should introduce the new function wrapInstance (so push the other patch), but add in a function calInstanceOf that shows the warning (once) and then call wrapInstance.

This way we can get rid of the function early in our code, but still allow extensions to transition to the new function. I'll upload a new patch to show what I mean.
Comment 36 Philipp Kewisch [:Fallen] 2013-02-11 02:04:21 PST
Created attachment 712373 [details] [diff] [review]
Warning message for calInstanceOf function - v2
Comment 37 Philipp Kewisch [:Fallen] 2013-02-11 02:05:18 PST
Created attachment 712374 [details] [diff] [review]
Warning message for calInstanceOf function - v2
Comment 38 Philipp Kewisch [:Fallen] 2013-02-11 02:06:48 PST
Pushed to comm-central changeset f339d89a90f8
Comment 39 Philipp Kewisch [:Fallen] 2013-02-11 02:07:01 PST
Pushed to comm-central changeset 905498205868
Comment 40 Philipp Kewisch [:Fallen] 2013-02-11 04:10:43 PST
Sorry, there are a few errors with this patch, we will need to either fix it within the next few days or back it out. Just a few problems I've found:

* Open a readonly event in the summary dialog (no details visible, cannot close)
* Cannot accept email event invitation

Mohit, can you take care of this and doublecheck that all touched code locations still function?
Comment 41 Mohit Kanwal [:redDragon] 2013-02-12 18:31:40 PST
Ah gross oversight on my part :-(

I put rule = calWrapInstance(rules[0], Components.interfaces.calIRecurrenceRule); instead of cal.WrapInstance ... for calendar-event-dialog-recurrence.js

I will double check that this does not break anything and upload a new one to apply on trunk by tonight :-)
Comment 42 Mohit Kanwal [:redDragon] 2013-02-14 03:51:03 PST
I realised doing this was wrong:

> aCalendar = cal.wrapInstance(aItem.calendar, Components.interfaces.calISchedulingSupport);
> if (aCalendar) {
> ...

coz if the wrapInstance function fails the aCalendar variable will have null value while the code below the above line might require aCalendar to not be null but perhaps a different object type. wrapInstance returns a wrappedInstance after QueryInterface otherwise null.

Uploading a cleaner patch with such instances removed.
Comment 43 Mohit Kanwal [:redDragon] 2013-02-14 03:54:40 PST
Created attachment 713857 [details] [diff] [review]
Correct the previous patch's behavior

Applies on top of the previously pushed patch.

I have tested through the scenarios as pointed out in comment 40 and found it to work properly without any errors now. Will appreciate a bit of testing so that this does not cause any new errors :-)
Comment 44 Stefan Sitter 2013-02-17 10:36:53 PST
Could this patch be responsible for Bug 842150 regression too? If yes: Will it be fixed before the branch uplift or will the patch that broke things be backed out?

The error in Bug 842150 points to a line |return (cal.wrapInstance(aObject, Components.interfaces.calIEvent) != null);|
Comment 45 Mohit Kanwal [:redDragon] 2013-02-17 10:52:22 PST
Yes it is definitely caused by this patch. comment 42 has the reason why.

Can try to apply patch attachment 713857 [details] [diff] [review] and see if it solves the issue.

The latest patch needs to be checked in to prevent a number of such errors from popping out.
Comment 46 Philipp Kewisch [:Fallen] 2013-02-18 01:37:18 PST
Comment on attachment 713857 [details] [diff] [review]
Correct the previous patch's behavior

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

Since the merge is tomorrow, I'm going to back out the original change for now. We can then push both patches at once when all issues are out of the way.

I'm going to have to r- for now, I think this is not quite correct:

::: calendar/base/content/agenda-listbox.js
@@ +602,4 @@
>  function refreshCalendarQuery(aStart, aEnd, aCalendar) {
>      let pendingRefresh = this.pendingRefresh;
>      if (pendingRefresh) {
> +        if (cal.wrapInstance(pendingRefresh, Components.interfaces.calIOperation)) {

Hmm isn't this exactly what we were trying to avoid by removing calInstanceOf?

There were circumstances where:

if (calInstanceOf(pendingRefresh, Ci.calIOperation)) {
  pendingRefresh.cancel(null);
}

didn't work because the pendingRefresh is not successfully QI'd to calIOperation. We wanted to work around this by replacing it with the method wrapInstance, that returns the QI'd instance, which can then be used correctly.

I think rather than changing all those instances, you should call it like so:

let pendingRefresh = cal.wrapInstance(this.pendingRefresh, Components.interfaces.calIOperation);
if (pendingRefresh) {
   ...
}

To make this work in all situations, there are two additional things you can do:

* Add documentation to cal.wrapInstance as to how it should be used
* Add a check to wrapInstance to return null if the passed object is null.
Comment 48 Philipp Kewisch [:Fallen] 2013-05-13 11:18:41 PDT
Mohit, could you fix these issues?
Comment 49 Mohit Kanwal [:redDragon] 2013-05-31 02:58:52 PDT
Created attachment 756468 [details] [diff] [review]
Fix for removal of calInstanceOf calls

Updated patch. So far no issues upon applying it to the trunk.
Comment 50 Philipp Kewisch [:Fallen] 2013-05-31 11:23:11 PDT
Comment on attachment 756468 [details] [diff] [review]
Fix for removal of calInstanceOf calls

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

::: calendar/base/modules/calUtils.jsm
@@ +637,5 @@
> +     *   ...
> +     *   pendingRefresh.onResult(...) // Assured of pendingRefresh to be of calIOperation Type
> +     * }
> +     * 
> +     * 

Some trailing whitespace issues here.

Also, maybe you could add a note on how to NOT use it. Consider this as a replacement for the current usage text:

Use this function to QueryInterface the object to a particular interface. You may only expect the return value to be wrapped, not the original passed object. For example: 

// BAD USAGE:
if (cal.wrapInstance(foo, Ci.nsIBar)) {
   foo.barMethod();
}
// GOOD USAGE:
foo = cal.wrapInstance(foo, Ci.nsIBar);
if (foo) {
  foo.barMethod();
}

@@ +648,5 @@
> +        } catch (e) {
> +            return null;
> +        }
> +    },
> +    

Extra newline and trailing whitespaces

::: calendar/providers/gdata/components/calGoogleUtils.js
@@ +589,4 @@
>                      // EXDATES require special casing, since they might contain
>                      // a TZID. To avoid the need for conversion of TZID strings,
>                      // convert to UTC before serialization.
> +                    prop.valueAsDatetime = wrappedRItem.date.getInTimezone(UTC());

Mind changing this to cal.UTC() (optional)

::: calendar/providers/storage/calStorageCalendar.js
@@ +2099,5 @@
>              this.prepareStatement(this.mInsertProperty);
>              var pp = this.mInsertProperty.params;
>              pp.key = propName;
> +            let wPropValue = cal.wrapInstance(propValue, Components.interfaces.calIDateTime);
> +            if (wPropValue) {

I guess its fine like this, but theoretically you could just do:

propValue = cal.wrapInstance(propValue, Components.interfaces.calIDateTime);

Might also be the case some other times (but not always!). This is an optional review comment.
Comment 51 Mohit Kanwal [:redDragon] 2013-06-01 10:44:34 PDT
Comment on attachment 756468 [details] [diff] [review]
Fix for removal of calInstanceOf calls

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

::: calendar/providers/storage/calStorageCalendar.js
@@ +2099,5 @@
>              this.prepareStatement(this.mInsertProperty);
>              var pp = this.mInsertProperty.params;
>              pp.key = propName;
> +            let wPropValue = cal.wrapInstance(propValue, Components.interfaces.calIDateTime);
> +            if (wPropValue) {

I thought about it, but in some cases where there is an additional logic (if-else) and let's say if wrapInstance fails, then we don't end up changing the value of propValue to null. So I thought just to create an additional variable for the wrapped purpose, although I agree that it creates some confusion.
Comment 52 Mohit Kanwal [:redDragon] 2013-06-01 11:10:34 PDT
Created attachment 757017 [details] [diff] [review]
Updated Fix-2

Updated Patch with the above review taken into account.
Comment 53 Philipp Kewisch [:Fallen] 2013-06-03 01:13:16 PDT
Comment on attachment 757017 [details] [diff] [review]
Updated Fix-2

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

Looks good, r=philipp. You have commit access, right?
Comment 54 Mohit Kanwal [:redDragon] 2013-06-03 02:00:46 PDT
Nope don't have commit access yet.
Comment 55 Ryan VanderMeulen [:RyanVM] 2013-06-03 05:38:16 PDT
This patch has no commit message and the bug history makes it rather unclear as to what it should be. If you're going to request checkin, please ensure that the patch is suitable first.
Comment 56 Mohit Kanwal [:redDragon] 2013-07-24 17:33:05 PDT
Created attachment 780707 [details] [diff] [review]
Patch for checkin

Apologies on my oversight. Corrected the patch with the necessary information.
Comment 57 Philipp Kewisch [:Fallen] 2013-08-05 08:22:31 PDT
Comment on attachment 780707 [details] [diff] [review]
Patch for checkin

approval for aurora, and also beta depending on if the tree opens in time before the merge today.
Comment 58 Philipp Kewisch [:Fallen] 2013-08-05 13:20:19 PDT
Comment on attachment 780707 [details] [diff] [review]
Patch for checkin

Also approving for beta since the merge has happened.
Comment 59 Ryan VanderMeulen [:RyanVM] 2013-08-06 06:44:59 PDT
https://hg.mozilla.org/comm-central/rev/b532ed24389a
Comment 61 Stefan Sitter 2013-08-08 11:59:01 PDT
Seems this checkin regressed Bug 902916.

Note You need to log in before you can comment on or make changes to this bug.