Summary: | [chromium] don't write .checksum files if a fallback platform has an embedded checksum | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tony Chang <tony> | ||||||
Component: | New Bugs | Assignee: | Tony Chang <tony> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | dpranke, mihaip, ojan, podivilov | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 56286 | ||||||||
Attachments: |
|
Description
Tony Chang
2011-04-04 13:26:59 PDT
Created attachment 88114 [details]
Patch
The bug is as follows: 1) The rebaseline tool writes a png results for win-xp 2) The rebaseline tool deletes the checksum result for win-xp, since it's not needed 3) The rebaseline tool tries to write a png result for win-vista. Since it matches the win-xp result, it does nothing. 4) The rebaseline tool writes a checksum result for win-vista, but since there's no win-xp checksum, it goes ahead and writes the file. The result is there's a .checksum file in win-vista that matches the .png file in win-xp. We shouldn't write the .checksum file. The patch fixes the bug by checking the fallback .png list rather than just checking for win-vista's .png. (In reply to comment #2) Dirk tells me I described the fallback wrong. Here's a version without platforms but with directories instead: 1) The rebaseline tool writes a png results in chromium-win 2) The rebaseline tool deletes the checksum result for chromium-win, since it's not needed 3) The rebaseline tool tries to write a png result for chromium-win-vista. Since it matches the chromium-win result, it does nothing. 4) The rebaseline tool writes a checksum result for chromium-win-vista, but since there's no chromium-win checksum, it goes ahead and writes the file. Comment on attachment 88114 [details] Patch LGTM. View in context: https://bugs.webkit.org/attachment.cgi?id=88114&action=review > Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:435 > + Nit: Can you pull this into a separate routine called png_for_checksum() or something like that to help clarify what's going on? Created attachment 88118 [details]
Patch
(In reply to comment #5) > Created an attachment (id=88118) [details] > Patch LGTM. Comment on attachment 88114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88114&action=review >> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:435 >> + png_fullpath = fs.join(all_baselines[0][0], all_baselines[0][1]) >> + > > Nit: Can you pull this into a separate routine called png_for_checksum() or something like that to help clarify what's going on? Done. Note: Code like this is why I hate returning tuples. (In reply to comment #7) > (From update of attachment 88114 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88114&action=review > > >> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:435 > >> + png_fullpath = fs.join(all_baselines[0][0], all_baselines[0][1]) > >> + > > > > Nit: Can you pull this into a separate routine called png_for_checksum() or something like that to help clarify what's going on? > > Done. Note: Code like this is why I hate returning tuples. It looks like expected_baselines() could and probably should be rewritten to only return a list of full paths to the tests, and then maybe we provide another helper function like baseline_dir_for_test() or something. That would probably clean things up somewhat. I know this code used to be more convoluted in the past and it can probably be made a fair amount simpler. Committed r82892: <http://trac.webkit.org/changeset/82892> |