Closed Bug 844373 Opened 7 years ago Closed 7 years ago

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

Categories

(Thunderbird :: Folder and Message Lists, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 22.0

People

(Reporter: aceman, Assigned: aceman)

Details

Attachments

(1 file, 1 obsolete file)

8.12 KB, patch
aceman
: review+
Details | Diff | Splinter Review
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.
Attached patch patch (obsolete) — Splinter Review
Attachment #717412 - Flags: review?(mkmelin+mozilla)
Attachment #717412 - Flags: feedback?(squibblyflabbetydoo)
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.
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? :)
(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 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+
Attached patch patch v2Splinter Review
Ok.
Attachment #717412 - Attachment is obsolete: true
Attachment #717412 - Flags: feedback?(squibblyflabbetydoo)
Attachment #719151 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/bead6b9aeb9f
Status: ASSIGNED → RESOLVED
Closed: 7 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.