782868 - nsIMsgFolderCompactor.compact should notify errors to listener.

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix

To write the test for bug 674742, the test needs to know whether the compact method is successful or not.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Mark, could you please review this first? I suppose the patch is independent how to write the test for bug 674742.

Comment 2

[Mark Banner (:standard8)]

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.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix without DEBUG macro

I made a mistake in the previous patch. The patch causes a failure for test_folderCompact.js on release build.

Comment 5

[Mark Banner (:standard8)]

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.

Comment 6

[Wayne Mery (:wsmwk)]

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.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Adapt to the latest trunk

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/6878aac88d58