Bug 57783 – [chromium] don't write .checksum files if a fallback platform has an embedded checksum

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

[chromium] don't write .checksum files if a fallback platform has an embedded checksum

[Tony Chang]

Created attachment 88114 [details]
Patch

[Tony Chang]

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.

[Tony Chang]

(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.

[Dirk Pranke]

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?

[Tony Chang]

Created attachment 88118 [details]
Patch

[Dirk Pranke]

(In reply to comment #5)
> Created an attachment (id=88118) [details]
> Patch

LGTM.

[Tony Chang]

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.

[Dirk Pranke]

(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.

[Tony Chang]

Committed r82892: <http://trac.webkit.org/changeset/82892>