[webkit-reviews] review denied: [Bug 69144] Apply color profile found to decoded bitmap (Skia on Mac) : [Attachment 109292] Patch

[bugzilla-daemon at webkit.org]

Stephen White <senorblanco at chromium.org> has denied Cary Clark
<caryclark at google.com>'s request for review:
Bug 69144: Apply color profile found to decoded bitmap (Skia on Mac)
https://bugs.webkit.org/show_bug.cgi?id=69144

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

------- Additional Comments from Stephen White <senorblanco at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109292&action=review


Please fix the #include path; the rest is up to you.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:34
> +#include "third_party/skia/include/utils/mac/SkCGUtils.h"

We don't hardcode chrome's third_party paths in WebKit code.  AFAICT, we -I the
skia include dirs in (chrome's) skia/skia.gyp.	It looks like it's already
there (under 'OS == "mac"' [...] 'include_dirs"), so you should just be able to
#include "SkCGUtils.h" here.  If not, you may have to land a Chrome change to
fix it first.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:124
> +    bitmap.lockPixels();

Nit:  Could use an SkAutoLockPixels guard instead.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:127
> +    if (bmap) {

Nit:  WebKit style is to use early-return where possible, so if you used the
SkAutoLockPixels above, you could just 

if (!bmap)
    return;

here.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:150
> +    m_colorProfile = colorProfile;

Is this file compiled on non-mac platforms?  (I've never looked into the image
decoders much before).	If so, I guess this will just be setting a color
profile that we subsequently ignore on non-mac.  Not a big deal, I suppose,
just that it will no generate a log message on incorrect code.