Closed
Bug 854162
Opened 11 years ago
Closed 11 years ago
Should use nsImapMailFolder::HasMsgOffline() instead of checking for nsMsgMessageFlag::Offline in nsAutoSyncState.cpp
Categories
(MailNews Core :: Networking: IMAP, defect)
Tracking
(thunderbird21+ fixed, thunderbird-esr1721+ fixed)
RESOLVED
FIXED
Thunderbird 22.0
People
(Reporter: dlech, Assigned: dlech)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
2.50 KB,
patch
|
standard8
:
approval-comm-aurora+
standard8
:
approval-comm-beta-
standard8
:
approval-comm-esr17+
|
Details | Diff | Splinter Review |
As stated in Bug 854161, nsMsgMessageFlag::Offline is broken in IMAP. There are 2 uses in the autosync code that need to be fixed. The breakage is causing the repeated downloading of messages such as those described in bug 816327.
Assignee | ||
Comment 1•11 years ago
|
||
by replacing the test for nsMsgMessageFlag::Offline with nsImapMailFolder::HasMsgOffline(), we don't change the functionality of autosync at all. I have run all of the imap tests to verify that this does not break autosync. Also, I have manually verified that this patch fixes bug 816327.
Updated•11 years ago
|
Comment 2•11 years ago
|
||
For general info, bug 816327 comment 18 has the critical explanation of why this is needed.
Comment 3•11 years ago
|
||
Comment on attachment 728694 [details] [diff] [review] patch v1 Review of attachment 728694 [details] [diff] [review]: ----------------------------------------------------------------- Stealing review from Irving as I was curious and started looking at this patch and to help him along a bit. I think this is fine for landing, though it does beg the question that if we've got a message where the body is stored in a different folder, shouldn't the offline bit be set? That might actually be a bit difficult from a sync & perf perspective due to not knowing where the other message headers are that are the same id without going searching. So I think this is fine to take for bug 816327 (and onto the ESR branch, as I assume this will largely fix that bug), and I think we might just need to reconsider what we do wrt nsMsgMessageFlag::Offline via bug 854161. ::: mailnews/imap/src/nsAutoSyncState.cpp @@ +352,5 @@ > { > + bool hasMessageOffline; > + folder->HasMsgOffline(mExistingHeadersQ[mProcessPointer], &hasMessageOffline); > + if (!hasMessageOffline) > + msgKeys.AppendElement(mExistingHeadersQ[mProcessPointer]); nit: should be 2-space indent (it has 3).
Attachment #728694 -
Flags: review?(irving) → review+
Updated•11 years ago
|
tracking-thunderbird21:
--- → +
tracking-thunderbird-esr17:
--- → 21+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 5•11 years ago
|
||
Comment on attachment 730211 [details] [diff] [review] patch v2 [Approval Request Comment] Regression caused by (bug #): 721316 User impact if declined: Poor performance for users of Gmail IMAP Testing completed (on c-c, etc.): All IMAP tests on local Windows machine Risk to taking this patch (and alternatives if risky): See comment #3
Attachment #730211 -
Flags: approval-comm-esr17?
Attachment #730211 -
Flags: approval-comm-beta?
Attachment #730211 -
Flags: approval-comm-aurora?
Comment 6•11 years ago
|
||
https://hg.mozilla.org/comm-central/rev/0589b7ee5974
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 22.0
Looks cool, if this solves the redownloading reported in other bugs. Can you close the referenced bugs like bug 816327 when this is tested by the reporters?
Blocks: 721316
Keywords: regression
Comment 8•11 years ago
|
||
Awesome work David. :-)
Comment 9•11 years ago
|
||
Comment on attachment 730211 [details] [diff] [review] patch v2 Ok, with the next merge happening on Monday, I think we should put this into Aurora so that it will be in central, aurora and beta for the next cycle. If it looks good from a user perspective, then we can put it into ESR for the cycle after.
Attachment #730211 -
Flags: approval-comm-beta?
Attachment #730211 -
Flags: approval-comm-beta-
Attachment #730211 -
Flags: approval-comm-aurora?
Attachment #730211 -
Flags: approval-comm-aurora+
Comment 10•11 years ago
|
||
https://hg.mozilla.org/releases/comm-aurora/rev/421dd14083af
status-thunderbird21:
--- → fixed
Updated•11 years ago
|
Attachment #730211 -
Flags: approval-comm-esr17? → approval-comm-esr17+
Comment 11•11 years ago
|
||
https://hg.mozilla.org/releases/comm-esr17/rev/f26d5d22997c
status-thunderbird-esr17:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•