Summary: | base port needs to implement diff_image | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
Component: | Tools / Tests | Assignee: | Hayato Ito <hayato> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, eric, hayato, ojan, webkit.review.bot | ||||||||
Priority: | P1 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2011-11-29 17:37:11 PST
A quick fix might be simply skipping writing the result of image_diff on mac port. Skipping writing result doesn't affect the result of tests. Later we should implement image_diff on mac port. Any thought? Created attachment 117096 [details]
Patch
As far as I know, chromium is the only port that implements diff_image. Comment on attachment 117096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117096&action=review Is it really hard to implement diff_image for other ports? I'd rather not leave it broken for other ports. > Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:74 > + # Mac port returns 'None' since it doesn't implement diff_image. This should say non-chromium port. > Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:75 > + if image_diff is not None: isn't != less verbose? Created attachment 117106 [details]
fix name
Comment on attachment 117106 [details] fix name View in context: https://bugs.webkit.org/attachment.cgi?id=117106&action=review > Tools/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). This line should appear before the description. Comment on attachment 117096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117096&action=review >> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:74 >> + # Mac port returns 'None' since it doesn't implement diff_image. > > This should say non-chromium port. Done. > Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:76 > + writer.write_image_diff_files(image_diff) "is not" is better in this case because None is singleton. I guess that's slightly faster and well accepted usage. Ops. I'll fix it and update the patch. (In reply to comment #6) > (From update of attachment 117106 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117106&action=review > > > Tools/ChangeLog:9 > > + Reviewed by NOBODY (OOPS!). > > This line should appear before the description. Created attachment 117109 [details]
fix changelog
I noticed that mac port actually implements diff_image. WebKitPort is a super class of Apple port, which is super class of MacPort. WebKitPort implements diff_image. The problem is WebKitPorts's diff_image seems to be wrongly implemented. Let me investigate further and file another bug. Committed r101455: <http://trac.webkit.org/changeset/101455> I filed another bug in: https://bugs.webkit.org/show_bug.cgi?id=73406 |