[webkit-reviews] review granted: [Bug 88565] Add monitor profile support for Win and Mac : [Attachment 148428] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 19 15:56:40 PDT 2012
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...
More information about the webkit-reviews
mailing list