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.
Is this still needed in light of https://bugs.webkit.org/show_bug.cgi?id=85897#c8 ?
Created attachment 141658 [details] Patch
Created attachment 141659 [details] Patch
*** Bug 86342 has been marked as a duplicate of this bug. ***
(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 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 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
Created attachment 141705 [details] Patch
LGTM, thank you for doing this! Adam, could you take a look for a formal r+ please?
Comment on attachment 141705 [details] Patch My pleasure. Thanks for driving this to completion.!
Comment on attachment 141705 [details] Patch Clearing flags on attachment: 141705 Committed r116973: <http://trac.webkit.org/changeset/116973>
All reviewed patches have been landed. Closing bug.