833009 - Remove nsContentUtils.h inclusion from header files

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Mounir Lamouri (:mounir)]

Including nsContentUtils.h in a header is not a very good idea. We should prevent that as much as we can. I will attach a few simple patches to clean up nsContentUtils.h inclusion and will open a couple of blocking bugs that do more complex stuff (especially I'm sad that Element.h includes nsContentUtils.h).

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Cleanup in layout/

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Cleanup in content/html/

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Cleanup in content/events/

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Cleanup in gfx/

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Cleanup in content/xbl/

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Cleanup in content/xslt/

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Cleanup in content/base/

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 704572 [details] [diff] [review]
Cleanup in content/html/

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

::: content/base/public/nsViewportInfo.h
@@ +4,5 @@
>  
>  #ifndef nsViewportInfo_h___
>  #define nsViewportInfo_h___
>  
> +#include <stdint.h>

mozilla/StandardInteger.h, still

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 704573 [details] [diff] [review]
Cleanup in content/events/

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

No ESM reviews for me, I don't want to be eaten by a dragon :)

Comment 10

[Mounir Lamouri (:mounir)]

With all those patches applied, looking for "nsContentUtils.h" in header files returns:
content/xslt/src/xpath/txXPathNode.h:#include "nsContentUtils.h" // For NameSpaceManager().
content/xslt/src/base/txStringUtils.h:#include "nsContentUtils.h" // For ASCIIToLower().
content/xbl/src/nsXBLProtoImplMember.h:#include "nsContentUtils.h" // For NS_CONTENT_DELETE_LIST_MEMBER.
content/base/src/mozAutoDocUpdate.h:#include "nsContentUtils.h" // For AddScriptBlocker() and RemoveScriptBlocker().
content/base/public/Element.h:#include "nsContentUtils.h"
dom/bindings/CallbackFunction.h:#include "nsContentUtils.h"

I think the big fish is Element.h because it is included every where and would explain why changing nsContentUtils.h make us rebuild nearly the entire tree.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 704575 [details] [diff] [review]
Cleanup in content/xbl/

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

::: content/xbl/src/nsXBLResourceLoader.cpp
@@ +56,5 @@
> +    mType = aType;
> +    mSrc = aSrc;
> +  }
> +
> +  ~nsXBLResource() { 

Nit: all three {s on their own line.

@@ +57,5 @@
> +    mSrc = aSrc;
> +  }
> +
> +  ~nsXBLResource() { 
> +    MOZ_COUNT_DTOR(nsXBLResource);  

Please fix the trailing whitespace while you're here.

Comment 12

[Mounir Lamouri (:mounir)]

Attachment: Cleanup in content/html/

I attached the wrong patch.

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Cleanup in content/base/

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 704573 [details] [diff] [review]
Cleanup in content/events/

Looks like this isn't used in super-hot code paths, so ok.

And I assume nsViewManager change is needed because ESM.h doesn't include
nsContentUtils.h anymore.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 704580 [details] [diff] [review]
Cleanup in content/html/

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

r=me modulo SetContentEditable

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 704580 [details] [diff] [review]
Cleanup in content/html/

Why does SetContentEditable need to end up out of line?  I don't see how it's using nsContentUtils.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 704570 [details] [diff] [review]
Cleanup in layout/

I'd really like to keep WillCauseReflow inline....

Can we just move scriptblockers to a different header containing just them?

r=me on the rest

Comment 18

[Mounir Lamouri (:mounir)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a2ae479d35c6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/00a33fee3c87
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd11feb55873
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/02e8dcde27b7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/32831201b9ef
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7c676b4c0d53
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f28e2f533c56

A big chunk of the nsContentUtils.h inclusions have been removed. Let's do the remaining parts in follow-ups.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a2ae479d35c6
https://hg.mozilla.org/mozilla-central/rev/00a33fee3c87
https://hg.mozilla.org/mozilla-central/rev/cd11feb55873
https://hg.mozilla.org/mozilla-central/rev/02e8dcde27b7
https://hg.mozilla.org/mozilla-central/rev/32831201b9ef
https://hg.mozilla.org/mozilla-central/rev/7c676b4c0d53
https://hg.mozilla.org/mozilla-central/rev/f28e2f533c56