Last Comment Bug 844373 - clean up _flagNameList iterating in mail/base/content/folderPane.js
: clean up _flagNameList iterating in mail/base/content/folderPane.js
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Folder and Message Lists (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 22.0
Assigned To: :aceman
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-22 17:33 PST by :aceman
Modified: 2013-02-27 13:45 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (8.31 KB, patch)
2013-02-22 17:36 PST, :aceman
mkmelin+mozilla: review+
Details | Diff | Splinter Review
patch v2 (8.12 KB, patch)
2013-02-27 13:22 PST, :aceman
acelists: review+
Details | Diff | Splinter Review

Description :aceman 2013-02-22 17:33:54 PST
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 2013-02-22 17:36:16 PST
Created attachment 717412 [details] [diff] [review]
patch
Comment 2 Magnus Melin 2013-02-26 13:19:30 PST
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 2013-02-26 13:48:32 PST
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 neil@parkwaycc.co.uk 2013-02-26 14:33:25 PST
(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 2013-02-27 11:38:49 PST
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 2013-02-27 13:22:13 PST
Created attachment 719151 [details] [diff] [review]
patch v2

Ok.
Comment 7 Ryan VanderMeulen [:RyanVM] 2013-02-27 13:45:51 PST
https://hg.mozilla.org/comm-central/rev/bead6b9aeb9f

Note You need to log in before you can comment on or make changes to this bug.