The basic idea is to use the offline capabilities of Thunderbird while online to make the UI more responsive, when operating on IMAP messages, out of the box, without the user having to tweak any settings. Preemptive/Automatic message download is the second feature implementation in this direction. See wiki.mozilla.org/MailNews:Better_Faster_IMAP2 more info.

Required prefs for this feature are; pref("mail.server.default.autosync_offline_stores",true); pref("mail.server.default.offline_download",true); Does this make Trash folders offline as well in new accounts?

Emre, we'd like to get this in to 3.0b1.

Crashing problem is solved. There is one download manager per imap folder. IdleService is also included for testing. To be continued...

Only problem we have is even pref("mail.server.default.offline_download",true); is set, newly created folders are not "offline" by default. David, 1. Currently we defer downloading messages over 10MB, but we don't have any parameter telling us to skip message if it is over certain size. What is the right strategy here do you think? 2. Currently message download group size is 50K. Should we make this value adjustable in the preferences? 3. I've implemented nsAutoSyncManager and nsAutoSyncState as regular C++ classes to keep the code simple and decomtaminated. Another option would be to convert nsAutoSyncManager into an XPCOM service. Do you think of any benefit in doing that? 4. Another option would be to convert everything into XP components in order to make the strategy class scriptable. Although I am not sure about performance implications, do you think it would be a desirable feature?

o pref("mail.server.default.offline_download",true) doesn't make discovered imap folders offline by default for newly created imap accounts. We need another pref for this purpose. Based on discussion with davida and clarkbw, I am going to do the following improvements on the current patch: 1- New pref to make discovered imap folders offline by default. 2- Convert strategy class to XP component 3- Implement a new strategy object for folder selections 4- Put cached connection into account for accounts with multiple folders

>pref("mail.server.default.offline_download",true) doesn't make discovered >imap folders offline by default for newly created imap accounts. We need >another pref for this purpose. Sounds like we should fix the bug with this pref, instead of inventing a new pref.

Ok, I wasn't sure it is a bug or not. I am going to fix it.

Major classes are converted to XP components with an appropriate XP interface. Strategy functions are scriptable and plug-able. nsAutoSyncManager is converted to a xpcom service. Not tested. What Next: mail.server.default.offline_download bug will be fixed. Strategy functions will be implemented in javascript.

.

Comment on attachment 335881 [details] Revision 2 - Ready for review that's a lot of work, I'm looking forward to trying this! some quick comments: try to keep lines roughly less than 80 chars, e.g., +NS_IMETHODIMP nsDefaultAutoSyncMsgStrategy::Advise(nsIMsgImapMailFolder *folder, nsIMsgDBHdr *msgHdr1, nsIMsgDBHdr *msgHdr2, PRInt32 *decision) No need to init rv to NS_OK here, getService will set it: + nsresult rv = NS_OK; + nsCOMPtr<nsIIdleService> idleService = do_GetService("@mozilla.org/widget/idleservice;1", &rv); No need to check both things here, just idleService is sufficient: + if (idleService && NS_SUCCEEDED(rv)) same thing here: + if (idleService && NS_SUCCEEDED(rv)) please add spaces around the '+' here: + if (mPriorityQ.Length() == index+1) +/** Chains folders belong to the same account O(n*n) */ "belonging"? and maybe some punctuation before O(n*n), e.g., "- O(n*n)" No need for result here: + PRBool result = PR_FALSE; + PRInt32 pqElemCount = mPriorityQ.Length(); + for (PRInt32 pqidx = 0; pqidx < pqElemCount; pqidx++) + { + PRBool sameServer; + nsresult rv = mPriorityQ[pqidx]->GetValue()->HasSameServer(autoSyncStObj, &sameServer); + + if (NS_SUCCEEDED(rv) && sameServer) + { + result = PR_TRUE; + break; + } + }//endfor + + return result; you can return TRUE from inside the loop, and return FALSE after the loop. Again, no need to init rv here: + nsresult rv = NS_OK; + + nsCOMPtr <nsIMsgAccountManager> accountManager = do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv); spaces around = + for ( PRUint32 i=0; i < accountCount; ++i ) should use QueryElementAt: + { + nsCOMPtr<nsIMsgAccount> account; + accounts->GetElementAt ( i, getter_AddRefs(account) ); and here: + nsCOMPtr<nsISupports> supports = getter_AddRefs(allDescendents->ElementAt(i)); + nsCOMPtr<nsIMsgFolder> folder = do_QueryInterface(supports, &rv); + *aFolderStrategy = mFolderStrategyImpl; + NS_IF_ADDREF(*aFolderStrategy); + return NS_OK; should put the assignment inside the NS_IF_ADDREF, and add an NS_ENSURE_ARG_POINTER(aFolderStrategy) at the start. this can use the ? operator instead of if else: + if ( !(msgFlags & MSG_FLAG_IMAP_DELETED) && !(msgFlags & MSG_FLAG_OFFLINE) ) + *result = PR_TRUE; + else + *result = PR_FALSE; need NS_ENSURE_ARG_POINTER(state) here, and with any interface method that takes an out pointer: +NS_IMETHODIMP nsAutoSyncState::GetState(PRInt32 *state) +{ + *state = mSyncState; + return NS_OK; +} these idl files don't seem to be in the diff: + nsIAutoSyncState.idl \ + nsIAutoSyncManager.idl \ + nsIAutoSyncFolderStrategy.idl \ + nsIAutoSyncMsgStrategy.idl \ and, for args to idl methods, we use the naming convention aFoo, both in the idl, and the implmentation functions... no need to init rv, or declare it before its used: + nsresult rv = NS_OK; + + PRUint32 count; + rv = messages->GetLength(&count); + if (NS_FAILED(rv)) return rv; two space indent, instead of four: + *msgCount = mDownloadQ.Length() - mOffset; + return NS_OK; that's enough for now...I'll look at it in more detail once you've attached a patch addressing the simple stuff...

o Suggested changes are made except "do_QueryElementAt" replacement. It causes reference counting problems. I will take a look into this more. o Download mode {Parallel | Chained} became an attribute of AutoSyncManager. o Some documentation will be added to idl files. Not done yet.

nsCOMPtr<nsIMsgAccount> account(do_QueryElementAt(accounts, i)); nsCOMPtr<nsIMsgFolder> folder(do_QueryElementAt(allDescendents, i)); caused reference counting problems? I suspect the reference counting problems are elsewhere. It's a very common pattern...

I agreed. But what I observe right now is when I make the change I start seeing assertions. Since this problem is very isolated (one function only), I didn't want to make the review wait until I understand the cause, and fix it. I am testing this as we speak.

Added the UI preference elements to the wiki page https://wiki.mozilla.org/MailNews:Better_Faster_IMAP_Plan#UX_Decisions_to_make

o GetElementAt() is replaced with do_QueryElementAt() o Folder strategy suggested by clarkbw has been implemented o Minor changes + There are couple optimizations can be done. I leave it for a separate bug/patch.

Comment on attachment 336374 [details] [diff] [review] Revision 4: Tested more + Couple Improvements + PRBool doesMsgFitDownloadCriteria(in nsIMsgDBHdr aMsgHdr); should be boolean what does "de" stand for in + const long deSame = 0; Is it an acronym like dm or an abbreviation? And can you document it in the code or use something more obviously meaningful? Or just omit it completely? If it stands for decision, I don't think it's really adding to the readability. these should all be on*, not On* + void OnNewMessageHeaders(in nsIAutoSyncState aAutoSyncStateObj); + + /** */ + void OnDownloadStarted(in nsIAutoSyncState aAutoSyncStateObj, in nsresult aStartCode); + are nsIAutoSyncFolderStrategy and nsIAutoSyncMsgStrategy ever going to be different? Perhaps they could be shared/made generic. + /** */ + PRBool hasSameServer(in nsIAutoSyncState aAnotherStateObj); boolean again. why not use attributes here? instead of + long getState(); + + /** */ + unsigned long getPendingMessageCount(); + + /** */ + nsIMsgImapMailFolder getOwnerFolder(); + + /** */ + void setState(in long state); attribute long state; readonly attribute long pendingMessageCount; readonly attribute nsIMsgImapMailFolder ownerFolder; and + /** */ + void getNextGroupOfMessages(out nsIMutableArray aMessageList); could simply be readonly attribute nsIMutableArray nextGroupOfMessages; and + /** Get auto-sync state object */ + nsIAutoSyncState getAutoSyncStateObj(); can be readonly attribute nsIAutoSyncState autoSyncStateObj; + *aResult = (msgFlags & MSG_FLAG_IMAP_DELETED) || (msgFlags & MSG_FLAG_OFFLINE) ? PR_FALSE : PR_TRUE; my bad, I should have noticed that you don't even need the ? operator if you just switch the sense of the boolean expression, e.g., *aResult = (! (msgFlags & MSG_FLAG_IMAP_DELETED|MSG_FLAG_OFFLINE)); I should have pointed this out before: If getService fails, you will crash downstream. And GetService can fail, e.g., if we're shutting down. +nsAutoSyncState::nsAutoSyncState() + : mSyncState(stCompletedIdle), mOwnerFolder(nsnull), mOffset(0), mLastOffset(0), + mLastSyncTime(LL_ZERO) +{ + mAutoSyncMgr = do_GetService(NS_AUTOSYNCMANAGER_CONTRACTID); + NS_ASSERTION(mAutoSyncMgr != nsnull, "*** Cannot get nsAutoSyncManager service."); +} This doesn't look complete; perhaps just assert if it fails? + nsCOMPtr<nsIAutoSyncManager> autoSyncMgr = do_GetService(NS_AUTOSYNCMANAGER_CONTRACTID); + if (NS_SUCCEEDED(rv) && autoSyncMgr) + { + // auto-sync manager initialization goes here + // do nothing, we use default strategy, and default values + } If you're going to have every imap folder create one of these in the constructor, why not make it a member var, and not a ref pointer? + + // initialize auto-sync state object. Delete is not needed. + m_autoSyncStateObj = new nsAutoSyncState(this); + // auto-sync (preemtive download) support + nsRefPtr<nsAutoSyncState> m_autoSyncStateObj; + Or, if there's some way you can figure out to avoid having every imap folder holding onto this extra data even when it's not being downloaded, that would be even better.

instead of doing this, and storing a reference to the service, is it better to get the reference just before we use it? What is the overhead of getting a service everytime - significant? If getService fails, you will crash downstream. And GetService can fail, e.g., if we're shutting down. +nsAutoSyncState::nsAutoSyncState() + : mSyncState(stCompletedIdle), mOwnerFolder(nsnull), mOffset(0), mLastOffset(0), + mLastSyncTime(LL_ZERO) +{ + mAutoSyncMgr = do_GetService(NS_AUTOSYNCMANAGER_CONTRACTID); + NS_ASSERTION(mAutoSyncMgr != nsnull, "*** Cannot get nsAutoSyncManager service."); +}

getservice is basically a hash table lookup, so it's fast enough, as long as you're not doing it in a tight loop or something like that.

>what does "de" stand for in + const long deSame = 0; Is it an acronym like dm >or an abbreviation? And can you document it in the code or use something more It's a prefix to remind the contributor that it is an enum, and not a type or anything like that defined in strategy class. > are nsIAutoSyncFolderStrategy and nsIAutoSyncMsgStrategy ever going to be > different? Perhaps they could be shared/made generic. The idea is to make them scriptable and replaceable by Add-Ons. > void getNextGroupOfMessages(out nsIMutableArray aMessageList); This one is not a good candidate for being an attribute. 'Get' does change the internal state of the object. >If you're going to have every imap folder create one of these in the >constructor, why not make it a member var, and not a ref pointer? Because I have to initialize it with a pointer to nsImapMailFolder at ctor. >Or, if there's some way you can figure out to avoid having every imap folder >holding onto this extra data even when it's not being downloaded, that would >be even better. Imap folder's last sync time is in the state object. So, every imap folder has to have one associated auto-sync object. I can move last sync time value to the folder (it was like that before) but I think it is more logical to keep it as part of auto-sync state.

(In reply to comment #19) > Created an attachment (id=336575) [details] > Revision 5 - Suggested changes are applied > > >what does "de" stand for in + const long deSame = 0; Is it an acronym like dm > >or an abbreviation? And can you document it in the code or use something more > It's a prefix to remind the contributor that it is an enum, and not a type or > anything like that defined in strategy class. Maybe it's just me, but it just confuses me...I keep thinking de stands for Germany :-) > > > are nsIAutoSyncFolderStrategy and nsIAutoSyncMsgStrategy ever going to be > > different? Perhaps they could be shared/made generic. > The idea is to make them scriptable and replaceable by Add-Ons. But the add-ons couldn't change the definitions of the interfaces, could they, because the core code uses the interfaces? So they would remain the same as each other. So unless I'm misunderstanding, my question remains... > > > void getNextGroupOfMessages(out nsIMutableArray aMessageList); > This one is not a good candidate for being an attribute. 'Get' does change the > internal state of the object. We have other attributes that do that, often caching the result so the next call is quick. > > >If you're going to have every imap folder create one of these in the > >constructor, why not make it a member var, and not a ref pointer? > Because I have to initialize it with a pointer to nsImapMailFolder at ctor. Then, you could do that in the constructor for the nsImapMailFolder, couldn't you? E.g., m_syncStrategy.m_folder = this, something like that...the sync strategy can have a pointer to nsImapMailFolder, since the classes are intimately tied with each other. > > >Or, if there's some way you can figure out to avoid having every imap folder > >holding onto this extra data even when it's not being downloaded, that would > >be even better. > Imap folder's last sync time is in the state object. So, every imap folder has > to have one associated auto-sync object. I can move last sync time value to the > folder (it was like that before) but I think it is more logical to keep it as > part of auto-sync state. OK, that makes sense.

>Maybe it's just me, but it just confuses me...I keep thinking de stands for >Germany :-) :) What about 'sd' prefix? >But the add-ons couldn't change the definitions of the interfaces, could they, >because the core code uses the interfaces? So they would remain the same as >each other. So unless I'm misunderstanding, my question remains... Right, but they can change the implementation of the strategy interface used by nsAutoSyncManager. If you are simply say that both interfaces are same, the signature of their Advise() methods are different. >Then, you could do that in the constructor for the nsImapMailFolder, couldn't >you? E.g., m_syncStrategy.m_folder = this, something like that...the sync >strategy can have a pointer to nsImapMailFolder, since the classes are >intimately tied with each other. True, this is another way to implement the same logic. I like passing ownerFolder to Ctor better over setting as an attribute because; 1. It forces the developer to pass an owner folder pointer when creating an instance of nsAutoSyncState class. 2. It eliminates the need to expose a public method/attribute (if not friend) on the interface to set the owner folder, therefore eliminates the possibility that the developer changes it on the run. Having said that, if you think there is more advantage in creating nsAutoSyncState as a member variable instead of nsRefPtr<>, I am ok with that too.

>Having said that, if you think there is more advantage in creating >nsAutoSyncState as a member variable instead of nsRefPtr<>, I am ok with that >too. I think it's worthwhile. It helps you avoid doubling the number of memory allocations per folder, and avoid the situation where you can't allocate the nsAutoSyncState object. You could use an Init method if you want... > :) What about 'sd' prefix? No idea what that would stand for. Since the main goal should be to make the code readable, I would suggest either doing something like decisionLower, or kLower, or just Lower. I think this is perfectly readable: + if (decision == nsIAutoSyncMsgStrategy::exclude) + break; // skip this message The compiler/interpreter would prevent you from trying to set it, since it's const. But using kExclude would be even more clear... >Right, but they can change the implementation of the strategy interface used by >nsAutoSyncManager. If you are simply say that both interfaces are same, the >signature of their Advise() methods are different. Ah, I see, you're right. Of course, you could always inherit from a common base interface that defines those constants and share them that way. One other note: the usage of the advise method is a bit odd, since sometimes you pass in the same folder twice, to figure out if the folder should just be excluded...it feels to me like that should just be a separate boolean method.

>> void getNextGroupOfMessages(out nsIMutableArray aMessageList); > This one is not a good candidate for being an attribute. 'Get' does change the > internal state of the object. You're right; that should stay as a method.

>I think it's worthwhile. It helps you avoid doubling the number of memory >allocations per folder, and avoid the situation where you can't allocate the >nsAutoSyncState object. You could use an Init method if you want... I believe the compiler emits exact same code to allocate and initialize member variables, in Ctor, implicitly. So it shouldn't be any difference in term of doubling the number of memory allocation. The difference is the memory footprint of nsImapMailFolder. In case of member variable it will be bigger. I guess It all boils down to the question how/where we expose this attribute/method on nsAutoSyncState. Should I make ownerFolder attribute not-readonly on nsIAutoSyncState, or should I expose a public method called SetOwnerFolder() on nsAutoSyncState class? or do it private and make nsImapMailFolder friend to nsAutoSyncState? Thoughts? >Ah, I see, you're right. Of course, you could always inherit from a common >base interface that defines those constants and share them that way. ok, single base interface for constants inherited by both strategy interfaces.. >One other note: the usage of the advise method is a bit odd, since sometimes >you pass in the same folder twice, to figure out if the folder should just be >excluded...it feels to me like that should just be a separate boolean method. I see your point, but in this case we should make 2 calls per decision (assuming that we prevent Advise() returning excluded). What I can do is to document this special case properly. There is no harm passing same folder/message for both arguments since the return value will always be SAME unless it is EXCLUDED.

timeless, >keep in mind that PRTime isn't safe in JS. What should I use instead? > // this is a simple interface which allows the imap folder to update some > values > // that the folder props js code will use to update the sharing and quota > tabs in the folder props. >again (last complaint), please use this notation: >/** > * > */ this is not part of my patch, nsIMsgImapMailFolder.idl has it. Do you want me to file a bug for this kind commenting style inconsistencies in idls? >#ifdef DEBUG_ebirol >+#define DEBUG_ebirol_AutoSyncManager_L0 >+#define DEBUG_ebirol_AutoSyncManager_L1 >+//#define DEBUG_ebirol_AutoSyncManager_L2 These debug symbols won't end-up in the final patch. I keep them for reviewer's convenience. >+ autoSyncStateObj->GetState(&state); > can this fail? No >+ if (type.Equals("imap")) > EqualsLiteral? or are you using Glue? I changed it based on your recommendation but don't understand the difference, not sure what I am using by calling type.Equals(). Can you explain? >+ nsCOMPtr<nsIAutoSyncManager> autoSyncMgr = > do_GetService(NS_AUTOSYNCMANAGER_CONTRACTID, &rv); >+ NS_ASSERTION(autoSyncMgr != nsnull, "*** Cannot get nsAutoSyncManager > service."); >+ NS_ENSURE_SUCCESS(rv, rv); > you're checking twice... it seems odd.... i don't think the ns_assertion is > technically valid, it should probably be removed I just want to print something if a reference to nsAutoSyncManager cannot be acquired. Thoughts? >+#define _nsAutoSyncState_H_ > the header guard doesn't look right (too few _'s ?) I saw different styles in nsImapIncomingServer.h, nsImapMailFolder.h. Which one is correct? You are right about comments. Like I said at comment #11, I am going to document the code - once we finalize the interfaces. Did you have a chance to apply the patch and play with it? At this stage, a review covering architectural and functional aspects of the patch would be a time saver for me. Thanks for the review.

Major changes: - we check the existing headers asynchronously in groups during idle now. - message sorting algorithm is converted to quick-sort through a nsTArray::Comparator adaptor for strategies.

Comment on attachment 337364 [details] Revision 6 - Suggestions applied + added comments + if (aStartCode != NS_OK) you should use !NS_SUCCEEDED(aStartCode), or NS_FAILED(aStartCode), not direct comparison with NS_OK. Now that I think about it, maybe it would be nicer to pass in nsIMsgFolder, not nsIMsgImapMailFolder here: + nsAutoSyncStrategyDecisionType sort(in nsIMsgImapMailFolder aFolder1, in nsIMsgImapMailFolder aFolder2); + + /** + * Tests whether the given folder should be excluded or not. + */ + boolean isExcluded(in nsIMsgImapMailFolder aFolder); e.g., if news wanted to take advantage of your approach. And you're QI'ing to nsIMsgFolder anyway in the impl. From what I can tell, it would simplify the code quite a bit if you didn't use nsIMsgImapMailFolder. moreToProcess sounds like a boolean. Maybe "leftToProcess"? + PRUint32 moreToProcess; + nsresult rv = autoSyncStateObj->ProcessExistingHeaders(kNumberOfHeadersToProcess, &moreToProcess); + + #if defined(DEBUG_ebirol) && defined(DEBUG_ebirol_AutoSyncManager_L1) + printf("Existing headers are processed for folder %s. There are %d more headers to be processed\n", + autoSyncMgr->DebugGetFolderName(autoSyncStateObj).get(), moreToProcess); + #endif + + if (NS_SUCCEEDED(rv) && 0 == moreToProcess) Why the comptr here? +nsresult nsAutoSyncManager::DownloadMessagesForOffline(nsIAutoSyncState *aAutoSyncStateObj) +{ + nsresult rv = NS_OK; + nsCOMPtr<nsIAutoSyncState> autoSyncStateObj(aAutoSyncStateObj); can't you just use aAutoSyncStateObj directly? You should return an error if new fails, typically NS_ERROR_MEMORY_FAILURE: + + // lazily create if it is not done already + if (!mMsgStrategyImpl) + mMsgStrategyImpl = new nsDefaultAutoSyncMsgStrategy; + + NS_IF_ADDREF(*aMsgStrategy = mMsgStrategyImpl); + return NS_OK; +} What happens if the error causing this isn't cleaned up? + if (aExitCode != NS_OK) + autoSyncStateObj->TryCurrentGroupAgain(); will we just try again infinitely? +NS_IMETHODIMP nsAutoSyncState::OnStopRunningUrl(nsIURI* aUrl, nsresult aExitCode) +{ + nsresult rv = mOwnerFolder->ReleaseSemaphore(static_cast<nsIMsgImapMailFolder*>(mOwnerFolder)); + NS_ASSERTION(NS_SUCCEEDED(rv), "*** Cannot release folder semaphore"); + + nsCOMPtr<nsIMsgMailNewsUrl> mailUrl = do_QueryInterface(aUrl); + if (mailUrl) + rv = mailUrl->UnRegisterListener(this); + + nsCOMPtr<nsIAutoSyncManager> autoSyncMgr = do_GetService(NS_AUTOSYNCMANAGER_CONTRACTID, &rv); + NS_ASSERTION(autoSyncMgr, "*** Cannot get nsAutoSyncManager service"); + NS_ENSURE_SUCCESS(rv, rv); + + NS_ADDREF_THIS(); why addref ourselves in OnStopRunningUrl? Have you checked that these objects get destroyed? returns like this should be on their own line: + rv = mOwnerFolder->BuildIdsAndKeyArray(aMessagesList, messageIds, msgKeys); + if (NS_FAILED(rv) || messageIds.IsEmpty()) return rv; + if (NS_FAILED(rv)) + return NS_ERROR_FAILURE; why drop the real error instead of returning rv?

+ LL_MUL(freqInUSec, kAutoSyncFreqInMin, PR_USEC_PER_SEC * 60UL /*1 minute*/); + + // calculate the next sync time + LL_ADD(timeForSync, lastSyncTime, freqInUSec); + + if (LL_CMP(timeForSync, <, PR_Now())) We don't need to use these macros anymore - you can just treat them like any other vars/values.

>+ nsCOMPtr<nsIAutoSyncState> autoSyncStateObj(aAutoSyncStateObj); > can't you just use aAutoSyncStateObj directly? I can. I just want to make sure that AutoSyncState object is alive until I am done calling its download method. >What happens if the error causing this isn't cleaned up? >+ if (aExitCode != NS_OK) >+ autoSyncStateObj->TryCurrentGroupAgain(); >will we just try again infinitely? It will try to download the same group of messages (actually, minus the messages already downloaded at the first try) during the next idle time. This approach mainly addresses network and folder semaphore acquisition problems (compacting etc..). The down-side is for some reason if this group fails every single time, TB won't be able to move on to other messages, or to other folders on the same server in 'chained' mode which is the default mode. Perhaps we can use a try-counter? or try to figure out the problem message and remove it from the group - can we get this from nsURI? Thoughts? >+NS_IMETHODIMP nsAutoSyncState::OnStopRunningUrl(nsIURI* aUrl, nsresult >aExitCode) >... >+ >+ NS_ADDREF_THIS(); >why addref ourselves in OnStopRunningUrl? Have you checked that these objects >get destroyed? The idea is whenever nsAutoSyncState passes a self-ref to nsAutoSyncManager it addrefs the pointer. nsAutoSyncManager uses this pointer in a nsCOMPtr without addref'd and only addrefs if it has to store it in the download and/or process queues. Original idea was to prevent a crash in case that the user removes an account while the folder in one of the queues. Rest of the suggestions have been applied. I don't request r+sr again. Let me know when you think the patch has come to the final stage.

>The idea is whenever nsAutoSyncState passes a self-ref to nsAutoSyncManager it >addrefs the pointer. I guess my question is, who releases the references? >Perhaps we can use a try-counter? or try to figure out the problem message and >remove it from the group - can we get this from nsURI? Thoughts? I'd suggest a try counter; Or keep track of the message you're downloading to figure out what the problem message is. If you're downloading more than one at a time, you can't find out from imap which one might have failed. Is there a reason you didn't use nsCOMArrays here? + nsTArray<nsIAutoSyncState*> mPriorityQ; + nsTArray<nsIAutoSyncState*> mDiscoveryQ; It might simplify some of the ref counting, since comarrays hold a reference for you, and release the reference when you remove the object from the array. I'm sorry to be a bore about this, but the more explicit addrefs and releases you have, the more likely you are to have ref counting bugs...and the more difficult the code is to understand and review :-(

gah, now i have to read and follow the bug :). first, i don't generally build thunderbird, it's not that i'm opposed or disinterested, it's that i don't have time/resources and can't justify it from work. >> keep in mind that PRTime isn't safe in JS. > What should I use instead? http://mxr.mozilla.org/mozilla-central/ident?i=nsISupportsPrimitives::nsISupportsPRTime the advantage is that the ToString operation there will *safely* convert to a string which js can parse, yes i know it'll suck to have to create that object, but hopefully it isn't done often. if it is, you can cache the constructor for that object (ask me later, please don't optimize early, you asked me about arch issues, my recommendation here is the proper arch answer). Alternatively if you don't mind being incestuous w/ spidermonkey it's possible to offer a method that will actually result in a real js date object, however that's not xpcom friendly, so you either need attribute nsISupportsPRTime or attribute PRTime + the js thing. > Do you want me to file a bug for this kind commenting style > inconsistencies in idls? please but for things where you are adding, please get them right here :) > These debug symbols won't end-up in the final patch. > I keep them for reviewer's convenience. if i'm building (which i suppose i might do), it'd be nice if i didn't have to #define DEBUG_you ... it's a style thing but it really does help to get into good habits. >>+ if (type.Equals("imap")) >> EqualsLiteral? or are you using Glue? > Can you explain? the alternative is this: type.EqualsLiteral("imap") which i think is more correct for a literal string. > I just want to print something if a reference to nsAutoSyncManager cannot be > acquired. Thoughts? don't bother, anyone who cares can figure it out from the debug message, note that NS_ASSERTIONs are supposed to be fatal (eventually, if not sooner). > Which one is correct? gah... from memory, leading underscores are reserved for the compiler/platform, so you want: 38 #ifndef nsInt64_h__ http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsISupportsBase.h?mark=36-37#34 (another example in the same style) note that the 'h' is lowercase. don't use !NS_SUCCEEDED, use NS_FAILED (and don't use !NS_FAILED, use NS_SUCCEEDED), the macros are actually smarter than you might think and try to hint to the compiler about which way things should go. NS_ERROR_MEMORY_FAILURE => NS_ERROR_OUT_OF_MEMORY

3.0b1 flag is going away in favour of 3.0 flag and milestone combination.

>I guess my question is, who releases the references? nsAutoSyncManager releases when it removes from the priority queue. But you are right, nsCOMArray fits better here. >I'd suggest a try counter Try counter it is. The comment at HandleDownloadErrorFor() method explains the error-handling policy for download errors. Other than those two, I made couple changes inline with timeless' review.

So I don't think you need (or should) be NS_ADDREFing in any of these places, because, as I understand it, the comarray is holding a reference to the autosync state object, and you're not removing it from the comarray until the download is done. Adding it to the comarray addrefs it, and removing it releases it, and these addrefs would all be extra. Sorry if I'm missing something... + if (mIsDownloadQChanged) + { + #if defined(DEBUG_ebirol) && defined(DEBUG_AutoSyncState_L1) + DebugPrintOwnerFolderName("Download Q is created for "); + #ifdef DEBUG_AutoSyncState_L2 + DebugPrintQWithSize(mDownloadQ, 0); + #endif + #endif + NS_ADDREF_THIS(); + nsresult rv = NS_OK; + + // if there is a problem to start the download, set rv with the + // corresponding error code. In that case, AutoSyncManager is going to + // set the autosync state to nsAutoSyncState::stReadyToDownload + // to resume downloading another time + + // TODO: is there a way to make sure that download started without + // problem through nsIURI interface? + + nsCOMPtr<nsIAutoSyncManager> autoSyncMgr = do_GetService(NS_AUTOSYNCMANAGER_CONTRACTID, &rv); + NS_ENSURE_SUCCESS(rv, rv); + + NS_ADDREF_THIS(); + return autoSyncMgr->OnDownloadStarted(this, rv); + nsresult rv = mOwnerFolder->ReleaseSemaphore(static_cast<nsIMsgImapMailFolder*>(mOwnerFolder)); + NS_ASSERTION(NS_SUCCEEDED(rv), "*** Cannot release folder semaphore"); + + nsCOMPtr<nsIMsgMailNewsUrl> mailUrl = do_QueryInterface(aUrl); + if (mailUrl) + rv = mailUrl->UnRegisterListener(this); + + nsCOMPtr<nsIAutoSyncManager> autoSyncMgr = do_GetService(NS_AUTOSYNCMANAGER_CONTRACTID, &rv); + NS_ENSURE_SUCCESS(rv, rv); + + NS_ADDREF_THIS(); + rv = autoSyncMgr->OnDownloadCompleted(this, aExitCode);

>So I don't think you need (or should) be NS_ADDREFing in any of these places, >because, as I understand it, the comarray is holding a reference to the >autosync state object, and you're not removing it from the comarray until the >download is done. Adding it to the comarray addrefs it, and removing it >releases it, and these addrefs would all be extra. I've kept these addrefs to guarantee that nsAutoSyncState stays alive until I added into the nsCOMArray safely. It doesn't cost us much, I think. I can remove it if you think it is too much defense especially in a single threaded app like ours. I noticed that I forgot to change 3 DEBUG_ebirol stuff in nsAutoSyncState.cpp. Feel free to change them to "DEBUG_me" and set your debug symbol at the beginning of the file to test/debug. I am going to change them properly in next patch.

If you addref without matching releases, you can cause memory leaks. As you say, in a single threaded system, I think we're fine without those addrefs. The folder is holding a reference to the state object anyway so it will keep it alive for the time that it's not in the comarray...

Yes but memory leak is not the case here, no. I guess I try to find out that the combination of nsCOMPtr<>(dont_AddRef(...)) in nsAUtoSyncManager, and ADD_REF in nsAutoSyncState is correct/acceptable or not. This would help me in my future code. Please let me know if you want me to submit another patch for this change. By default, can I assume that I don't have to submit new patch every time?

(In reply to comment #38) > Yes but memory leak is not the case here, no. I guess I try to find out that > the combination of nsCOMPtr<>(dont_AddRef(...)) in nsAUtoSyncManager, and > ADD_REF in nsAutoSyncState is correct/acceptable or not. This would help me in > my future code. It seems like a needless complication of the ref-counting model, given that we have a clear ownership model (the folder owns it) - the more complicated your ref-counting is, the more likely it is that someone could break it. > > Please let me know if you want me to submit another patch for this change. By > default, can I assume that I don't have to submit new patch every time? no, I can make those changes myself, when I try the patch.

(In reply to comment #32) > > >> keep in mind that PRTime isn't safe in JS. timeless, by this, I'm assuming you mean that integers in JS will overflow after 53 bits rather than after 64 bits. Correct? If that's what you're referring to, I disagree that it's worth architecting around, given how long it's gonna take for PRTime to hit that.

I took the patch, got rid of the non-ref adding comptrs and ADDREF_THIS, and ran with it. The download stuff hammers the connection cache pretty hard, so it exposed an existing issue there pretty quickly, which I have a local fix for. I've also seen some corrupt offline stores, so I need to figure out what's going on there...

Last night I did some tests on Windows XP VM. I don't know if this is the issue you mention but I noticed that if I keep TB running long enough with auto-sync patch, I get a boundary check assertion in m_connectionCache array's nsVoidArray::FastElementAt() method - which leads to a crash eventually. I attach the stack above (aIndex is 4 so is Count()). I was getting the same assertion on Mac as well but only if I debug too long - long enough for a connection timeouts/drops. My understanding is this problem is orthogonal to auto-sync patch, and surfaces when auto-sync calls UpdateFolder() frequently on a flaky connection. This problem also could be the reason of recent *crashiness* of nightlies. I can reproduce it almost consistently, let me know if you need more debugging on my side. Hope this helps.

Yes, that's the stack, and I'll attach a patch soon. This bug has been there forever so it's unlikely to be a cause of recent crashiness. I suspect the recent crashiness was the new folder lookup stuff, which should be fixed now.

I've filed Bug 454932 for the assertion and potential crash (I haven't seen it crash, but maybe I've just been lucky)

It's a little odd that the preemptive download stuff is encountering so many dead connections - I'm a little suspicious that something is killing connections, so I'll keep an eye on it.

Running the patch, I noticed that we are leaving the db's open for every folder we try to sync - this is an occupational hazard of code that iterates over every folder, but it can lead to enormous memory bloat. When looking at the place to fix this (nsAutoSyncManager::OnDownloadCompleted, before we remove the state from the queue), I noticed that you have wrappers/reimplementations around the nsCOMArray functions to get the index of, add to, or remove an entry from the queues. Now that you're using nsCOMArrays, which do all this stuff for you, we can simplify the code as follows: calls to nsAutoSyncManager::DoesQContain(queue, object) can be replaced with simply queue->IndexOf(object) != -1). calls to nsAutoSyncManager::RemoveFromPriorityQ(nsIAutoSyncState *aAutoSyncStateObj) can be replaced with mPriorityQ->RemoveObject(aAutoSyncStateObj); void nsAutoSyncManager::PlaceIntoPriorityQ(nsIAutoSyncState *aAutoSyncStateObj, PRInt32 aIndex) can just be mPriorityQ->InsertObjectAt(aIndex); Here's code that shows you how tell a folder to let go of its cached msg db pointer: nsCOMPtr<nsIMsgMailSession> session = do_GetService(NS_MSGMAILSESSION_CONTRACTID, &rv); if (NS_SUCCEEDED(rv) && session) { PRBool folderOpen; PRUint32 folderFlags; folder->GetFlags(&folderFlags); session->IsFolderOpenInWindow(folder, &folderOpen); if (!folderOpen && ! (folderFlags & nsMsgFolderFlags::Inbox)) folder->SetMsgDatabase(nsnull); } I can make these changes before I checkin, if you like...

>Now that you're using nsCOMArrays, which do all this stuff for >you, we can simplify the code as follows: Makes sense, there are there from the days I used nsTArray<>. Also, I noticed 2 logic errors in the code. - The first one is in nsAutoSyncManager::OnDownloadQChanged where we start synching the folder immediately. We should first check whether the folder is excluded or not here. - The second one is in nsAutoSyncManager::ChainFoldersInQ. When we create a flatten version of the priority queue, before we add the highest priority folder immediately, we must ensure that there is no on going download of a folder owned by the same imap server. Otherwise it starts a new download before the first is completed - which defeats the purpose of chaining. This usually happens when idle kicks-in when tb downloading a large message priority folder. I am going to fix these issues and submit a new patch. Thanks.

Fixed the problems. I put the db cache cleanup inside nsAutoSyncState; every time the folder changes state to stCompletedIdle, we cleanup the cache. Does make sense? When I test the new patch I got a new assertion/or an assertion that I haven't noticed before: ###!!! ASSERTION: ### mem cache leaking entries? : 'mEntryCount == 0', file /Users/ebirol/Projects/mozilla-hg/mozilla/mozilla/netwerk/cache/src/nsMemoryCacheDevice.cpp, line 130 Looks like it is related to networking stuff, but I wanted to point it out. == Another issue is while auto-sync manager downloads a large message, if the user deletes all the messages of the same folder, TB doesn't do anything and waits until the download is completed with the following assertion: ###!!! ASSERTION: Some other operation is in progress: 'PR_FALSE', file /Users/ebirol/Projects/mozilla-hg/mozilla/mailnews/db/msgdb/src/nsMailDatabase.cpp, line 177 I think it perfectly makes sense, and it doesn't cause any crash or anything. But my question is should/can we stop the download (using PseudoInterrupt or something else) before doing any pseudo-offline operation on a folder in download-in-progress state. Thoughts?

More polished and Gmail aware: It excludes the [Gmail]/... folders automatically.

thx for fixing those things. When going through the code, I also noted that you haven't addressed this comment: > From what I can tell, it would simplify the > code quite a bit if you didn't use nsIMsgImapMailFolder. I did that in my tree, and determined there's no reason for you ever to use nsIMsgImapMailFolder, since everywhere you used it, you really want an nsIMsgFolder. >I put the db cache cleanup inside nsAutoSyncState; every >time the folder changes state to stCompletedIdle, we cleanup the cache. Does >make sense? That seems good. I think something is still causing us to open every db for every imap folder that goes into the priority queue - I need to track that down. I'll look for the memory cache assertion. I have not seen that. Re the question of what to do if the user deletes all the messages while a download is in progress, I'll have to think about that. But that brings up a question that was bothering me this morning - since we put all the folders into the priority queue the first chance we get, how do we handle the case that a folder gets deleted at some later point? And since we're not holding onto a reference to the folder, the folder object really could get deleted out from under us. We could use a weak reference to the nsIMsgFolder in the autocompact state; that would allow us to tell if the underlying folder gets deleted out from under us...

OK, I see why all the db's are getting opened - nsresult nsAutoSyncManager::AutoUpdateFolders() calls imapFolder->InitiateAutoSync(), which calls UpdateFolder. If I'm reading the code correctly you call update folder on every imap folder in the profile, in a tight loop. That's really not a great thing to do all at once. Either those should be chained, or you should leave out the UpdateFolder call, and update the folder when you're actually doing the sync. Yes, we're idle when you do it, but you've kicked off a potentially large amount of work, and the user returning from idle won't stop any of it...

Oh, I was wrong about never having to use nsIMsgImapMailFolder - AutoUpdateFolders does need nsIMsgImapMailFolder so it can call GetAutoSyncStateObj. But no one else needs it...

>But that brings up a question that was bothering me this morning - since we put >all the folders into the priority queue the first chance we get, how do we >handle the case that a folder gets deleted at some later point? And since >we're not holding onto a reference to the folder, the folder object really >could get deleted out from under us. At early stage of this patch, I tested the case where the user deletes the whole account while its folders are in the download queue. For some reason folders stay in the memory (not because of us I think, since we have a raw pointer to it), and when AutoSyncMan calls nsAutoSyncState::xxDownload method on them, nsAutosyncState returns a "can't open database" error immediately. As a result, HandleDownloadErrorFor() switches to the next folder and so on. The deleted folders stay in the memory until all messages are tried. It causes some inefficiency but not a crash. If that didn't work, I was planning to store the URL of the folder in the nAutoSyncState object along with the pointer to test the existence of the folder in the manager before using the pointer. Since manager always has a ref to autosyncstate object always, I thought that would do it. >That's really not a great thing to do all at once. Either those should be >chained, or you should leave out the UpdateFolder call, and update the folder >when you're actually doing the sync. Calling update during idle time triggers the header-fetch and header-fetch triggers the auto-sync process, kinda chicken-egg problem. I personally think that the idea of TB doing the folder discovery and message downloads automatically, during idle time, is very convenient, but this is just me. I suggest to keep the 'update' but chain it if you think that would solve our problem. Thoughts?

Yeah, we shouldn't count on the folder getting held in memory - that would be counting on a memory leak, basically. Weak references are the usual way we handle this kind of thing. See do_queryReferent for examples of how this is done. > I personally think >that the idea of TB doing the folder discovery and message downloads > automatically, during idle time, is very convenient, but this is just me. Yes, that's the whole idea, but you shouldn't update every folder simultaneously. I believe that's probably what's causing the connection cache issues we were both seeing. Chaining it, by which I mean, doing an update, when that finishes, do the next update, if we're still idle, etc, is the way to go. You don't want to do an unbounded amount of work the instant we go idle.

see bug 423354 for some possible fallout from hammering an imap server as hard and fast as possible.

- Chained folder updates - nsWeakPtr to the owner folder

Comment on attachment 338481 [details] [diff] [review] Revision 8.3 I don't think these are PRTime's - they can be PRUint32's + static const PRTime kAutoSyncFreqInMin = 60UL; // 1hr + static const PRTime kUpdateFreqInMin = 5UL; // 5min Since you're only persisting the 32 bit time, why not only keep track of the same? Then you won't have a js issue with PRTime. + /** + * Last time the existing headers are completely processed. + */ + readonly attribute PRTime lastSyncTime; Similarly for last update time. I don't think we need a sub-second resolution here. You're still using nsIMsgImapMailFolder where nsIMsgFolder would be more appropriate, e.g.: + nsAutoSyncStrategyDecisionType sort(in nsIMsgImapMailFolder aFolder1, in nsIMsgImapMailFolder aFolder2); is there more code to come here, or should it just be return NS_OK; ? +// to chain update folder actions +NS_IMETHODIMP nsAutoSyncManager::OnStartRunningUrl(nsIURI* aUrl) +{ + nsresult rv = NS_OK; + return rv; +} So now we have 3 queues, update, priority, and discovery - it would be useful to have a comment explaining how these queues interact. It would also be nice to explain the whole sibling thinking - it's unclear to me why we care - If I have three imap servers, I'd like the 3 inboxes synced before any other folders on the servers are synced. Does that happen? Or do we process all folders on a server before moving on to the next server? Anyway, I'll try this new patch. Thx for putting the chaining in - it should help a lot with the usability when coming back from the first idle.

Comment on attachment 338481 [details] [diff] [review] Revision 8.3 this doesn't quite compile because NS_IMETHODIMP nsAutoSyncState::SetLastUpdateTime(PRTime aLastUpdateTime) needs to return a result.

>Since you're only persisting the 32 bit time, why not only keep track of the >same? Then you won't have a js issue with PRTime. >+ readonly attribute PRTime lastSyncTime; Actually, I think we shouldn't even expose this attribute to javascript. It will be used by autosync manager only. Would [noscript] tag hide this attribute from javscript for me? >+NS_IMETHODIMP nsAutoSyncManager::OnStartRunningUrl(nsIURI* aUrl) Returns just NS_OK now. On a side note, this method never gets called during update operation. Is this a sign of a problem? >If I have three imap servers, I'd like the 3 inboxes synced >before any other folders on the servers are synced. Does that happen? Yes, we sync these 3 inboxes (or whatever highest priority folders are according to the given folder strategy) before any other folders on the servers are synced. And we do that **simultaneously** giving that each imap server has its own connection(s) to the server. In other word, if chained mode is selected which is the default, we process the folders of the same imap server (siblings) sequentially, but the folders of different imap servers simultaneously.

(In reply to comment #60) > >Since you're only persisting the 32 bit time, why not only keep track of the > >same? Then you won't have a js issue with PRTime. > >+ readonly attribute PRTime lastSyncTime; > > Actually, I think we shouldn't even expose this attribute to javascript. It > will be used by autosync manager only. Would [noscript] tag hide this attribute > from javscript for me? yes, right, it would. I'd still prefer it to be an unsigned long, since that's what we do everywhere else, to be consistent. > > >+NS_IMETHODIMP nsAutoSyncManager::OnStartRunningUrl(nsIURI* aUrl) > > Returns just NS_OK now. On a side note, this method never gets called during > update operation. Is this a sign of a problem? You're right, I'm not hitting your OnStartRunningUrl, though I'm hitting OnStop - I'm hitting other listeners' OnStart, so I'm not sure why it's not working in your case. It's probably worth my time to figure out... > > >If I have three imap servers, I'd like the 3 inboxes synced > >before any other folders on the servers are synced. Does that happen? > > Yes, we sync these 3 inboxes (or whatever highest priority folders are > according to the given folder strategy) before any other folders on the servers > are synced. And we do that **simultaneously** giving that each imap server has > its own connection(s) to the server. > > In other word, if chained mode is selected which is the default, we process the > folders of the same imap server (siblings) sequentially, but the folders of > different imap servers simultaneously. ah, ok, thx. Is that also true for updating, or are those orthogonal? In other words, do we update the 3 inboxes and then all the other folders? It would seem that updating and syncing should happen at the same time, since to do syncing/downloading, we have to select the folder first, which is all updating does anyway...

>Is that also true for updating, or are those orthogonal? In other >words, do we update the 3 inboxes and then all the other folders? No, they are orthogonal. I have tendency to think of updating as a way to biff during idle time. This is how the queues work in general: nsAutoSyncState has an internal priority queue to store messages waiting to be downloaded. nsAutoSyncMsgStrategy object determines the order in this queue, nsAutoSyncManager uses this queue to manage downloads. Two events cause a change in this queue: 1) nsImapMailFolder::HeaderFetchCompleted: is triggered when TB notices that there are pending messages on the server -- via IDLE command from the server, via explicit select from the user, or via automatic Update during idle time. If it turns out that there are pending messages on the server, it adds them into nsAutoSyncState's download queue. 2) nsAutoSyncState::ProcessExistingHeaders: is triggered for every imap folder every hour or so. nsAutoSyncManager uses an internal queue called Discovery queue to keep track of this task. The purpose of ProcessExistingHeaders() method is to check existing headers of a given folder in batches and discover the messages without bodies, in asynchronous fashion. This process is sequential, one and only one folder at any given time, very similar to indexing. Again, if it turns out that the folder in hand has messages w/o bodies, ProcessExistingHeaders adds them into nsAutoSyncState's download queue. Any change in nsAutoSyncState's download queue, notifies nsAutoSyncManager and nsAutoSyncManager puts the requesting nsAutoSyncState into its internal priority queue (called mPriorityQ) -- if the folder is not already there. nsAutoSyncFolderStrategy object determines the order in this queue. This queue is processed in two modes: chained and parallel. i) Chained: One folder per imap server any given time. Folders owned by different imap servers are simultaneous. ii) Parallel: All folders at the same time, using all cached-connections - a.k.a 'Folders gone wild' mode. The order the folders are added into the mPriorityQ doesn't matter since every time a batch completed for an imap server, nsAutoSyncManager adjusts the order. So, lets say that updating a sub-folder starts downloading message immediately, when an higher priority folder is added into the queue, nsAutoSyncManager switches to this higher priority folder instead of processing the next group of messages of the lower priority one. Setting group size too high might delay this switch at worst. And finally, Update queue helps nsAutoSyncManager to keep track of folders waiting to be updated. With the latest change, we update one and only one folder at any given time. Frequency of updating is 5 min. We add folders into the update queue during idle time, if they are not in mPriorityQ already. >It would seem that updating and syncing should happen at the same time, since >to do syncing/downloading, we have to select the folder first, which is all >updating does anyway Assuming that HeaderFetchCompleted() is indirectly triggered by UpdateFolder(), I think this is the case for most of the time - unless the number of folders does exceed cached-connection count. Am I right? > I'd still prefer it to be an unsigned long, since that's what we do >everywhere else, to be consistent. I understand that probably there are historical reasons behind this usage but wouldn't be better if we fix that now. This data type is "time" and what can represent this better than PRTime?

OK, thanks for the explanation - it would be very useful to have it as a comment in the code. If you're going to try to update all folders every five minutes, I think you either need to have a user-visible pref for this, or, better, I think, use the already existing per server check for new mail interval (which I believe defaults to 10 minutes, so the user can have a bit of control. >Assuming that HeaderFetchCompleted() is indirectly triggered by UpdateFolder(), >I think this is the case for most of the time - unless the number of folders >does exceed cached-connection count. Am I right? Not sure what you mean about the connection cache count - if you call UpdateFolder on 100 folders, we will call HeaderFetchCompleted on all of them, indirectly, eventually. We'll kick off 5 selects; when one finishes, we'll kick off the 6th, etc. But in either case, it will be async/indirect. >I understand that probably there are historical reasons behind this usage but >wouldn't be better if we fix that now. This data type is "time" and what can >represent this better than PRTime PRTime is when you need millisecond resolution, historically. We use it when we need that; otherwise, not so much. But if you feel that need, it's OK. However, this comment is misleading: +void nsAutoSyncState::SetLastSyncTimeInSec(PRInt32 aLastSyncTime) +{ + mLastSyncTime = ((PRTime)aLastSyncTime * PR_USEC_PER_SEC); // increase the precision since you're not gaining any precision here...

>If you're going to try to update all folders every five minutes, I think you >either need to have a user-visible pref for this, or, better, I think, use the >already existing per server check for new mail interval (which I believe >defaults to 10 minutes, so the user can have a bit of control. We don't update folders every five minutes. This is a threshold to prevent AutoSyncManager from updating folders every time TB becomes idle. In other word, we update a folder only when TB becomes idle, and at least 5 min passed since the last update of this folder. Sorry If I wasn't clear before. >NS_IMETHODIMP nsAutoSyncState::SetLastUpdateTime(PRTime >aLastUpdateTime) >needs to return a result. Wow, GCC compiles this with a warning which disappears under the warning flood. /Users/ebirol/Projects/mozilla-hg/mozilla/mailnews/imap/src/nsAutoSyncState.cpp:644: warning: control reaches end of non-void function

> > We don't update folders every five minutes. This is a threshold to prevent > AutoSyncManager from updating folders every time TB becomes idle. In other > word, we update a folder only when TB becomes idle, and at least 5 min passed > since the last update of this folder. Sorry If I wasn't clear before. OK, with a 10 second idle, I'm going to claim that this is pretty much the same thing. I don't think you should update more frequently than the check for new mail interval, do you?

In an IRC/phone conversation today, we (clarkbw, bienvenu, emre, me) hashed out the minimal UI we need to be able to land this feature in b1. This patch is my take at implementing what we discussed. Spec is at: https://wiki.mozilla.org/MailNews:Better_Faster_IMAP_Plan#UX_Decisions_to_make Notes: - I'll upgrade screenshots in the AM for bryan - I haven't tested the upgrade scenario yet - I think the upgrade dialog copy is a bit weak -- maybe we should explain _why_ we're doing it? - the new grouping for "Message Sync and Disk Space" is too wide on the mac. Maybe we want a shorter label? All these aside, I think the code is roughly right. David, it'd be good to get your review.

Comment on attachment 338814 [details] [diff] [review] Patch for the UI changes required by this code asking myself for review :-)

Comment on attachment 338814 [details] [diff] [review] Patch for the UI changes required by this code Longish drive-by! + // tell people that we've switched their folders to offline mode. + function showOfflineMigrationDialog() { + window.openDialog("chrome://messenger/content/offlineMigrationDialog.xul", + "DefaultClient", "modal,centerscreen,chrome,resizable=no"); + } + setTimeout(showOfflineMigrationDialog, 0); All this needs to be run once, right? +<!DOCTYPE window [ + <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" > Why the space before > ? + %brandDTD; + <!ENTITY % defaultClientDTD SYSTEM "chrome://messenger/locale/offlineMigrationDialog.dtd" > + %defaultClientDTD; Why the space before > ? +]> + +<dialog xmlns:html="http://www.w3.org/1999/xhtml" + xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" + id="offlineMigrationDialog" + buttons="extra1,accept" + width="350" + height="150" + buttonlabelextra1="&preferences.label;" + buttonaccessextra1="&preferences.accesskey;" + ondialogextra1="window.opener.openOptionsDialog('paneGeneral'); window.close();" + title="&offlineMigrationDialog.title;"> + Trailing space. + <script type="application/x-javascript" src="chrome://browser/content/utilityOverlay.js"/> browser? And maybe prefer application/javascript + <groupbox> + <caption label="&offlineUsage.label;"/> + <hbox align="start"> + <radiogroup id="offlineGroup" preference="offline_download" class="indent" Trailing space. + orient="vertical" aria-labelledby="XXX"> I think there is no need for aria-labelledby at all, so your lucky;) + <radio value="true" Trailing space. + label="&offlineEnabled.label;" accesskey="&offlineEnabled.accesskey;" + id="offlineenabled"/> + <radio value="false" label="&offlineDisabled.label;" Trailing space. + accesskey="&offlineDisabled.accesskey;" + id="offlinedisabled"/> + </radiogroup> + </hbox> + </groupbox> Code intedention - don't mix, and prefer 2 space. The whole radio group looks to be too have too much to the left. <groupbox id="systemDefaultsGroup" orient="vertical"> <caption label="&systemDefaults.label;"/> diff --git a/mail/locales/en-US/chrome/messenger/am-offline.dtd b/mail/locales/en-US/chrome/messenger/am-offline.dtd --- a/mail/locales/en-US/chrome/messenger/am-offline.dtd +++ b/mail/locales/en-US/chrome/messenger/am-offline.dtd @@ -1,7 +1,7 @@ <!ENTITY doNotDownloadPop3Movemail.label "To save disk space, do not download:"> <!ENTITY doNotDownload.label "To save disk space, do not download for offline use:"> -<!ENTITY offlineNewFolder.label "When I create new folders, select them for offline use"> -<!ENTITY offlineNewFolder.accesskey "c"> +<!ENTITY allFoldersOffline.label "Keep messages for this account on this computer"> Keep messages for offline use maybe? +<!ENTITY allFoldersOffline.accesskey "o"> <!ENTITY offlineNotDownload.label "Messages larger than"> <!ENTITY offlineNotDownload.accesskey "M"> <!ENTITY kb.label "KB"> @@ -25,10 +25,7 @@ <!ENTITY nntpRemoveBody.accesskey "O"> <!ENTITY offlineSelectNntp.label "Select newsgroups for offline use…"> <!ENTITY offlineSelectNntp.accesskey "S"> -<!ENTITY offlineSelectImap.label "Select folders for offline use…"> +<!ENTITY offlineSelectImap.label "Advanced…"> You have to bump the entity name when changing the text, otherwise localizers can't keep up. (Yes, it sucks!) <!ENTITY offlineSelectImap.accesskey "S"> And this is wrong now. -<!ENTITY offlineDesc.label "You can download your messages locally so that they are available when you are working offline."> -<!ENTITY makeInboxMsgsAvailable.label "Make the messages in my Inbox available when I am working offline"> -<!ENTITY makeInboxMsgsAvailable.accesskey "k"> -<!ENTITY offlineGroupTitle.label "Offline"> +<!ENTITY offlineGroupTitle.label "Message Syncing"> <!ENTITY diskspaceGroupTitle.label "Disk Space"> diff --git a/mail/locales/en-US/chrome/messenger/offlineMigrationDialog.dtd b/mail/locales/en-US/chrome/messenger/offlineMigrationDialog.dtd new file mode 100644 --- /dev/null +++ b/mail/locales/en-US/chrome/messenger/offlineMigrationDialog.dtd @@ -0,0 +1,5 @@ +<!ENTITY offlineMigrationDialog.title "Offline Message Viewing"> +<!ENTITY offlineMigrationDialog.intro "&brandShortName; will now keep messages on this computer for offline viewing. This is a change from previous behavior and can be reverted through the preferences.:"> There's some extra space before "This", and an odd colon on the end. +<!ENTITY preferences.label "Open Preferences"> +<!ENTITY preferences.accesskey "P"> diff --git a/mail/locales/en-US/chrome/messenger/preferences/general.dtd b/mail/locales/en-US/chrome/messenger/preferences/general.dtd --- a/mail/locales/en-US/chrome/messenger/preferences/general.dtd +++ b/mail/locales/en-US/chrome/messenger/preferences/general.dtd @@ -1,3 +1,9 @@ +<!ENTITY offlineUsage.label "Message Syncing"> +<!ENTITY offlineEnabled.label "Keep all messages on this computer"> Somehow "offline" needs to get mentioned here to, imho. +<!ENTITY offlineEnabled.accesskey "m"> +<!ENTITY offlineDisabled.label "Do not keep messages on this computer, always re-download"> +<!ENTITY offlineDisabled.accesskey "n"> Please align. <!ENTITY systemDefaults.label "System Defaults"> <!ENTITY alwaysCheckDefault.label "Always check to see if &brandShortName; is the default mail client on startup"> <!ENTITY alwaysCheckDefault.accesskey "l"> diff --git a/mail/locales/en-US/chrome/messenger/prefs.properties b/mail/locales/en-US/chrome/messenger/prefs.properties --- a/mail/locales/en-US/chrome/messenger/prefs.properties +++ b/mail/locales/en-US/chrome/messenger/prefs.properties @@ -43,7 +43,7 @@ # account manager stuff prefPanel-server=Server Settings prefPanel-copies=Copies & Folders -prefPanel-offline-and-diskspace=Offline & Disk Space +prefPanel-offline-and-diskspace=Message Syncing & Disk Space Have to bump the name. prefPanel-diskspace=Disk Space prefPanel-addressing=Composition & Addressing prefPanel-advanced=Advanced diff --git a/mail/locales/jar.mn b/mail/locales/jar.mn --- a/mail/locales/jar.mn +++ b/mail/locales/jar.mn @@ -88,6 +88,7 @@ locale/@AB_CD@/messenger/shellservice.properties (%chrome/messenger/shellservice.properties) locale/@AB_CD@/messenger/shutdownWindow.properties (%chrome/messenger/shutdownWindow.properties) locale/@AB_CD@/messenger/configEditorOverlay.dtd (%chrome/messenger/configEditorOverlay.dtd) + locale/@AB_CD@/messenger/offlineMigrationDialog.dtd (%chrome/messenger/offlineMigrationDialog.dtd) locale/@AB_CD@/messenger/addressbook/abMainWindow.dtd (%chrome/messenger/addressbook/abMainWindow.dtd) locale/@AB_CD@/messenger/addressbook/abNewCardDialog.dtd (%chrome/messenger/addressbook/abNewCardDialog.dtd) locale/@AB_CD@/messenger/addressbook/abContactsPanel.dtd (%chrome/messenger/addressbook/abContactsPanel.dtd) diff --git a/mailnews/base/prefs/resources/content/am-offline.js b/mailnews/base/prefs/resources/content/am-offline.js --- a/mailnews/base/prefs/resources/content/am-offline.js +++ b/mailnews/base/prefs/resources/content/am-offline.js @@ -21,6 +21,7 @@ * * Contributor(s): * dianesun@netscape.com + * dascher@mozillamessaging.com * * Alternatively, the contents of this file may be used under the terms of * either of the GNU General Public License Version 2 or later (the "GPL"), @@ -41,6 +42,9 @@ var gImapIncomingServer; var gPref = null; var gLockedPref = null; +var gOfflineMap = null; // map of folder URLs to offline flags +let Cc = Components.classes; +let Ci = Components.interfaces; Any reason to use let here? function onInit(aPageId, aServerId) { @@ -50,6 +54,7 @@ initServerSettings(); initRetentionSettings(); initDownloadSettings(); + initOfflineSettings(); onCheckItem("offline.notDownloadMin", "offline.notDownload"); onCheckItem("nntp.downloadMsgMin", "nntp.downloadMsg"); @@ -57,9 +62,14 @@ onCheckKeepMsg(); } +function initOfflineSettings() +{ + checkOffline(); + gOfflineMap = collectOfflineFolders(); +} + function initServerSettings() -{ - +{ document.getElementById("offline.notDownload").checked = gIncomingServer.limitOfflineMessageSize; if(gIncomingServer.maxMessageSize > 0) document.getElementById("offline.notDownloadMin").setAttribute("value", gIncomingServer.maxMessageSize); @@ -68,8 +78,7 @@ if(gServerType == "imap") { gImapIncomingServer = gIncomingServer.QueryInterface(Components.interfaces.nsIImapIncomingServer); - document.getElementById("offline.downloadBodiesOnGetNewMail").checked = gImapIncomingServer.downloadBodiesOnGetNewMail; - document.getElementById("offline.newFolder").checked = gImapIncomingServer.offlineDownload; + document.getElementById("offline.folders").checked = gImapIncomingServer.offlineDownload; Get rid of the extra space while you're at it } } @@ -142,6 +151,13 @@ } +function onCancel() +{ + // restore the offline flags for all folders Trailing space. + restoreOfflineFolders(gOfflineMap); + return true; +} + function onSave() { var downloadSettings = @@ -151,7 +167,7 @@ gIncomingServer.limitOfflineMessageSize = document.getElementById("offline.notDownload").checked; gIncomingServer.maxMessageSize = document.getElementById("offline.notDownloadMin").value; - var retentionSettings = saveCommonRetentionSettings(); + var retentionSettings = saveCommonRetentionSettings(); retentionSettings.daysToKeepBodies = document.getElementById("nntp.removeBodyMin").value; retentionSettings.cleanupBodiesByDays = document.getElementById("nntp.removeBody").checked; @@ -164,8 +180,9 @@ gIncomingServer.downloadSettings = downloadSettings; if (gImapIncomingServer) { - gImapIncomingServer.downloadBodiesOnGetNewMail = document.getElementById("offline.downloadBodiesOnGetNewMail").checked; - gImapIncomingServer.offlineDownload = document.getElementById("offline.newFolder").checked; + // set the pref on the incomingserver, and set the flag on all folders. Please use Capital letter to start the sentence + gImapIncomingServer.offlineDownload = document.getElementById("offline.folders").checked; + Trailing spaces. } } @@ -206,9 +223,6 @@ // the load/unload/disable. keep in mind new prefstrings and changes // to code in AccountManager, and update these as well. var allPrefElements = [ - { prefstring:"offline_download", id:"offline.newFolder"}, - { prefstring:"download_bodies_on_get_new_mail", - id:"offline.downloadBodiesOnGetNewMail"}, { prefstring:"limit_offline_message_size", id:"offline.notDownload"}, { prefstring:"max_size", id:"offline.notDownloadMin"}, { prefstring:"downloadUnreadOnly", id:"nntp.notDownloadRead"}, @@ -241,3 +255,66 @@ element.setAttribute("disabled", "true"); } } + +function checkOffline() +{ + let offline = document.getElementById("offline.folders").checked; + let folderPickerButton = document.getElementById('selectImapFoldersButton'); + if (offline) { + folderPickerButton.removeAttribute('disabled') + } else { + folderPickerButton.setAttribute('disabled', true) + } Wouldn't f|olderPickerButton.disabled = offline| do it. Either way, missing semi colons. +} +function toggleOffline() +{ + checkOffline() + let offline = document.getElementById("offline.folders").checked; + var rootFolder = gIncomingServer.rootFolder; + var allFolders = Cc["@mozilla.org/supports-array;1"].createInstance(Ci.nsISupportsArray); + rootFolder.ListDescendents(allFolders); + var numFolders = allFolders.Count(); + var folder; + for (var folderIndex = 0; folderIndex < numFolders; folderIndex++) + { + folder = allFolders.GetElementAt(folderIndex).QueryInterface(Ci.nsIMsgFolder); + if (offline) + folder.setFlag(Components.interfaces.nsMsgFolderFlags.Offline); + else + folder.clearFlag(Components.interfaces.nsMsgFolderFlags.Offline); + } + Trailing spaces. And please be consistent with "let" and "var" here. +} + +function collectOfflineFolders() +{ + let offlineFolderMap = {}; + var rootFolder = gIncomingServer.rootFolder; + var allFolders = Cc["@mozilla.org/supports-array;1"].createInstance(Ci.nsISupportsArray); + rootFolder.ListDescendents(allFolders); + var numFolders = allFolders.Count(); + var folder; + for (var folderIndex = 0; folderIndex < numFolders; folderIndex++) + { + folder = allFolders.GetElementAt(folderIndex).QueryInterface(Ci.nsIMsgFolder); + offlineFolderMap[folder.folderURL] = folder.getFlag(Components.interfaces.nsMsgFolderFlags.Offline); + } + return offlineFolderMap; +} And please be consistent with "let" and "var" here too. +function restoreOfflineFolders(offlineFolderMap) +{ + var rootFolder = gIncomingServer.rootFolder; + var allFolders = Cc["@mozilla.org/supports-array;1"].createInstance(Ci.nsISupportsArray); + rootFolder.ListDescendents(allFolders); + var numFolders = allFolders.Count(); + var folder; + for (var folderIndex = 0; folderIndex < numFolders; folderIndex++) + { + folder = allFolders.GetElementAt(folderIndex).QueryInterface(Ci.nsIMsgFolder); + if (offlineFolderMap[folder.folderURL]) + folder.setFlag(Components.interfaces.nsMsgFolderFlags.Offline); + else + folder.clearFlag(Components.interfaces.nsMsgFolderFlags.Offline); + } +} \ No newline at end of file ^^^ Some general comments: - Do we really need the the "upgrade dialog" at all. We have changed features in the past without them. Personally, i don't think it's worth it. - I think it's a problem to have the message syncing in general preferences. It's very IMAP centric. Makes no sense at all for POP, nntp offline support is what it is, feeds hmm. I think it's also unfortunate if the strings we use aren't thought through and already at this point know we will change a lot of them.

Comment on attachment 338814 [details] [diff] [review] Patch for the UI changes required by this code I might be wrong, but I think this will appear when the user starts up w/ a brand new profile, which we don't want. + // tell people that we've switched their folders to offline mode. + function showOfflineMigrationDialog() { + window.openDialog("chrome://messenger/content/offlineMigrationDialog.xul", + "DefaultClient", "modal,centerscreen,chrome,resizable=no"); + } + setTimeout(showOfflineMigrationDialog, 0); + + I think the way to tell if it was a new profile is here: // verifyAccounts returns true if the callback won't be called if (verifyAccounts(LoadPostAccountWizard)) LoadPostAccountWizard(); if verifyAccounts returns false, then it brought up the account wizard, and we don't want to do this prompt. +} \ No newline at end of file diff --git a/mailnews/base/prefs/resources/content/am-offline.xul b/mailnews/base/prefs/resources/content/am-offline.xul I'm uncertain about disabling the folder picker if the user turns off auto-download, but I suppose if they really want to turn on limited download on some folders, they can do it from the folder properties dialog. +function checkOffline() +{ + let offline = document.getElementById("offline.folders").checked; + let folderPickerButton = document.getElementById('selectImapFoldersButton'); + if (offline) { + folderPickerButton.removeAttribute('disabled') + } else { + folderPickerButton.setAttribute('disabled', true) + } +} Other than those things, this looks fine.

Ugh, Magnus reminds me that most users don't have imap at all - we should check that there's an imap server before putting up this dialog.

re: #65, will do. I am going to submit another patch. This change will add a new private method to nsAutoSyncManager to get the expected value from prefs, therefore will be minimal. Giving the time constraint of this patch, if you feel like continuing to review until I submit it, please do.

Here are the remaining issues that I've brought up but haven't been addressed in a patch: You're still using nsIMsgImapMailFolder where nsIMsgFolder would be more appropriate, e.g.: + nsAutoSyncStrategyDecisionType sort(in nsIMsgImapMailFolder aFolder1, in nsIMsgImapMailFolder aFolder2); Add that excellent description of the way the queues work as a comment in the code somewhere. NS_IMETHODIMP nsAutoSyncState::SetLastUpdateTime(PRTime >aLastUpdateTime) needs to return a result. Don't update more frequently than the check for new mail interval, which will give the user some measure of control.

Revision 8.4 that I have submitted yesterday addresses these issues. >Don't update more frequently than the check for new mail interval, which will >give the user some measure of control. New patch will address only this issue.

My mistake, you did partially address the nsIMsgImapMailFolder comment. But patch 8.4 has this: + + readonly attribute nsIMsgImapMailFolder ownerFolder; and then half a dozen places that QI that to a msg folder and just one place that treats it as an nsIMsgImapMailFolder - that seems less convenient. You're referencing the mDatabase pointer directly - I don't see any need to do that. You can simply use your nsIMsgFolder interface pointer and call: nsIMsgDatabase getMsgDatabase(in nsIMsgWindow msgWindow); passing in null for the msgWindow. So, instead of this pattern, which is repeated several times + SafeRawPtr<nsIMsgImapMailFolder, nsImapMailFolder> ownerFolder(mOwnerFolder, &rv); + NS_ENSURE_SUCCESS(rv, rv); + + // make sure that the database does exist + ownerFolder->GetDatabase(nsnull); + + nsCOMPtr<nsIMsgDatabase> database = ownerFolder->mDatabase; + if(!database) + return NS_ERROR_FAILURE; You can simply say nsCOMPtr <nsIMsgFolder> folder = do_QueryReferent(mOwnerFolder, &rv); NS_ENSURE_SUCCESS(rv, rv); nsCOMPtr<nsIMsgDatabase> databasefolder->GetMsgDatabase(nsnull, getter_AddRefs(database)); if(!database) return NS_ERROR_FAILURE; I don't see how SafeRawPtr buys you anything in this situation where all you need is the nsIMsgFolder interface pointer...but maybe I'm missing something. It would be great if you could avoid knowledge of nsImapMailFolder internals completely. ReleaseSemaphore is a method on nsIMsgFolder, as is GetFlags. The one tricky part is that you're calling BuildIdsAndKeyArray, which is a method on nsImapMailFolder. You could either duplicate that code, or instead of building up an nsIMutableArray of message headers, build up a list of message keys, and call nsImapMailFolder::AllocateUidStringFromKeys(keys.Elements(), kKeys.Length(), uids); which is a public static utility method on nsImapMailFolder. Sorry, the saferefptr stuff seems to be new to this patch and I missed it before...

This patches addresses all review comments, afaict. Note that we're no longer showing the dialog, as per mnyronmyr's comments. Instead, we'll talk about it in the starting page (and we'll have something altogether different for b2 or final)

re: #74 > But patch 8.4 has this: > >+ readonly attribute nsIMsgImapMailFolder ownerFolder; Yes, because we have never discussed to make it nsIMsgFolder before. I can understand making strategy interface generic as much as possible but in case of nsAutoSyncState, the owner folder can only be an imap folder, nothing else. I am confused why we try to make it generic now, to prevent QIing?

right, to avoid the extra QI code. Plus, I can easily imagine wanting to use this code for news or other kinds of folders.

As far as I know there is no plan to extend this patch to support news right now - especially with a time constraints like that. If I do change this, I have to change every GetOwnerFolder() in nsAutoSyncManager into nsIMsgFolder as well. I am really question the effort I am going to put vs the benefit we might get at this point.

I've already done it in my tree once, so I'm happy to do it again. If it allows us to get rid of the whole new wrapper class around weak ref pointers, it would be worth it to me.

Well, how SafeRawPtr<> is related to changing the ownerFolder type issue? I can get rid of SafeRawPtr without changing the type of ownerFolder attribute, right? > I've already done it in my tree once, so I'm happy to do it again. It is all about priorities. What we discuss here is to change the scope of the problem. This patch addresses IMAP preemptive downloads, not generic folder preemtive downloads. My point is instead trying to change the scope right now, lets focus on landing this patch asap. At this point, please feel free to do whatever you think is necessary to get this patch rolling. I am going to submit a patch addressing the interval and smartPtr issue.

Bienvenu pointed out that exposing the global pref is not that helpful, as it won't actually change the per-server version of the prefs. So we'll just use the per-account (& per folder) prefs.

Comment on attachment 338938 [details] [diff] [review] re-revised UI patch, w/o the general preferences this looks OK to me, except that, brace yourself for some ugliness, you need to change the seamonkey version of am-offline.dtd as well. It is in suite/locales/en-US/chrome/mailnews/pref

This part of the UI affects seamonkey, so passing it by Kairo first.

Again, including seamonkey am-offline changes, and a property name change.

Update interval = IncomingServer::BiffMinutes (mail.check_time) Replaced SmartRawPtr according to suggestions. Patch only tested on Mac OS X.

Comment on attachment 338975 [details] [diff] [review] Revision 9 great, thx a lot for doing this. I'll take the diff and change the mOwnerFolder stuff and attach a new patch when I'm done...

I'd really like to get rid of the befriending, but that'll have to wait until I can type with two hands tomorrow :-) I'll test out this patch.

oops, so sorry, that last patch won't build with DEBUG_ebirol, but the fix is simple...in +void nsAutoSyncManager::DebugDumpQ, replace nsIMsgMailFolder with nsIMsgFolder.

Comment on attachment 338956 [details] [diff] [review] patch including UI changes for seamonkey >diff --git a/suite/locales/en-US/chrome/mailnews/pref/am-offline.dtd b/suite/locales/en-US/chrome/mailnews/pref/am-offline.dtd >--- a/suite/locales/en-US/chrome/mailnews/pref/am-offline.dtd >+++ b/suite/locales/en-US/chrome/mailnews/pref/am-offline.dtd >@@ -30,5 +30,5 @@ > <!ENTITY offlineDesc.label "You can download your messages locally so that they are available when you are working offline."> > <!ENTITY makeInboxMsgsAvailable.label "Make the messages in my Inbox available when I am working offline"> > <!ENTITY makeInboxMsgsAvailable.accesskey "k"> >-<!ENTITY offlineGroupTitle.label "Offline"> >+<!ENTITY syncGroupTitle.label "Message Synchronizing"> > <!ENTITY diskspaceGroupTitle.label "Disk Space"> I thought the usual word was "Synchronization"? But then, I'm no native English speaker... >diff --git a/suite/locales/en-US/chrome/mailnews/pref/prefs.properties b/suite/locales/en-US/chrome/mailnews/pref/prefs.properties >--- a/suite/locales/en-US/chrome/mailnews/pref/prefs.properties >+++ b/suite/locales/en-US/chrome/mailnews/pref/prefs.properties >@@ -81,7 +81,7 @@ > # account manager stuff > prefPanel-server=Server Settings > prefPanel-copies=Copies & Folders >-prefPanel-offline-and-diskspace=Offline & Disk Space >+prefPanel-syncing-and-diskspace=Syncing & Disk Space > prefPanel-diskspace=Disk Space > prefPanel-addressing=Composition & Addressing > prefPanel-junk=Junk Settings Uh, that word sounds like it's calling for L10n problems, actually. The label is already pretty long, and the word "Syncing" is an abbreviation by itself, which means most localizations probably need to resort that to a non-abbreviated word, which makes this label even longer. And then, I'm once again not sure if the word "Syncing" even exists, I tend to think that even just "Sync" is better than that. All that said, Standard8, Mnyromyr, IanN, or Neil are better reviewers for SeaMonkey MailNews code than me, I'm mostly doing organizational stuff in the project. Feel free to check in those SeaMonkey-related changes on trunk with a=me given they have proper review though.

Comment on attachment 338956 [details] [diff] [review] patch including UI changes for seamonkey I made a few changes to the suite's dtd file - I'll attach a new patch

I made some parallel changes to the suite's am-offline.dtd file - I apologize in advance if I broke SM.

Comment on attachment 338996 [details] [diff] [review] offline ui patch as landed - checked in >+var Cc = Components.classes; >+var Ci = Components.interfaces; Bah, the rot sets in :-P >+function onCancel() Bad news. Nobody calls this. Oops. > function onSave() Eww, is this entire page instant-apply? (Other pages only use onSave to copy field values to the internal data structures that get saved when you click OK in the main window.)

I'm hitting a scary assertion now. It implies a ref counting problem with the db, though I haven't tracked it down yet: msgdb.dll!nsMsgDatabase::~nsMsgDatabase() Line 887 + 0x2b bytes C++ msgdb.dll!nsMailDatabase::~nsMailDatabase() Line 69 + 0x32 bytes C++ msgdb.dll!nsImapMailDatabase::~nsImapMailDatabase() Line 55 + 0x16 bytes C++ msgdb.dll!nsImapMailDatabase::`scalar deleting destructor'() + 0xf bytes C++ msgdb.dll!nsMsgDatabase::Release() Line 892 + 0xdc bytes C++ msgimap.dll!nsCOMPtr<nsIMsgDatabase>::assign_assuming_AddRef(nsIMsgDatabase * newPtr=0x00000000) Line 511 C++ msgimap.dll!nsCOMPtr<nsIMsgDatabase>::assign_with_AddRef(nsISupports * rawPtr=0x00000000) Line 1187 C++ msgimap.dll!nsCOMPtr<nsIMsgDatabase>::operator=(nsIMsgDatabase * rhs=0x00000000) Line 656 C++ msgimap.dll!nsImapMailFolder::SetAclFlags(unsigned int aclFlags=0x00000001) Line 5288 C++ msgimap.dll!nsMsgIMAPFolderACL::UpdateACLCache() Line 5477 C++ msgimap.dll!nsMsgIMAPFolderACL::SetFolderRightsForUser(const nsACString_internal & userName={...}, const nsACString_internal & rights={...}) Line 5516 C++ msgimap.dll!nsImapMailFolder::AddFolderRights(const nsACString_internal & userName={...}, const nsACString_internal & rights={...}) Line 4817 C++ msgimap.dll!nsImapIncomingServer::AddFolderRights(const nsACString_internal & mailboxName={...}, const nsACString_internal & userName={...}, const nsACString_internal & rights={...}) Line 1270 + 0x21 bytes C++ xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000005, unsigned int methodIndex=0x00000003, unsigned int paramCount=0x0a9efc78, nsXPTCVariant * params=0x0012f84c) Line 102 C++ I ran into this assertion as well, which you mentioned previously. ###!!! ASSERTION: Some other operation is in progress: 'PR_FALSE', file /Users/ebirol/Projects/mozilla-hg/mozilla/mailnews/db/msgdb/src/nsMailDatabase.cpp, line 177 The issue has to do with our grabbing of the folder semaphore, and then running a url which causes a select to get issued, which, in some cases, makes us want to delete messages locally which are deleted on the server, which requires getting the semaphore. This could be avoided by combining the updates with the message downloads because then the message download wouldn't cause a select, since the update already did one, but I understand we want to keep those completely separate. I'll have to think about this. I don't think it's fatal, but it's not harmless either - in some situations, we'll leave deleted messages in the user's imap folder and show them to the user, which is not a great UX.

this patch gets rid of the friend relationship between nsImapMailFolder and nsAutoSyncState, fixes the idl for getNextGroupOfMessages(), and ensures that we're online before trying to do all the idle processing. If I can convince myself that the assertions are harmless, then this patch can be landed...

Thx for the changes David. I think we don't have to start the Discovery/Update timer if offline neither. What about moving it here: + else + { + SetIdleState(idle); + if (WeAreOffline()) + return NS_OK; + + StartTimerIfNeeded(); + }

ah, Thx, Emre, I'll do it that way.

ok, patch landed - changeset: 367:4df6a0bfc947 Thx for being patient, Emre. We'll monitor feedback and file followup bugs on any issues that arrive. The release listeners assertion is bug 455809 and appears unrelated to this patch.

marking fixed

Mark, does this fix the assertion in static builds?

Comment on attachment 338975 [details] [diff] [review] Revision 9 clearing obsolete request.

Comment on attachment 338905 [details] general/sync settings clearing out old request

Comment on attachment 338902 [details] account settings dialog (os x) clearing out old request

Comment on attachment 338956 [details] [diff] [review] patch including UI changes for seamonkey I'm a bit late getting to this review, my main comments are: -prefPanel-offline-and-diskspace=Offline & Disk Space +prefPanel-syncing-and-diskspace=Syncing & Disk Space Surely we could think of a better word than "Syncing"? Although at the moment I don't have any proposals (apart from Replication which is also a bit long). +var Cc = Components.classes; +var Ci = Components.interfaces; At the moment we're not using these in comm-central. They are inconsistently used in mozilla-central and cause life to be difficult for extensions. Since I'm late coming to this review, I'll attach a patch in a moment that will revert these to the original way.

Review comments fix.

Comment on attachment 343030 [details] [diff] [review] [checked in] Fix review comments - drop the Cc/Ci abbreviations >- folder = allFolders.GetElementAt(folderIndex).QueryInterface(Ci.nsIMsgFolder); >+ folder = allFolders.GetElementAt(folderIndex) >+ .QueryInterface(Components.interfaces.nsIMsgFolder); Any reason not to use QueryElementAt?

(In reply to comment #109) > (From update of attachment 343030 [details] [diff] [review]) > >- folder = allFolders.GetElementAt(folderIndex).QueryInterface(Ci.nsIMsgFolder); > >+ folder = allFolders.GetElementAt(folderIndex) > >+ .QueryInterface(Components.interfaces.nsIMsgFolder); > Any reason not to use QueryElementAt? Nope, fixed.

Comment on attachment 343030 [details] [diff] [review] [checked in] Fix review comments - drop the Cc/Ci abbreviations Checked in, changeset id 609:614bb57facfa

Why is this bug set to fixed? Seamonkey 2.0.11 When via IMAP a mail-body comes with flag "download on demand" (like from some apple or from gmx-web-mailer!) it is impossible to reread/refresh that body. I can see it exactly one time - (autosync==true + showinlineattachments=yes) I am sure it wasn't that way before! And now? Martin