[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