WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
82039
[Chromium] ImageDiff should be build for host on Android
https://bugs.webkit.org/show_bug.cgi?id=82039
Summary
[Chromium] ImageDiff should be build for host on Android
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
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
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.
Top of Page
Format For Printing
XML
Clone This Bug