453820 - Audit front-end code for unnecessary rdf QIs

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Joey Minta]

Attachment: patch v1

There are a number of spots in front-end code where we take folders and QI them to nsIRDFResources unnecessarily. Most of the time this is to access the .Value property, which as I understand it, is identical to nsIMsgFolder's .URI property. This patch converts these.

Comment 1

[Joey Minta]

Attachment: patch v2

Sorry, still getting back into the swing of things. jcranmer reminded me that to fix many areas, you have to do double the work.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 337052 [details] [diff] [review]
patch v2

>-        var re = new RegExp("^" + folderURI + "$|^" + folderURI + "\||\|" + folderURI + "$|\|" + folderURI +"\|");

Please break this to two lines.

>+    var msgDatabase = currentLoadedFolder.getMsgDatabase(msgWindow);
>+    var dbFolderInfo = msgDatabase.dBFolderInfo;
>+    var srchFolderUri = dbFolderInfo.getCharProperty("searchFolderUri");
>+    var re = new RegExp("^" + folderURI + "$|^" + msgfolder.URI + "\||\|" + msgfolder.URI + "$|\|" + msgfolder.URI +"\|");

Break the line please.

>+    var retval = (currentLoadedFolder.URI.match(re));
>+    return retval;
>   }

Why the tmp.

>-  return false;
>+  var currentURI = currentLoadedFolder.URI;
>+  return(currentURI == folder.URI);

Here too (yes, you're just moving it...)

---

Compact folder (at least from the context menu) doesn't work with this patch
Error: selectedFolder is not defined
Source File: chrome://messenger/content/widgetglue.js
Line: 99

... though that's a trivial fix.

r=me for the /mail bits with those fixed

Comment 3

[Joey Minta]

Attachment: patch v3

Nits picked, just need an sr on the identical mailnews/ changes.

Comment 4

[Joey Minta]

Pushed "patch v3" as 364:b078dbe584b0

Comment 5

[Ian Neal]

(In reply to comment #3)
> Created an attachment (id=339088) [details]
> patch v3
> 
> Nits picked, just need an sr on the identical mailnews/ changes.

Patch seems to be missing the subscribe.js changes for mail/ even though they are there for mailnews/

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> Patch seems to be missing the subscribe.js changes for mail/ even though they
> are there for mailnews/

(Later) Done by bug 455947.