[Chromium] LayoutTestHelper fails to install generic color profile

• Component: Tools / Tests
• Status: RESOLVED FIXED

When running layout tests via new-run-layout-tests (chromium-mac), I get the following error:

Starting helper ...2012-01-05 14:54:32.724 LayoutTestHelper[71934:507] failed install the generic color profile, pixmaps won't match. Error: -50

I looked at the source, and the API used by the LayoutTestHelper is deprecated since 10.6, but it's neither documented what error -50 is, nor what you should use instead.

Kent, any idea? svn blame says you wrote the code :)

[Alexey Proskuryakov]

-50 is just generic paramErr.

[Kent Tamura]

I'm not so familiar with OS X.
Apple Mac's code to reset the profile looks different:
http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm#L70

Chromium's:
http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/chromium/LayoutTestHelper.mm#L56

Created attachment 121420 [details]
Patch

[Simon Fraser (smfr)]

See bug 75662.

(In reply to comment #5)
> See bug 75662.

ok, but this bug is not affecting the chromium port.

Ojan, do you know whether there are plans to switch the layout tests on chromium-mac to not requiring a specific display color profile?

[Ojan Vafai]

(In reply to comment #6)
> Ojan, do you know whether there are plans to switch the layout tests on chromium-mac to not requiring a specific display color profile?

I don't know anything about this. Looking at bug 75662, it seems we should make sure the chromium mac port to also not depend on having a specific color profile, then we can change LayoutTestHelper to not mess with your color profile and thus get rid of this warning.

Maybe Eric or Dirk knows more...

[Dirk Pranke]

(In reply to comment #7)
> (In reply to comment #6)
> > Ojan, do you know whether there are plans to switch the layout tests on chromium-mac to not requiring a specific display color profile?
> 
> I don't know anything about this. Looking at bug 75662, it seems we should make sure the chromium mac port to also not depend on having a specific color profile, then we can change LayoutTestHelper to not mess with your color profile and thus get rid of this warning.
> 
> Maybe Eric or Dirk knows more...

Not ringing a bell, but it sure sounds like a good idea.

[Eric Seidel (no email)]

Yes.  It didn't used to be possible to set a per-app or per-window colorspace. :)  It appears folks have come up with a better solution since!

[Eric Seidel (no email)]

Comment on attachment 121420 [details]
Patch

Can we please share thsi code with Mac instead?  Can't we abstract out the code in their DRT into a separate file and just re-use it directly?

(In reply to comment #10)
> (From update of attachment 121420 [details])
> Can we please share thsi code with Mac instead?  Can't we abstract out the code in their DRT into a separate file and just re-use it directly?

Mac got rid of this code.

I wonder whether with the switch from CG to skia, we could just remove the code as well

[Ryosuke Niwa]

Comment on attachment 121420 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121420&action=review

r- due to various nits.

> Tools/DumpRenderTree/chromium/LayoutTestHelper.mm:41
> +#define PROFILE_PATH "/System/Library/ColorSync/Profiles/Generic RGB Profile.icc" // FIXME: This cannot be more than CS_MAX_PATH (256 characters)

Why are you using define instead of const char*? And what's the meaning of FIXME? The path is clearly less than 256 characters.

> Tools/DumpRenderTree/chromium/LayoutTestHelper.mm:43
> +static CMProfileLocation sInitialProfileLocation; // The locType field is initialized to 0 which is the same as cmNoProfileBase

Wrong naming convention. Static variables should start with s_ followed by a camelCase variable name.

> Tools/DumpRenderTree/chromium/LayoutTestHelper.mm:62
> +              @"Error: %d", (int)error);

Wrong indentation. We don't align wrapped lines like this. @ should be exactly 4 spaces right to NSLog (i.e. this line should have exactly 12 spaces from column 0)

> Tools/DumpRenderTree/chromium/LayoutTestHelper.mm:87
> +                  @"Displays -> Color to reset. Error: %d", (int)error);

Ditto about indentation.

Created attachment 121559 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 121559 [details]
Patch

This is using deprecated ColorSync APIs. I'll attach a patch using newer ones.

[Simon Fraser (smfr)]

Created attachment 121694 [details]
Updated color profile switching code

Comment on attachment 121694 [details]
Updated color profile switching code

View in context: https://bugs.webkit.org/attachment.cgi?id=121694&action=review

> Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm:65
> +    RetainPtr<CFUUIDRef> displayRef(AdoptCF, CGDisplayCreateUUIDFromDisplayID(CGMainDisplayID()));

I see that CGDisplayCreateUUIDFromDisplayID is defined in ColorSync, but I can't find any header that declares this function?

(In reply to comment #16)
> (From update of attachment 121694 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121694&action=review
> 
> > Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm:65
> > +    RetainPtr<CFUUIDRef> displayRef(AdoptCF, CGDisplayCreateUUIDFromDisplayID(CGMainDisplayID()));
> 
> I see that CGDisplayCreateUUIDFromDisplayID is defined in ColorSync, but I can't find any header that declares this function?

Also, given that for the time being, chromium still supports Leopard, can we use those new APIs at all?

[Simon Fraser (smfr)]

Maybe not. Leopard is so old.

(In reply to comment #18)
> Maybe not. Leopard is so old.

I propose to stick with the version using the deprecated APIs then until chromium drops support for Leopard. At that point, we might want to switch to a model like the mac port is using, where we don't have to change the display's color space.

wdyt?

[Mark Mentovai]

Comment on attachment 121559 [details]
Patch

I have two suggestions that shouldn’t have any impact on the result, but one’s for code cleanliness and one’s for buffer safety.

Can you use sizeof(initialColorProfileLocation) instead of sizeof(CMProfileLocation)?

Can you use strncpy instead of strcpy?

Created attachment 122076 [details]
Patch

(In reply to comment #20)
> (From update of attachment 121559 [details])
> I have two suggestions that shouldn’t have any impact on the result, but one’s for code cleanliness and one’s for buffer safety.
> 
> Can you use sizeof(initialColorProfileLocation) instead of sizeof(CMProfileLocation)?

done

> 
> Can you use strncpy instead of strcpy?

CMPathLocation is defined as char path[256]; (http://developer.apple.com/library/mac/#documentation/GraphicsImaging/Reference/ColorSync_Manager/Reference/reference.html)

done

[Mark Mentovai]

Comment on attachment 122076 [details]
Patch

>+    strncpy(location.u.pathLoc.path, colorProfilePath, 255);

255’s not helpful for a buffer of size 256, because you won’t get '\0' termination unless you do it yourself. Since this is specified as a fixed-size buffer and nobody’s said anything about termination, I think it’s fine to just use its full width (256) here. Or you can '\0'-terminate yourself, or use strlcpy to do it for you. I don’t really care about what happens in the overflow case as long as we don’t scribble past the end of the buffer.

However, I would suggest using sizeof(location.u.pathLoc.path) instead of hard-coding some number.

Created attachment 122078 [details]
Patch

[Mark Mentovai]

Comment on attachment 122078 [details]
Patch

OK, I would r+ this version if I were a WebKit reviewer.

[Tony Chang]

Comment on attachment 122078 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122078&action=review

> Tools/DumpRenderTree/chromium/LayoutTestHelper.mm:55
> +    int error;

Nit: Maybe remove this and use 'int error = ...' below?

[Tony Chang]

Comment on attachment 122078 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122078&action=review

> Tools/DumpRenderTree/chromium/LayoutTestHelper.mm:45
> +CMProfileLocation initialColorProfileLocation; // The locType field is initialized to 0 which is the same as cmNoProfileBase

Nit: End the comment with a period.

> Tools/DumpRenderTree/chromium/LayoutTestHelper.mm:82
> +    // This is used as a signal handler, and thus the calls into ColorSync are unsafe

Nit: End the comment with a period.

Created attachment 122085 [details]
Patch

[WebKit Review Bot]

Comment on attachment 122085 [details]
Patch

Clearing flags on attachment: 122085

Committed r104745: <http://trac.webkit.org/changeset/104745>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.