[webkit-reviews] review denied: [Bug 65587] Implemented skia support for caching resizes of cropped images. : [Attachment 104553] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied John Bates
<jbates at google.com>'s request for review:
Bug 65587: Implemented skia support for caching resizes of cropped images.
https://bugs.webkit.org/show_bug.cgi?id=65587

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104553&action=review


> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:150
> +    return shouldCacheResamplingInternal(destWidth, destHeight, destWidth,
destHeight, srcSubset, destSubset);

I still have the same question:

"are you sure you are computing the destSubsetWidth and destSubsetHeight
parameters
properly?  shouldn't they be taken from destSubset?"

> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:45
> +    // CachedImageInfo is used to uniquely identify cached or requested
image

Sorry, I explained poorly.  I thought where you placed the struct in this
header file
was fine.  I was pointing out that the method implementations in the .cpp file
appear
in the middle of the NativeImageSkia methods.  It is helpful when things in the
.cpp
file are listed in the same order as they appear in the .h file.

I would move this back down to where you had it, and then I would just move the

methods in the .cpp file to be at the bottom of the .cpp file.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:94
> +    SkBitmap resizedBitmap(int width, int height, bool allowCaching) const;

what about jamesr's suggestion to switch allowCaching over to an enum?