Closed Bug 782868 Opened 8 years ago Closed 7 years ago

nsIMsgFolderCompactor.compact should notify errors to listener.

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 28.0

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Fix (obsolete) — Splinter Review
To write the test for bug 674742, the test needs to know whether the compact method is successful or not.
Attachment #651973 - Flags: review?(mbanner)
Blocks: 784888
No longer blocks: destroys_encrypted
Mark, could you please review this first? I suppose the patch is independent how to write the test for bug 674742.
Comment on attachment 651973 [details] [diff] [review]
Fix

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

::: mailnews/base/src/nsMsgFolderCompactor.cpp
@@ +466,5 @@
>      rv = msgDBService->OpenFolderDB(m_folder, true, getter_AddRefs(m_db));
> +#ifdef DEBUG
> +    // These errors are expected.
> +    rv = (rv == NS_MSG_ERROR_FOLDER_SUMMARY_MISSING ||
> +          rv == NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE) ? NS_OK : rv;

I don't like the way we're doing this for debug-only. That changes the rules, not just for tests, but for developers as well.

I'm thinking that we may want to have this for non-debug as well, but I'd probably want Bienvenu to chime in on that.
Attachment #651973 - Flags: review?(mbanner) → review-
(In reply to Mark Banner (:standard8) from comment #2)
> Comment on attachment 651973 [details] [diff] [review]
> Fix
> 
> Review of attachment 651973 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/base/src/nsMsgFolderCompactor.cpp
> @@ +466,5 @@
> >      rv = msgDBService->OpenFolderDB(m_folder, true, getter_AddRefs(m_db));
> > +#ifdef DEBUG
> > +    // These errors are expected.
> > +    rv = (rv == NS_MSG_ERROR_FOLDER_SUMMARY_MISSING ||
> > +          rv == NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE) ? NS_OK : rv;
> 
> I don't like the way we're doing this for debug-only. That changes the
> rules, not just for tests, but for developers as well.

I agree.

You mean that we should first fix the DEBUG macro in nsMsgDBService::OpenFolderDB()?

https://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#138
Attached patch Fix without DEBUG macro (obsolete) — Splinter Review
I made a mistake in the previous patch. The patch causes a failure for test_folderCompact.js on release build.
Attachment #651973 - Attachment is obsolete: true
Attachment #656768 - Flags: review?(mbanner)
Comment on attachment 656768 [details] [diff] [review]
Fix without DEBUG macro

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

Sorry for the delay, I think the idea is fine, but this needs an updated patch for the recent changes that we've done.
Attachment #656768 - Flags: review?(mbanner) → feedback+
Hi Hiro. Can you revise this patch?

(In reply to Mark Banner (:standard8) from comment #5)
> Comment on attachment 656768 [details] [diff] [review]
> Fix without DEBUG macro
> 
> Review of attachment 656768 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay, I think the idea is fine, but this needs an updated
> patch for the recent changes that we've done.
Assignee: nobody → hiikezoe
Blocks: 406851
Attachment #656768 - Attachment is obsolete: true
Attachment #825051 - Flags: review?(mbanner)
Attachment #825051 - Flags: review?(mbanner) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/6878aac88d58
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 28.0
You need to log in before you can comment on or make changes to this bug.