[WK2] Including SecItemShim.h should be guarded by ENABLE(SEC_ITEM_SHIM)
Created attachment 218449 [details] Patch
Created attachment 218456 [details] Patch
Comment on attachment 218456 [details] Patch Clearing flags on attachment: 218456 Committed r160136: <http://trac.webkit.org/changeset/160136>
All reviewed patches have been landed. Closing bug.
> [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).
(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.
(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