Bug 88422 - [Chromium] Remove header dependency of ImageDiff to WTF
Summary: [Chromium] Remove header dependency of ImageDiff to WTF
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xianzhu Wang
URL:
Keywords:
Depends on: 88487
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-06 08:59 PDT by Xianzhu Wang
Modified: 2012-06-07 18:36 PDT (History)
5 users (show)

See Also:


Attachments
patch (2.18 KB, patch)
2012-06-06 11:42 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
patch v2 (2.63 KB, patch)
2012-06-07 10:37 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xianzhu Wang 2012-06-06 08:59:34 PDT
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.
Comment 1 Xianzhu Wang 2012-06-06 11:42:42 PDT
Created attachment 146077 [details]
patch
Comment 2 Adam Barth 2012-06-06 14:28:04 PDT
Would you like this landed via the commit-queue?
Comment 3 Xianzhu Wang 2012-06-06 14:30:07 PDT
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.
Comment 4 Peter Beverloo 2012-06-06 14:30:37 PDT
LGTM as well, thank you very much for doing this!
Comment 5 WebKit Review Bot 2012-06-06 18:46:55 PDT
Comment on attachment 146077 [details]
patch

Clearing flags on attachment: 146077

Committed r119662: <http://trac.webkit.org/changeset/119662>
Comment 6 WebKit Review Bot 2012-06-06 18:47:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Ryosuke Niwa 2012-06-06 20:11:33 PDT
This broke builds. Rolling it out. We're having too many build failures at the moment.
Comment 8 Ryosuke Niwa 2012-06-06 20:12:10 PDT
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
Comment 9 WebKit Review Bot 2012-06-06 20:13:03 PDT
Re-opened since this is blocked by 88487
Comment 10 Ryosuke Niwa 2012-06-07 01:08:44 PDT
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?
Comment 11 Xianzhu Wang 2012-06-07 09:12:45 PDT
(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.
Comment 12 Xianzhu Wang 2012-06-07 09:14:36 PDT
(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.
Comment 13 Xianzhu Wang 2012-06-07 10:37:40 PDT
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 14 Ryosuke Niwa 2012-06-07 14:04:35 PDT
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.
Comment 15 Ryosuke Niwa 2012-06-07 14:05:25 PDT
(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.
Comment 16 Xianzhu Wang 2012-06-07 15:59:38 PDT
(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 17 WebKit Review Bot 2012-06-07 18:36:07 PDT
Comment on attachment 146318 [details]
patch v2

Clearing flags on attachment: 146318

Committed r119781: <http://trac.webkit.org/changeset/119781>
Comment 18 WebKit Review Bot 2012-06-07 18:36:12 PDT
All reviewed patches have been landed.  Closing bug.