1048035 - Remove occurences of deprecated parseHeadersWithArray from calendar code

Status: RESOLVED FIXED

Description

[[:MakeMyDay]]

parseHeadersWithArray is currently used several times in calItipUtils.jsm and calendar-event-dialog-attendees.xml. It is deprecated and can be replaced by makeFromDisplayAddress(), which allows to split an address to its parts in a more efficient way.

Comment 2

[[:MakeMyDay]]

Created attachment 8629590 [details] [diff] [review]
RemoveParseHeadersWithArray-V1.diff

This patch takes care of

- replacing parseHeadersWithArray() with makeFromDisplayAddress()
- consolidating code for adding/removing of "mailto:" prefix to an email address at the same places

This is fixes the misbehaviour described in bug 1179023, so I'm requesting approval for backporting up to release, even if it's probably too late for 4.0.1.

Comment 3

[Philipp Kewisch [:Fallen]]

Comment on attachment 8629590 [details] [diff] [review]
RemoveParseHeadersWithArray-V1.diff

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

4.0.1 will be built tomorrow, so in theory we could take it. Given there are still some issues I think we should let it bake on trunk for a while first. I would be more confident if we had tests to cover this.

r- for now since the forEach thing will most definitely make it fail.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
@@ +254,4 @@
>            }
>  
> +          // trime spaces if any
> +          inputValue.trim();

String.trim() isn't in-place, this needs an assignment.

::: calendar/base/modules/calItipUtils.jsm
@@ +368,5 @@
>  
> +        let checkRecipient = function (aRecipient) {
> +            if (aRecipient.name.toLowerCase() in emailMap) {
> +                // Return the first found recipient
> +                return aRecipient;

Given you are calling this with forEach, returning a value won't do anything. You are going to have to stay with normal iteration here.

Comment 4

[[:MakeMyDay]]

I don't get this in tonight, so let's head for 4.0.2.

Comment 5

[[:MakeMyDay]]

Created attachment 8632430 [details] [diff] [review]
RemoveParseHeadersWithArray-V2.diff

Updated patch with comments considered.

Comment 6

[[:MakeMyDay]]

Created attachment 8632459 [details] [diff] [review]
RemoveParseHeadersWithArray-V3.diff

Added two missing functions to calUtils.jsm and some minor optimizations. Sorry for the inconvinience if you already started your review.

Comment 8

[Philipp Kewisch [:Fallen]]

Comment on attachment 8632459 [details] [diff] [review]
RemoveParseHeadersWithArray-V3.diff

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

::: calendar/base/modules/calItipUtils.jsm
@@ +376,5 @@
>          }
>  
>          // Maybe we are in the CC list?
> +        let ccList = MailServices.headerParser.makeFromDisplayAddress(aMsgHdr.ccList);
> +        for each (let recipient in ccList) {

Can you use for..of here?

::: calendar/base/modules/calUtils.jsm
@@ +332,5 @@
>  
>      /**
> +     * Prepends a mailto: prefix to an email address like string
> +     * 
> +     * @param  {string}        the string to prepend the prefix if not already there

Whitespaces snuck in here.

@@ +341,5 @@
> +    },
> +
> +    /**
> +     * Removes an existing mailto: prefix from an attendee id
> +     * 

...and here

Comment 9

[[:MakeMyDay]]

Created attachment 8636247 [details] [diff] [review]
RemoveParseHeadersWithArray-V4.diff

Comment 10

[Philipp Kewisch [:Fallen]]

If you want this in 4.0.2, can you upload an unbitrotted patch for that tree?

Comment 11

[[:MakeMyDay]]

Created attachment 8644188 [details] [diff] [review]
RemoveParseHeadersWithArray-V5.diff

debitrotted patch applicable down to esr38.

Comment 12

[[:MakeMyDay]]

https://hg.mozilla.org/comm-central/rev/77b709d65ee6

Comment 14

[Philipp Kewisch [:Fallen]]

url:        https://hg.mozilla.org/releases/comm-aurora/rev/987e9543647fa525308ddce0123f0d8af6477beb
changeset:  987e9543647fa525308ddce0123f0d8af6477beb
user:       MakeMyDay <[e-mail]>
date:       Thu Aug 06 10:23:25 2015 +0200
description:
Bug 1048035 - Remove occurences of deprecated parseHeadersWithArray from calendar code;r+a=philipp

Comment 15

[Philipp Kewisch [:Fallen]]

url:        https://hg.mozilla.org/releases/comm-beta/rev/49ea4c6cf1959b0e45a724577e5e5150d6ec0cb6
changeset:  49ea4c6cf1959b0e45a724577e5e5150d6ec0cb6
user:       MakeMyDay <[e-mail]>
date:       Thu Aug 06 10:23:25 2015 +0200
description:
Bug 1048035 - Remove occurences of deprecated parseHeadersWithArray from calendar code;r+a=philipp

Comment 16

[Philipp Kewisch [:Fallen]]

url:        https://hg.mozilla.org/releases/comm-esr38/rev/3deddbdeb50ff565278845d7d9bed2d79d2d0785
changeset:  3deddbdeb50ff565278845d7d9bed2d79d2d0785
user:       MakeMyDay <[e-mail]>
date:       Thu Aug 06 10:23:25 2015 +0200
description:
Bug 1048035 - Remove occurences of deprecated parseHeadersWithArray from calendar code;r+a=philipp

Comment 17

[Stefan Sitter]

Please check if this patch regressed Bug 1197320. User in comment 7 complains that free/busy is broken and mentions missing mailto:

If this is a regression from this bug we should consider the patch landing strategy. The were some regressions in the 4.0.* builds that might have been caused by direct check-in without previous time for testing on central, aurora, or beta branch. And it shows that we are missing unit tests for large part of our code base.

Comment 18

[Philipp Kewisch [:Fallen]]

Indeed, I am not happy with the amount of regressions in 4.0.x. My fault for accepting too much on the release branch. From now on only tested regression fixes! Reading the latest comment in bug 1197320 this is likely because of the includes/contains string prototype change that is not on esr38.

Comment 19

[[:MakeMyDay]]

The analysis in bug 1197320 is probably right, there is a wrong comparison for the the search result. On esr38 only, this masks the includes/contains issue (switch was for TB40). So yes, this is a regression and very unfortunate. I'll provide a fix on the other bug.