[webkit-reviews] review denied: [Bug 75618] [Chromium] LayoutTestHelper fails to install generic color profile : [Attachment 121420] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied jochen at chromium.org's request for
review:
Bug 75618: [Chromium] LayoutTestHelper fails to install generic color profile
https://bugs.webkit.org/show_bug.cgi?id=75618

Attachment 121420: Patch
https://bugs.webkit.org/attachment.cgi?id=121420&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.