133785 2014-06-11 22:22:57 -0700 Fix build break on EFL port since r169869 2014-06-13 12:43:51 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 133779 1 gyuyoung.kim gyuyoung.kim aestes gyuyoung.kim ossy simon.fraser oldest_to_newest 1015025 0 gyuyoung.kim 2014-06-11 22:22:57 -0700 There is build error since rr169869. In file included from /mnt/buildbot/WebKit-BuildSlave/efl-linux-64-release-wk2/build/Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleAPICast.h:31:0, from /mnt/buildbot/WebKit-BuildSlave/efl-linux-64-release-wk2/build/Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:36: /mnt/buildbot/WebKit-BuildSlave/efl-linux-64-release-wk2/build/Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:96:5: error: "TARGET_OS_IPHONE" is not defined [-Werror=undef] #if TARGET_OS_IPHONE ^ 1015026 1 232931 gyuyoung.kim 2014-06-11 22:24:23 -0700 Created attachment 232931 Patch 1015028 2 232931 ossy 2014-06-11 22:46:33 -0700 Comment on attachment 232931 Patch Clearing flags on attachment: 232931 Committed r169879: <http://trac.webkit.org/changeset/169879> 1015029 3 ossy 2014-06-11 22:46:41 -0700 All reviewed patches have been landed. Closing bug. 1015035 4 simon.fraser 2014-06-11 23:13:09 -0700 Sorry for the breakage. 1015036 5 ossy 2014-06-11 23:14:49 -0700 (In reply to comment #4) > Sorry for the breakage. Not problem, unfortunately this fix broke Mac, but I'll land the proper fix immediately. 1015037 6 ossy 2014-06-11 23:19:01 -0700 Fix landed in https://trac.webkit.org/changeset/169880 1015040 7 aestes 2014-06-11 23:53:26 -0700 Why not just do this: #if defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE On Apple platforms, TARGET_OS_IPHONE is always defined, it's just set to 0 on the Mac. Also, in the .cpp file you can just use PLATFORM(IOS). TARGET_OS_IPHONE is only necessary in API headers. 1015042 8 ossy 2014-06-11 23:56:41 -0700 (In reply to comment #7) > Why not just do this: > #if defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE It was the first idea, but unfortunately GCC's preprocessor doesn't do short circuit macro evaluation, and the build fails when it tries to evaluate the non defined right side of &&. 1015043 9 aestes 2014-06-11 23:58:20 -0700 > It was the first idea, but unfortunately GCC's preprocessor doesn't do short circuit macro evaluation I didn't know that! Thanks for the explanation. 1015135 10 ap 2014-06-12 09:41:37 -0700 This fix broke Mac port, fixed in r169900. > It was the first idea, but unfortunately GCC's preprocessor doesn't do short circuit macro evaluation This is very surprising, because we've always used this technique, even before switching to clang. > Fix landed in https://trac.webkit.org/changeset/169880 This patch had [EFL][GTK] in its title despite touching cross-platform files. This confused me for a moment. These bracketed prefixes are intended for bugs and patches that don't touch cross-platform code at all, making them of zero interest to people who don't work on the particular platform. For example, adding code in an #if doesn't qualify, because it could be introducing a design problem that will make refactoring cross-platorfm code harder. 1015352 11 ossy 2014-06-13 01:05:16 -0700 (In reply to comment #10) > This fix broke Mac port, fixed in r169900. Sorry for the breakage, unfortunately we haven't noticed it. +1 for having a buildstep for this and having style checker against c++ style comments in Platform.h ( Bug133802 ) > > It was the first idea, but unfortunately GCC's preprocessor doesn't do short circuit macro evaluation > > This is very surprising, because we've always used this technique, even before switching to clang. After checking it in details, you are true. In this case the short circuit evaluation really works fine with all of my installed GCC version. I confused because of a similar problem, where it doesn't work at all: test.cpp:5:45: error: missing binary operator before token "(" #if defined(__has_include) && __has_include(<stdio.h>) ^ Here only nested ifs help for GCC users. > This patch had [EFL][GTK] in its title despite touching cross-platform files. This confused me for a moment. > > These bracketed prefixes are intended for bugs and patches that don't touch cross-platform code at all, making them of zero interest to people who don't work on the particular platform. For example, adding code in an #if doesn't qualify, because it could be introducing a design problem that will make refactoring cross-platorfm code harder. It's new for me, as far as I remember everybody use [GTK]/[EFL]/[Mac]/[iOS]/[iOS][WK2] title prefix for bugs which touches cross platform files, but surely (or likely) has no effect for other platforms. I remember many [iOS] and [Mac] prefixed bug which touched cross platform code and broke other platform. I'm not against the rule you mentioned, it could help us. 1015353 12 ossy 2014-06-13 01:16:06 -0700 Let's go back to the original problem for a minute. The problem was adding new "#if TARGET_OS_IPHONE" guards which caused undef warning(=error). I don't want to expect using "#if defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE" always in the future. I'm pretty sure if folks will forget it sometimes and will break the GCC builds accidentally again and again. That's why I added the false definition for TARGET_OS_IPHONE on EFL and GTK. If you don't have objection, I would add back a c style comment to Platform.h like: /* To avoid possible build breakages because of undefined TARGET_OS_IPHONE */ 1015498 13 aestes 2014-06-13 12:43:51 -0700 (In reply to comment #12) > Let's go back to the original problem for a minute. The problem was adding > new "#if TARGET_OS_IPHONE" guards which caused undef warning(=error). I > don't want to expect using "#if defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE" > always in the future. I'm pretty sure if folks will forget it sometimes and > will break the GCC builds accidentally again and again. I don't think this situation will arise very often. We only use TARGET_OS_IPHONE in API headers since those headers are included in translation units that don't use our prefix header. In all other places, we'll use PLATFORM(IOS). Further, only a subset of our API headers are cross-platform (e.g. the Cocoa API headers are only used on Apple platforms), and API headers don't tend to change as often as other files in the project. 232931 2014-06-11 22:24:23 -0700 2014-06-11 22:46:33 -0700 Patch bug-133785-20140612142358.patch text/plain 2435 gyuyoung.kim U3VidmVyc2lvbiBSZXZpc2lvbjogMTY5ODc3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggNjQ0MDY1MTkyMmU3YTZm NzQ5ODdjNDc4YzYwYzI1NDcyMjczZjQ0NS4uMDBkNWRkOTIzN2YyMDM0MTUyZGVkMjU1NDVmZDlj ZGExMDk3YTJkNCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDE0LTA2LTExICBHeXV5 b3VuZyBLaW0gIDxneXV5b3VuZy5raW1Ac2Ftc3VuZy5jb20+CisKKyAgICAgICAgRml4IGJ1aWxk IGJyZWFrIG9uIEVGTCBwb3J0IHNpbmNlIHIxNjk4NjkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEzMzc4NQorCisgICAgICAgIFVucmV2aWV3ZWQsIGZp eCBhIGJ1aWxkIGJyZWFrIG9uIEVGTCBwb3J0LgorCisgICAgICAgICogV2ViUHJvY2Vzcy9Jbmpl Y3RlZEJ1bmRsZS9BUEkvYy9XS0J1bmRsZVBhZ2UuY3BwOiBVc2UgZGVmaW5lZChUQVJHRVRfT1Nf SVBIT05FKS4KKyAgICAgICAgKiBXZWJQcm9jZXNzL0luamVjdGVkQnVuZGxlL0FQSS9jL1dLQnVu ZGxlUGFnZVByaXZhdGUuaDogZGl0dG8uCisKIDIwMTQtMDYtMTEgIFNpbW9uIEZyYXNlciAgPHNp bW9uLmZyYXNlckBhcHBsZS5jb20+CiAKICAgICAgICAgW2lPUyBXSzJdIEdpdmUgV2ViS2l0VGVz dFJ1bm5lciBhIHZpZXdwb3J0IGNvbmZpZ3VyYXRpb24gd2l0aCBpbml0aWFsIHNjYWxlPTEgZm9y IHRlc3RpbmcKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvSW5qZWN0ZWRC dW5kbGUvQVBJL2MvV0tCdW5kbGVQYWdlLmNwcCBiL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3Mv SW5qZWN0ZWRCdW5kbGUvQVBJL2MvV0tCdW5kbGVQYWdlLmNwcAppbmRleCAzOGEyYWIzZDc2Yjdj MmUyNzc3Mzk4M2MxMjRmNjJlODA3MDg1MDljLi40N2VkN2E5MTE0OGNiMmE1MzUyZmRkNTlhODcz YTQzNDgwMGI2ZWViIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL0luamVj dGVkQnVuZGxlL0FQSS9jL1dLQnVuZGxlUGFnZS5jcHAKKysrIGIvU291cmNlL1dlYktpdDIvV2Vi UHJvY2Vzcy9JbmplY3RlZEJ1bmRsZS9BUEkvYy9XS0J1bmRsZVBhZ2UuY3BwCkBAIC01NjYsNyAr NTY2LDcgQEAgYm9vbCBXS0J1bmRsZVBhZ2VJc1VzaW5nRXBoZW1lcmFsU2Vzc2lvbihXS0J1bmRs ZVBhZ2VSZWYgcGFnZVJlZikKICAgICByZXR1cm4gdG9JbXBsKHBhZ2VSZWYpLT51c2VzRXBoZW1l cmFsU2Vzc2lvbigpOwogfQogCi0jaWYgVEFSR0VUX09TX0lQSE9ORQorI2lmIGRlZmluZWQoVEFS R0VUX09TX0lQSE9ORSkKIHZvaWQgV0tCdW5kbGVQYWdlU2V0VXNlVGVzdGluZ1ZpZXdwb3J0Q29u ZmlndXJhdGlvbihXS0J1bmRsZVBhZ2VSZWYgcGFnZVJlZiwgYm9vbCB1c2VUZXN0aW5nVmlld3Bv cnRDb25maWd1cmF0aW9uKQogewogICAgIHRvSW1wbChwYWdlUmVmKS0+c2V0VXNlVGVzdGluZ1Zp ZXdwb3J0Q29uZmlndXJhdGlvbih1c2VUZXN0aW5nVmlld3BvcnRDb25maWd1cmF0aW9uKTsKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvSW5qZWN0ZWRCdW5kbGUvQVBJL2Mv V0tCdW5kbGVQYWdlUHJpdmF0ZS5oIGIvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9JbmplY3Rl ZEJ1bmRsZS9BUEkvYy9XS0J1bmRsZVBhZ2VQcml2YXRlLmgKaW5kZXggYzY2ZTc0YzA3ZjBiZTdh ZWMwYmExNzRmMTAxNDAzMDM3MWRlMDY0NC4uYzQwOWIwYjlhNzA4NjkxYzIxZDE2MDEwMTVlNDAz OWI1ZDAwMGQ1MCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9JbmplY3Rl ZEJ1bmRsZS9BUEkvYy9XS0J1bmRsZVBhZ2VQcml2YXRlLmgKKysrIGIvU291cmNlL1dlYktpdDIv V2ViUHJvY2Vzcy9JbmplY3RlZEJ1bmRsZS9BUEkvYy9XS0J1bmRsZVBhZ2VQcml2YXRlLmgKQEAg LTkzLDcgKzkzLDcgQEAgdHlwZWRlZiB1bnNpZ25lZCBXS1JlbmRlcmluZ1N1cHByZXNzaW9uVG9r ZW47CiBXS19FWFBPUlQgV0tSZW5kZXJpbmdTdXBwcmVzc2lvblRva2VuIFdLQnVuZGxlUGFnZUV4 dGVuZEluY3JlbWVudGFsUmVuZGVyaW5nU3VwcHJlc3Npb24oV0tCdW5kbGVQYWdlUmVmKTsKIFdL X0VYUE9SVCB2b2lkIFdLQnVuZGxlUGFnZVN0b3BFeHRlbmRpbmdJbmNyZW1lbnRhbFJlbmRlcmlu Z1N1cHByZXNzaW9uKFdLQnVuZGxlUGFnZVJlZiwgV0tSZW5kZXJpbmdTdXBwcmVzc2lvblRva2Vu KTsKIAotI2lmIFRBUkdFVF9PU19JUEhPTkUKKyNpZiBkZWZpbmVkKFRBUkdFVF9PU19JUEhPTkUp CiBXS19FWFBPUlQgdm9pZCBXS0J1bmRsZVBhZ2VTZXRVc2VUZXN0aW5nVmlld3BvcnRDb25maWd1 cmF0aW9uKFdLQnVuZGxlUGFnZVJlZiwgYm9vbCk7CiAjZW5kaWYKIAo=