Bug 88422

Created attachment 146077 [details] patch

Would you like this landed via the commit-queue?

Comment on attachment 146077 [details] patch (In reply to comment #2) > Would you like this landed via the commit-queue? Yes, I'm doing that. Thanks for review.

LGTM as well, thank you very much for doing this!

Comment on attachment 146077 [details] patch Clearing flags on attachment: 146077 Committed r119662: <http://trac.webkit.org/changeset/119662>

All reviewed patches have been landed. Closing bug.

This broke builds. Rolling it out. We're having too many build failures at the moment.

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

Re-opened since this is blocked by 88487

Comment on attachment 146077 [details] 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?

(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.

(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.

Created attachment 146318 [details] 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 :)

Comment on attachment 146318 [details] 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.

(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.

(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

Comment on attachment 146318 [details] patch v2 Clearing flags on attachment: 146318 Committed r119781: <http://trac.webkit.org/changeset/119781>

All reviewed patches have been landed. Closing bug.