Last Comment Bug 1048035 - Remove occurences of deprecated parseHeadersWithArray from calendar code
: Remove occurences of deprecated parseHeadersWithArray from calendar code
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
-- minor (vote)
: 4.0.2
Assigned To: [:MakeMyDay]
:
:
Mentors:
: 1179023 1184836 1192157 (view as bug list)
Depends on:
Blocks: 1180413 ltn402 1209399
  Show dependency treegraph
 
Reported: 2014-08-03 10:44 PDT by [:MakeMyDay]
Modified: 2015-09-28 23:08 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
RemoveParseHeadersWithArray-V1.diff (13.53 KB, patch)
2015-07-04 10:17 PDT, [:MakeMyDay]
philipp: review-
Details | Diff | Splinter Review
RemoveParseHeadersWithArray-V2.diff (13.39 KB, patch)
2015-07-11 04:07 PDT, [:MakeMyDay]
no flags Details | Diff | Splinter Review
RemoveParseHeadersWithArray-V3.diff (15.75 KB, patch)
2015-07-11 08:28 PDT, [:MakeMyDay]
philipp: review+
Details | Diff | Splinter Review
RemoveParseHeadersWithArray-V4.diff (15.74 KB, patch)
2015-07-20 14:51 PDT, [:MakeMyDay]
makemyday: review+
Details | Diff | Splinter Review
RemoveParseHeadersWithArray-V5.diff (14.79 KB, patch)
2015-08-06 01:29 PDT, [:MakeMyDay]
makemyday: review+
philipp: approval‑calendar‑aurora+
philipp: approval‑calendar‑beta+
philipp: approval‑calendar‑esr+
Details | Diff | Splinter Review

Description User image [:MakeMyDay] 2014-08-03 10:44:17 PDT
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 1 User image [:MakeMyDay] 2015-07-04 05:32:19 PDT
*** Bug 1179023 has been marked as a duplicate of this bug. ***
Comment 2 User image [:MakeMyDay] 2015-07-04 10:17:02 PDT
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 User image Philipp Kewisch [:Fallen] 2015-07-05 12:09:56 PDT
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 User image [:MakeMyDay] 2015-07-05 13:23:39 PDT
I don't get this in tonight, so let's head for 4.0.2.
Comment 5 User image [:MakeMyDay] 2015-07-11 04:07:58 PDT
Created attachment 8632430 [details] [diff] [review]
RemoveParseHeadersWithArray-V2.diff

Updated patch with comments considered.
Comment 6 User image [:MakeMyDay] 2015-07-11 08:28:57 PDT
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 7 User image [:MakeMyDay] 2015-07-17 22:02:55 PDT
*** Bug 1184836 has been marked as a duplicate of this bug. ***
Comment 8 User image Philipp Kewisch [:Fallen] 2015-07-20 03:21:09 PDT
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 User image [:MakeMyDay] 2015-07-20 14:51:13 PDT
Created attachment 8636247 [details] [diff] [review]
RemoveParseHeadersWithArray-V4.diff
Comment 10 User image Philipp Kewisch [:Fallen] 2015-08-04 14:53:19 PDT
If you want this in 4.0.2, can you upload an unbitrotted patch for that tree?
Comment 11 User image [:MakeMyDay] 2015-08-06 01:29:52 PDT
Created attachment 8644188 [details] [diff] [review]
RemoveParseHeadersWithArray-V5.diff

debitrotted patch applicable down to esr38.
Comment 13 User image [:MakeMyDay] 2015-08-08 02:21:30 PDT
*** Bug 1192157 has been marked as a duplicate of this bug. ***
Comment 14 User image Philipp Kewisch [:Fallen] 2015-08-08 03:26:58 PDT
url:        https://hg.mozilla.org/releases/comm-aurora/rev/987e9543647fa525308ddce0123f0d8af6477beb
changeset:  987e9543647fa525308ddce0123f0d8af6477beb
user:       MakeMyDay <makemyday@gmx-topmail.de>
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 User image Philipp Kewisch [:Fallen] 2015-08-08 03:34:41 PDT
url:        https://hg.mozilla.org/releases/comm-beta/rev/49ea4c6cf1959b0e45a724577e5e5150d6ec0cb6
changeset:  49ea4c6cf1959b0e45a724577e5e5150d6ec0cb6
user:       MakeMyDay <makemyday@gmx-topmail.de>
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 User image Philipp Kewisch [:Fallen] 2015-08-08 03:40:43 PDT
url:        https://hg.mozilla.org/releases/comm-esr38/rev/3deddbdeb50ff565278845d7d9bed2d79d2d0785
changeset:  3deddbdeb50ff565278845d7d9bed2d79d2d0785
user:       MakeMyDay <makemyday@gmx-topmail.de>
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 User image Stefan Sitter 2015-08-26 07:04:59 PDT
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 User image Philipp Kewisch [:Fallen] 2015-08-26 07:55:04 PDT
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 User image [:MakeMyDay] 2015-08-27 00:01:18 PDT
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.

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