Bug 82039

Summary: [Chromium] ImageDiff should be build for host on Android
Product: WebKit Reporter: Peter Beverloo <peter>
Component: Tools / TestsAssignee: Wei James (wistoch) <james.wei>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, james.wei, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 84843    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Peter Beverloo
Reported 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.
Attachments
Patch (3.36 KB, patch)
2012-05-14 00:12 PDT, Wei James (wistoch) 		no flags
DetailsFormatted DiffDiff
Patch (2.26 KB, patch)
2012-05-14 00:37 PDT, Wei James (wistoch) 		no flags
DetailsFormatted DiffDiff
Patch (3.56 KB, patch)
2012-05-14 05:59 PDT, Wei James (wistoch) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2012-05-08 22:35:08 PDT
Is this still needed in light of https://bugs.webkit.org/show_bug.cgi?id=85897#c8 ?
Wei James (wistoch)
Comment 2 2012-05-14 00:12:43 PDT
Created attachment 141658 [details] Patch
Wei James (wistoch)
Comment 3 2012-05-14 00:37:03 PDT
Created attachment 141659 [details] Patch
Wei James (wistoch)
Comment 4 2012-05-14 00:37:40 PDT
*** Bug 86342 has been marked as a duplicate of this bug. ***
Wei James (wistoch)
Comment 5 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
Peter Beverloo
Comment 6 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.
Wei James (wistoch)
Comment 7 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
Wei James (wistoch)
Comment 8 2012-05-14 05:59:52 PDT
Created attachment 141705 [details] Patch
Peter Beverloo
Comment 9 2012-05-14 06:08:51 PDT
LGTM, thank you for doing this! Adam, could you take a look for a formal r+ please?
Adam Barth
Comment 10 2012-05-14 11:06:48 PDT
Comment on attachment 141705 [details] Patch My pleasure. Thanks for driving this to completion.!
WebKit Review Bot
Comment 11 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>
WebKit Review Bot
Comment 12 2012-05-14 11:26:06 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.