Bug 83799

Created attachment 136946 [details] Patch

Comment on attachment 136946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136946&action=review This seems fine. We have PNGs for testing color profiles, so I think its ok to remove the profiles from these PNGs, which are for testing other aspects of the engine. > LayoutTests/ChangeLog:6 > + Convert layout test resource PNGs to sRGB > + https://bugs.webkit.org/show_bug.cgi?id=83799 > + > + Reviewed by NOBODY (OOPS!). Can you add some information to the ChangeLog about why we're making this change?

Created attachment 136977 [details] Patch

(In reply to comment #2) > (From update of attachment 136946 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136946&action=review > > This seems fine. We have PNGs for testing color profiles, so I think its ok to remove the profiles from these PNGs, which are for testing other aspects of the engine. > > > LayoutTests/ChangeLog:6 > > + Convert layout test resource PNGs to sRGB > > + https://bugs.webkit.org/show_bug.cgi?id=83799 > > + > > + Reviewed by NOBODY (OOPS!). > > Can you add some information to the ChangeLog about why we're making this change? Done.

Comment on attachment 136977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136977&action=review > LayoutTests/ChangeLog:9 > + https://bugs.webkit.org/show_bug.cgi?id=83799 The usual format for ChangeLogs is to have the bug title, followed by the bug link on the next line, followed by a blank line and then the description. It's probably not worth re-uploading this patch, but it's something for next time.

Comment on attachment 136977 [details] Patch Clearing flags on attachment: 136977 Committed r114056: <http://trac.webkit.org/changeset/114056>

All reviewed patches have been landed. Closing bug.

Multiple people recently worked on ensuring that layout tests don't fail because of color matching issues. Did you consult with any of the people involved before suggesting this change? Adam, did you consult with them before reviewing? In this bug itself, there is no indication that this is a thought through change - the system is convoluted enough for obvious answer to not necessarily be correct. See also: bug 67880.

(In reply to comment #8) > Multiple people recently worked on ensuring that layout tests don't fail because of color matching issues. Did you consult with any of the people involved before suggesting this change? Adam, did you consult with them before reviewing? I posted on webkit-dev and got no reply. > In this bug itself, there is no indication that this is a thought through change - the system is convoluted enough for obvious answer to not necessarily be correct. > > See also: bug 67880. Note that this is a separate issue from color profiles in the expectation PNGs. Here, the issue is color profiles in the test resources. For me, the fact that the image-color-matching test was expecting a non-sRGB image to match an image without a profile was extremely suspect. I've spent almost a month trying to understand why the LayoutTests are the way they are. I won't claim to be an expert, but I've had no luck finding anyone who can explain the current situation.

Alexey, I don't understand what you're concerned about. Bug 67880 seems unrelated to this issue. Can you explain more explicitly what you're worried about?

> I posted on webkit-dev and got no reply. That's sufficient, thank you. It usually helps to mention previous webkit-dev discussion in Bugzilla, because bugs do feel like they have incomplete rationale and/or vetting otherwise.

I think many sample images have no color profile. So now we have some with sRGB, and many with no profile.