Bug 75618 - [Chromium] LayoutTestHelper fails to install generic color profile
Summary: [Chromium] LayoutTestHelper fails to install generic color profile
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.7
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-05 05:57 PST by jochen
Modified: 2012-01-11 14:10 PST (History)
8 users (show)

See Also:


Attachments
Patch (4.96 KB, patch)
2012-01-06 03:39 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (5.15 KB, patch)
2012-01-07 14:59 PST, jochen
no flags Details | Formatted Diff | Diff
Updated color profile switching code (6.28 KB, patch)
2012-01-09 11:15 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (5.15 KB, patch)
2012-01-11 12:44 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (5.18 KB, patch)
2012-01-11 12:53 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (5.17 KB, patch)
2012-01-11 13:40 PST, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2012-01-05 05:57:38 PST
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.
Comment 1 jochen 2012-01-05 06:01:20 PST
Kent, any idea? svn blame says you wrote the code :)
Comment 2 Alexey Proskuryakov 2012-01-05 09:21:10 PST
-50 is just generic paramErr.
Comment 3 Kent Tamura 2012-01-05 19:35:25 PST
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
Comment 4 jochen 2012-01-06 03:39:07 PST
Created attachment 121420 [details]
Patch
Comment 5 Simon Fraser (smfr) 2012-01-06 09:21:15 PST
See bug 75662.
Comment 6 jochen 2012-01-06 09:51:07 PST
(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?
Comment 7 Ojan Vafai 2012-01-06 17:05:07 PST
(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...
Comment 8 Dirk Pranke 2012-01-06 17:12:08 PST
(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.
Comment 9 Eric Seidel (no email) 2012-01-06 17:16:14 PST
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!
Comment 10 Eric Seidel (no email) 2012-01-06 17:17:12 PST
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?
Comment 11 jochen 2012-01-07 01:46:08 PST
(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
Comment 12 Ryosuke Niwa 2012-01-07 11:43:08 PST
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.
Comment 13 jochen 2012-01-07 14:59:03 PST
Created attachment 121559 [details]
Patch
Comment 14 Simon Fraser (smfr) 2012-01-09 11:13:08 PST
Comment on attachment 121559 [details]
Patch

This is using deprecated ColorSync APIs. I'll attach a patch using newer ones.
Comment 15 Simon Fraser (smfr) 2012-01-09 11:15:04 PST
Created attachment 121694 [details]
Updated color profile switching code
Comment 16 jochen 2012-01-10 03:12:16 PST
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?
Comment 17 jochen 2012-01-10 11:08:55 PST
(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?
Comment 18 Simon Fraser (smfr) 2012-01-10 14:37:16 PST
Maybe not. Leopard is so old.
Comment 19 jochen 2012-01-10 14:47:40 PST
(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?
Comment 20 Mark Mentovai 2012-01-11 12:30:26 PST
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?
Comment 21 jochen 2012-01-11 12:44:01 PST
Created attachment 122076 [details]
Patch
Comment 22 jochen 2012-01-11 12:45:22 PST
(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
Comment 23 Mark Mentovai 2012-01-11 12:50:05 PST
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.
Comment 24 jochen 2012-01-11 12:53:02 PST
Created attachment 122078 [details]
Patch
Comment 25 Mark Mentovai 2012-01-11 12:53:42 PST
Comment on attachment 122078 [details]
Patch

OK, I would r+ this version if I were a WebKit reviewer.
Comment 26 Tony Chang 2012-01-11 13:37:06 PST
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?
Comment 27 Tony Chang 2012-01-11 13:38:02 PST
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.
Comment 28 jochen 2012-01-11 13:40:23 PST
Created attachment 122085 [details]
Patch
Comment 29 WebKit Review Bot 2012-01-11 14:10:35 PST
Comment on attachment 122085 [details]
Patch

Clearing flags on attachment: 122085

Committed r104745: <http://trac.webkit.org/changeset/104745>
Comment 30 WebKit Review Bot 2012-01-11 14:10:42 PST
All reviewed patches have been landed.  Closing bug.