Closed
Bug 1048035
Opened 7 years ago
Closed 6 years ago
Remove occurences of deprecated parseHeadersWithArray from calendar code
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
RESOLVED
FIXED
4.0.2
People
(Reporter: MakeMyDay, Assigned: MakeMyDay)
References
Details
Attachments
(1 file, 4 obsolete files)
14.79 KB,
patch
|
MakeMyDay
:
review+
Fallen
:
approval-calendar-aurora+
Fallen
:
approval-calendar-beta+
Fallen
:
approval-calendar-esr+
|
Details | Diff | Splinter Review |
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 | ||
Comment 2•6 years ago
|
||
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 3•6 years ago
|
||
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•6 years ago
|
||
I don't get this in tonight, so let's head for 4.0.2.
Blocks: ltn402
Assignee | ||
Comment 5•6 years ago
|
||
Updated patch with comments considered.
Attachment #8629590 -
Attachment is obsolete: true
Attachment #8632430 -
Flags: review?(philipp)
Assignee | ||
Comment 6•6 years ago
|
||
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)
Comment 8•6 years ago
|
||
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•6 years ago
|
||
Attachment #8632459 -
Attachment is obsolete: true
Attachment #8636247 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 10•6 years ago
|
||
If you want this in 4.0.2, can you upload an unbitrotted patch for that tree?
Assignee | ||
Comment 11•6 years ago
|
||
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)
Updated•6 years ago
|
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•6 years ago
|
||
https://hg.mozilla.org/comm-central/rev/77b709d65ee6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.4
Updated•6 years ago
|
Whiteboard: [aurora][beta][release]
Comment 14•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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
Updated•6 years ago
|
Comment 17•6 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.
Comment 18•6 years ago
|
||
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•6 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•