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

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[:aceman]

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.

Comment 1

[:aceman]

Attachment: patch

Comment 2

[Magnus Melin [:mkmelin]]

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.

Comment 3

[:aceman]

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

[[e-mail]]

(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

[Magnus Melin [:mkmelin]]

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

Comment 6

[:aceman]

Attachment: patch v2

Ok.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/bead6b9aeb9f