The default bug view has changed. See this FAQ.

do some trivial optimisations in mailnews/base/utils/folderUtils.jsm

RESOLVED FIXED in Thunderbird 22.0

Status

MailNews Core
Backend
--
trivial
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)

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

Description

4 years ago
These functions are called for all folders in the folder pane repeatedly, e.g. when the mouse is going over them or scrolling. So if the changes in the patch speed them up, it should be good.
(Assignee)

Comment 1

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

Comment 2

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

As far as I know, SeaMonkey doesn't use these functions, so maybe mkmelin can review instead?

>+  const nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags;
>+  const flags = aFolder.flags;
I don't like using const for values that change between function calls.
Attachment #724176 - Flags: review?(neil)
Attachment #724176 - Flags: review?(mkmelin+mozilla)
Attachment #724176 - Flags: feedback?(mkmelin+mozilla)
Attachment #724176 - Flags: feedback+
(Assignee)

Comment 3

4 years ago
Neil, can you tell if the 'const flags = aFolder.flags' actually saves anything? I.e. if the real value of aFolder.flags is stored into 'flags' so that further uses of 'flags' do not reach down to the nsIMsgFolder and retrieve the flags value again from C++? Or is that just a link (alias, pointer) and it does not save anything?

Comment 4

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

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

Looks good, r=mkmelin

::: mailnews/base/util/folderUtils.jsm
@@ +18,5 @@
>   * @param aFolder  the nsIMsgFolder whose special type should be returned
>   */
>  function getSpecialFolderString(aFolder) {
> +  const nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags;
> +  const flags = aFolder.flags;

yes, please don't use const for non-constants. (let instead)

aFolder.flags crosses the xpconnect bounderies, using the local variable caches that (if i understand it correctly).

@@ +78,5 @@
>      properties.push("newMessages-true");
>  
> +  if (aFolder.isServer) {
> +    properties.push("isServer-true");
> +  } else {

i'd prefer elses on new line
Attachment #724176 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 5

4 years ago
(In reply to Magnus Melin from comment #4)
> >  function getSpecialFolderString(aFolder) {
> > +  const nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags;
> > +  const flags = aFolder.flags;
> 
> yes, please don't use const for non-constants. (let instead)
For the flags variable or both of them?

> aFolder.flags crosses the xpconnect bounderies, using the local variable
> caches that (if i understand it correctly).
That would be what I want to achieve.

Comment 6

4 years ago
nsMsgFolderFlags is kind of a constant so that should be a const
(Assignee)

Comment 7

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

OK, I've put a printf into nsMsgDBFolder::GetFlags and I see there are much less calls of the function now, so the caching works.
Attachment #724176 - Attachment is obsolete: true
Attachment #724493 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/bbf12a153816
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.