Auto-Sync improvement: new e-mail download policy change

RESOLVED FIXED in Thunderbird 3.0b2

Status

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: bugmil.ebirol, Assigned: bugmil.ebirol)

Tracking

Trunk
Thunderbird 3.0b2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
According to the current policy, if the serial model is selected, when new
emails arrive during non-idle time, auto-sync immediately downloads first 60K
(see kFirstGroupSizeLimit) worth of messages, if and only if there is no other
pending* sibling** of the receiving folder in the priority queue.

Rational: Under the serial model, auto-sync limits itself to download one and
only one folder of a particular imap account at any given time. Since
downloads of large messages can overflow to non-idle time, auto-sync makes a
safe and lazy decision by not starting a new download that might potentially overlap with an on-going one.

Proposed improvement: Auto-Sync can make a better decision by checking the status of pending siblings; If the status of the siblings are different than
stDownloadInProgress then it can assume that it is safe to start a new
download.  

*waiting its turn to get downloaded during the next system idle time
**sibling means another folder of the same imap account
Flags: blocking-thunderbird3?
As a bit of UX background:

With the current policy, if, for whatever reason, auto-sync has scheduled to do some downloading of any folder on the current account, and the user selects a folder with messages for which we have headers but no bodies, then those messages will _not_ be downloaded until the idle-time autosync happens.  If nothing has been scheduled for download, then the experience is _much_ better, as the messages that the user is expressing interest in are downloaded right away.
(Assignee)

Comment 2

10 years ago
One correction to David's comment; this behavior is independent from whether we have the headers or not. it treats the newly downloaded headers the exact same way. As a result, when the selected folder gets an IDLE message from the server, the current policy prevents autosync from downloading the first group of pending messages do fit into 60K limit - assuming that another folder of the current account is already in the q.
(Assignee)

Comment 3

10 years ago
Created attachment 352600 [details] [diff] [review]
Rev 1

This path forces autosync to download the first group of messages do fit into the 60K size limit, when there is no sibling in download-in-progress state.
Attachment #352600 - Flags: superreview?(bienvenu)
Attachment #352600 - Flags: review?(bienvenu)

Comment 4

10 years ago
Comment on attachment 352600 [details] [diff] [review]
Rev 1

it doesn't look like anyone is using the outIndex of DoesQContainAnySibling - is that for future work? If so, can that be documented with a comment?

I don't see any code handling the -1 state - I would expect it here:

+    if (NS_SUCCEEDED(rv) && aState == state)
+      break;
+    else
+      offset++;

This looks OK otherwise, except that for the given scenario of the user actually clicking on a folder, I think we should always do a sync of new headers in that folder, no matter what's going on w/ auto sync. But I guess we really don't know that the user has clicked on a folder in the auto sync code (though I think there is a handy method sMsgMailSession::IsFolderOpenInWindow(nsIMsgFolder *folder, PRBool *aResult) that might be of some use)
(Assignee)

Comment 5

10 years ago
>it doesn't look like anyone is using the outIndex of DoesQContainAnySibling -
>is that for future work? If so, can that be documented with a comment?

IIRC we refactored out the method that was using this argument at some point in the past. I didn't remove outIndex from the signature of 
DoesQContainAnySiblingOf since it is handy and _optional_.

>I don't see any code handling the -1 state - I would expect it here:

We deal with this special case here;
+  if (aState == -1)
+    return (nsnull != SearchQForSibling(aQueue, aAutoSyncStateObj, 0, aIndex));

>..I think we should always do a sync of new headers in that folder, 
> no matter what's going on w/ auto sync.

This patch will provide us the behavior closest to what you have described. Unfortunately we cannot totally ignore what's going on with auto sync because of "one download per account any given time" limitation (at least in serial download model of auto sync).

I seems to me that we have two options to truly realize the scenario that you have mention; We can cancel the on-going download operation (bug 456839), and start a new one immediately for the manually selected folder, or, we can start a second download for the selected folder regardless of the on-going download on the same account. Implementing the later is easy but I don't understand all the side effects. Former is relatively difficult since we have to implement a cancel operation for auto sync/imap which is, as I understand it, expensive. Thoughts?
 
>sMsgMailSession::IsFolderOpenInWindow(nsIMsgFolder *folder, PRBool *aResult)
>that might be of some use)

This could be very handy when prioritizing the folders. I am going to test this 
method to see how it behaves and file another bug for this improvement. Thanks.

Updated

10 years ago
Attachment #352600 - Flags: superreview?(bienvenu)
Attachment #352600 - Flags: superreview+
Attachment #352600 - Flags: review?(bienvenu)
Attachment #352600 - Flags: review+

Comment 6

10 years ago
Comment on attachment 352600 [details] [diff] [review]
Rev 1

d'uh, ok - I see.

No, I wasn't suggesting canceling the previous request - I meant it would be OK in this particular situation to fire off an other request for the current folder. By default, we can get up to 5 connnections going, so using 2 temporarily for autosync in this particular situation seems OK to me...
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/2fd550b6f62f
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: blocking-thunderbird3?
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.