[webkit-reviews] review denied: [Bug 65587] Implemented skia support for caching resizes of cropped images. : [Attachment 104247] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Aug 18 15:31:19 PDT 2011
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 104247: Patch
https://bugs.webkit.org/attachment.cgi?id=104247&action=review
------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104247&action=review
> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:64
> +NativeImageSkia::CachedImageInfo::CachedImageInfo()
nit: The CachedImageInfo definition should probably move to either the top
or the bottom of the file so as not to interrupt the flow of NativeImageSkia
method implementations.
> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:110
> +
it seems like you should rewrite this function to avoid duplicating the call
to ImageOperations::Resize.
since SkBitmap is internally reference counted, this should be cheap to do:
SkBitmap result = skia::ImageOperations::Resize(*this,
skia::ImageOperations::RESIZE_LANCZOS3, w, h);
if (m_isDataComplete && allowCaching)
m_resizedImage = result;
return result;
> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:126
> + return skia::ImageOperations::Resize(subset,
skia::ImageOperations::RESIZE_LANCZOS3, w, h, destSubset);
same nit
> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:140
> + return shouldCacheResamplingInternal(destWidth, destHeight,
destSubsetWidth, destSubsetHeight, srcSubset, destSubset);
it seems like this function could also be implemented in terms of a call
to shouldCacheResampling(destWidth, destHeight, srcSubset, destSubset).
my assumption is that we could compute destSubset given destSubsetWidth
and destSubsetHeight. am i missing something?
> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:148
> + return shouldCacheResamplingInternal(destWidth, destHeight, destWidth,
destHeight, srcSubset, destSubset);
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:112
> +
nit: nuke this new line
More information about the webkit-reviews
mailing list