544621 - Use or remove '#ifdef HAVE_PORT' code

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Serge Gautherie (:sgautherie)]

(Noticed while working on bug 530414.)

{
/mailnews/local/src/nsLocalMailFolder.cpp
    * line 1390 -- #ifdef HAVE_PORT
/mailnews/imap/src/nsImapProtocol.cpp
    * line 2835 -- #ifdef HAVE_PORT
}

Comment 1

[Serge Gautherie (:sgautherie)]

Fwiw,
some of these were added in r1.1
and removed in
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mailnews/local/src/nsLocalMailFolder.cpp&mark=1.428#1.429

Comment 2

[:Cykesiopka]

Attachment: Proposed Patch v1

I can't even compile TB if I #define HAVE_PORT in either .cpp file or if I remove the ifdefs... So I guess removing this code is the way to go...

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 727402 [details] [diff] [review]
Proposed Patch v1

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

Thanks for doing the patch. I think we could go a little bit further however, looking at the (get)requiresCleanup function:

http://mxr.mozilla.org/comm-central/search?string=RequiresCleanup&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

it isn't actually called anywhere, additionally, all the functions in the other files either don't return anything, return an error, or return false. So I think we can drop all the instances of requiresCleanup that you can see in that link - clearRequiresCleanup.

Could you update the patch to remove the redundant functions?

Note that because you'll also remove functions from nsIMsgFolder.idl, you'll also need to change the uuid of nsIMsgFolder (near the top of the file), information on how to get a new uuid is available here: https://developer.mozilla.org/en-US/docs/Generating_GUIDs

Comment 4

[Mark Banner (:standard8)]

As you're working on this, assigning it to you for tracking purposes.

Comment 5

[:Cykesiopka]

For clarification, do you want me to remove all of the following?
  - boolean requiresCleanup & clearRequiresCleanup() in nsIMsgFolder.idl
  - All the GetRequiresCleanup() implementations
  - All the ClearRequiresCleanup() implementations

I'm unsure of the ClearRequiresCleanup() stuff in particular, sorry...

Comment 6

[Mark Banner (:standard8)]

(In reply to Cykesiopka from comment #5)
> For clarification, do you want me to remove all of the following?
>   - boolean requiresCleanup & clearRequiresCleanup() in nsIMsgFolder.idl
>   - All the GetRequiresCleanup() implementations
>   - All the ClearRequiresCleanup() implementations

Yes, all of the above.

> I'm unsure of the ClearRequiresCleanup() stuff in particular, sorry...

ClearRequiresCleanup isn't called from anywhere and only is defined in one place, so it is safe to remove.

A quick summary of what is happening here: the functions are defined as interfaces in nsIMsgFolder.idl, there's then various classes that inherit from nsIMsgFolder and implement those functions. So if you remove them from nsIMsgFolder.idl (which is safe to do as I previously noted), you'll need to remove the implementations as well.

You'll also note that once removed from nsIMsgFolder, if you haven't removed the implementations you will get compile issues.

Feel free to send me patches or ask me other questions direct if you need more help here.

Comment 7

[:Cykesiopka]

Thanks for the clarification!

Comment 8

[:Cykesiopka]

Attachment: Patch v2

+Dead code removal

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 730403 [details] [diff] [review]
Patch v2

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

That's excellent. Thanks for the patch, a nice bit of cleanup :-)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/5b9c506ce030