88422 2012-06-06 08:59:34 -0700 [Chromium] Remove header dependency of ImageDiff to WTF 2012-06-07 18:36:12 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 88487 1 wangxianzhu wangxianzhu abarth peter rniwa tkent webkit.review.bot oldest_to_newest 642515 0 wangxianzhu 2012-06-06 08:59:34 -0700 As discussed in bug 88147, we should also remove header dependency of ImageDiff to WTF. Proposed patch is in #6 of bug 88147. Will create a formal patch soon. 642652 1 146077 wangxianzhu 2012-06-06 11:42:42 -0700 Created attachment 146077 patch 642813 2 abarth 2012-06-06 14:28:04 -0700 Would you like this landed via the commit-queue? 642814 3 146077 wangxianzhu 2012-06-06 14:30:07 -0700 Comment on attachment 146077 patch (In reply to comment #2) > Would you like this landed via the commit-queue? Yes, I'm doing that. Thanks for review. 642815 4 peter 2012-06-06 14:30:37 -0700 LGTM as well, thank you very much for doing this! 643072 5 146077 webkit.review.bot 2012-06-06 18:46:55 -0700 Comment on attachment 146077 patch Clearing flags on attachment: 146077 Committed r119662: <http://trac.webkit.org/changeset/119662> 643073 6 webkit.review.bot 2012-06-06 18:47:04 -0700 All reviewed patches have been landed. Closing bug. 643124 7 rniwa 2012-06-06 20:11:33 -0700 This broke builds. Rolling it out. We're having too many build failures at the moment. 643126 8 rniwa 2012-06-06 20:12:10 -0700 http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder%20%28dbg%29/builds/24712/steps/compile/logs/stdio 3>..\chromium\ImageDiff.cpp(347) : error C3861: 'strtok_r': identifier not found 3>..\chromium\ImageDiff.cpp(348) : error C3861: 'strtok_r': identifier not found 3>..\chromium\ImageDiff.cpp(485) : error C2065: 'PATH_MAX' : undeclared identifier 3>..\chromium\ImageDiff.cpp(486) : error C2057: expected constant expression 3>..\chromium\ImageDiff.cpp(486) : error C2466: cannot allocate an array of constant size 0 3>..\chromium\ImageDiff.cpp(486) : error C2133: 'stdinBuffer' : unknown size 3>..\chromium\ImageDiff.cpp(487) : error C2057: expected constant expression 3>..\chromium\ImageDiff.cpp(487) : error C2466: cannot allocate an array of constant size 0 3>..\chromium\ImageDiff.cpp(487) : error C2133: 'firstName' : unknown size 643128 9 webkit.review.bot 2012-06-06 20:13:03 -0700 Re-opened since this is blocked by 88487 643306 10 146077 rniwa 2012-06-07 01:08:44 -0700 Comment on attachment 146077 patch View in context: https://bugs.webkit.org/attachment.cgi?id=146077&action=review > Tools/DumpRenderTree/chromium/ImageDiff.cpp:45 > -#if OS(WINDOWS) > +#if defined(OS_WINDOWS) Why don't we just use defined(WIN32) || defined(_WIN32) instead? 643603 11 wangxianzhu 2012-06-07 09:12:45 -0700 (In reply to comment #10) > (From update of attachment 146077 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146077&action=review > > > Tools/DumpRenderTree/chromium/ImageDiff.cpp:45 > > -#if OS(WINDOWS) > > +#if defined(OS_WINDOWS) > > Why don't we just use defined(WIN32) || defined(_WIN32) instead? I think both are OK. "#if defined(OS_WINDOWS)" is defined by chromium gyp and widely used in chromium code. 643605 12 wangxianzhu 2012-06-07 09:14:36 -0700 (In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 146077 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=146077&action=review > > > > > Tools/DumpRenderTree/chromium/ImageDiff.cpp:45 > > > -#if OS(WINDOWS) > > > +#if defined(OS_WINDOWS) > > > > Why don't we just use defined(WIN32) || defined(_WIN32) instead? > > I think both are OK. "#if defined(OS_WINDOWS)" is defined by chromium gyp and widely used in chromium code. Sorry, I didn't notice that the break might caused by it. Will investigate it. 643675 13 146318 wangxianzhu 2012-06-07 10:37:40 -0700 Created attachment 146318 patch v2 Sorry for the mistake. I thought OS_xxxs were defined in gyp but actually they are defined in chromium's build/build_config.h. Even they were useable, there is no OS_WINDOWS but OS_WIN. Changed to use _WIN32. Untested on chromium-win. Hope it work :) 643882 14 146318 rniwa 2012-06-07 14:04:35 -0700 Comment on attachment 146318 patch v2 (In reply to comment #13) > Sorry for the mistake. I thought OS_xxxs were defined in gyp but actually they are defined in chromium's build/build_config.h. Even they were useable, there is no OS_WINDOWS but OS_WIN. !? _WIN32 is normally defined by MSVC unless you specify otherwise. > Changed to use _WIN32. Untested on chromium-win. Hope it work :) Given that this change is mostly about Chromium Windows, I don't think it's okay not to test it on Windows prior to landing. Please ask someone around you to test this patch. I'll r- this patch for now because this patch is landable until we confirm that it builds on Chromium Windows. 643885 15 rniwa 2012-06-07 14:05:25 -0700 (In reply to comment #14) > I'll r- this patch for now because this patch is landable until we confirm that it builds on Chromium Windows. NOT landable. I'll revert my r- when the author confirms that the patch doesn't break builds. 644020 16 wangxianzhu 2012-06-07 15:59:38 -0700 (In reply to comment #14) > (From update of attachment 146318 [details]) > (In reply to comment #13) > > Sorry for the mistake. I thought OS_xxxs were defined in gyp but actually they are defined in chromium's build/build_config.h. Even they were useable, there is no OS_WINDOWS but OS_WIN. > > !? _WIN32 is normally defined by MSVC unless you specify otherwise. I meant my misused OS_WINDOWS macro. I should use _WIN32. (In reply to comment #15) > (In reply to comment #14) > > I'll r- this patch for now because this patch is landable until we confirm that it builds on Chromium Windows. > > NOT landable. I'll revert my r- when the author confirms that the patch doesn't break builds. Tried the patch on Chromium try bot (win). Not finished yet, but the current result shows that ImageDiff.cpp has been successfully compiled and linked: http://build.chromium.org/p/tryserver.chromium/builders/win/builds/14831/steps/compile/logs/stdio 644184 17 146318 webkit.review.bot 2012-06-07 18:36:07 -0700 Comment on attachment 146318 patch v2 Clearing flags on attachment: 146318 Committed r119781: <http://trac.webkit.org/changeset/119781> 644185 18 webkit.review.bot 2012-06-07 18:36:12 -0700 All reviewed patches have been landed. Closing bug. 146077 2012-06-06 11:42:42 -0700 2012-06-07 10:37:40 -0700 patch 88422 text/plain 2237 wangxianzhu SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDExOTYwMCkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE0IEBACisyMDEyLTA2LTA2ICBYaWFuemh1IFdhbmcgIDx3YW5neGlhbnpodUBjaHJvbWl1 bS5vcmc+CisKKyAgICAgICAgW0Nocm9taXVtXSBSZW1vdmUgaGVhZGVyIGRlcGVuZGVuY3kgb2Yg SW1hZ2VEaWZmIHRvIFdURgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9ODg0MjIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICAqIER1bXBSZW5kZXJUcmVlL0R1bXBSZW5kZXJUcmVlLmd5cC9EdW1wUmVuZGVyVHJl ZS5neXA6CisgICAgICAgICogRHVtcFJlbmRlclRyZWUvY2hyb21pdW0vSW1hZ2VEaWZmLmNwcDoK KyAgICAgICAgKHVudGVzdGVkQ29tcGFyZUltYWdlcyk6CisKIDIwMTItMDYtMDYgIFJvYmVydCBL cm9lZ2VyICA8cmprcm9lZ2VAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIEFkZCBSb2JlcnQgS3Jv ZWdlciAgdG8gY29tbWl0ZXJzLnB5CkluZGV4OiBUb29scy9EdW1wUmVuZGVyVHJlZS9EdW1wUmVu ZGVyVHJlZS5neXAvRHVtcFJlbmRlclRyZWUuZ3lwCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFRvb2xzL0R1bXBS ZW5kZXJUcmVlL0R1bXBSZW5kZXJUcmVlLmd5cC9EdW1wUmVuZGVyVHJlZS5neXAJKHJldmlzaW9u IDExOTU5NCkKKysrIFRvb2xzL0R1bXBSZW5kZXJUcmVlL0R1bXBSZW5kZXJUcmVlLmd5cC9EdW1w UmVuZGVyVHJlZS5neXAJKHdvcmtpbmcgY29weSkKQEAgLTU4LDcgKzU4LDYgQEAKICAgICAgICAg ICAgICAgICAnPChjaHJvbWl1bV9zcmNfZGlyKS93ZWJraXQvc3VwcG9ydC93ZWJraXRfc3VwcG9y dC5neXA6d2Via2l0X3N1cHBvcnRfZ2Z4JywKICAgICAgICAgICAgIF0sCiAgICAgICAgICAgICAn aW5jbHVkZV9kaXJzJzogWwotICAgICAgICAgICAgICAgICc8KHNvdXJjZV9kaXIpL1dURicsCiAg ICAgICAgICAgICAgICAgJzwoREVQVEgpJywKICAgICAgICAgICAgIF0sCiAgICAgICAgICAgICAn c291cmNlcyc6IFsKSW5kZXg6IFRvb2xzL0R1bXBSZW5kZXJUcmVlL2Nocm9taXVtL0ltYWdlRGlm Zi5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gVG9vbHMvRHVtcFJlbmRlclRyZWUvY2hyb21pdW0vSW1hZ2VE aWZmLmNwcAkocmV2aXNpb24gMTE5NTk0KQorKysgVG9vbHMvRHVtcFJlbmRlclRyZWUvY2hyb21p dW0vSW1hZ2VEaWZmLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMzQsOCArMzQsNiBAQAogLy8gVGhl IGV4YWN0IGZvcm1hdCBvZiB0aGlzIHRvb2wncyBvdXRwdXQgdG8gc3Rkb3V0IGlzIGltcG9ydGFu dCwgdG8gbWF0Y2gKIC8vIHdoYXQgdGhlIHJ1bi13ZWJraXQtdGVzdHMgc2NyaXB0IGV4cGVjdHMu CiAKLSNpbmNsdWRlICJjb25maWcuaCIKLQogI2luY2x1ZGUgIndlYmtpdC9zdXBwb3J0L3dlYmtp dF9zdXBwb3J0X2dmeC5oIgogI2luY2x1ZGUgPGFsZ29yaXRobT4KICNpbmNsdWRlIDxpdGVyYXRv cj4KQEAgLTQ0LDcgKzQyLDcgQEAKICNpbmNsdWRlIDxzdHJpbmcuaD4KICNpbmNsdWRlIDx2ZWN0 b3I+CiAKLSNpZiBPUyhXSU5ET1dTKQorI2lmIGRlZmluZWQoT1NfV0lORE9XUykKICNpbmNsdWRl IDx3aW5kb3dzLmg+CiAjZGVmaW5lIFBBVEhfTUFYIE1BWF9QQVRICiAjZW5kaWYKQEAgLTM0Miw3 ICszNDAsNyBAQCBpbnQgdW50ZXN0ZWRDb21wYXJlSW1hZ2VzKEltYWdlQ29tcGFyaXNvCiAgICAg d2hpbGUgKGZnZXRzKGJ1ZmZlciwgc2l6ZW9mKGJ1ZmZlciksIHN0ZGluKSkgewogICAgICAgICBp ZiAoIXN0cm5jbXAoIkNvbnRlbnQtbGVuZ3RoOiAiLCBidWZmZXIsIDE2KSkgewogICAgICAgICAg ICAgY2hhciogY29udGV4dDsKLSNpZiBPUyhXSU5ET1dTKQorI2lmIGRlZmluZWQoT1NfV0lORE9X UykKICAgICAgICAgICAgIHN0cnRva19zKGJ1ZmZlciwgIiAiLCAmY29udGV4dCk7CiAgICAgICAg ICAgICBpbnQgaW1hZ2VTaXplID0gc3RydG9sKHN0cnRva19zKDAsICIgIiwgJmNvbnRleHQpLCAw LCAxMCk7CiAjZWxzZQo= 146318 2012-06-07 10:37:40 -0700 2012-06-07 18:36:07 -0700 patch v2 88422a text/plain 2696 wangxianzhu SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDExOTczMikKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE0IEBACisyMDEyLTA2LTA3ICBYaWFuemh1IFdhbmcgIDx3YW5neGlhbnpodUBjaHJvbWl1 bS5vcmc+CisKKyAgICAgICAgW0Nocm9taXVtXSBSZW1vdmUgaGVhZGVyIGRlcGVuZGVuY3kgb2Yg SW1hZ2VEaWZmIHRvIFdURgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9ODg0MjIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICAqIER1bXBSZW5kZXJUcmVlL0R1bXBSZW5kZXJUcmVlLmd5cC9EdW1wUmVuZGVyVHJl ZS5neXA6CisgICAgICAgICogRHVtcFJlbmRlclRyZWUvY2hyb21pdW0vSW1hZ2VEaWZmLmNwcDoK KyAgICAgICAgKHVudGVzdGVkQ29tcGFyZUltYWdlcyk6CisKIDIwMTItMDYtMDcgIERhbmllbCBF cmF0ICA8ZGVyYXRAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFtjaHJvbWl1bV0gQWRkIExheW91 dFRlc3RDb250cm9sbGVyOjpzZXRUZXh0U3VicGl4ZWxQb3NpdGlvbmluZygpIGZvciBMaW51eC4K SW5kZXg6IFRvb2xzL0R1bXBSZW5kZXJUcmVlL0R1bXBSZW5kZXJUcmVlLmd5cC9EdW1wUmVuZGVy VHJlZS5neXAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gVG9vbHMvRHVtcFJlbmRlclRyZWUvRHVtcFJlbmRlclRy ZWUuZ3lwL0R1bXBSZW5kZXJUcmVlLmd5cAkocmV2aXNpb24gMTE5NzMyKQorKysgVG9vbHMvRHVt cFJlbmRlclRyZWUvRHVtcFJlbmRlclRyZWUuZ3lwL0R1bXBSZW5kZXJUcmVlLmd5cAkod29ya2lu ZyBjb3B5KQpAQCAtNTgsNyArNTgsNiBAQAogICAgICAgICAgICAgICAgICc8KGNocm9taXVtX3Ny Y19kaXIpL3dlYmtpdC9zdXBwb3J0L3dlYmtpdF9zdXBwb3J0Lmd5cDp3ZWJraXRfc3VwcG9ydF9n ZngnLAogICAgICAgICAgICAgXSwKICAgICAgICAgICAgICdpbmNsdWRlX2RpcnMnOiBbCi0gICAg ICAgICAgICAgICAgJzwoc291cmNlX2RpcikvV1RGJywKICAgICAgICAgICAgICAgICAnPChERVBU SCknLAogICAgICAgICAgICAgXSwKICAgICAgICAgICAgICdzb3VyY2VzJzogWwpJbmRleDogVG9v bHMvRHVtcFJlbmRlclRyZWUvY2hyb21pdW0vSW1hZ2VEaWZmLmNwcAo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBU b29scy9EdW1wUmVuZGVyVHJlZS9jaHJvbWl1bS9JbWFnZURpZmYuY3BwCShyZXZpc2lvbiAxMTk3 MzIpCisrKyBUb29scy9EdW1wUmVuZGVyVHJlZS9jaHJvbWl1bS9JbWFnZURpZmYuY3BwCSh3b3Jr aW5nIGNvcHkpCkBAIC0zNCw4ICszNCw2IEBACiAvLyBUaGUgZXhhY3QgZm9ybWF0IG9mIHRoaXMg dG9vbCdzIG91dHB1dCB0byBzdGRvdXQgaXMgaW1wb3J0YW50LCB0byBtYXRjaAogLy8gd2hhdCB0 aGUgcnVuLXdlYmtpdC10ZXN0cyBzY3JpcHQgZXhwZWN0cy4KIAotI2luY2x1ZGUgImNvbmZpZy5o IgotCiAjaW5jbHVkZSAid2Via2l0L3N1cHBvcnQvd2Via2l0X3N1cHBvcnRfZ2Z4LmgiCiAjaW5j bHVkZSA8YWxnb3JpdGhtPgogI2luY2x1ZGUgPGl0ZXJhdG9yPgpAQCAtNDQsMTIgKzQyLDEzIEBA CiAjaW5jbHVkZSA8c3RyaW5nLmg+CiAjaW5jbHVkZSA8dmVjdG9yPgogCi0jaWYgT1MoV0lORE9X UykKKyNpZiBkZWZpbmVkKF9XSU4zMikKICNpbmNsdWRlIDx3aW5kb3dzLmg+CiAjZGVmaW5lIFBB VEhfTUFYIE1BWF9QQVRICisjZGVmaW5lIHN0cnRva19yIHN0cnRva19zCiAjZW5kaWYKIAotLy8g RGVmaW5lIG1hY3JvIGhlcmUgdG8gbWFrZSBJbWFnZURpZmYgaW5kZXBlbmRlbnQgb2YgSmF2YVNj cmlwdENvcmUuCisvLyBEZWZpbmUgbWFjcm8gaGVyZSB0byBtYWtlIEltYWdlRGlmZiBpbmRlcGVu ZGVudCBvZiBXVEYuCiAjaWZkZWYgTkRFQlVHCiAjZGVmaW5lIEFTU0VSVChhc3NlcnRpb24pIGRv IHsgfSB3aGlsZSAoMCkKICNlbHNlCkBAIC0zNDIsMTMgKzM0MSw4IEBAIGludCB1bnRlc3RlZENv bXBhcmVJbWFnZXMoSW1hZ2VDb21wYXJpc28KICAgICB3aGlsZSAoZmdldHMoYnVmZmVyLCBzaXpl b2YoYnVmZmVyKSwgc3RkaW4pKSB7CiAgICAgICAgIGlmICghc3RybmNtcCgiQ29udGVudC1sZW5n dGg6ICIsIGJ1ZmZlciwgMTYpKSB7CiAgICAgICAgICAgICBjaGFyKiBjb250ZXh0OwotI2lmIE9T KFdJTkRPV1MpCi0gICAgICAgICAgICBzdHJ0b2tfcyhidWZmZXIsICIgIiwgJmNvbnRleHQpOwot ICAgICAgICAgICAgaW50IGltYWdlU2l6ZSA9IHN0cnRvbChzdHJ0b2tfcygwLCAiICIsICZjb250 ZXh0KSwgMCwgMTApOwotI2Vsc2UKICAgICAgICAgICAgIHN0cnRva19yKGJ1ZmZlciwgIiAiLCAm Y29udGV4dCk7CiAgICAgICAgICAgICBpbnQgaW1hZ2VTaXplID0gc3RydG9sKHN0cnRva19yKDAs ICIgIiwgJmNvbnRleHQpLCAwLCAxMCk7Ci0jZW5kaWYKIAogICAgICAgICAgICAgYm9vbCBzdWNj ZXNzID0gZmFsc2U7CiAgICAgICAgICAgICBpZiAoaW1hZ2VTaXplID4gMCAmJiAhYWN0dWFsSW1h Z2UuaGFzSW1hZ2UoKSkgewo=