remove some unneeded includes in /mailnews

RESOLVED FIXED in Thunderbird 61.0

Status

enhancement
P5
trivial
RESOLVED FIXED
Last year
Last year

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 61.0

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: only check in when nothing else can be checked in and you want to invoke a new build on trunk)

Attachments

(9 attachments, 3 obsolete attachments)

+++ This bug was initially created as a clone of Bug #1416033 +++

There are some includes in the /mailnews c++ files (.h, .cpp, .mm) that seem unneeded, the code does not use any identifier from the included file.
I've made a quick script to identify where nsI* objects aren't used in the file, but nsI*.h file is included. Many of them are unneeded.
Cleaning up the needless includes may speed up compiling by a tiny bit ;)

I will attach several patches into this bug that cover the individual code directories under /mailnews.
There is no priority to check them in after review.
Jorg, you can use the patches for check-ins when needed, similar to bug 1399756.
Comment on attachment 8960880 [details] [diff] [review]
1447498.patch - news [landed in comment 6]

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

::: mailnews/news/src/nsNewsDownloader.h
@@ +14,5 @@
>  #include "nsIMsgWindow.h"
>  #include "nsIMsgSearchNotify.h"
>  #include "nsIMsgSearchSession.h"
> +#include "nsIMsgStatusFeedback.h"
> +#include "nsTArray.h"

Preferable to leave these in the C++ file, no?
The .h file uses those objects too.
Comment on attachment 8960880 [details] [diff] [review]
1447498.patch - news [landed in comment 6]

Yes, I checked. So how does this compile now? nsNewsDownloader.h is included in nsNewsDownloader.cpp *before* these two are also included there. Sure, someone else will have included nsTArray, but nsIMsgStatusFeedback.h?
Attachment #8960880 - Flags: review?(jorgk) → review+
(In reply to :aceman from comment #0)
> Jorg, you can use the patches for check-ins when needed, similar to bug
> 1399756.
Good, I'm running out of white-space and spelling mistakes.
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/cc67b8f8435c
remove some unneeded includes in mailnews/news. r=jorgk
(In reply to Jorg K (GMT+1) from comment #4)
> Yes, I checked. So how does this compile now? nsNewsDownloader.h is included
> in nsNewsDownloader.cpp *before* these two are also included there. Sure,
> someone else will have included nsTArray, but nsIMsgStatusFeedback.h?

What do you mean here?
nsNewsDownloader.h needs/needed nsIMsgStatusFeedback.h but didn't include it. So where did it come from? I can't have come through the C++ file since that included nsIMsgStatusFeedback.h after nsNewsDownloader.h.
Yeah, so it came from somewhere else.
I think the .h file does not need anything for itself, it isn't compiled alone. But when it is included in some .cpp file, then either the nsIMsgStatusFeedback.h should be in the .h file or the .cpp file and then it will work. Just that I think the includes for objects used in the .h file should be in the .h file too. When a .cpp file includes a .h file, it shouldn't also try to determine what other .h files it needs to include so that compiling that .h part works. They should some in the package (the .h file).
The answer to the question is that nsIMsgWindow.h has a forward declaration of nsIMsgStatusFeedback and nsIMsgWindow.h was included in nsNewsDownloader.h.
Posted patch 1447498-mime1.patch (obsolete) — Splinter Review
Attachment #8961168 - Flags: review?(jorgk)
Comment on attachment 8961168 [details] [diff] [review]
1447498-mime1.patch

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

Nice, but clarify the nsISupports.h.

::: mailnews/mime/cthandlers/glue/mimexpcom.cpp
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.h"

Are you sure this is needed? Where, why?
Attachment #8961168 - Flags: review?(jorgk) → review+
Right, I got compile errors without this include. However you are right, they were actually all from the included nsIMimeObjectClassAccess.h, which is missing this nsISupports.h. Fixed, thanks.
Attachment #8961168 - Attachment is obsolete: true
Attachment #8961178 - Flags: review+
Attachment #8961202 - Flags: review?(jorgk)
Comment on attachment 8961202 [details] [diff] [review]
1447498-mime2.patch [landed in comment #23]

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

Thanks.

::: mailnews/mime/src/mimemsig.cpp
@@ -13,5 @@
>  #include "nsMimeTypes.h"
>  #include "msgCore.h"
>  #include "nsMimeStringResources.h"
>  #include "mimemoz2.h"
> -#include "nsIMimeConverter.h" // for MimeConverterOutputCallback

Strange, no MimeConverterOutputCallback in there.
Attachment #8961202 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+1) from comment #15)
> > -#include "nsIMimeConverter.h" // for MimeConverterOutputCallback
> Strange, no MimeConverterOutputCallback in there.

Yes, and it even isn't defined in nsIMimeConverter, that's why I changed all these includes to modmimee.h .
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5e1cd8191d43
remove some unneeded includes in mailnews/mime - part 1. r=jorgk
Attachment #8961617 - Flags: review?(jorgk)
Posted patch 1447498-local2.patch (obsolete) — Splinter Review
Attachment #8961618 - Flags: review?(jorgk)
Attachment #8961178 - Attachment description: 1447498-mime1.patch v1.1 → 1447498-mime1.patch v1.1 [landed in comment 17]
Attachment #8960880 - Attachment description: 1447498.patch - news → 1447498.patch - news [landed in comment 6]
Comment on attachment 8961617 [details] [diff] [review]
1447498-local1.patch [landed in comment #26]

OK, I usually compile these before landing them ;-)
Attachment #8961617 - Flags: review?(jorgk) → review+
Comment on attachment 8961649 [details] [diff] [review]
1447498-local2.patch v1.1 [landed in comment #32]

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

Thanks.

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ -48,5 @@
>  #include "nsIPop3URL.h"
>  #include "nsIMsgMailSession.h"
>  #include "nsIMsgFolderCompactor.h"
>  #include "nsNetCID.h"
> -#include "nsIMsgMailNewsUrl.h"

I'm surprised that isn't used, but it isn't.
Attachment #8961649 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2bfd3297e48d
remove some unneeded includes in mailnews/mime - part 2. r=jorgk
Attachment #8961975 - Flags: review?(jorgk)
Attachment #8961982 - Flags: review?(jorgk)
Attachment #8961202 - Attachment description: 1447498-mime2.patch → 1447498-mime2.patch [landed in comment #23]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5c6080601881
remove some unneeded includes in mailnews/local - part 1. r=jorgk
Attachment #8961617 - Attachment description: 1447498-local1.patch → 1447498-local1.patch [landed in comment #26]
Comment on attachment 8961975 [details] [diff] [review]
1447498-jsaccount.patch [landed in comment #33]

Not a great saving here. And you added many includes which were already included through other files, for example nsMsgSend.h already includes a bunch of stuff. Maybe OK for clarity.
Attachment #8961975 - Flags: review?(jorgk) → review+
Comment on attachment 8961982 [details] [diff] [review]
1447498-mapi.patch [landed in comment #34]

Better savings here ;-)
Attachment #8961982 - Flags: review?(jorgk) → review+
Comment on attachment 8962132 [details] [diff] [review]
1447498-import1.patch [landed comment #35]

Nice savings here, too ;-)
Attachment #8962132 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+1) from comment #28)
> Not a great saving here. And you added many includes which were already
> included through other files, for example nsMsgSend.h already includes a
> bunch of stuff. Maybe OK for clarity.

Yes, some of the added includes are just for safety. We use the object, so include the definition and not rely on some hidden includes through other files, that may go away any time.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ecb5839542ce
remove some unneeded includes in mailnews/local - part 2. r=jorgk
Attachment #8961649 - Attachment description: 1447498-local2.patch v1.1 → 1447498-local2.patch v1.1 [landed in comment #38]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6abbf00b7ed3
remove some unneded includes from mailnews/jsaccount. r=jorgk
Attachment #8961975 - Attachment description: 1447498-jsaccount.patch → 1447498-jsaccount.patch [landed in comment #33]
Attachment #8961649 - Attachment description: 1447498-local2.patch v1.1 [landed in comment #38] → 1447498-local2.patch v1.1 [landed in comment #32]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c79eb711d4d4
remove some unneeded includes from mailnews/mapi. r=jorgk
Attachment #8961982 - Attachment description: 1447498-mapi.patch → 1447498-mapi.patch [landed in comment #34]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b691118300c2
remove some unneded includes from mailnews/import - part 1. r=jorgk
Comment on attachment 8962132 [details] [diff] [review]
1447498-import1.patch [landed comment #35]

I reverted the "class xx" -> "#include 'xx.h'" changes. They weren't necessary.

Can I have some more patches please.
Attachment #8962132 - Attachment description: 1447498-import1.patch → 1447498-import1.patch [landed comment #35]
Comment on attachment 8973589 [details] [diff] [review]
1447498-import2.patch [landed comment #39]

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

Nice-ö. Can I have more please.

::: mailnews/import/text/src/nsTextImport.cpp
@@ +8,2 @@
>    Text import addressbook interfaces
> +*/

That comment block doesn't look right.

::: mailnews/import/winlivemail/nsWMImport.cpp
@@ +6,3 @@
>  /*
> +  Windows Live Mail (Win32) import mail and addressbook interfaces
> +*/

and this. I'll fix it before checkin.
Attachment #8973589 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/33d4535ecb47
remove some unneded includes from mailnews/import - part 2. r=jorgk
Attachment #8973589 - Attachment description: 1447498-import2.patch → 1447498-import2.patch [landed comment #39]
Attachment #8974158 - Attachment is obsolete: true
Attachment #8974158 - Flags: review?(jorgk)
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.