Currently, when TB does an offline move/copy/delete operation, it assigns fake keys to the newly created messages in the destination folder. This is reasonable since it doesn't know what keys will be given to these messages until it syncs up with the server. During the sync, it queries all the existing keys for given folder, and updates its local storage based on the keys fetched from the server. It removes the non existing keys, and adds the new ones. If autosync_offline_stores option is enabled, it also starts to download automatically the message bodies referenced by the new keys. Since TB starts to do all imap operations asynchronously in the context of Better Faster IMAP initiative, the behavior mentioned above need some adjustments, improvements. The focus of this task will be in the following areas: o Eliminating unnecessary message key removal/insertion to the local store o Eliminating unnecessary/multiple message body downloads

Dale, this is one part of the imap revamp that might be interesting for you. The issue is this: When we do a move/copy of an imap message that we have an offline copy of, we move/copy the offline copy to the target folder's offline store as well. If you're truly offline, that allows you to see the message in the target folder. But, when you open the target folder online, we lose the fact that we have the message body in the offline store, because we don't know that the newly downloaded header corresponds to the "fake" offline header we had. What we were thinking of doing was the following: As Emre said, when we sync a folder with the server, we would compare the new headers we're downloading with the "fake" headers we had before, and if they match, we set the MSG_FLAG_OFFLINE flag on the new header, and set the offline msg offset in the new header to the value that was in the fake header. Figuring out that they match would probably be something simple like comparing message-id and message size of the new header with the fake header. The harder problem is remembering the fake header. We delete the fake header very early in the sync process, long before we get the new header. Here's where I think we delete the fake header: http://mxr.mozilla.org/mozilla/source/mailnews/imap/src/nsImapOfflineSync.cpp#816 So we currently delete the fake header early so that we don't get confused about fake headers vs real headers when figuring out which headers we need to download, but we would also like to remember the information in the fake header (in particular, the msg size, message-id, and offset of the offline copy of the message in the offline store). Here's where we figure out which headers to download: http://mxr.mozilla.org/mozilla/source/mailnews/imap/src/nsImapMailFolder.cpp#2400 We basically list all the keys in the db, and compare that with the list of UID's we've gotten back from the server. One way of doing this would be for the offline sync code that's deleting the fake headers to save the interesting information, and tell the nsImapMailFolder about it, and the imap folder would cache it away. Then, when it's downloading headers, it would compare the new header vs the cached fake header information, and set the offline fields on the new header. I hope that makes some sense - let me know if you're interested in doing this, and if you have any questions.

I'll take a crack at this one. I understand the issue. As I explore further I'll probably come back with some questions.

Dale, do you still work on this one?

Yes, though I have had a number of distractions and haven't made much progress. I should be back on this full time next week.

We try to get "Better Faster IMAP" bugs into 3.0b1, do you think this can make it? If not, I am going to remove the dependency.

thx for working on this, Dale. I don't see this code remembering the offset into the offline store of the saved message, associated with the fake-header, when we download the new header. It just seems to be avoiding downloading the message again. But it seems like we need to hook up the offline offset for the new header for this to be useful. addHeaderForMovedMsg(in voidPtr aHeader); I hate seeing voidPtrs in the idl - perhaps this could just take the message id and message size? Instead of nsVoidArray, you could use nsTArray, I think

Comment on attachment 348853 [details] [diff] [review] First pass at maintaining offline copy of message Sorry to say this so late, but I tried to review this a couple of times over the last weeks, but I really lack the depth of knowledge of our IMAP code to do this properly... :-(

Dale, did you see my previous comment? Are we really hooking up the new header to the old offline offset with your patch?

Comment on attachment 348853 [details] [diff] [review] First pass at maintaining offline copy of message Dale, you need to answer David's questions, therefore cancelling review request for the time being. Two quick comments though: >Index: mailnews/imap/public/nsIMsgImapMailFolder.idl >--- mailnews/imap/public/nsIMsgImapMailFolder.idl Tue Nov 18 18:00:15 2008 >+++ mailnews/imap/public/nsIMsgImapMailFolder.idl Tue Nov 18 16:13:54 2008 >@@ -155,9 +155,11 @@ > /** > * Change the number of "pending" unread messages in a folder, > * unread messages we know about, but don't have the headers for yet > * > * @param aDelta amount to change the unread count by. > */ > void changePendingUnread(in long aDelta); > >+ [noscript] >+ void addHeaderForMovedMsg(in voidPtr aHeader); > }; You should be adding documentation for the new function, and also changing the uuid for the interface.

>I don't see this code remembering the offset into the offline store of the >saved message, associated with the fake-header, when we download the new >header. It just seems to be avoiding downloading the message again. But it >seems like we need to hook up the offline offset for the new header for this to >be useful. Unless I am misunderstanding your comment I think the code is doing the right thing. The offset contained in the nsIMsgDBHdr object that my code uses contains the offsets in the destination mailbox. Please clarify if I am missing what you are saying.

This should address all concerns except David's offset concern which I am not sure about. If I need to do something with the offset I will create a new patch.

Comment on attachment 356362 [details] [diff] [review] Second pass at maintain offline copy. >+ void addHeaderForMovedMsg(in unsigned long aMessageSize, in ACString aMessageId); IMHO this should take an nsIMsgDBHdr so that the choice of header entries to store is internal to the imap folder. >+ for (; (headerIndex < numMovedHeaders) && !matchFound; ++headerIndex) >+ { >+ headerToCompare = (HeaderInfoStruct *)m_movedMsgHeaders.ElementAt(headerIndex); >+ if (headerToCompare && >+ (headerToCompare->messageSize == sizeOfMsgToMatch) && >+ headerToCompare->messageId.Equals(messageIdOfMsgToMatch)) >+ { >+ matchFound = PR_TRUE; >+ } >+ } Define a comparator (or operator==) and use RemoveElement: HeaderInfoStruct infoOfMsgToMatch; pHeader->GetMessageSize(&infoOfMsgToMatch.messageSize); pHeader->GetMessageId(getter_Copies(infoOfMsgToMatch.messageId)); if (!m_movedMsgHeaders.RemoveElement(infoOfMsgToMatch) // We have not downloaded the message body so do it now. keysOfMessagesToDownload->AppendElement(msgKey); >+ // List of header structs for messages moved into this mailbox. >+ nsTArray<HeaderInfoStruct*> m_movedMsgHeaders; Too many pointers. An nsTArray is a pointer to its elements and an nsCString is a pointer to its characters; no need to add another indirection! To append elements you can either use a [default?] copy constructor from a stack-allocated temporary, or call AppendElement() and manipulate the result.

I tried this patch - when I copied a message between imap folders, and then opened the destination folder, the destination folder did not know the offset of the message in the offline store, nor that it had the message offline, so that we still had to fetch the message from the imap server when we wanted to do something with it. I'll try to tweak the patch so that it does what I think it needs to do...

OK. I am almost done reworking the patch to address other concerns, so if you can do the tweak for the offset I can incorporate that into my other changes.

(In reply to comment #13) > (From update of attachment 356362 [details] [diff] [review]) > >+ void addHeaderForMovedMsg(in unsigned long aMessageSize, in ACString aMessageId); > IMHO this should take an nsIMsgDBHdr so that the choice of header entries to > store is internal to the imap folder. Excellent idea. I have done that. > >+ for (; (headerIndex < numMovedHeaders) && !matchFound; ++headerIndex) > >+ { > >+ headerToCompare = (HeaderInfoStruct *)m_movedMsgHeaders.ElementAt(headerIndex); > >+ if (headerToCompare && > >+ (headerToCompare->messageSize == sizeOfMsgToMatch) && > >+ headerToCompare->messageId.Equals(messageIdOfMsgToMatch)) > >+ { > >+ matchFound = PR_TRUE; > >+ } > >+ } > Define a comparator (or operator==) and use RemoveElement: > HeaderInfoStruct infoOfMsgToMatch; > pHeader->GetMessageSize(&infoOfMsgToMatch.messageSize); > pHeader->GetMessageId(getter_Copies(infoOfMsgToMatch.messageId)); > if (!m_movedMsgHeaders.RemoveElement(infoOfMsgToMatch) > // We have not downloaded the message body so do it now. > keysOfMessagesToDownload->AppendElement(msgKey); That is an extremely neat solution (though I don't find it as readable). The problem is that we are comparing a HeaderInfoStruct and an nsIMsgDBHdr, not two HeaderInfoStructs. I have made the code neater by making a Compare() method that compares a HeaderInfoStruct and an nsIMsgDBHdr. > >+ // List of header structs for messages moved into this mailbox. > >+ nsTArray<HeaderInfoStruct*> m_movedMsgHeaders; > Too many pointers. An nsTArray is a pointer to its elements and an nsCString is > a pointer to its characters; no need to add another indirection! To append > elements you can either use a [default?] copy constructor from a > stack-allocated temporary, or call AppendElement() and manipulate the result. While my pointer method may have been funky I don't necessarily understand why we want to keep copying the data fields over again, particularly the message ID string. Why not keep the pointers to the existing structs in a list rather than creating new copies just to add them to a list?

this actually fixes the problem - I used the pending attributes on headers to remember the offline offset and size and flag, and apply them to the newly downloaded header. This prevents us from trying to download the message again in the dest folder because the offline flag is set. This patch needs a bit of cleanup, including getting rid of the duplicate code, and remembering the imap flags correctly - right now, I just crunch the flags from the server with the Offline flag, which isn't right. I think I need to treat the flags from the pending header as flags to OR in with the existing flags - this probably means remembering the original flags before applying the pending attributes. Dale, sorry for leading you into the weeds a bit on this - it turns out the pending attributes does all we need to fix this.

The tweak I was thinking of turns out to be all that's required - when setting up pending attributes for a header moved/copied to an other folder, include the offline flag, offline size, and offline offset. We won't try to redownload those message bodies because the offline flag will be set on the header. The patch is a little more complicated because it turned out we weren't using the flags parameter anymore, so I got rid of that. And I had to treat the pending flags attribute as an OR of the flags, because the flags on the message could have changed on the imap server if the user accessed the message from an other machine/client. This should help with the gmail lockdown in sector 4(?) error, among other things, so I'd like to get it in for b2.

I tested this by copying a message to an other imap folder, selecting the copied message in the target folder, and verifying from the imap protocol log that we didn't try to redownload the message in the target folder.

(In reply to comment #16) > While my pointer method may have been funky I don't necessarily understand why > we want to keep copying the data fields over again, particularly the message ID > string. We don't copy strings, except out of small nsAutoStrings into other strings. nsString1 = nsString2; simply updates an internal pointer to a shared buffer.

Comment on attachment 361195 [details] [diff] [review] proposed fix >- NS_ASSERTION((NS_SUCCEEDED(rv) && (!curDraftIdURL.IsEmpty())), "RemoveCurrentDraftMessage can't get draft id"); >+// NS_ASSERTION((NS_SUCCEEDED(rv) && (!curDraftIdURL.IsEmpty())), "RemoveCurrentDraftMessage can't get draft id"); Obviously not deserving of being an assertion, but not the best fix ;-) >- NS_IMETHODIMP DeleteMessages(nsTArray<nsMsgKey>* nsMsgKeys, >+ NS_IMETHOD SetAttributeOnPendingHdr(nsIMsgDBHdr *pendingHdr, const char *property, >+ const char *propertyVal); >+ NS_IMETHOD SetUint32AttributeOnPendingHdr(nsIMsgDBHdr *pendingHdr, const char *property, >+ PRUint32 propertyVal); >+ NS_IMETHOD DeleteMessages(nsTArray<nsMsgKey>* nsMsgKeys, > nsIDBChangeListener *instigator); I don't think the reindentation of DeleteMessages was intentional. (Ironically you also failed to delete the trailing whitespace!) >+ mdb_token m_pendingHdrsTableKindToken; Another trailing space on a reindented line ;-) >+ pendingRow.swap(*row); Since you didn't initialise *row you should use .forget() instead of .swap()

this addresses Neil's comments, and fixes an unused var I just noticed...carrying forward sr, requesting r.

putting in b3, though I'd love to get it in for b2.

pinging for review, when you get a chance...

Comment on attachment 361335 [details] [diff] [review] fix addressing Neil's comments Looks good, r=me.

this should help with offline store size (move/copy target folders previously would get two copies of messages, I believe), and auto sync should have less work to do.