Last Comment Bug 832034 - support non-IMAP async folder create types in processNextBatch of mailWindowOverlay.js
: support non-IMAP async folder create types in processNextBatch of mailWindowO...
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 23.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 63369 861767
  Show dependency treegraph
 
Reported: 2013-01-17 14:52 PST by Kent James (:rkent)
Modified: 2013-04-15 02:59 PDT (History)
8 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP patch (14.80 KB, patch)
2013-03-11 10:42 PDT, :aceman
no flags Details | Diff | Review
WIP patch v2 (16.52 KB, patch)
2013-03-11 10:45 PDT, :aceman
rkent: feedback+
Pidgeot18: feedback+
Details | Diff | Review
patch v3 (18.12 KB, patch)
2013-03-12 15:04 PDT, :aceman
no flags Details | Diff | Review
patch v4 (28.16 KB, patch)
2013-03-14 13:23 PDT, :aceman
rkent: review-
florian: feedback+
Details | Diff | Review
patch v5 (28.42 KB, patch)
2013-03-20 12:17 PDT, :aceman
rkent: review+
neil: review+
standard8: superreview+
Details | Diff | Review

Description Kent James (:rkent) 2013-01-17 14:52:27 PST
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.
Comment 1 :aceman 2013-02-13 00:12:49 PST
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?
Comment 2 Kent James (:rkent) 2013-02-13 12:03:49 PST
(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.
Comment 3 :aceman 2013-02-13 12:13:37 PST
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).
Comment 4 Joshua Cranmer [:jcranmer] 2013-02-13 17:45:40 PST
(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).
Comment 5 :aceman 2013-03-11 10:42:46 PDT
Created attachment 723547 [details] [diff] [review]
WIP patch

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.
Comment 6 :aceman 2013-03-11 10:45:15 PDT
Created attachment 723549 [details] [diff] [review]
WIP patch v2
Comment 7 Kent James (:rkent) 2013-03-11 13:15:24 PDT
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.
Comment 8 Joshua Cranmer [:jcranmer] 2013-03-11 13:42:32 PDT
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.
Comment 9 :aceman 2013-03-12 15:04:18 PDT
Created attachment 724154 [details] [diff] [review]
patch v3

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?
Comment 10 Kent James (:rkent) 2013-03-13 13:23:29 PDT
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?
Comment 11 :aceman 2013-03-13 13:55:54 PDT
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).
Comment 12 neil@parkwaycc.co.uk 2013-03-13 15:37:18 PDT
(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 13 neil@parkwaycc.co.uk 2013-03-13 15:39:31 PDT
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.
Comment 14 :aceman 2013-03-14 01:01:54 PDT
(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.
Comment 15 neil@parkwaycc.co.uk 2013-03-14 01:36:08 PDT
(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.
Comment 16 :aceman 2013-03-14 01:51:02 PDT
OK, so look if they also need this treatment of imap -> any async server ?
Comment 17 neil@parkwaycc.co.uk 2013-03-14 02:24:30 PDT
Exactly, thanks.
Comment 18 :aceman 2013-03-14 13:23:01 PDT
Created attachment 725071 [details] [diff] [review]
patch v4

OK, some occurrences in /suite too :)
Comment 19 Florian Quèze [:florian] [:flo] 2013-03-19 16:26:10 PDT
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.
Comment 20 Kent James (:rkent) 2013-03-20 11:52:14 PDT
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
Comment 21 :aceman 2013-03-20 12:17:16 PDT
Created attachment 727312 [details] [diff] [review]
patch v5

Thanks.
Comment 22 Kent James (:rkent) 2013-03-21 15:31:38 PDT
Comment on attachment 727312 [details] [diff] [review]
patch v5

Looks good. Thanks for doing this!
Comment 23 :aceman 2013-04-13 02:57:47 PDT
Neil, could you just confirm the suite changes?
Comment 24 neil@parkwaycc.co.uk 2013-04-13 07:55:18 PDT
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)
Comment 25 :aceman 2013-04-13 12:16:05 PDT
Yes, I plan to use the new attribute where it is useful throughout the tree, as I mentioned in comment 9.
Comment 26 Ryan VanderMeulen [:RyanVM] 2013-04-14 18:25:15 PDT
https://hg.mozilla.org/comm-central/rev/d3b26176b0df

Should this have tests?
Comment 27 :aceman 2013-04-14 23:58:50 PDT
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.
Comment 28 :aceman 2013-04-15 00:54:33 PDT
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.

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