125255 2013-12-04 14:29:43 -0800 [WK2] Including SecItemShim.h should be guarded by ENABLE(SEC_ITEM_SHIM) 2013-12-06 10:35:06 -0800 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 nick.diego nick.diego andersca ap commit-queue oldest_to_newest 956613 0 nick.diego 2013-12-04 14:29:43 -0800 [WK2] Including SecItemShim.h should be guarded by ENABLE(SEC_ITEM_SHIM) 956614 1 218449 nick.diego 2013-12-04 14:30:42 -0800 Created attachment 218449 Patch 956629 2 218456 nick.diego 2013-12-04 14:48:11 -0800 Created attachment 218456 Patch 956658 3 218456 commit-queue 2013-12-04 15:23:42 -0800 Comment on attachment 218456 Patch Clearing flags on attachment: 218456 Committed r160136: <http://trac.webkit.org/changeset/160136> 956659 4 commit-queue 2013-12-04 15:23:44 -0800 All reviewed patches have been landed. Closing bug. 956968 5 ap 2013-12-05 11:15:35 -0800 > [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). 956993 6 ap 2013-12-05 11:53:20 -0800 (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. 957295 7 nick.diego 2013-12-06 10:35:06 -0800 (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 218449 2013-12-04 14:30:42 -0800 2013-12-04 14:45:47 -0800 Patch bug-125255-20131204184238.patch text/plain 1486 nick.diego U3VidmVyc2lvbiBSZXZpc2lvbjogMTYwMTI3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggMDdkNWFiOTRhNjcxOWE1 MDUxN2MzZWVhNTI0N2JiNGU3NWUxMTQ0Zi4uZDMzZjVhMDljNGU2OTUzZGU3YTRkNDY3Zjc0MzA2 YjQ2Mzg0ZGIzMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE0IEBACiAyMDEzLTEyLTA0ICBOaWNr IERpZWdvIFlhbWFuZSAgPG5pY2sueWFtYW5lQG9wZW5ib3NzYS5vcmc+CiAKKyAgICAgICAgW1dL Ml0gSW5jbHVkaW5nIFNlY0l0ZW1TaGltLmggc2hvdWxkIGJlIGd1YXJkZWQgYnkgRU5BQkxFKFNF Q19JVEVNX1NISU0pCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xMjUyNTUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICAqIFVJUHJvY2Vzcy9XZWJQcm9jZXNzUHJveHkuY3BwOgorCisyMDEzLTEyLTA0ICBOaWNr IERpZWdvIFlhbWFuZSAgPG5pY2sueWFtYW5lQG9wZW5ib3NzYS5vcmc+CisKICAgICAgICAgW0dU S11bV0syXSBGaXggYnVpbGQgYWZ0ZXIgcjE2MDEwNAogICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTI1MjQwCiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJL aXQyL1VJUHJvY2Vzcy9XZWJQcm9jZXNzUHJveHkuY3BwIGIvU291cmNlL1dlYktpdDIvVUlQcm9j ZXNzL1dlYlByb2Nlc3NQcm94eS5jcHAKaW5kZXggNjhiMDQ2Yzk1YzdhNWE4Mjc0MGQ5ZmNmM2Jl NGI3YjJjMzVjZDg5ZS4uNjc3NzRlYzZiZGMxMjNjN2VlMmMwZDQ3NDYxNTVkZGJmMjFkZjgwMiAx MDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL1dlYlByb2Nlc3NQcm94eS5jcHAK KysrIGIvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL1dlYlByb2Nlc3NQcm94eS5jcHAKQEAgLTMx LDcgKzMxLDYgQEAKICNpbmNsdWRlICJEb3dubG9hZFByb3h5TWFwLmgiCiAjaW5jbHVkZSAiUGx1 Z2luSW5mb1N0b3JlLmgiCiAjaW5jbHVkZSAiUGx1Z2luUHJvY2Vzc01hbmFnZXIuaCIKLSNpbmNs dWRlICJTZWNJdGVtU2hpbVByb3h5LmgiCiAjaW5jbHVkZSAiVGV4dENoZWNrZXIuaCIKICNpbmNs dWRlICJUZXh0Q2hlY2tlclN0YXRlLmgiCiAjaW5jbHVkZSAiV2ViQmFja0ZvcndhcmRMaXN0SXRl bS5oIgpAQCAtNTQsNiArNTMsMTAgQEAKICNpbmNsdWRlICJQREZQbHVnaW4uaCIKICNlbmRpZgog CisjaWYgRU5BQkxFKFNFQ19JVEVNX1NISU0pCisjaW5jbHVkZSAiU2VjSXRlbVNoaW1Qcm94eS5o IgorI2VuZGlmCisKIHVzaW5nIG5hbWVzcGFjZSBXZWJDb3JlOwogCiAjZGVmaW5lIE1FU1NBR0Vf Q0hFQ0soYXNzZXJ0aW9uKSBNRVNTQUdFX0NIRUNLX0JBU0UoYXNzZXJ0aW9uLCBjb25uZWN0aW9u KCkpCg== 218456 2013-12-04 14:48:11 -0800 2013-12-04 15:23:42 -0800 Patch bug-125255-20131204190008.patch text/plain 2273 nick.diego U3VidmVyc2lvbiBSZXZpc2lvbjogMTYwMTI3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggMDdkNWFiOTRhNjcxOWE1 MDUxN2MzZWVhNTI0N2JiNGU3NWUxMTQ0Zi4uODg2MWEwOGYwZDM1YTE5ZjkzMjc3NDZlNGZjNmEy NmU4OGI1OGJhMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE1IEBACiAyMDEzLTEyLTA0ICBOaWNr IERpZWdvIFlhbWFuZSAgPG5pY2sueWFtYW5lQG9wZW5ib3NzYS5vcmc+CiAKKyAgICAgICAgW1dL Ml0gSW5jbHVkaW5nIFNlY0l0ZW1TaGltLmggc2hvdWxkIGJlIGd1YXJkZWQgYnkgRU5BQkxFKFNF Q19JVEVNX1NISU0pCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xMjUyNTUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICAqIFVJUHJvY2Vzcy9XZWJQcm9jZXNzUHJveHkuY3BwOgorICAgICAgICAqIFdlYlByb2Nl c3MvV2ViUHJvY2Vzcy5jcHA6CisKKzIwMTMtMTItMDQgIE5pY2sgRGllZ28gWWFtYW5lICA8bmlj ay55YW1hbmVAb3BlbmJvc3NhLm9yZz4KKwogICAgICAgICBbR1RLXVtXSzJdIEZpeCBidWlsZCBh ZnRlciByMTYwMTA0CiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xMjUyNDAKIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL1dlYlBy b2Nlc3NQcm94eS5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvV2ViUHJvY2Vzc1Byb3h5 LmNwcAppbmRleCA2OGIwNDZjOTVjN2E1YTgyNzQwZDlmY2YzYmU0YjdiMmMzNWNkODllLi42Nzc3 NGVjNmJkYzEyM2M3ZWUyYzBkNDc0NjE1NWRkYmYyMWRmODAyIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViS2l0Mi9VSVByb2Nlc3MvV2ViUHJvY2Vzc1Byb3h5LmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0 Mi9VSVByb2Nlc3MvV2ViUHJvY2Vzc1Byb3h5LmNwcApAQCAtMzEsNyArMzEsNiBAQAogI2luY2x1 ZGUgIkRvd25sb2FkUHJveHlNYXAuaCIKICNpbmNsdWRlICJQbHVnaW5JbmZvU3RvcmUuaCIKICNp bmNsdWRlICJQbHVnaW5Qcm9jZXNzTWFuYWdlci5oIgotI2luY2x1ZGUgIlNlY0l0ZW1TaGltUHJv eHkuaCIKICNpbmNsdWRlICJUZXh0Q2hlY2tlci5oIgogI2luY2x1ZGUgIlRleHRDaGVja2VyU3Rh dGUuaCIKICNpbmNsdWRlICJXZWJCYWNrRm9yd2FyZExpc3RJdGVtLmgiCkBAIC01NCw2ICs1Mywx MCBAQAogI2luY2x1ZGUgIlBERlBsdWdpbi5oIgogI2VuZGlmCiAKKyNpZiBFTkFCTEUoU0VDX0lU RU1fU0hJTSkKKyNpbmNsdWRlICJTZWNJdGVtU2hpbVByb3h5LmgiCisjZW5kaWYKKwogdXNpbmcg bmFtZXNwYWNlIFdlYkNvcmU7CiAKICNkZWZpbmUgTUVTU0FHRV9DSEVDSyhhc3NlcnRpb24pIE1F U1NBR0VfQ0hFQ0tfQkFTRShhc3NlcnRpb24sIGNvbm5lY3Rpb24oKSkKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUHJvY2Vzcy5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9X ZWJQcm9jZXNzL1dlYlByb2Nlc3MuY3BwCmluZGV4IDQwNGJhMzJjMWYyMzdkOTNjNzcxMGMyNjgw NzY0OGMzNGNlNzE4MjcuLjFlMGI1ZGQ4NjliYzczOGFmZmE0ZThlNjk0ZTY0ZTYzNTM4Yjc0NDYg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUHJvY2Vzcy5jcHAKKysr IGIvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQcm9jZXNzLmNwcApAQCAtMzIsNyArMzIs NiBAQAogI2luY2x1ZGUgIkluamVjdGVkQnVuZGxlVXNlck1lc3NhZ2VDb2RlcnMuaCIKICNpbmNs dWRlICJMb2dnaW5nLmgiCiAjaW5jbHVkZSAiUGx1Z2luUHJvY2Vzc0Nvbm5lY3Rpb25NYW5hZ2Vy LmgiCi0jaW5jbHVkZSAiU2VjSXRlbVNoaW0uaCIKICNpbmNsdWRlICJTdGF0aXN0aWNzRGF0YS5o IgogI2luY2x1ZGUgIldlYkFwcGxpY2F0aW9uQ2FjaGVNYW5hZ2VyLmgiCiAjaW5jbHVkZSAiV2Vi Q29ubmVjdGlvblRvVUlQcm9jZXNzLmgiCkBAIC05OCw2ICs5Nyw5IEBACiAjaW5jbHVkZSAiTmV0 d29ya1Byb2Nlc3NDb25uZWN0aW9uLmgiCiAjZW5kaWYKIAorI2lmIEVOQUJMRShTRUNfSVRFTV9T SElNKQorI2luY2x1ZGUgIlNlY0l0ZW1TaGltLmgiCisjZW5kaWYKIAogI2lmIEVOQUJMRShDVVNU T01fUFJPVE9DT0xTKQogI2luY2x1ZGUgIkN1c3RvbVByb3RvY29sTWFuYWdlci5oIgo=