Bug 82039 – [Chromium] ImageDiff should be build for host on Android

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Peter Beverloo]

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.

[Adam Barth]

Is this still needed in light of https://bugs.webkit.org/show_bug.cgi?id=85897#c8 ?

[Wei James (wistoch)]

Created attachment 141658 [details]
Patch

[Wei James (wistoch)]

Created attachment 141659 [details]
Patch

[Wei James (wistoch)]

*** Bug 86342 has been marked as a duplicate of this bug. ***

[Wei James (wistoch)]

(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 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 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)]

Created attachment 141705 [details]
Patch

[Peter Beverloo]

LGTM, thank you for doing this!

Adam, could you take a look for a formal r+ please?

[Adam Barth]

Comment on attachment 141705 [details]
Patch

My pleasure.  Thanks for driving this to completion.!

[WebKit Review Bot]

Comment on attachment 141705 [details]
Patch

Clearing flags on attachment: 141705

Committed r116973: <http://trac.webkit.org/changeset/116973>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.