Closed Bug 774124 Opened 10 years ago Closed 6 years ago

Clean up includes in content/html with include-what-you-use

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dzbarsky, Assigned: Ms2ger)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [include-what-you-use])

Attachments

(4 files)

No description provided.
Attached patch PatchSplinter Review
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.
Whiteboard: [include-what-you-use]
Assignee: nobody → Ms2ger
Blocks: iwyu
Blocks: includehell
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.
Attachment #797411 - Flags: review?(dzbarsky) → review+
Attachment #797409 - Flags: review?(dzbarsky) → review+
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.