Bug 88147 – Remove dependency from ImageDiff to WTF

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Xianzhu Wang]

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.

[Xianzhu Wang]

Created attachment 145405 [details]
patch

[Adam Barth]

Comment on attachment 145405 [details]
patch

Ok.

[WebKit Review Bot]

Comment on attachment 145405 [details]
patch

Clearing flags on attachment: 145405

Committed r119309: <http://trac.webkit.org/changeset/119309>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Peter Beverloo]

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

[Xianzhu Wang]

(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': [

[Adam Barth]

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?

[Peter Beverloo]

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

[Xianzhu Wang]

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

[Peter Beverloo]

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

[Xianzhu Wang]

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