Bug 80571

<rdar://problem/11008529>

Created attachment 130771 [details] patch Patch, but needs more testing (especially that colors output match chromium as much as possible, otherwise required rebaselines will be even more significant). In either case, many mac baselines committed between https://bugs.webkit.org/show_bug.cgi?id=75662 landing and now will probably need to be regenerated.

Attachment 130771 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/D..." exit_code: 1 Tools/Scripts/webkitpy/layout_tests/port/mac.py:150: trailing whitespace [pep8/W291] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 130771 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=130771&action=review > Tools/ChangeLog:11 > + causes system color profile to not affect results on Lion. _the_ system color profile? I'd call it the display color profile, too. > Tools/DumpRenderTree/mac/LayoutTestHelper.m:58 > + CFUUIDRef displayRef = CGDisplayCreateUUIDFromDisplayID(CGMainDisplayID()); > + > + if (!sInitialProfileURL) { > + CFDictionaryRef deviceInfo = ColorSyncDeviceCopyDeviceInfo(kColorSyncDisplayDeviceClass, displayRef); > + CFDictionaryRef profileInfo = (CFDictionaryRef)CFDictionaryGetValue(deviceInfo, kColorSyncCustomProfiles); > + if (profileInfo) > + sInitialProfileURL = (CFURLRef)CFDictionaryGetValue(profileInfo, CFSTR("1")); > + } I guess it doesn't really matter that this function leaks like a sieve? > Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm:56 > - RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateDeviceRGB()); > + // Creating this bitmap in generic RGB (which matches the device color space as we have set it) prevents any color conversion when the image of the web view is drawn into it. > + RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB)); Hrm, are these changes necessary? Will they affect existing pixel results?

(In reply to comment #4) > (From update of attachment 130771 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130771&action=review > > > Tools/ChangeLog:11 > > + causes system color profile to not affect results on Lion. > > _the_ system color profile? > > I'd call it the display color profile, too. *Main* display, even. > > Tools/DumpRenderTree/mac/LayoutTestHelper.m:58 > > + CFUUIDRef displayRef = CGDisplayCreateUUIDFromDisplayID(CGMainDisplayID()); > > + > > + if (!sInitialProfileURL) { > > + CFDictionaryRef deviceInfo = ColorSyncDeviceCopyDeviceInfo(kColorSyncDisplayDeviceClass, displayRef); > > + CFDictionaryRef profileInfo = (CFDictionaryRef)CFDictionaryGetValue(deviceInfo, kColorSyncCustomProfiles); > > + if (profileInfo) > > + sInitialProfileURL = (CFURLRef)CFDictionaryGetValue(profileInfo, CFSTR("1")); > > + } > > I guess it doesn't really matter that this function leaks like a sieve? Not really? But, we might as well clean up anyway. > > Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm:56 > > - RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateDeviceRGB()); > > + // Creating this bitmap in generic RGB (which matches the device color space as we have set it) prevents any color conversion when the image of the web view is drawn into it. > > + RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB)); > > Hrm, are these changes necessary? Will they affect existing pixel results? I expect they could be necessary in the case where interpretation of deviceRGB (which is unfortunately not actually defined to mean anything specific) changes, causing conversion between data in other color spaces and this to be different. It's one dimension I haven't had a chance to double check yet (and one of the reasons I didn't set r? yet). The old code (before https://bugs.webkit.org/show_bug.cgi?id=75662) also used Generic RGB here, just in a slightly more roundabout way.

Created attachment 130886 [details] patch for safekeeping Patch, but still needs investigation of buffer colorspace choice vs. Chromium and going forward; just for safekeeping.

(In reply to comment #6) > Created an attachment (id=130886) [details] > patch for safekeeping > > Patch, but still needs investigation of buffer colorspace choice vs. Chromium and going forward; just for safekeeping. I've generated most results of the SVG pixel tests by forcing Generic RGB profile on my iMac before. I've rebaselined to trunk, and here are my findings: - When I switch to Generic RGB profile manually, all tests pass, except the ones you've added to platform/mac/test_expectations.txt (they're now failing) - When I switch to iMac profile, more tests are failing, but also including those that you marked as IMAGE failing in the test_expectations.txt file on mac. It's quite frustrating for me, as the trunk pixel test baseline (before the recent changes) passed with tolerance 0 on my both 64 bit machines, one running Lion, one running SL, and I'm used to test this way. Anyhow, now it's broken and I don't know which route to take to fix it. What's your plan to move on here? Why can't we add this now for Mac, until we find consensus with the cr guys on how to hook it into their TestShell.

(In reply to comment #7) Okay, I partly have to take back what I wrote: It's partly related to Ojans massive rebaselining, which moved dozens of platform/mac/svg results to platform/mac-future/svg/, which is wrong. I'll restore some results, to clean up this mess. Mailed Ojan about this as well.

(In reply to comment #8) > (In reply to comment #7) > Okay, I partly have to take back what I wrote: It's partly related to Ojans massive rebaselining, which moved dozens of platform/mac/svg results to platform/mac-future/svg/, which is wrong. I'll restore some results, to clean up this mess. Mailed Ojan about this as well. I fixed the SVG pixel test baseline on Lion - generated all results using the Generic RGB profile where needed (I already did that before for most of them) in r10292. No more skipped svg/ tests because of this bug, still some filters/ test need rebaselines. Anyhow just wanted to let you know that I rebaselined everything in Generic RGB. If you want to run tests, you have to manually switch to Generic RGB profile on mac now, as long as this bug is fixed.

(In reply to comment #7) > (In reply to comment #6) > > Created an attachment (id=130886) [details] [details] > > patch for safekeeping > > > > Patch, but still needs investigation of buffer colorspace choice vs. Chromium and going forward; just for safekeeping. > > I've generated most results of the SVG pixel tests by forcing Generic RGB profile on my iMac before. > I've rebaselined to trunk, and here are my findings: > > - When I switch to Generic RGB profile manually, all tests pass, except the ones you've added to platform/mac/test_expectations.txt (they're now failing) > - When I switch to iMac profile, more tests are failing, but also including those that you marked as IMAGE failing in the test_expectations.txt file on mac. > > It's quite frustrating for me, as the trunk pixel test baseline (before the recent changes) passed with tolerance 0 on my both 64 bit machines, one running Lion, one running SL, and I'm used to test this way. > Anyhow, now it's broken and I don't know which route to take to fix it. > What's your plan to move on here? Why can't we add this now for Mac, until we find consensus with the cr guys on how to hook it into their TestShell. My plan to move on here is to make sure that our results match cr-mac in the very simplest of cases, and then go with it! This is one of my plans for today. I agree that it's very unfortunate, that's why I brought it up and am fixing it :-D Thanks for rebaselining all of the tests, by the way! Hopefully those results will stay stable.

Created attachment 132867 [details] patch

Created attachment 132868 [details] patch

Comment on attachment 132868 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=132868&action=review > Tools/DumpRenderTree/mac/LayoutTestHelper.m:38 > +// This is a simple helper app that changes the ColorSync profile to the I think the comment should say "the color profile for the main display". > Tools/DumpRenderTree/mac/LayoutTestHelper.m:51 > + CFUUIDRef mainDisplay = CGDisplayCreateUUIDFromDisplayID(CGMainDisplayID()); mainDisplayID would be a better name. > Tools/DumpRenderTree/mac/LayoutTestHelper.m:84 > + fprintf(stderr, "Failed to set color profile for main display! Many pixel tests may fail as a result.\n"); One space after the ! pls. > Tools/DumpRenderTree/mac/LayoutTestHelper.m:117 > + exit(128 + sig); What is 128 + sig? > Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:59 > - [m_window setColorSpace:[[NSScreen mainScreen] colorSpace]]; > + [m_window setColorSpace:[NSColorSpace genericRGBColorSpace]]; Does DRT need a similar change?

(In reply to comment #13) > > Tools/DumpRenderTree/mac/LayoutTestHelper.m:117 > > + exit(128 + sig); > > What is 128 + sig? Those are the error codes reserved for death-by-signal.

(In reply to comment #13) > > Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:59 > > - [m_window setColorSpace:[[NSScreen mainScreen] colorSpace]]; > > + [m_window setColorSpace:[NSColorSpace genericRGBColorSpace]]; > > Does DRT need a similar change? Actually, WKTR doesn't need that change, since the color space of the main screen *is* GenericRGB. Tests continue to pass without it.

Landed as http://trac.webkit.org/changeset/111429

This broke SL tools build because it uses API that was made public in Lion. I have an ugly patch that basically #ifdefs the whole file to use the old API if on SL or below.

Created attachment 132908 [details] ugly, dirty patch (not tested on SL! however, very similar code is in DRT/chromium tree so it should be ok)

Follow up SL DRT build fix in http://trac.webkit.org/changeset/111452.