Bug 125255 – [WK2] Including SecItemShim.h should be guarded by ENABLE(SEC_ITEM_SHIM)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Nick Diego Yamane (diegoyam)]

[WK2] Including SecItemShim.h should be guarded by ENABLE(SEC_ITEM_SHIM)

[Nick Diego Yamane (diegoyam)]

Created attachment 218449 [details]
Patch

[Nick Diego Yamane (diegoyam)]

Created attachment 218456 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 218456 [details]
Patch

Clearing flags on attachment: 218456

Committed r160136: <http://trac.webkit.org/changeset/160136>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> [WK2] Including SecItemShim.h should be guarded by ENABLE(SEC_ITEM_SHIM)

Why is it so? SecItemShim.h already has #if USE(SECURITY_FRAMEWORK) in it. And it's not cool that these guards don't even match.

It's best to have conditional guards inside headers, and to include them unconditionally. You win multiple things that way:

- Include lists in CPP files are smaller and better organized (nothing beats a linear alphabetic list).

- There is an opportunity to reduce build failures due to different include lists. We try to include generic headers before conditional compilation guards.

It's unfortunately not the case in SecItemShim.h when it includes Connection.h, but if this include was outside the guard as it should be, and some patch relied on SecItemShim.h including Connection.h, the patch wouldn't break the build for platforms that don't ENABLE(SEC_ITEM_SHIM).

[Alexey Proskuryakov]

(In reply to comment #5)
> And it's not cool that these guards don't even match.

Looks like my checkout was old, this changed yesterday. My other comments still stand.

[Nick Diego Yamane (diegoyam)]

(In reply to comment #5)
> > [WK2] Including SecItemShim.h should be guarded by ENABLE(SEC_ITEM_SHIM)
> 
> Why is it so? SecItemShim.h already has #if USE(SECURITY_FRAMEWORK) in it. And it's not cool that these guards don't even match.
> 
> It's best to have conditional guards inside headers, and to include them unconditionally. You win multiple things that way:
> 
> - Include lists in CPP files are smaller and better organized (nothing beats a linear alphabetic list).
> 
> - There is an opportunity to reduce build failures due to different include lists. We try to include generic headers before conditional compilation guards.
> 
> It's unfortunately not the case in SecItemShim.h when it includes Connection.h, but if this include was outside the guard as it should be, and some patch relied on SecItemShim.h including Connection.h, the patch wouldn't break the build for platforms that don't ENABLE(SEC_ITEM_SHIM).

Hi, Actually I was not sure on how to proceed to fix that build fail. Gtk and Efl build was broken because SecItemShim.h isn't in their include path, since that file is in a "/mac" directory and neither gtk nor efl build system currently include any "/mac" in their include path I preferred this guard-based solution. I asked about this issue to andersca and he recommended to add this #if as well.

Let me know if you have another better solution to this. BTW, I agree with you that we should minimize this kind of include list variations as much as we can.

Thanks