Bug 51789 – Don't include Inspector headers when Inspector is disabled

• Status: RESOLVED
• Resolution: INVALID
• Component: Web Inspector (Deprecated)

[Konstantin Tokarev]

Some source files include unneeded Inspector headers even when Inspector is disabled, leading to transitive inclusion of other unrelated headers.  Attached patch makes inclusion of Inspector headers conditional.

[Konstantin Tokarev]

Created attachment 77740 [details]
Don't include Inspector headers when Inspector is disabled

[Darin Adler]

Comment on attachment 77740 [details]
Don't include Inspector headers when Inspector is disabled

View in context: https://bugs.webkit.org/attachment.cgi?id=77740&action=review

> WebCore/bindings/js/JSDOMWindowBase.cpp:34
> +
> +#if ENABLE(INSPECTOR)
>  #include "InspectorController.h"
> +#endif
> +

Conditional includes should go in separate paragraphs after the unconditional includes, not sorted in the middle of the unconditional includes.

[Konstantin Tokarev]

Created attachment 77743 [details]
Don't include Inspector headers when Inspector is disabled

Fixed headers' order

[WebKit Commit Bot]

Comment on attachment 77743 [details]
Don't include Inspector headers when Inspector is disabled

Clearing flags on attachment: 77743

Committed r74847: <http://trac.webkit.org/changeset/74847>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

I thought that the preferred approach was to have #include guards in headers, not in every .cpp file. Is that wrong?

> leading to transitive inclusion of other unrelated headers

This reason in particular change seems wrong to me. Changing which other headers are included indirectly is only likely to break no-inspector builds more often, and has no positive effect that I could see.

[Darin Adler]

(In reply to comment #6)
> I thought that the preferred approach was to have #include guards in headers, not in every .cpp file. Is that wrong?

Probably right. We have a mix of both techniques, and I forgot what is the approved way.

> > leading to transitive inclusion of other unrelated headers
> 
> This reason in particular change seems wrong to me. Changing which other headers are included indirectly is only likely to break no-inspector builds more often, and has no positive effect that I could see.

I agree with you on this, Alexey.

Sorry I didn’t think of these things before approving the patch.

[Alexey Proskuryakov]

This is a fairly big change, so I suggest rolling it out. I can do it next week, if no one beats me to that.

[Konstantin Tokarev]

(In reply to comment #6)
> This reason in particular change seems wrong to me. Changing which other headers are included indirectly is only likely to break no-inspector builds more often, and has no positive effect that I could see.

In perspective I'd like to introduce option for building WebKit without JavaScript engine. It's needed for running WebKit on device with very limited memory and storage. This patch is really a first step in this way, because inspector #includes bring JavaScript engine dependencies in some sources which don't relly use it.

I'm afraid this change will be out of your scope. However, I'd like to help you with making WebKit more configurable and easier to shrink, also making my local modifications as small as it's possible

Please look through these patch. I really cannot see anything that it may break. Patch just disables one or two headers in each of modified files, and shrinks unused inspector cookies is some places. Not so big change, as it may seem.

>I thought that the preferred approach was to have #include guards in headers, not in every .cpp file.

I'm afraid shrinking header may break compilation of no-inspector builds on platforms which I don't test.

[Alexey Proskuryakov]

Could you tell me more about how this change helps you?

What I'm saying is that our normal approach is to have checks like #if ENABLE(INSPECTOR) in header files, not in .cpp files. So, the header becomes effectively empty when a feature is disabled.

See e.g. loader/appcache/ApplicationCache.h for how we normally do this.

There are multiple benefits in doing it this way:
- .cpp files are much easier to read and write, because headers are in simple alphabetical order, not in three-line blocks per feature define;
- it's somewhat easier to avoid breaking the build, because #if guards are likely added earlier in development cycle, when everyone remembers that the feature is optional;
- we can start having unrelated includes in headers outside #if guards - as mentioned in my previous comment, that would also make breaking the build less likely. But that's not something we do today.

As far as I can tell, adding proper #if guards to header files would be fine for your use case, too.

> Not so big change, as it may seem.

I'm saying that it's big because it touches many files, and thus noticeably increases the chance that someone will copy the wrong idiom from one of them. We currently have a mix of the two approaches, and should move to standardizing on one.

[Konstantin Tokarev]

> See e.g. loader/appcache/ApplicationCache.h for how we normally do this.

I agree, it looks more reasonable. But conitional includes are used almost everywhere, so I was mislead

[Alexey Proskuryakov]

Reverted in r74877. Please feel free to open a new bug for adding preprocessor guards with ENABLE(INSPECTOR).