505456 - move/copying multiple imap messages to local folder bypasses offline store and redownloads messages. Need to preflight the move/copy.

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[David :Bienvenu]

If you move/copy multiple imap messages to a local folder, we run it as a single imap url with multiple messages. This bypasses the offline store cache checking, which means we'll fetch the messages from the server unnecessarily if we have the messages in the offline cache.

To fix this, I think we need to preflight the move/copy, and check if we have all the messages offline - if so, just copy the offline copies to the local folder. If we have a mix of offline and not offline messages, we could break it into two operations, but the homogeneous case is the low-hanging fruit.

Comment 1

[Wayne Mery (:wsmwk)]

hmm, that explains slowness (I frequently do this)

Comment 2

[Magnus Melin [:mkmelin]]

This sure would be nice.

Comment 3

[Wayne Mery (:wsmwk)]

if Bug 579520 doesn't get closed for odd reasons, perhaps this would help him.

Comment 4

[gene smith]

Yes, I just tested this and emails are fetched from server before they are copied of a folder in Local Folders, even if the source imap folder has offline storage. However, if the copy is to another imap server's folder, the messages are just read from offline store and then imap appended to the destination folder. I didn't know this and it may be relevant to Bug 538375
Note also that if you go offline and do the copy to Local Folders, the source data is obtained from offline store.

Comment 5

[gene smith]

Attachment: 505456-proposed-change.patch

With this change IMAP messages copied to Local Folders are now obtained from offline storage if all messages being copied reside in offline storage. Otherwise, they are fetched from the IMAP server.

Previously, if more than one IMAP messages is copied to Local Folders and if TB is online, the messages are always fetched from the IMAP server.

So this fixes the "homogeneous, low-hanging fruit" case described in Comment 0.

This greatly impacts Bug 1566717 and other similar bugs regarding copying from IMAP folders to Local Folders (See Bug 1566717 comment 53).

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9124922 [details] [diff] [review]
505456-proposed-change.patch

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

Nice low hanging fruit! r=mkmelin

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +1321,5 @@
>    nsCString protocolType;
>    rv = srcFolder->GetURI(protocolType);
>    protocolType.SetLength(protocolType.FindChar(':'));
>  
> +  // If TB is offline and the source folder is imap or news, to do the

nit: we usually comment in the "we" form.
If we're offline....

@@ +1343,5 @@
>        */
>        totalMsgSize += msgSize + 200;
>  
> +      // Check if each source folder message has offline storage regardless
> +      // of whether TB is online or offline.

here too

@@ +1349,5 @@
> +      bool hasMsgOffline = false;
> +      srcFolder->HasMsgOffline(key, &hasMsgOffline);
> +      allMsgsHaveOfflineStore = allMsgsHaveOfflineStore && hasMsgOffline;
> +
> +      // If TB is offline and not all messages are in offline storage, the copy

and here

@@ +1489,5 @@
>        (void)OnCopyCompleted(srcSupport, false);
>      }
>    } else {
> +    // This obtains the source messages from local/offline storage to do the
> +    // copy. Note: CopyMessageTo() actually handles one or more messages.

Maybe as a follow-up we could split up into two arrays and do both cases: first copy what we got locally, and only after that go and copy the ones we didn't have locally yet.

Comment 7

[gene smith]

Attachment: 505456-proposed-change(fixup1).patch

Changed "TB is" to "we're" in 3 places.

Maybe as a follow-up we could split up into two arrays and do both cases: first copy what we got locally, and only after that go and copy the ones we didn't have locally yet.

Good idea. I'll keep this in mind.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9125139 [details] [diff] [review]
505456-proposed-change(fixup1).patch

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

Thanks Gene! Looks good. One thing: the commit message should start "Bug 505456 - ", not "Bug 505456:"
I can fix that before landing.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9185bf0f28e2
Make imap folders with offline store not fetch from IMAP when copying to local folders. r=mkmelin