The default bug view has changed. See this FAQ.

clean up _flagNameList iterating in mail/base/content/folderPane.js

RESOLVED FIXED in Thunderbird 22.0

Status

Thunderbird
Folder and Message Lists
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 22.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

Description

4 years ago
While in the Unified folder view I noticed this JS warning:

Warning: ReferenceError: reference to undefined property (intermediate value)[2]
Source file: chrome://messenger/content/folderPane.js
Line: 1436

It is about this code:
 for each (let [, type,,] in Iterator(this._flagNameList)) {
   if (type[1] == aName)
     return type;

It seems there are several places where iterating over this._flagNameList can be made better.
(Assignee)

Comment 1

4 years ago
Created attachment 717412 [details] [diff] [review]
patch
Attachment #717412 - Flags: review?(mkmelin+mozilla)
Attachment #717412 - Flags: feedback?(squibblyflabbetydoo)

Comment 2

4 years ago
Comment on attachment 717412 [details] [diff] [review]
patch

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

::: mail/base/content/folderPane.js
@@ +1593,5 @@
>            let smartRoot = smartServer.rootFolder;
>            // In theory, a folder can have multiple flags set, so we need to
>            // check each flag separately.
> +          for (let [flag, name,,] of this._flagNameList) {
> +            if (aFolder.getFlag(flag))

Why the change from aFolder.flags & flag)?
Not sure i like it, at least it crosses a xpconnect bondary (or what is it called?), which is slower then just using the properties.
(Assignee)

Comment 3

4 years ago
I thought retrieving flags attribute or the getflag() method of nsIMsgFolder would be the same boundary-wise. Is it not the case?

The logic why I am doing this is that I do not like that JS code uses knowledge about the internal representation of flags and uses bit operations directly. If the representation of flags changes in the future, e.g. to an array, all the code will break. getFlag hides the internal details and would return proper results at all times (provided its implementation is updated when somebody changes the internal handling of flags).

In C++ it is less bad as it would immediately fail to compile at all spots.

Am I future-proofing too much? :)

Comment 4

4 years ago
(In reply to Magnus Melin from comment #2)
> (From update of attachment 717412 [details] [diff] [review])
> > +          for (let [flag, name,,] of this._flagNameList) {
> > +            if (aFolder.getFlag(flag))
> 
> Why the change from aFolder.flags & flag)?
> Not sure i like it, at least it crosses a xpconnect bondary (or what is it
> called?), which is slower then just using the properties.

aFolder.flags also crosses the xpconnect boundary, but you could avoid that by caching it ;-)

Comment 5

4 years ago
Comment on attachment 717412 [details] [diff] [review]
patch

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

Looks reasonable, but yes, that's likely future-proofing too much. r=mkmelin with the "old" style flag checking
Attachment #717412 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 6

4 years ago
Created attachment 719151 [details] [diff] [review]
patch v2

Ok.
Attachment #717412 - Attachment is obsolete: true
Attachment #717412 - Flags: feedback?(squibblyflabbetydoo)
Attachment #719151 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/bead6b9aeb9f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 22.0
You need to log in before you can comment on or make changes to this bug.