[webkit-reviews] review granted: [Bug 111956] PDFPlugin: Return PDFKit's data instead of the original resource data for save/etc. : [Attachment 192411] patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 111956: PDFPlugin: Return PDFKit's data instead of the original resource
data for save/etc.
https://bugs.webkit.org/show_bug.cgi?id=111956

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

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


Looks fine overall, and the comments should be easily addressable.

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h:251
> -    virtual bool getResourceData(const unsigned char*& /* bytes */,
unsigned& /* length */) const OVERRIDE { return false; }
> +    virtual PassRefPtr<WebCore::SharedBuffer> getResourceData() const
OVERRIDE { return WebCore::SharedBuffer::create(); }

I think that returning 0 would be more idiomatic than returning an empty
buffer.

It probably doesn't match too much here, but
PassRefPtr<WebCore::SharedBuffer>() is a little faster than returning 0.

> Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm:-679
> -    if (m_data && m_pdfDocument) {
> -	   bytes = CFDataGetBytePtr(m_data.get());
> -	   length = CFDataGetLength(m_data.get());

Do we still have a reason to preserve m_data?

> Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm:680
> +    NSData* pdfData = [m_pdfDocument.get() dataRepresentation];

Misplaced star.

> Source/WebKit2/WebProcess/Plugins/Plugin.h:34
> +#include <WebCore/SharedBuffer.h>

Is this #include actually needed?

> Source/WebKit2/WebProcess/Plugins/Plugin.h:267
> -    virtual bool getResourceData(const unsigned char*& bytes, unsigned&
length) const = 0;
> +    virtual PassRefPtr<WebCore::SharedBuffer> getResourceData() const = 0;

We normally use "get" prefix with functions that return result in a reference,
like this one used to do. Also, there is nothing in the name that allows for
the proposed behavior - it doesn't say "ForSaving", for example.

Did you check other call sites to confirm that it's OK to return mutated PDF
data? I see that there is only one call site changed in the patch, but I didn't
check what happens in UI process.

> Source/WebKit2/WebProcess/Plugins/PluginProxy.h:136
> +    virtual PassRefPtr<WebCore::SharedBuffer> getResourceData() const
OVERRIDE { return WebCore::SharedBuffer::create(); }

Here as well, I'd return a null pointer.

> Source/WebKit2/WebProcess/Plugins/PluginView.h:104
> -    bool getResourceData(const unsigned char*& bytes, unsigned& length)
const;
> +    virtual PassRefPtr<WebCore::SharedBuffer> getResourceData() const;

Unsure why this this function is virtual now.