Bug 88422 – [Chromium] Remove header dependency of ImageDiff to WTF

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Xianzhu Wang]

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.

[Xianzhu Wang]

Created attachment 146077 [details]
patch

[Adam Barth]

Would you like this landed via the commit-queue?

[Xianzhu Wang]

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.

[Peter Beverloo]

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

[WebKit Review Bot]

Comment on attachment 146077 [details]
patch

Clearing flags on attachment: 146077

Committed r119662: <http://trac.webkit.org/changeset/119662>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[WebKit Review Bot]

Re-opened since this is blocked by 88487

[Ryosuke Niwa]

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?

[Xianzhu Wang]

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

[Xianzhu Wang]

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

[Xianzhu Wang]

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 :)

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Xianzhu Wang]

(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

[WebKit Review Bot]

Comment on attachment 146318 [details]
patch v2

Clearing flags on attachment: 146318

Committed r119781: <http://trac.webkit.org/changeset/119781>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.