If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

convert some internal objects in the Activity manager to Maps

RESOLVED FIXED in Thunderbird 48.0

Status

Thunderbird
General
--
enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 48.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

21.70 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
I propose to convert some internal objects in activity manager to Maps, because storing things in properties of a object seems fragile (there exist properties of an object we need to skip over) and Map is the object created to store [key,value] pairs. Also in some places we dereference object[id], when the .id property actually does not exist. It can produce a JS srict warning. The result is "undefined" which we then check for (see e.g. pop3Download.js::onDownloadCompleted()). Converting to map.get(id) also returns "undefined" but at least does not produce a warning. Also we have the possibility to call map.has(id) to get a clean existence check.
(Assignee)

Comment 1

2 years ago
Created attachment 8737613 [details] [diff] [review]
patch

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ad6b1a8ddf70baed297b02452cbcfb208ea84445
Attachment #8737613 - Flags: review?(mkmelin+mozilla)

Comment 2

2 years ago
Comment on attachment 8737613 [details] [diff] [review]
patch

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

::: mail/components/activity/modules/pop3Download.js
@@ +89,5 @@
>  
>    onDownloadCompleted : function(aFolder, aNumMsgsDownloaded) {
>      this.log.info("in onDownloadCompleted");
>  
> +    this.activityMgr.removeActivity(this._mostRecentActivityForFolder.get(aFolder.URI).eventID);

odd indention (or bugzilla just displaying it wrong?)

::: mail/components/activity/nsActivity.js
@@ +56,5 @@
>      this._subjects.push(aSubject);
>    },
>  
>    getSubjects: function(aCount) {
> +    let list = [...this._subjects];

maybe just 
let list = this._subjects.slice();
Attachment #8737613 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 3

2 years ago
Created attachment 8737631 [details] [diff] [review]
patch v1.1

Thanks.
The indentation was correct, but the line was too long so the display may have been affected.
Attachment #8737613 - Attachment is obsolete: true
Attachment #8737631 - Flags: review+
(Assignee)

Comment 4

2 years ago
I've split the line.
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/e0699438de93
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 48.0
You need to log in before you can comment on or make changes to this bug.