Last Comment Bug 781650 - gloda should allow search of mail folder types added by extensions
: gloda should allow search of mail folder types added by extensions
Product: MailNews Core
Classification: Components
Component: Search (show other bugs)
: Trunk
: x86_64 Windows 7
-- normal (vote)
: Thunderbird 17.0
Assigned To: Kent James (:rkent)
Depends on:
  Show dependency treegraph
Reported: 2012-08-09 14:05 PDT by Kent James (:rkent)
Modified: 2012-08-14 11:44 PDT (History)
1 user (show)
rkent: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Don't check folder type (1.37 KB, patch)
2012-08-10 15:32 PDT, Kent James (:rkent)
bugmail: review+
Details | Diff | Splinter Review

Description User image Kent James (:rkent) 2012-08-09 14:05:13 PDT

Comment 1 User image Kent James (:rkent) 2012-08-09 14:15:33 PDT
In index_msg.js we have the following code:
  shouldIndexFolder: function(aMsgFolder) {
    let folderFlags = aMsgFolder.flags;
    if (!(folderFlags & Ci.nsMsgFolderFlags.Mail) ||
        (folderFlags & Ci.nsMsgFolderFlags.Virtual))
      return false;
    if (!(aMsgFolder instanceof nsIMsgLocalMailFolder) &&
        !(aMsgFolder instanceof nsIMsgImapMailFolder))
      return false;

This hard-wires gloda to only support existing folder types, local and imap. But mail folder types added by an extension should also be searchable. The real reason for the "instanceof" checks here is to make sure that RSS folders (which derive from nsIMsgLocalFolder) are not searched. I propose to add an explicit check for rss type here rather than prevent other mail folders (who are correctly setting their Ci.nsMsgFolderFlags.Mail flag, unlike rss!) from also being searched.

This is similiar to the check that is added in newMailNotificationService:

169     // If it's not a mail folder we don't count it by default
170     if (!(aFolder.flags & Ci.nsMsgFolderFlags.Mail))
171 = false;
173     // For whatever reason, RSS folders have the 'Mail' flag
174     else if ((srv = aFolder.server) && (srv.type == "rss"))
175 = false;
Comment 2 User image Andrew Sutherland [:asuth] 2012-08-09 15:16:33 PDT
Sounds good.
Comment 3 User image Kent James (:rkent) 2012-08-09 15:37:33 PDT
After I reread my comment 1, I realized that was incorrect. In fact, rss passes both tests, and gets true from shouldIndexfolder.

After IRC discussions with asuth, we could not establish a case where the instanceof checks catch anything in normal operation, so the patch will simply remove those tests.
Comment 4 User image Kent James (:rkent) 2012-08-10 15:32:31 PDT
Created attachment 651018 [details] [diff] [review]
Don't check folder type
Comment 5 User image Kent James (:rkent) 2012-08-14 11:43:37 PDT
Comment on attachment 651018 [details] [diff] [review]
Don't check folder type

Checked in

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