368074 - Look for ISP Spam .sfd Files in extension directories

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Scott MacGregor]

Very similar to Bug #360702

It should be possible for ISPs to install .sfd files as extensions and Thunderbird should read them out of the extensions directory in user profiles. 

We're thinking of just letting the .sfd files sit alongside the isp rdf files in an isp sub directory.

Additionally, we should make it possible for isps to associate an account with a .sfd file in their rdf configuration file.

Comment 1

[Scott MacGregor]

Attachment: step one, re-factor the isp directory enumerator code

In order to better leverage code between the isp rdf walker and the .sfd file walker, push more of the directory generation work into our isp directory enumerator. 

The old enumerator just generated a list of isp directories contained in enabled extensions. It was up to the caller (in this case the isp rdf walking code in nsMsgServiceProvider) to also walk through bin\isp, and to also look in isp\locale for both dist\bin and for each extension directory.

I pushed all this logic back into the file enumerator.

Comment 2

[Scott MacGregor]

Comment on attachment 252730 [details] [diff] [review]
step one, re-factor the isp directory enumerator code

Hi David, see comment #1 for details. Making the isp directory enumerator smarter by including the application directory's isp file and the local subdirectories in each isp directory will make it easier to leverage shared code between the isp walker for the account wizard and the spam filter .sfd finder code.

Comment 3

[Scott MacGregor]

I've checked attachment 252730 [details] [diff] [review] into the trunk and the branch.

Comment 4

[Scott MacGregor]

Attachment: make the code that finds .sfd files search through isp directories

For MOZ_XUL_APPs, iterate over the isp directory enumerator looking for .sfd files.

GetServerFilterFile is a new method on nsISpamSettings. It enumerates over the same list, and the first sfd file it finds that has the same name as the value of serverFilterFileName is the first one we use.

I know Neil doesn't like using the xul pre-processor (i.e. am-junk.js), but it seems like the best way to ensure that the old code gets removed from the tree when seamonkey defines MOZ_XUL_APP by default.

Comment 5

[Scott MacGregor]

Comment on attachment 252852 [details] [diff] [review]
make the code that finds .sfd files search through isp directories

Hi Neil,

I'm most interested in your thoughts on the change to am-junk.js since I know you like to minimize use of the xul pre-processor in shared mail front end code. I could just test for the isp directory property but having the ifdef makes it more obvious that we can remove code once seamonkey becomes a moz_xul_app.

Comment 6

[[e-mail]]

Attachment: Changes to nsMailDirProvider.cpp to make it compile in "xpfe" suite

Note: we'll need some build changes to actually compile this, but the idea is that when this is done then xpfe builds will automatically enumerate the base folders which means you can remove the old code from the JS files.

Note²: would it be better to run the appending enumerator on the union, rather than appending each of the individual enumerators and uniting the result?

Comment 7

[Scott MacGregor]

Attachment: updated fix that removes moz_xul_app dependencies

Based on Neil's suggestion to build the mail dir provider for 'xpfe' seamonkey as well as moz_xul_app's. This eliminated a lot of ifdefs. 

*I have not tested this patch on seamonkey but I did add the build foo for it to compile.*

I also took Neil's suggestion to make a union of the directories and then run the appending enumerator on the union. Excellent idea. 

What do you think of this Neil? Please note that I intend to land this on the branch too, not sure if you'd rather go back to the ifdef's on the branch.

Comment 8

[Scott MacGregor]

Attachment: don't forget to include the changes in base\public

I forgot to include this diff.

Comment 9

[Scott MacGregor]

Hi Karsten, see comment #7 specifically the last about about using ifdef moz_xul_app on the branch.

Comment 10

[[e-mail]]

Comment on attachment 253678 [details] [diff] [review]
updated fix that removes moz_xul_app dependencies

> const KEY_APPDIR = "XCurProcD";
> const KEY_PROFILEDIR = "PrefD";
You're not using these any more here. Note that there's currently no way for xpfe builds to read sfd files in the profile with this patch, which might be a problem on branch; perhaps the profile folder could be enumerated in this #else block:
>+#else
>+  directoryEnumerator.swap(combinedEnumerator);
>+#endif

>+      if (ispDirectory)
>+      {
>+        nsCOMPtr<nsISimpleEnumerator> enumerator;
>+        rv = ispDirectory->GetDirectoryEntries(getter_AddRefs(enumerator));
>+        if (NS_FAILED(rv))
>+          break;
>+
>+        nsCOMPtr<nsIDirectoryEnumerator> files(do_QueryInterface(enumerator));
>+        if (!files)
>+          break;
>+
>+        nsCOMPtr<nsIFile> file;
>+        while (NS_SUCCEEDED(files->GetNextFile(getter_AddRefs(file))) && file) 
>+        {
>+          nsCAutoString leafName;
>+          file->GetNativeLeafName(leafName);
>+          if (leafName.Equals(serverFilterFileName))
>+          {
>+            mServerFilterFile = file;
>+            break;
>+          }
>+        } // if we have a file in the directory to look at
>+      } // if we have a directory to examine
>+    } // until we find the location of mServerFilterName
>+  } // if we haven't already stored mServerFilterFile
This is overly complex. Just append the known leaf name to each directory as you enumerate and stop when you find a file that exists.

Comment 11

[Scott MacGregor]

Attachment: updated fix with review comments

Neil, here's an updated patch in a fresh tree (there was some cross-patch pollution in the old patch) that includes your review comments. Very nice suggestion on the change to nsSpamSettings::GetServerFilterFile.

Comment 12

[Scott MacGregor]

Attachment: updated patch with some more review comments

Neil gave me a suggestion over IM for optimizing the while loop in nsSpamSettings::GetServerFilterFile

Comment 13

[Scott MacGregor]

Comment on attachment 254593 [details] [diff] [review]
updated patch with some more review comments

for the trunk. I'll check in with Karsten about how to land this on the branch.

Comment 14

[Scott MacGregor]

Attachment: remove the existing .sfd files from defaults\messenger in the installer

Comment 15

[Scott MacGregor]

Attachment: port to the branch

Hi Karsten,

Neil helped me with this patch on the trunk such that it worked for seamonkey and Thunderbird and now I'd like to land it on the branch for Thunderbird 2. But I wanted to check with you first. We could always put the ifdefs back so it stays turned off for seamonkey on the branch. Thanks!

Comment 16

[Scott MacGregor]

Hi Karsten, can you weigh in with an opinion about what you want for seamonkey on the branch? I need to land this soon and want to know if you want me to add seamonkey ifdefs. This has been in the trunk for Thunderbird and seamonkey since 02/10. Thanks!

Comment 17

[Karsten Düsterloh]

Comment on attachment 255528 [details] [diff] [review]
port to the branch

>Index: mailnews/base/build/nsMsgFactory.cpp
>===================================================================

Don't we need this for nsMailModule.cpp, too?

>+      "mail director provider",

Typo?

Comment 18

[Scott MacGregor]

Thanks Karsten, I fixed the typo when I landed this on the branch. nsMailModule was already building the mail directory provider.