Bug 69997 - [Chromium] Reduce dependencies of ImageDiff to compile it for Android.
Summary: [Chromium] Reduce dependencies of ImageDiff to compile it for Android.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 66687 73623
  Show dependency treegraph
 
Reported: 2011-10-12 21:41 PDT by Hao Zheng
Modified: 2011-12-01 22:18 PST (History)
3 users (show)

See Also:


Attachments
Proposed patch. (5.11 KB, patch)
2011-10-17 21:32 PDT, Hao Zheng
tony: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Proposed patch 2. (6.32 KB, patch)
2011-10-18 20:47 PDT, Hao Zheng
no flags Details | Formatted Diff | Diff
Proposed patch 3. (6.37 KB, patch)
2011-10-18 20:49 PDT, Hao Zheng
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Proposed patch 4. (6.34 KB, patch)
2011-10-19 02:08 PDT, Hao Zheng
no flags Details | Formatted Diff | Diff
Proposed patch 5. (6.34 KB, patch)
2011-10-19 19:46 PDT, Hao Zheng
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hao Zheng 2011-10-12 21:41:57 PDT
Make ImageDiff depend on webkit_support_gfx only. Build it as toolsets:host for Android.

This change is required to make ImageDiff for Android, and can benefit other platforms, too. On Android, most targets are built as toolsets:target. while ImageDiff needs to be built as toolsets:host. Currently, building the standalone target of ImageDiff depends on about 85 other targets (try  make ImageDiff'), which is inefficient. After the change, ImageDiff only depends on webkit_support_gfx, which in turn depends on libpng and zlib.

Related to chromium change: http://codereview.chromium.org/8226018/
Comment 1 Hao Zheng 2011-10-17 21:32:42 PDT
Created attachment 111376 [details]
Proposed patch.
Comment 2 WebKit Review Bot 2011-10-18 09:36:06 PDT
Comment on attachment 111376 [details]
Proposed patch.

Rejecting attachment 111376 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
iff] Error 1
make: *** Waiting for unfinished jobs....

Failed to run "['Tools/Scripts/build-webkit', '--debug', '--chromium', '--update-chromium']" exit_code: 2
target/ImageDiff/Tools/DumpRenderTree/chromium/ImageDiff.o: in function Image::setPixelAt(int, int, unsigned int) const:Tools/DumpRenderTree/chromium/ImageDiff.cpp:153: error: undefined reference to 'WTFReportBacktrace'
collect2: ld returned 1 exit status
make: *** [out/Debug/ImageDiff] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/10124291
Comment 3 Hao Zheng 2011-10-18 20:47:37 PDT
Created attachment 111557 [details]
Proposed patch 2.

Sorry, I didn't try Debug build. Now I completely removed code dependent on JavaScriptCore.
Comment 4 Hao Zheng 2011-10-18 20:49:40 PDT
Created attachment 111558 [details]
Proposed patch 3.
Comment 5 WebKit Review Bot 2011-10-18 20:51:33 PDT
Attachment 111557 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Tools/DumpRenderTree/chromium/ImageDiff.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 WebKit Review Bot 2011-10-18 20:55:30 PDT
Comment on attachment 111558 [details]
Proposed patch 3.

Attachment 111558 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10142236
Comment 7 Hao Zheng 2011-10-19 02:08:05 PDT
Created attachment 111580 [details]
Proposed patch 4.
Comment 8 Tony Chang 2011-10-19 09:52:40 PDT
Comment on attachment 111580 [details]
Proposed patch 4.

View in context: https://bugs.webkit.org/attachment.cgi?id=111580&action=review

I think we should just depend on wtf because it's pretty small.  Or are you trying to avoid having host and target in JavaScriptCore.gyp?

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:51
> +#include <limits.h>

I would just include this on all platforms if a single platform needs it.
Comment 9 Hao Zheng 2011-10-19 19:46:03 PDT
Created attachment 111711 [details]
Proposed patch 5.
Comment 10 Hao Zheng 2011-10-19 19:48:12 PDT
(In reply to comment #8)
> (From update of attachment 111580 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111580&action=review
> 
> I think we should just depend on wtf because it's pretty small.  Or are you trying to avoid having host and target in JavaScriptCore.gyp?

No, it's because wtf would depend on icu, which in turn would depend on others...

> 
> > Tools/DumpRenderTree/chromium/ImageDiff.cpp:51
> > +#include <limits.h>
> 
> I would just include this on all platforms if a single platform needs it.

Done.
Comment 11 WebKit Review Bot 2011-10-20 10:45:29 PDT
Comment on attachment 111711 [details]
Proposed patch 5.

Clearing flags on attachment: 111711

Committed r97998: <http://trac.webkit.org/changeset/97998>
Comment 12 WebKit Review Bot 2011-10-20 10:45:35 PDT
All reviewed patches have been landed.  Closing bug.