739908 - add more attributes to nsIMsgIncomingServer to show capabilities an IM server does not have (like localPath, spamSettings)

Status: NEW

(MailNews Core :: Backend, enhancement)

Description

[:aceman]

We have identified some capabilities on an IM server type that it does not have and we'd like to check for them. Testing for the existing attributes produces at least a JS strict warning (undefined property) or an error (that the property is null).
Currently these are:
bug 738487: .spamSettings
bug 739555: .localPath

So currently by have to do checks like if (server.type != "im") { server.localPath ...  }

That is not future-proof and is something bug 63369 wants to eliminate.

So the point of this bug is to extend nsIMsgIncomingServer with new attributes like .hasSpamSettings, .hasLocalPath that will say if the server has the capability and could be checked for cleanly. This is similar to existing attributes like .canBeDefaultServer .

Comment 1

[Florian Quèze [:florian]]

I would like something like "hasMsgFolders" and then remove the half backed nsIMsgFolder implementation at http://mxr.mozilla.org/comm-central/source/mail/components/im/imIncomingServer.js#216
I resorted to using this hack for now because there are lots of places in mailnews that assume any account has a rootMsgFolder, and not respecting this assumption can cause crashes in random places.

Comment 2

[Florian Quèze [:florian]]

Maybe we would want them in http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgProtocolInfo.idl#49 instead.

Comment 3

[:aceman]

Rkent, instead of adding new attributes, would it be better here to handle spamSettings == null as indication that the server does not handle junk ?

Comment 4

[:aceman]

Or we can make sure that when server.spamSettings.level == 0 we never touch the other attributes, because the server has no spam capability and the attributes will not have useful/sane values (may even throw).

Which one would you like?

Comment 5

[:aceman]

OK server.spamSettings.level == 0 can mean spam capability is only turned off, but the server supports it. So the spamSettings == null seems better.

Comment 6

[Kent James (:rkent)]

(Trying to respond to old requests)

Any time I see "add more attributes" I get nervous, as the long-term goal would be less attributes, not more.

server.spamSettings.level = 0 would force you to implement a dummy nsISpamSettings just to show that it is irrelevant, so the spamSettings == null is a better choice.

But we have this unresolved "standard" that we can check the error return value as an anti-crash measure, so following that standard you would see code like:

nsCOMPtr<nsISpamSettings> spamSettings;
rv = server->GetSpamSettings(getter_AddRefs(spamSettings));
NS_ENSURE_SUCCESS(rv, rv);
spamSettings->GetLevel(&level);

which would crash is someone assumed that your code was following that "standard". I think we should do away with that standard, and just do what I do in my code:

nsCOMPtr<nsISpamSettings> spamSettings;
server->GetSpamSettings(getter_AddRefs(spamSettings));
NS_ENSURE_STATE(spamSettings);
spamSettings->GetLevel(&level);

as the usual code when the expected response is that the object exists. So if we can get rid of that, then your approach make sense.

But really the direction I would like to see us go is to leave nsIMsgIncomingServer as including the features for code derived from nsMsgIncomingServer.cpp, but have a smaller interface, maybe nsIMsgBaseServer, which is used in most places, and does not require the use of nsIMsgIncomingServer. That would be true of other base interfaces as well (nsIMsgDatabase, nsIMsgFolder, etc.) so that one could do a smaller js-only implementation (such as IM attempted to do, or TweeQuilla) without having so much irrelevant baggage to carry forward.

So the issue is larger than just this bug, and I think that we need to have a larger discussion about possibly changing some of the ways that we structure the overall code if it is to be more flexible in the future.