Comment on attachment 642438 [details] [diff] [review] Patch Review of attachment 642438 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsClientRect.cpp @@ +4,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +#include <architecture/i386/math.h> > + > +#include "dombindings_gen.h" I think this one should just be dombindings.h everywhere... ::: content/html/content/src/nsHTMLBodyElement.cpp @@ -78,5 @@ > #define EVENT(name_, id_, type_, struct_) /* nothing; handled by the shim */ > #define FORWARDED_EVENT(name_, id_, type_, struct_) \ > NS_IMETHOD GetOn##name_(JSContext *cx, jsval *vp); \ > NS_IMETHOD SetOn##name_(JSContext *cx, const jsval &v); > -#include "nsEventNameList.h" This is an issue I never figured out how to fix... ::: content/html/content/src/nsHTMLInputElement.cpp @@ +75,1 @@ > // input type=radio I don't think those comments make sense anymore ::: content/html/document/src/nsHTMLContentSink.cpp @@ -118,1 @@ > #undef HTML_TAG And here

FWIW, I don't really agree with what iwyu does for C++, and I think we should look very carefully at what changes to make.

I agree with both of you, this patch was a half-baked idea. I can clean it up if there is interest, but I don't want to spend more time on this if we don't want it.

Also, it is possible to tell IWYU to not remove the *List.h headers we use, but that involves adding IWYU pragman comments to our code.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #3) > FWIW, I don't really agree with what iwyu does for C++, and I think we > should look very carefully at what changes to make. What do you disagree with? Just the inclusion of redundant headers? You can tell it not to duplicate particular headers by using "// IWYU pragma: export". The added header lines make files longer in some cases, but shouldn't slow compilation much because of include guards. It also adds robustness. If a header stops including a different header, you can remove the include from the first header without breaking things that include it and were depending on the second header. This is particularly important when automatically removing redundant headers using a tool like IWYU. If it relied on secondary inclusions like that, it would remove them from all files, which meant that removing the include from the first header could break hundreds of files. On the other hand, every *removed* header dependency means both less code to compile, and fewer files that have to be recompiled when changing a header. So I think the potential perf benefits are worth it, even if we aren't completely happy with the aesthetics.

Comment on attachment 797410 [details] [diff] [review] Part b: Add annotations to nsDisplayItemTypes.h and nsDisplayItemTypesList.h Review of attachment 797410 [details] [diff] [review]: ----------------------------------------------------------------- I don't understand what this annotation means or why we're adding it.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > Comment on attachment 797410 [details] [diff] [review] > Part b: Add annotations to nsDisplayItemTypes.h and nsDisplayItemTypesList.h > > Review of attachment 797410 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't understand what this annotation means or why we're adding it. It means that, whenever we need something that iwyu thinks is defined in nsDisplayItemTypesList.h, rather than suggesting to include that directly (which isn't going to work), it suggests that we include nsDisplayList.h instead.

https://hg.mozilla.org/mozilla-central/rev/db4c9548a304 https://hg.mozilla.org/mozilla-central/rev/8ee0b6427a6f https://hg.mozilla.org/mozilla-central/rev/90a17a78cf79