Closed Bug 854162 Opened 7 years ago Closed 7 years ago

Should use nsImapMailFolder::HasMsgOffline() instead of checking for nsMsgMessageFlag::Offline in nsAutoSyncState.cpp

Categories

(MailNews Core :: Networking: IMAP, defect)

x86_64
Windows 7
defect
Not set

Tracking

(thunderbird21+ fixed, thunderbird-esr1721+ fixed)

RESOLVED FIXED
Thunderbird 22.0
Tracking Status
thunderbird21 + fixed
thunderbird-esr17 21+ fixed

People

(Reporter: dlech, Assigned: dlech)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → david
Status: NEW → ASSIGNED
Attachment #728694 - Flags: review?(irving)
Blocks: 854161
No longer depends on: 854161
For general info, bug 816327 comment 18 has the critical explanation of why this is needed.
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+
Attached patch patch v2Splinter Review
fixed nit
Attachment #728694 - Attachment is obsolete: true
Keywords: checkin-needed
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?
https://hg.mozilla.org/comm-central/rev/0589b7ee5974
Status: ASSIGNED → RESOLVED
Closed: 7 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
Awesome work David. :-)
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+
Attachment #730211 - Flags: approval-comm-esr17? → approval-comm-esr17+
You need to log in before you can comment on or make changes to this bug.