Bug 82039 - [Chromium] ImageDiff should be build for host on Android
Summary: [Chromium] ImageDiff should be build for host on Android
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: Wei James (wistoch)
URL:
Keywords:
: 86342 (view as bug list)
Depends on:
Blocks: 84843
  Show dependency treegraph
 
Reported: 2012-03-23 03:11 PDT by Peter Beverloo
Modified: 2012-05-14 11:26 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.36 KB, patch)
2012-05-14 00:12 PDT, Wei James (wistoch)
no flags Details | Formatted Diff | Diff
Patch (2.26 KB, patch)
2012-05-14 00:37 PDT, Wei James (wistoch)
no flags Details | Formatted Diff | Diff
Patch (3.56 KB, patch)
2012-05-14 05:59 PDT, Wei James (wistoch)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Beverloo 2012-03-23 03:11:31 PDT
The Android bot runs ImageDiff on host rather than target as transferring the pixel results to a device is too expensive. Per the WTF move, the dependency tree got significantly more complicated and various other targets also have to be build for host.

Provide a proper fix and re-enable building ImageDiff for host on Chromium-Android.
Comment 1 Adam Barth 2012-05-08 22:35:08 PDT
Is this still needed in light of https://bugs.webkit.org/show_bug.cgi?id=85897#c8 ?
Comment 2 Wei James (wistoch) 2012-05-14 00:12:43 PDT
Created attachment 141658 [details]
Patch
Comment 3 Wei James (wistoch) 2012-05-14 00:37:03 PDT
Created attachment 141659 [details]
Patch
Comment 4 Wei James (wistoch) 2012-05-14 00:37:40 PDT
*** Bug 86342 has been marked as a duplicate of this bug. ***
Comment 5 Wei James (wistoch) 2012-05-14 00:42:00 PDT
(In reply to comment #1)
> Is this still needed in light of https://bugs.webkit.org/show_bug.cgi?id=85897#c8 ?

adam, I tried to build the ImageDiff without WTF and got it. 

I did some modification for the DumpRenderTree.gyp to remove WTF dependency, include WTF header files and then enable ImageDiff build. 

could you help to review it? thanks
Comment 6 Peter Beverloo 2012-05-14 04:19:53 PDT
Comment on attachment 141659 [details]
Patch

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

Chromium r136197 rolled in ICU changes required for this, which also are available in WebKit now. Do you think that using Adam's patch from Bug 85897 with the addendum of re-enabling the ImageDiff target (the last two of your changes to DumpRenderTree.gyp) would be a better approach?

> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:62
> +                '<(source_dir)/WTF',

This is how ImageDiff previously worked, but just including WTF rather than depending on it does create a risk. If part of the functionality ImageDiff relies on moves out of a .h file to an actual implementation file, linking errors would occur. As such, I'd prefer to depend on WTF if ImageDiff uses anything from WTF.

> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:70
> +                    'toolsets': ['host'],

Should we add a comment here saying why Android only wants to build ImageDiff for host? Something like "The Chromium Android port will compare images on host rather than target (a device or emulator) for performance reasons" may work.
Comment 7 Wei James (wistoch) 2012-05-14 05:56:23 PDT
Comment on attachment 141659 [details]
Patch

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

>> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:62
>> +                '<(source_dir)/WTF',
> 
> This is how ImageDiff previously worked, but just including WTF rather than depending on it does create a risk. If part of the functionality ImageDiff relies on moves out of a .h file to an actual implementation file, linking errors would occur. As such, I'd prefer to depend on WTF if ImageDiff uses anything from WTF.

yes, I agreed with you. my previous patch workes in this way. But I found the comment on #85897 so replace the dependency with the header files. 

I will fix it with your suggestion. thanks

>> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:70
>> +                    'toolsets': ['host'],
> 
> Should we add a comment here saying why Android only wants to build ImageDiff for host? Something like "The Chromium Android port will compare images on host rather than target (a device or emulator) for performance reasons" may work.

fixed. thanks
Comment 8 Wei James (wistoch) 2012-05-14 05:59:52 PDT
Created attachment 141705 [details]
Patch
Comment 9 Peter Beverloo 2012-05-14 06:08:51 PDT
LGTM, thank you for doing this!

Adam, could you take a look for a formal r+ please?
Comment 10 Adam Barth 2012-05-14 11:06:48 PDT
Comment on attachment 141705 [details]
Patch

My pleasure.  Thanks for driving this to completion.!
Comment 11 WebKit Review Bot 2012-05-14 11:25:57 PDT
Comment on attachment 141705 [details]
Patch

Clearing flags on attachment: 141705

Committed r116973: <http://trac.webkit.org/changeset/116973>
Comment 12 WebKit Review Bot 2012-05-14 11:26:06 PDT
All reviewed patches have been landed.  Closing bug.