88147 2012-06-01 16:13:43 -0700 Remove dependency from ImageDiff to WTF 2012-06-06 09:01:17 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 66687 1 wangxianzhu wangxianzhu abarth peter webkit.review.bot zhenghao oldest_to_newest 639835 0 wangxianzhu 2012-06-01 16:13:43 -0700 For now ImageDiff depends on WTF in gyp and on Android WTF is requires to be build for host. Actually ImageDiff.cpp doesn't depend on anything in WTF except the definitions in some headers. 639859 1 145405 wangxianzhu 2012-06-01 16:38:53 -0700 Created attachment 145405 patch 639863 2 145405 abarth 2012-06-01 16:40:48 -0700 Comment on attachment 145405 patch Ok. 639931 3 145405 webkit.review.bot 2012-06-01 19:47:57 -0700 Comment on attachment 145405 patch Clearing flags on attachment: 145405 Committed r119309: <http://trac.webkit.org/changeset/119309> 639932 4 webkit.review.bot 2012-06-01 19:48:01 -0700 All reviewed patches have been landed. Closing bug. 639989 5 peter 2012-06-02 00:26:29 -0700 I previously argued against this change, and I still think it's the wrong thing to do. By removing the dependency on WTF, but actually introducing an additional include dir to WTF, you're creating the image that WTF can be used while some files actually do require to be compiled (as #include directives work fine). The #ifdefs for assertions on line 52 of ImageDiff.cpp are already an artifact of this.. 640130 6 wangxianzhu 2012-06-02 21:18:38 -0700 (In reply to comment #5) > I previously argued against this change, and I still think it's the wrong thing to do. By removing the dependency on WTF, but actually introducing an additional include dir to WTF, you're creating the image that WTF can be used while some files actually do require to be compiled (as #include directives work fine). The #ifdefs for assertions on line 52 of ImageDiff.cpp are already an artifact of this.. Sorry I didn't see your previous comment about this change. Would it look good if I totally remove the header dependency looking like the following? Index: Tools/DumpRenderTree/chromium/ImageDiff.cpp =================================================================== --- Tools/DumpRenderTree/chromium/ImageDiff.cpp (revision 119351) +++ Tools/DumpRenderTree/chromium/ImageDiff.cpp (working copy) @@ -34,8 +34,6 @@ // The exact format of this tool's output to stdout is important, to match // what the run-webkit-tests script expects. -#include "config.h" - #include "webkit/support/webkit_support_gfx.h" #include <algorithm> #include <iterator> @@ -44,7 +42,7 @@ #include <string.h> #include <vector> -#if OS(WINDOWS) +#if defined(OS_WINDOWS) #include <windows.h> #define PATH_MAX MAX_PATH #endif @@ -342,7 +340,7 @@ while (fgets(buffer, sizeof(buffer), stdin)) { if (!strncmp("Content-length: ", buffer, 16)) { char* context; -#if OS(WINDOWS) +#if defined(OS_WINDOWS) strtok_s(buffer, " ", &context); int imageSize = strtol(strtok_s(0, " ", &context), 0, 10); #else Index: Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp =================================================================== --- Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp (revision 119351) +++ Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp (working copy) @@ -58,7 +58,6 @@ '<(chromium_src_dir)/webkit/support/webkit_support.gyp:webkit_support_gfx', ], 'include_dirs': [ - '<(source_dir)/WTF', '<(DEPTH)', ], 'sources': [ 640144 7 abarth 2012-06-02 23:38:24 -0700 Sorry Peter. I should have given you a chance to comment on this bug. Do you think we should revert the patch while we sort this out? 640160 8 peter 2012-06-03 01:14:47 -0700 (In reply to comment #6) > Sorry I didn't see your previous comment about this change. Would it look good if I totally remove the header dependency looking like the following? > This command has a bit more information: https://bugs.webkit.org/show_bug.cgi?id=82039#c6 In terms of correctness, I feel that if we use WTF, we should formally depend on it and compile files as needed. Alternatively, if we only include config.h for two #ifdefs and use stdlib/non-WTF in the rest of the file, I'd be inclined to get rid of all WTF-usage altogether. The Chromium port for Android currently is the only gyp-using environment where platform!=host. Building WTF (including ICU) for host is trivial in comparison to the whole compile and the bot is already (one of) the fastest on the waterfall, so the costs of either solution is rather low. (In reply to comment #7) > Sorry Peter. I should have given you a chance to comment on this bug. Do you think we should revert the patch while we sort this out? Since it's a small change either way I think we can discuss first. While it only impacts Chrome for Android right now, the change itself is mostly platform agnostic. 640223 9 wangxianzhu 2012-06-03 12:33:48 -0700 (In reply to comment #8) > (In reply to comment #6) > > Sorry I didn't see your previous comment about this change. Would it look good if I totally remove the header dependency looking like the following? > > > > This command has a bit more information: > https://bugs.webkit.org/show_bug.cgi?id=82039#c6 > > In terms of correctness, I feel that if we use WTF, we should formally depend on it and compile files as needed. Alternatively, if we only include config.h for two #ifdefs and use stdlib/non-WTF in the rest of the file, I'd be inclined to get rid of all WTF-usage altogether. > > The Chromium port for Android currently is the only gyp-using environment where platform!=host. Building WTF (including ICU) for host is trivial in comparison to the whole compile and the bot is already (one of) the fastest on the waterfall, so the costs of either solution is rather low. > The actual dependency on compiled WTF has been removed in https://bugs.webkit.org/show_bug.cgi?id=69997. For now the only dependency is the definition of OS(). My concern about WTF (and icu) dependency is that if in the future WTF depends on some new module, we need to modify the gyp of the new module to compile it for host on Android, which might be trivial or non-trivial, while we actually don't need any code in the module. As with the current patch, ImageDiff.cpp doesn't depend on any compiled code of WTF, and I don't think the OS() macro would have any dependency on compiled code of WTF in the future, I'd suggest to keep the current submitted code. I'd also accept to remove WTF from include_dirs to ensure correctness. I overlooked bug 82039 because bug 85897 did almost the same thing. This bug is a follow-up of bug 85897#8 and 85897#10. > (In reply to comment #7) > > Sorry Peter. I should have given you a chance to comment on this bug. Do you think we should revert the patch while we sort this out? > > Since it's a small change either way I think we can discuss first. While it only impacts Chrome for Android right now, the change itself is mostly platform agnostic. 642466 10 peter 2012-06-06 07:53:32 -0700 (In reply to comment #9) > The actual dependency on compiled WTF has been removed in https://bugs.webkit.org/show_bug.cgi?id=69997. For now the only dependency is the definition of OS(). My concern about WTF (and icu) dependency is that if in the future WTF depends on some new module, we need to modify the gyp of the new module to compile it for host on Android, which might be trivial or non-trivial, while we actually don't need any code in the module. > > As with the current patch, ImageDiff.cpp doesn't depend on any compiled code of WTF, and I don't think the OS() macro would have any dependency on compiled code of WTF in the future, I'd suggest to keep the current submitted code. I'd also accept to remove WTF from include_dirs to ensure correctness. > > I overlooked bug 82039 because bug 85897 did almost the same thing. This bug is a follow-up of bug 85897#8 and 85897#10. > Sorry if I'm not clear, to summarize my opinion: if we use anything from WTF I think we should formally depend on it, and not just pull in the header files through gyp's include_paths. As such, I'd be in favor of applying the patch from comment 6. 642520 11 wangxianzhu 2012-06-06 09:01:17 -0700 (In reply to comment #10) > (In reply to comment #9) > > The actual dependency on compiled WTF has been removed in https://bugs.webkit.org/show_bug.cgi?id=69997. For now the only dependency is the definition of OS(). My concern about WTF (and icu) dependency is that if in the future WTF depends on some new module, we need to modify the gyp of the new module to compile it for host on Android, which might be trivial or non-trivial, while we actually don't need any code in the module. > > > > As with the current patch, ImageDiff.cpp doesn't depend on any compiled code of WTF, and I don't think the OS() macro would have any dependency on compiled code of WTF in the future, I'd suggest to keep the current submitted code. I'd also accept to remove WTF from include_dirs to ensure correctness. > > > > I overlooked bug 82039 because bug 85897 did almost the same thing. This bug is a follow-up of bug 85897#8 and 85897#10. > > > > Sorry if I'm not clear, to summarize my opinion: if we use anything from WTF I think we should formally depend on it, and not just pull in the header files through gyp's include_paths. As such, I'd be in favor of applying the patch from comment 6. Thanks for your explanation. I knew your point and just wanted to confirm if you could accept the only dependency of OS() macro :) Filed bug 88422 to remove the header and include_dirs dependency. 145405 2012-06-01 16:38:53 -0700 2012-06-01 19:47:57 -0700 patch 88147 text/plain 4372 wangxianzhu SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxMTkyOTkpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDEyIEBACisyMDEyLTA2LTAxICBYaWFuemh1IFdhbmcgIDx3YW5n eGlhbnpodUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmVtb3ZlIGRlcGVuZGVuY3kgZnJvbSBJ bWFnZURpZmYgdG8gV1RGCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD04ODE0NworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgICogV1RGLmd5cC9XVEYuZ3lwOgorCiAyMDEyLTA2LTAxICBUb3IgQXJuZSBWZXN0YsO4 ICA8dG9yLmFybmUudmVzdGJvQG5va2lhLmNvbT4KIAogICAgICAgICBbUXRdIFNhdmUgb25lIGNv cHkgd2hlbiBnb2luZyBmcm9tIDgtYml0IFdURjo6U3RyaW5nIHRvIFFTdHJpbmcKSW5kZXg6IFNv dXJjZS9XVEYvV1RGLmd5cC9XVEYuZ3lwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvV1RGLmd5 cC9XVEYuZ3lwCShyZXZpc2lvbiAxMTkyOTkpCisrKyBTb3VyY2UvV1RGL1dURi5neXAvV1RGLmd5 cAkod29ya2luZyBjb3B5KQpAQCAtOTQsMTMgKzk0LDYgQEAKICAgICAgICAgICB9XSwKICAgICAg ICAgXSwKICAgICAgIH0sCi0gICAgICAnY29uZGl0aW9ucyc6IFsKLSAgICAgICAgWydPUz09ImFu ZHJvaWQiIGFuZCBhbmRyb2lkX2J1aWxkX3R5cGU9PTAnLCB7Ci0gICAgICAgICAgIyBBbmRyb2lk IGJ1aWxkcyBJbWFnZURpZmYgZm9yIGhvc3QsIHdoaWNoIGhhcyBhIGRlcGVuZGVuY3kgb24gd3Rm LgotICAgICAgICAgICMgVGhhdCBtZWFucyB3ZSBuZWVkIHRvIGJ1aWxkIHRoaXMgdGFyZ2V0IGZv ciBib3RoIGhvc3QgYW5kIHRhcmdldC4KLSAgICAgICAgICAndG9vbHNldHMnOiBbJ2hvc3QnLCAn dGFyZ2V0J10sCi0gICAgICAgIH1dLAotICAgICAgXQogICAgIH0sCiAgICAgewogICAgICAgJ3Rh cmdldF9uYW1lJzogJ3d0ZicsCkBAIC0xODksMTEgKzE4Miw2IEBACiAgICAgICAgICAgICB9XSwK ICAgICAgICAgICBdLAogICAgICAgICB9XSwKLSAgICAgICAgWydPUz09ImFuZHJvaWQiIGFuZCBh bmRyb2lkX2J1aWxkX3R5cGU9PTAnLCB7Ci0gICAgICAgICAgIyBBbmRyb2lkIGJ1aWxkcyBJbWFn ZURpZmYgZm9yIGhvc3QsIHdoaWNoIGhhcyBhIGRlcGVuZGVuY3kgb24gd3RmLgotICAgICAgICAg ICMgVGhhdCBtZWFucyB3ZSBuZWVkIHRvIGJ1aWxkIHRoaXMgdGFyZ2V0IGZvciBib3RoIGhvc3Qg YW5kIHRhcmdldC4KLSAgICAgICAgICAndG9vbHNldHMnOiBbJ2hvc3QnLCAndGFyZ2V0J10sCi0g ICAgICAgIH1dLAogICAgICAgXSwKICAgICB9LAogICBdCkluZGV4OiBUb29scy9DaGFuZ2VMb2cK PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PQotLS0gVG9vbHMvQ2hhbmdlTG9nCShyZXZpc2lvbiAxMTkyOTkpCisrKyBUb29s cy9DaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAgLTEsMyArMSwxMiBAQAorMjAxMi0wNi0wMSAg WGlhbnpodSBXYW5nICA8d2FuZ3hpYW56aHVAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJlbW92 ZSBkZXBlbmRlbmN5IGZyb20gSW1hZ2VEaWZmIHRvIFdURgorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODgxNDcKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIER1bXBSZW5kZXJUcmVlL0R1bXBSZW5kZXJUcmVl Lmd5cC9EdW1wUmVuZGVyVHJlZS5neXA6CisKIDIwMTItMDYtMDEgIERpcmsgUHJhbmtlICA8ZHBy YW5rZUBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgbnJ3dCBzZWVtcyB0byBsZWFrIHRtcGRpcnMK SW5kZXg6IFRvb2xzL0R1bXBSZW5kZXJUcmVlL0R1bXBSZW5kZXJUcmVlLmd5cC9EdW1wUmVuZGVy VHJlZS5neXAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gVG9vbHMvRHVtcFJlbmRlclRyZWUvRHVtcFJlbmRlclRy ZWUuZ3lwL0R1bXBSZW5kZXJUcmVlLmd5cAkocmV2aXNpb24gMTE5Mjk5KQorKysgVG9vbHMvRHVt cFJlbmRlclRyZWUvRHVtcFJlbmRlclRyZWUuZ3lwL0R1bXBSZW5kZXJUcmVlLmd5cAkod29ya2lu ZyBjb3B5KQpAQCAtNTYsMjEgKzU2LDIzIEBACiAgICAgICAgICAgICAndHlwZSc6ICdleGVjdXRh YmxlJywKICAgICAgICAgICAgICdkZXBlbmRlbmNpZXMnOiBbCiAgICAgICAgICAgICAgICAgJzwo Y2hyb21pdW1fc3JjX2Rpcikvd2Via2l0L3N1cHBvcnQvd2Via2l0X3N1cHBvcnQuZ3lwOndlYmtp dF9zdXBwb3J0X2dmeCcsCi0gICAgICAgICAgICAgICAgJzwoc291cmNlX2RpcikvV1RGL1dURi5n eXAvV1RGLmd5cDp3dGYnLAogICAgICAgICAgICAgXSwKICAgICAgICAgICAgICdpbmNsdWRlX2Rp cnMnOiBbCi0gICAgICAgICAgICAgICAgJzwoc291cmNlX2RpcikvSmF2YVNjcmlwdENvcmUnLAor ICAgICAgICAgICAgICAgICc8KHNvdXJjZV9kaXIpL1dURicsCiAgICAgICAgICAgICAgICAgJzwo REVQVEgpJywKICAgICAgICAgICAgIF0sCiAgICAgICAgICAgICAnc291cmNlcyc6IFsKICAgICAg ICAgICAgICAgICAnPCh0b29sc19kaXIpL0R1bXBSZW5kZXJUcmVlL2Nocm9taXVtL0ltYWdlRGlm Zi5jcHAnLAogICAgICAgICAgICAgXSwKICAgICAgICAgICAgICdjb25kaXRpb25zJzogWwotICAg ICAgICAgICAgICAgIFsnT1M9PSJhbmRyb2lkIicsIHsKKyAgICAgICAgICAgICAgICBbJ09TPT0i YW5kcm9pZCIgYW5kIGFuZHJvaWRfYnVpbGRfdHlwZT09MCcsIHsKICAgICAgICAgICAgICAgICAg ICAgIyBUaGUgQ2hyb21pdW0gQW5kcm9pZCBwb3J0IHdpbGwgY29tcGFyZSBpbWFnZXMgb24gaG9z dCByYXRoZXIKICAgICAgICAgICAgICAgICAgICAgIyB0aGFuIHRhcmdldCAoYSBkZXZpY2Ugb3Ig ZW11bGF0b3IpIGZvciBwZXJmb3JtYW5jZSByZWFzb25zLgogICAgICAgICAgICAgICAgICAgICAn dG9vbHNldHMnOiBbJ2hvc3QnXSwKICAgICAgICAgICAgICAgICB9XSwKKyAgICAgICAgICAgICAg ICBbJ09TPT0iYW5kcm9pZCIgYW5kIGFuZHJvaWRfYnVpbGRfdHlwZSE9MCcsIHsKKyAgICAgICAg ICAgICAgICAgICAgJ3R5cGUnOiAnbm9uZScsCisgICAgICAgICAgICAgICAgfV0sCiAgICAgICAg ICAgICBdLAogICAgICAgICB9LAogICAgICAgICB7CkBAIC0yNDMsNyArMjQ1LDYgQEAKICAgICAg ICAgICAgICAgICBbJ09TPT0iYW5kcm9pZCInLCB7CiAgICAgICAgICAgICAgICAgICAgICd0eXBl JzogJ3NoYXJlZF9saWJyYXJ5JywKICAgICAgICAgICAgICAgICAgICAgJ2RlcGVuZGVuY2llcyc6 IFsKLSAgICAgICAgICAgICAgICAgICAgICAgICdJbWFnZURpZmYjaG9zdCcsCiAgICAgICAgICAg ICAgICAgICAgICAgICAnPChjaHJvbWl1bV9zcmNfZGlyKS9iYXNlL2Jhc2UuZ3lwOnRlc3Rfc3Vw cG9ydF9iYXNlJywKICAgICAgICAgICAgICAgICAgICAgICAgICc8KGNocm9taXVtX3NyY19kaXIp L3Rvb2xzL2FuZHJvaWQvZm9yd2FyZGVyL2ZvcndhcmRlci5neXA6Zm9yd2FyZGVyJywKICAgICAg ICAgICAgICAgICAgICAgICAgICc8KGNocm9taXVtX3NyY19kaXIpL3Rlc3RpbmcvYW5kcm9pZC9u YXRpdmVfdGVzdC5neXA6bmF0aXZlX3Rlc3RfbmF0aXZlX2NvZGUnLApAQCAtMjY1LDYgKzI2Niwx MSBAQAogICAgICAgICAgICAgICAgICAgICAgICAgWydleGNsdWRlJywgJ0FuZHJvaWRcXC5jcHAk J10sCiAgICAgICAgICAgICAgICAgICAgIF0sCiAgICAgICAgICAgICAgICAgfV0sCisgICAgICAg ICAgICAgICAgWydPUz09ImFuZHJvaWQiIGFuZCBhbmRyb2lkX2J1aWxkX3R5cGU9PTAnLCB7Cisg ICAgICAgICAgICAgICAgICAgICdkZXBlbmRlbmNpZXMnOiBbCisgICAgICAgICAgICAgICAgICAg ICAgICAnSW1hZ2VEaWZmI2hvc3QnLAorICAgICAgICAgICAgICAgICAgICBdLAorICAgICAgICAg ICAgICAgIH1dLAogICAgICAgICAgICAgICAgIFsnaW5zaWRlX2Nocm9taXVtX2J1aWxkPT0xIGFu ZCBjb21wb25lbnQ9PSJzaGFyZWRfbGlicmFyeSInLCB7CiAgICAgICAgICAgICAgICAgICAgICdz b3VyY2VzJzogWwogICAgICAgICAgICAgICAgICAgICAgICAgJzwoc291cmNlX2RpcikvV2ViS2l0 L2Nocm9taXVtL3NyYy9DaHJvbWl1bUN1cnJlbnRUaW1lLmNwcCcsCg==