435775 - Clean up nsIMsgIncomingServer's attribute list

Status: NEW

(MailNews Core :: Backend, enhancement)

Description

[Joshua Cranmer [:jcranmer]]

Working on my listarchive extension, I have come across some members of nsIMsgIncomingServer that are either unused or used only within nsMsgIncomingServer and thus should be elided from JS components defining such a class. Documenting these entities would also be helpful.

Comment 1

[Joshua Cranmer [:jcranmer]]

Basic outline of what I would like to see:

Short term (i.e., to be included in patch):
Remove:
+ forgetSessionPassword (replace calls to with forgetPassword)
+ isSecureServer (use nsIMsgProtocolInfo instead)
+ get/set*Attribute (use respective get/set*Value instead)
Move to nsMsgIncomingServer:
+ onUserOrHostNameChanged
Move to server-specific implementation:
+ logonFallback
+ downloadOnBiff
Other:
+ make canOnlyDelete readonly

Medium term (not going to be done on this bug):
+ Remove the real* properties. The dichotomy is bound to confuse people.
+ Make type readonly or remove at altogether in favor of instanceof'ing.
+ Push localStoreType to be used only internally to nsMsgIncomingServer
+ Clean out the absolute/relative preferences on *FileValue

Long term (requires a lot of changes that can't be done anytime soon):
+ Split nsIMsgIncomingServer into two objects: one that represents a specifics-agnostic object just doing stuff like handling password saving, pref selecting, and a second object that represents stuff that are very server-specific (like getNewMessages or performBiff). In fact, this is already done to some degree with the ns*Service classes.

I'm only about 1/4 of the way through the class. Finding usages and documenting what a method does ex post facto ain't fun...

Comment 2

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 0.5

This is a start on what I have. Some of the documentation yet needs to be tweaked.

I am now somewhat more than half the way through. My goal is to finish the patch during the TB 3.0b1 code freeze, since most changes to the IDL file will cause the entire patch to rejected thanks to the copious amounts of new documentation.

I would also like to take the time to propose something moderately radical: to move deferredToAccount to nsIMsgIncomingServer and remove rootMsgFolder, probably with a paragraph or two of documentation explaining what's going on.

Comment 3

[Joshua Cranmer [:jcranmer]]

Summary of what patch 1.0 will do:
Move elsewhere:
+ onUserOrHostNameChanged (nsMsgIncomingServer)
+ logonFallback (nsIPop3IncomingServer)

Remove altogether:
+ constructedPrettyName
+ forgetSessionPassword
+ isSecureServer
+ setFilterList (possibly)          **
+ downloadMessagesAtStartup
+ canEmptyTrashOnExit (possibly)    **
+ generatePrettyNameForMigration
+ defaultCopiesAndPrefsToServer (should be same as canCreateFoldersOnServer)
+ {g,s}et*Attribute (in favor of {g,s}et*Value)

Make readonly:
+ rootFolder
+ canDelete (would remove, but it could break stuff)

Probably move to nsIMsgProtocolInfo:
+ canBeDefaultServer                **
+ canSearchMessages                 **
+ canCreateFoldersOnServer          **
+ canFileMessagesOnServer           **
+ canCompactFoldersOnServer         **
+ canUndoDeleteOnServer             **

Other changes that I would like to make but will not in this patch:
+ Remove the absolute prefs from *FileValue
+ Move deferredToAccount to nsIMsgIncomingServer

Comment 4

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 0.LAST

This is a patch indicating more or less what the final product will converge to. However, I am not going to be evil and request review on this monolithic patch. Instead, the changes will be done in at least three patches:

1. Fix the get*Value/get*Attribute mess
2. Remove the AOL IMAP server hacks
3. Everything else

Other related patches that I may work on, but will not be discussed in this bug:
4. The Wonderful World of Deferred Accounts™
5. The JS version of nsMsgIncomingServer
6. Extensibility of search/filtering

The documentation comments I have on the attributes will more or less stay the same in the final product, so feel free to comment on the comments now.

I will also not be getting any of the patches up for review until after kill-rdf lands, since that will probably bitrot a lot of the changes.

Comment 5

[Joshua Cranmer [:jcranmer]]

Attachment: [checked in] Remove redundant get*Attribute

This removes all of the get*Attribute methods from nsIMsgIncomingServer, as they are 100% redundant with get*Value (and much less used than those methods).

Comment 6

[Joshua Cranmer [:jcranmer]]

Comment on attachment 353836 [details] [diff] [review]
[checked in] Remove redundant get*Attribute

First patch checked in as changeset 85bf1d0523fd.

Comment 7

[Ludovic Hirlimann [:Usul]]

What's needed for patch v2 ?

Comment 8

[WADA:World Anti-bad-Duping Agency]

Attachment: List of current properties of incomingServer(by for in, Object for Gmail IMAP server to see IMAP properties)

This is list of current properties of incomingServer for a Gmail IMAP account obtained by writing data of "(for key in obj)key+"="+obj[key];" to Error Console with Thuderbird 17.0.2 on Win-XP.
For ease of viewing, properties are separated to non-function propery and function property, and property is sorted in alphabetic order of property name.

Which property should be improved next?

Comment 9

[WADA:World Anti-bad-Duping Agency]

Attachment: List of current properties of incomingServer(by for in, Object for Gmail IMAP server to see IMAP properties)

Comment 10

[:aceman]

It seems the patch 2 basically means taking the parts from Patch 0.LAST that were not split out to the already checked in patch.

If that is the case I could continue with this. I just wonder if all the assumptions in the full patch are still valid. E.g. it hardcodes canHaveFilters to true, reduces offlineSupportLevel to just a bool. Are all the changes still valid today, e.g. after introducing IM accounts? There is a comment, that other server types should override the whole method so maybe that is fine (and the specific server type will manage the offline levels internally if needed).

I also see that the proposed properties are not yet moved to nsIMsgProtocolInfo. That will be done later for manageability of the changes.

Jcranmer, do I understand it correctly? May I take this?

Comment 11

[Kent James (:rkent)]

The original intent of this bug as I understand it was to make it easier to implement new account types through extensions by simplifying the methods that are needed to be defined in js-based nsIMsgIncomingServer implementations. That larger effort has been mostly abandoned by jcranmer as far as I can tell. The only extant examples of generating new account types are those of my extensions, and I have taken a different approach, namely to provide a C++ wrapper around (or subclassing) the existing nsMsgIncomingServer implementation, and extending that either through javascript (in TweeQuilla) or in C++ (in ExQuilla).

Unless the changes in this code are accompanied by a renewed commitment to make it easier to implement a js-based new account type, then I'm afraid that continuing the work here may just be meaningless code churn, that will force extension writers to adapt without any real benefit. Do you intend to continue this work to the point of creating a canonical js-based nsIMsgIncomingServer implementation (or alternatively implementing something like my msqIOverride interface that I use in SkinkGlue to allow js to override a base C++ implementation)?

But ignoring that for a minute, progress in simplification should result in removing methods from nsIMsgIncomingServer, or conversely by adding new server attributes so that server features can be specified in nsIMsgIncomingServer, rather than hard-wired in UI code that looks like "if server.type == 'imap' then ... else if server.type == 'news' then ..." (See bug 832034 for an example of this issue).

I don't see that changes like "hardcodes canHaveFilters to true, reduces offlineSupportLevel to just a bool" move us forward much in regard to simplification. What I would rather see are attempts to move certain attributes into their subclass variants such as msIMsgPop3IncomingServer, or to some other protocol-specific interface, so that nsIMsgIncomingServer is simpler. Moving to nsIMsgProtocolInfo, while it might have some architectural benefit, does not really help the problem of new account types, as that interface also must be implemented in a new account type, so you just move the work from one method to another.

What do you see the larger purpose of continuing the work in this bug?

Comment 12

[Joshua Cranmer [:jcranmer]]

rkent is correct--when I originally filed this bug, the idea was that people would be largely reimplementing nsIMsgIncomingServer from scratch in JS, so I wanted to minimize the amount of necessary boilerplate. That approach is now pretty hopeless, and I think the right way to go there is to fix bug 514409 (although not in the way I did it--rkent's msqIOverride framework is the better way to go, although I think its interfaces could use a rename).

There may be some value in removing unneeded parts of the interfaces, or in removing unnecessarily complication (like nsIMsgFolder and its 5 different name properties). Some of the things I was looking at doing "later" (like rigidly standardizing the deferred-to notions) are arguably still useful for new account types, but it's all worthless until we actually have evidence that people are attempting to do this.

At this point, probably the only part of my original patch that is still worth updating and checking is all of the documentation I added to nsIMsgIncomingServer.

Comment 13

[:aceman]

(In reply to Kent James (:rkent) from comment #11)
> What do you see the larger purpose of continuing the work in this bug?

I can't help with the larger purpose you both are talking about. I do not understand what need to be done there. I just wanted to refresh the remaining parts of this patch here (mostly comments) as I can understand it and I like that some attributes could be changed to readonly.

> What I would rather see are attempts to move certain
> attributes into their subclass variants such as msIMsgPop3IncomingServer, or
> to some other protocol-specific interface, so that nsIMsgIncomingServer is
> simpler. Moving to nsIMsgProtocolInfo, while it might have some
> architectural benefit, does not really help the problem of new account
> types, as that interface also must be implemented in a new account type, so
> you just move the work from one method to another.

I think this is done in the patch at least for the "logon_fallback", downloadMessagesAtStartup, IsSecureServer, setDefaultLocalPath.

If you look at the patch '0.LAST', it only covers steps 1.-3. from comment 4. It seems most of the patch is just adding comments and shuffling the attributes I just wrote. It seems it is worth to apply these from what you say.
Maybe something more could be cherrypicked from comment 1 to comment 3.

I can't implement steps 4.-6. But I do not see that steps 1.-3. are not worth to land without doing steps 4.-6. too.

Comment 14

[Kent James (:rkent)]

Are the documentation changes that jcranmer did landed? If not, those would definitely worth completing.

I had not noticed the changes that you mentioned the reduced the size of the nsIMsgIncomingServer. Those have some value.

But who are you trying to help here? If you are trying to help extension writers that support new account types, then I am currently your main customer. I would much rather that you take on bug 832034 as that forces new account type addons to provide a local duplicate of a core js function, which is much more fragile than the usual overlay where you wrap the existing function and call it along with some new functionality.

Comment 15

[:aceman]

(In reply to Kent James (:rkent) from comment #14)
> But who are you trying to help here? If you are trying to help extension
> writers that support new account types, then I am currently your main
> customer. I would much rather that you take on bug 832034 as that forces new
> account type addons to provide a local duplicate of a core js function,
> which is much more fragile than the usual overlay where you wrap the
> existing function and call it along with some new functionality.

I want to help the case of not loosing useful code changes that jcranmer already prepared. If you discourage me from salvaging parts of the patch here, you still won't win me for related work on bugs like 832034. It is not a matter of redistributing my time, but of my lack of knowledge. So in that case I just move on to other completely unrelated stuff.

Comment 16

[Kent James (:rkent)]

I'm sure that you have figured out by now that I am probably the most conservative contributor when it comes to changing existing code. "If it ain't broke, then don't fix it" is pretty important to me.

This bug is a really good example of that already. The first patch caused the regression of bug 473781 (and I was the one who made the connection between that bug and this bug). So code churn can waste a lot of time, not only from the initial code and review, but from the subsequent regressions and addon fixes.

So from my perspective, any documentation changes from these existing patches are worth keeping. But any code changes are not, as they are directed toward a purpose that no longer exists, so the time and risk associated with them are not worthwhile.

Comment 17

[:aceman]

It looked like the code changes in the existing patch are basically fine. You just didn't want new changes unless a new target/strategy is properly determined.
Now you don't want any changes...

Comment 18

[Joshua Cranmer [:jcranmer]]

I am so far out of touch with what is needed to get new account types working properly (I haven't touched my prototype for 2 years now) that I can't recall what changes I made here or why I made them. I stopped maintaining WIP patches here long ago, since it broke everytime someone touched nsIMsgIncomingServer (which was sadly more common than one would hope), and I know one of the changes in that patch is no longer true (the isSecureServer patch is now treated as a shortcut for querying the socket type).

The documentation comments haven't been checked in, although I see that a few of the methods had some of the documentation comments from this WIP checked in other patches (the filter list and getPasswordWithUI, e.g.).

I've vacillated over the years about what to do with respect to new message account types, but I don't think cleaning up nsIMsgIncomingServer is a worthwhile target at this point in time. A much more worthwhile goal would be ending hardcoding of server types everwhere, or making our database operations inherently asynchronous, or even just plain de-RDF.

My gut reaction is to just keep the documentation changes and jettison the rest of the WIP code until some as-yet undetermined future date.

Comment 19

[:aceman]

So let me refresh this and maybe I will better understand what you guys actually want.

If answer to bug 832024 comment 1 is 'yes', then I could play with those and add some server 'capabilities' so that direct type comparisions (server.type == 'imap') are not needed.

Comment 20

[:aceman]

jcranmer, you wanted to remove AOL server hacks. Is that one no longer supported?

Comment 21

[Kent James (:rkent)]

(In reply to :aceman from comment #19)
> So let me refresh this and maybe I will better understand what you guys
> actually want.
> 
> If answer to bug 832024 comment 1 is 'yes', then I could play with those and
> add some server 'capabilities' so that direct type comparisions (server.type
> == 'imap') are not needed.

(you mean bug 832034 comment 1). The answer is "yes".

If you search for "server.type" in mxr you will see plenty of other examples of this type of issue. I only picked on the one issue in bug 832034 because getting that code to work is critical to support for archiving, and the only way that an extension can overlay it is to replace the entire routine with an extension copy. That means that any fixes to that part of the code in core would also need to get propagated to the extension, which is not desirable. In many other cases, the extension can fix issues by adding some code at the beginning or end of a javascript function, and in that case the overlay can call the original routine, which is a cleaner form of overlay.

Comment 22

[:aceman]

Yes. I know more such places from account manager, where various server capabilities are determined as hardcoded checks for the type.
So I think we can add these new attributes in a separate bug.

Comment 23

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.