Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Remove occurences of deprecated parseHeadersWithArray from calendar code

RESOLVED FIXED in 4.0.2

Status

Calendar
General
--
minor
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: MakeMyDay, Assigned: MakeMyDay)

Tracking

Dependency tree / graph

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1179023
(Assignee)

Comment 2

2 years ago
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.
Assignee: nobody → makemyday
Status: NEW → ASSIGNED
Attachment #8629590 - Flags: review?(philipp)
Attachment #8629590 - Flags: approval-calendar-release?(philipp)
Attachment #8629590 - Flags: approval-calendar-beta?(philipp)
Attachment #8629590 - Flags: approval-calendar-aurora?(philipp)
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.
Attachment #8629590 - Flags: review?(philipp)
Attachment #8629590 - Flags: review-
Attachment #8629590 - Flags: approval-calendar-release?(philipp)
Attachment #8629590 - Flags: approval-calendar-beta?(philipp)
Attachment #8629590 - Flags: approval-calendar-aurora?(philipp)
(Assignee)

Comment 4

2 years ago
I don't get this in tonight, so let's head for 4.0.2.
Blocks: 1180416
(Assignee)

Updated

2 years ago
Blocks: 1180413
(Assignee)

Comment 5

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

Updated patch with comments considered.
Attachment #8629590 - Attachment is obsolete: true
Attachment #8632430 - Flags: review?(philipp)
(Assignee)

Comment 6

2 years ago
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.
Attachment #8632430 - Attachment is obsolete: true
Attachment #8632430 - Flags: review?(philipp)
Attachment #8632459 - Flags: review?(philipp)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1184836
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
Attachment #8632459 - Flags: review?(philipp) → review+
(Assignee)

Comment 9

2 years ago
Created attachment 8636247 [details] [diff] [review]
RemoveParseHeadersWithArray-V4.diff
Attachment #8632459 - Attachment is obsolete: true
Attachment #8636247 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
If you want this in 4.0.2, can you upload an unbitrotted patch for that tree?
(Assignee)

Comment 11

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

debitrotted patch applicable down to esr38.
Attachment #8636247 - Attachment is obsolete: true
Attachment #8644188 - Flags: review+
Attachment #8644188 - Flags: approval-calendar-release?(philipp)
Attachment #8644188 - Flags: approval-calendar-beta?(philipp)
Attachment #8644188 - Flags: approval-calendar-aurora?(philipp)
Attachment #8644188 - Flags: approval-calendar-release?(philipp)
Attachment #8644188 - Flags: approval-calendar-release+
Attachment #8644188 - Flags: approval-calendar-beta?(philipp)
Attachment #8644188 - Flags: approval-calendar-beta+
Attachment #8644188 - Flags: approval-calendar-aurora?(philipp)
Attachment #8644188 - Flags: approval-calendar-aurora+
(Assignee)

Comment 12

2 years ago
https://hg.mozilla.org/comm-central/rev/77b709d65ee6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.4
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1192157
Whiteboard: [aurora][beta][release]
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
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
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
Keywords: checkin-needed
Whiteboard: [aurora][beta][release]
Target Milestone: 4.4 → 4.0.2

Comment 17

2 years ago
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.
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.
(Assignee)

Comment 19

2 years ago
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.

Updated

2 years ago
Blocks: 1209399
You need to log in before you can comment on or make changes to this bug.