[webkit-reviews] review denied: [Bug 88565] [chromium] Add monitor profile support for Win and Mac : [Attachment 146344] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Tony Payne
<tpayne at chromium.org>'s request for review:
Bug 88565: [chromium] Add monitor profile support for Win and Mac
https://bugs.webkit.org/show_bug.cgi?id=88565

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=146344&action=review


> Source/Platform/chromium/public/Platform.h:181
> +    virtual WebVector<char>* getMonitorColorProfile() { return new
WebVector<char>(); }

What is the actual data being represented here?

It's more common for the default impl to return null for a function like this,
not an empty value.

What is the ownership model of this value - is the caller supposed to delete
it?  That's a bit strange.

We typically don't use "get" for functions like this (see the rest of the
file).

Which monitor is this function referring to in a multi-monitor setup?

Can this value change - for instance, if the user changes the monitor's color
profile, plugs a new monitor in, moves a window from one monitor to another,
etc?  If so, what happens then?

> Source/WebCore/platform/image-decoders/ImageDecoder.h:36
> +#include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebVector.h"

we don't use absolute include paths like this in WebKit, and this won't even be
valid for most users of this file.

WebVector is a chromium-specific type and this appears to be a cross-platform
file based on the build path.  This should be guarded by #if PLATFORM(CHROMIUM)
or refer to a cross-platform concept

> Source/WebCore/platform/image-decoders/ImageDecoder.h:307
> +		   OwnPtr<WebKit::WebVector<char> > profile =
WTF::adoptPtr(WebKit::Platform::current()->getMonitorColorProfile());

This will only compile on chromium, but this doesn't appear to be a
chromium-specific file or a chromium-specific block.  What should happen for
other platforms?

> LayoutTests/platform/chromium/TestExpectations:3623
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-blur-hw.html = IMAGE

do these tests all contain source images with ICC profiles?  is this an
intentional part of the test?