[webkit-reviews] review granted: [Bug 88565] Add monitor profile support for Win and Mac : [Attachment 148428] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Tony Payne <tpayne at chromium.org>'s
request for review:
Bug 88565: Add monitor profile support for Win and Mac
https://bugs.webkit.org/show_bug.cgi?id=88565

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=148428&action=review


I feel like I'm nit-picking your function names, which probably means we're all
set.  :)

> Source/Platform/chromium/public/Platform.h:181
> +    // Supplies the system monitor color profile.
> +    virtual void monitorColorProfile(const WebString&, WebVector<char>*) { }


I would have named the string here, or maybe included a description of what
this function does in the comment.

> Source/WebCore/platform/chromium/PlatformScreenChromium.cpp:82
> +void screenColorProfile(Widget*, const String& type, ColorProfile&
toProfile)
> +{
> +    // FIXME: Add support for multiple monitors.
> +    WebKit::WebVector<char> profile;
> +   
WebKit::Platform::current()->monitorColorProfile(WebKit::WebString(type),
&profile);
> +    toProfile.append(profile.data(), profile.size());
> +}

It's a bit strange that the function changes names here.  I would have expected
screenColorProfile() to always return the color profile of the screen. 
Similarly, I would have expected the API to be called colorProfile if it can
give you back an "sRGB" profile, which would seem to have nothing to do with a
monitor...