[webkit-reviews] review granted: [Bug 121207] [mac] Cache rendered image in PDFDocumentImage : [Attachment 211457] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 121207: [mac] Cache rendered image in PDFDocumentImage
https://bugs.webkit.org/show_bug.cgi?id=121207

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=211457&action=review


> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:132
> +    if (dstRect.size() != m_cachedDestinationSize)
> +	   return false;
> +
> +    if (srcRect != m_cachedSourceRect)
> +	   return false;

You should do these cheap tests before the expensive decompose-based ones

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:156
> +	   bufferContext->scale(FloatSize(hScale, vScale));
> +	   bufferContext->scale(FloatSize(1, -1));

You could just do -vScale on the first line.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:166
> +	   int oldCachedBytes = m_cachedBytes;

size_t

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:170
> +	       imageObserver()->decodedSizeChanged(this, m_cachedBytes -
oldCachedBytes);

But beware of unsigned math here.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:59
> +    virtual bool isPDFDocumentImage() const OVERRIDE { return true; }

FINAL?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:85
> +    bool cacheParametersMatch(GraphicsContext*, const FloatRect& dstRect,
const FloatRect& srcRect);

can this be const?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:97
> +    int m_cachedBytes;

size_t?