1447498 - remove some unneeded includes in /mailnews

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement, P5)

Description

[:aceman]

+++ 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 1

[:aceman]

Attachment: 1447498.patch - news [landed in comment 6]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=143b930319b2a796c022272d8439c31e8f602aa3

Comment 2

[Jorg K (CEST = GMT+2)]

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?

Comment 3

[:aceman]

The .h file uses those objects too.

Comment 4

[Jorg K (CEST = GMT+2)]

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?

Comment 5

[Jorg K (CEST = GMT+2)]

(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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cc67b8f8435c
remove some unneeded includes in mailnews/news. r=jorgk

Comment 7

[:aceman]

(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?

Comment 8

[Jorg K (CEST = GMT+2)]

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.

Comment 9

[:aceman]

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).

Comment 10

[Jorg K (CEST = GMT+2)]

The answer to the question is that nsIMsgWindow.h has a forward declaration of nsIMsgStatusFeedback and nsIMsgWindow.h was included in nsNewsDownloader.h.

Comment 11

[:aceman]

Attachment: 1447498-mime1.patch

Comment 12

[Jorg K (CEST = GMT+2)]

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?

Comment 13

[:aceman]

Attachment: 1447498-mime1.patch v1.1 [landed in comment 17]

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.

Comment 14

[:aceman]

Attachment: 1447498-mime2.patch [landed in comment #23]

Comment 15

[Jorg K (CEST = GMT+2)]

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.

Comment 16

[:aceman]

(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 .

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5e1cd8191d43
remove some unneeded includes in mailnews/mime - part 1. r=jorgk

Comment 18

[:aceman]

Attachment: 1447498-local1.patch [landed in comment #26]

Comment 19

[:aceman]

Attachment: 1447498-local2.patch

Comment 20

[:aceman]

Attachment: 1447498-local2.patch v1.1 [landed in comment #32]

This part failed on Windows: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c91fb1797f4432813844e99992ae884755a58079 .

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 8961617 [details] [diff] [review]
1447498-local1.patch [landed in comment #26]

OK, I usually compile these before landing them ;-)

Comment 22

[Jorg K (CEST = GMT+2)]

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.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2bfd3297e48d
remove some unneeded includes in mailnews/mime - part 2. r=jorgk

Comment 24

[:aceman]

Attachment: 1447498-jsaccount.patch [landed in comment #33]

Comment 25

[:aceman]

Attachment: 1447498-mapi.patch [landed in comment #34]

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5c6080601881
remove some unneeded includes in mailnews/local - part 1. r=jorgk

Comment 27

[:aceman]

Attachment: 1447498-import1.patch [landed comment #35]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8d29b2cd153e941a4055279417b602104f9e0353

Comment 28

[Jorg K (CEST = GMT+2)]

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.

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8961982 [details] [diff] [review]
1447498-mapi.patch [landed in comment #34]

Better savings here ;-)

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 8962132 [details] [diff] [review]
1447498-import1.patch [landed comment #35]

Nice savings here, too ;-)

Comment 31

[:aceman]

(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.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ecb5839542ce
remove some unneeded includes in mailnews/local - part 2. r=jorgk

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6abbf00b7ed3
remove some unneded includes from mailnews/jsaccount. r=jorgk

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c79eb711d4d4
remove some unneeded includes from mailnews/mapi. r=jorgk

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b691118300c2
remove some unneded includes from mailnews/import - part 1. r=jorgk

Comment 36

[Jorg K (CEST = GMT+2)]

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.

Comment 37

[:aceman]

Attachment: 1447498-import2.patch [landed comment #39]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fd5b6a52d7c0234f77bcfcca7e1b5188944f1a55

Comment 38

[Jorg K (CEST = GMT+2)]

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.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/33d4535ecb47
remove some unneded includes from mailnews/import - part 2. r=jorgk

Comment 40

[:aceman]

Attachment: 1447498-ext.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a4f2870dd962722eb603e3a883b5ac9bdac83082