[webkit-reviews] review granted: [Bug 120651] [mac] PDFDocumentImage should use PDFKit to draw : [Attachment 210419] binary patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 120651: [mac] PDFDocumentImage should use PDFKit to draw
https://bugs.webkit.org/show_bug.cgi?id=120651

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=210419&action=review


> Source/WebCore/ChangeLog:40
> +	   (WebCore::PDFDocumentImage::setCurrentPage):

Isn't this mostly dead code, as we only ever display the first page?

> Source/WebCore/ChangeLog:41
> +	   Make all page-number-related things unsigned, since they are in CG
and PDFKit,

Would be nice to have comments about what is 0-based and what is 1-based.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:163
> +    // We use the GetBytesAtPosition callback rather than the GetBytePointer
one because SharedBuffer
> +    // does not provide a way to lock down the byte pointer and guarantee
that it won't move, which

I'm not sure if this is relevant, as we don't mutate the SharedBuffer. Although
maybe its data can be replaced when picking up a resource from CFNetwork cache?


Brady would know for sure.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:189
> +    return m_document ? CGPDFDocumentGetNumberOfPages(m_document) : 0;

No need for null check.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:52
> +    virtual String filenameExtension() const;

OVERRIDE

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

OVERRIDE

And so on...

> Source/WebCore/platform/graphics/mac/PDFDocumentImageMac.mm:29
> +#if PLATFORM(MAC)

Is the purpose of this check to compile this out on iOS?

> Source/WebCore/platform/graphics/mac/PDFDocumentImageMac.mm:46
> +    m_document = [[getPDFDocumentClass() alloc] initWithData:nsData.get()];

This leaks the whole document.

> Source/WebCore/platform/graphics/mac/PDFDocumentImageMac.mm:51
> +    RetainPtr<PDFPage> pdfPage = [m_document pageAtIndex:m_currentPage];

I don't think that this needs a RetainPtr, a raw pointer should be just fine.

> Source/WebCore/platform/graphics/mac/PDFDocumentImageMac.mm:61
> +    return m_document ? [m_document pageCount] : 0;

No need for null check.

> Source/WebCore/platform/graphics/mac/PDFDocumentImageMac.mm:69
> +    [[m_document pageAtIndex:m_currentPage]
drawWithBox:kPDFDisplayBoxMediaBox];

It looks suspicious that we use media box unconditionally here.