Closed Bug 832034 Opened 11 years ago Closed 11 years ago

support non-IMAP async folder create types in processNextBatch of mailWindowOverlay.js

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 23.0

People

(Reporter: rkent, Assigned: aceman)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(1 file, 4 obsolete files)

The archive function requires that folders be created, which is done async in some server types.  processNextBatch of mailWindowOverlay.js does its processing async only when the server type is "imap" but other server types (for example Exchange Web Services) also require async operation.

This forces my ExQuilla extension to duplicate this rather large method, just changing a few lines where the async->imap assumption is made. I need to add some sort of hook (perhaps a server property that asserts that folders are created async) instead.
Assignee: nobody → kent
Status: NEW → ASSIGNED
Do you suggest here to add a nsIMsgIncomingServer::canDoAsyncFolders attribute that will default to false, but IMAP and other new server types can override it to true?
Then processNextBatch will check this attribute and if found true, it will run the async operations?
(In reply to :aceman from comment #1)
> Do you suggest here to add a nsIMsgIncomingServer::canDoAsyncFolders
> attribute that will default to false, but IMAP and other new server types
> can override it to true?
> Then processNextBatch will check this attribute and if found true, it will
> run the async operations?

Yes that is the general idea, or at least that is the easiest way to do this.

If I was doing the code from scratch, what I would probably do is to make folder creation always async, but local servers would return really quickly. But that would have a large regression risk at this point, so a server attribute (maybe ::foldersCreatedAsync) that you could check would accomplish the end goal a little easier.

I have not really thought through the issue of server property versus nsIMsgProtocolInfo property, but generally the server is an easier object to get, so I would lean toward that unless there was a good reason to prefer the protocol instead. jcranmer might have opinions on that.
I see the protocolInfo must be queried separately (from a service) from the server and is a bit ugly. Can we link the proper protocol info from the server object? Add a server.protocolInfo attribute, that would return the correct one and we could query it directly, e.g. server.protocolInfo.foldersCreatedAsync (and could move all the other can* attributes there).

I think having an attribute directly on the server makes sense if the value can change even among servers of the same type (imagine an isDefault attribute).
(In reply to Kent James (:rkent) from comment #2)
> If I was doing the code from scratch, what I would probably do is to make
> folder creation always async, but local servers would return really quickly.
> But that would have a large regression risk at this point, so a server
> attribute (maybe ::foldersCreatedAsync) that you could check would
> accomplish the end goal a little easier.

If we had good hybrid C++/JS analysis, I would say it's worth doing an investigation into how hard making everything be async would be. But we don't, so I think we shouldn't block anything on this. And saying more here would be too tangential.

> I have not really thought through the issue of server property versus
> nsIMsgProtocolInfo property, but generally the server is an easier object to
> get, so I would lean toward that unless there was a good reason to prefer
> the protocol instead. jcranmer might have opinions on that.

You are discussing ideas of mine from 5 years ago when I was young and naive and well before I came to appreciate the sheer complexity of TB code. :-)

Mostly, from an aesthetic standpoint, I would reason that nsIMsgProtocolInfo is invariant on the type of the server, and so things that are invariant to account types should go there. However, I will concede that said ship set sail over a decade ago, and I think that the difficulty of using nsIMsgProtocolInfo far outweighs the aesthetic benefit (the only one).
Blocks: 63369
Attached patch WIP patch (obsolete) — Splinter Review
So what if we could access the proper protocolInfo much easier? (The patch does not yet work, need to wire up the async value in the specific server types, but it shows the pattern of usage).

Or do we want to try the new capability framework as discussed in bug 272490 comment 10 ? If it could inherit default values from nsMsgIncomingServer and the specific servers would not need to add any code if they agree with the default, it would be nice. In the protocolInfo case, it looks like I need to add C++ boilerplate to all the servers.
On the other hand, the capability function may be slower as it needs to decode which capability is needed.
Attachment #723547 - Flags: feedback?(kent)
Attached patch WIP patch v2 (obsolete) — Splinter Review
Attachment #723547 - Attachment is obsolete: true
Attachment #723547 - Flags: feedback?(kent)
Attachment #723549 - Flags: feedback?(kent)
Attachment #723549 - Flags: feedback?(Pidgeot18)
Comment on attachment 723549 [details] [diff] [review]
WIP patch v2

I can see that having information that is constant for a given type is best placed in the appropriate nsIMsgProtocolInfo service and not in the per-server nsIMsgIncomingServer, and that making it easier to get that helps encourage that. So I like this approach.
Attachment #723549 - Flags: feedback?(kent) → feedback+
Comment on attachment 723549 [details] [diff] [review]
WIP patch v2

Review of attachment 723549 [details] [diff] [review]:
-----------------------------------------------------------------

Well, you're missing three of the nsIMsgProtocolInfo implementations, but besides that, this should work fine.

I hadn't realized that we already had a GetProtocolInfo helper for nsIMsgIncomingServer; exposing it via IDL means that the pain of use it is rather reduced. This doesn't solve all the problems, but for obviously static protocol capabilities, this should be a workable approach.
Attachment #723549 - Flags: feedback?(Pidgeot18) → feedback+
Assignee: kent → acelists
Component: Mail Window Front End → Backend
Product: Thunderbird → MailNews Core
Version: unspecified → Trunk
Attached patch patch v3 (obsolete) — Splinter Review
I added the definition of the attribute to all the server types now.

In opposite to the original getProtocolInfo helper, the new one caches the protocolInfo service so it should be faster. Also, callers will no longer need to request the service anew at each usage. This will become more visible in a followup bug I'll file where I make use of the protocol attribute where possible throughout the tree.

I also assume the new foldersCreatedAsync attribute will need to be used in more places than just the one in the bug description. rkent, is that right?
Attachment #723549 - Attachment is obsolete: true
Attachment #724154 - Flags: review?(kent)
Attachment #724154 - Flags: feedback?(neil)
Attachment #724154 - Flags: feedback?(florian)
I'm going to give the feedback requests time to come in before completing a review.

Looking at this briefly though, I'm not sure that "In opposite to the original getProtocolInfo helper, the new one caches the protocolInfo service so it should be faster." is the right approach. Faster, yes - but at the expense of additional memory use. Can you make the case that this "faster" is useful in some real cases?
As I said, there will be a follow-up bug that will use the protocolInfo attribute in more places.

We can probably convert these at the minimum: http://mxr.mozilla.org/comm-central/search?string=nsIMsgProtocolInfo&find=.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

And some of these:
http://mxr.mozilla.org/comm-central/search?string=nsIMsgProtocolInfo&find=.cpp&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

How much memory will this take? I think in any case it will be nothing against the grain background (200x200px transparent image) for the folder pane that was accepted right now without thoughts about memory use.

Also remember we need to add some more attributes (e.g. the canHaveVirtualFolders, hasSpamSettings) to remove the hardcoded type comparisons, so either we add it to protocolInfo and use this new handy helper or invent some new infrastructure (with new memory usage).
(In reply to Kent James from comment #2)
> (In reply to aceman from comment #1)
> > Do you suggest here to add a nsIMsgIncomingServer::canDoAsyncFolders
> > attribute that will default to false, but IMAP and other new server types
> > can override it to true?
> > Then processNextBatch will check this attribute and if found true, it will
> > run the async operations?
> 
> Yes that is the general idea, or at least that is the easiest way to do this.
> 
> If I was doing the code from scratch, what I would probably do is to make
> folder creation always async, but local servers would return really quickly.

A couple of alternative while I think of them:
1. Make passing a null callback fail when the folder creation needs to be async
2. Add an extra boolean forceAsync parameter to the creation method
3. Make the creation method return the async status.
Note: I don't actually know how feasible these options are.
Comment on attachment 724154 [details] [diff] [review]
patch v3

I think you should audit all the callers of createStorageIfMissing.

I'm also not convinced that we need to cache the protocol info.
(In reply to neil@parkwaycc.co.uk from comment #13)
> Comment on attachment 724154 [details] [diff] [review]
> patch v3
> 
> I think you should audit all the callers of createStorageIfMissing.
For what?

> I'm also not convinced that we need to cache the protocol info.
So it is trivial to make it non-cached if you all agree on that.
(In reply to aceman from comment #14)
> (In reply to comment #13)
> > I think you should audit all the callers of createStorageIfMissing.
> For what?
Emphasis was on the word all, not on the word audit.
OK, so look if they also need this treatment of imap -> any async server ?
Exactly, thanks.
Attached patch patch v4 (obsolete) — Splinter Review
OK, some occurrences in /suite too :)
Attachment #724154 - Attachment is obsolete: true
Attachment #724154 - Flags: review?(kent)
Attachment #724154 - Flags: feedback?(neil)
Attachment #724154 - Flags: feedback?(florian)
Attachment #725071 - Flags: review?(kent)
Attachment #725071 - Flags: feedback?(florian)
Attachment #725071 - Flags: review?(neil)
Comment on attachment 725071 [details] [diff] [review]
patch v4

I'm assuming the f? for me is because of the change to mail/components/im/imProtocolInfo.js. Adding a new default value there is fine with me.
Attachment #725071 - Flags: feedback?(florian) → feedback+
Comment on attachment 725071 [details] [diff] [review]
patch v4

Review of attachment 725071 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK except for a few minor issues.

::: mail/base/content/mailWindowOverlay.js
@@ +1680,5 @@
>            return;
>        }
>  
>        let forceSingle = !archiveFolder.canCreateSubfolders;
> +      if (!forceSingle && isAsync)

This is really a check for IMAP here rather than a check for isAsync. So I think that you need to replace this with:

... && (archiveFolder.server instanceof Components.interfaces.nsIImapIncomingServer)

and the get rid of the QI on the following line.

::: mailnews/base/util/nsMsgUtils.cpp
@@ +959,5 @@
>      // for local folders, path is to the berkeley mailbox.
>      // for imap folders, path needs to have .msf appended to the name
>      msgFolder->GetFilePath(getter_AddRefs(folderPath));
>  
>      nsCString type;

type is only used to check for imap (which is now deleted), so you can eliminate the GetType code
Attachment #725071 - Flags: review?(kent) → review-
Attached patch patch v5Splinter Review
Thanks.
Attachment #725071 - Attachment is obsolete: true
Attachment #725071 - Flags: review?(neil)
Attachment #727312 - Flags: review?(kent)
Comment on attachment 727312 [details] [diff] [review]
patch v5

Looks good. Thanks for doing this!
Attachment #727312 - Flags: review?(kent) → review+
Attachment #727312 - Flags: superreview?(mbanner)
Attachment #727312 - Flags: review?(neil)
Attachment #727312 - Flags: superreview?(mbanner) → superreview+
Neil, could you just confirm the suite changes?
Attachment #727312 - Flags: review?(neil) → review+
Comment on attachment 727312 [details] [diff] [review]
patch v5

>+  /**
>+   * The proper instance of nsIMsgProtocolInfo corresponding to this server type.
>+   */
>+  readonly attribute nsIMsgProtocolInfo protocolInfo;
There should be a new bug to audit existing consumers e.g.

let protocolinfo = Components.classes["@mozilla.org/messenger/protocol/info;1?type=" + currentServer.type].getService(Components.interfaces.nsIMsgProtocolInfo);
if (protocolinfo.canLoginAtStartUp && currentServer.loginAtStartUp)

becomes

if (currentServer.protocolInfo.canLoginAtStartUp && currentServer.loginAtStartUp)
Yes, I plan to use the new attribute where it is useful throughout the tree, as I mentioned in comment 9.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/d3b26176b0df

Should this have tests?
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
Not sure what to test here. The new attribute? By comparing it to Components.classes["@mozilla.org/messenger/protocol/info;1?type=" + currentServer.type].getService(Components.interfaces.nsIMsgProtocolInfo)? Or that we did not break existing code by converting it to the new attribute? Hopefully existing tests cover that.
Marking this addon-compat not because this could break addons (it shouldn't as it is just added code) but because addons may want to hook onto this new attribute.
Keywords: addon-compat
Blocks: 861767
You need to log in before you can comment on or make changes to this bug.