Bug 111956 – PDFPlugin: Return PDFKit's data instead of the original resource data for save/etc.

• Component: PDF
• Status: RESOLVED FIXED

[Tim Horton]

<rdar://problem/13352282>

[Tim Horton]

Created attachment 192407 [details]
patch

[Early Warning System Bot]

Comment on attachment 192407 [details]
patch

Attachment 192407 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17149011

[EFL EWS Bot]

Comment on attachment 192407 [details]
patch

Attachment 192407 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17134079

[Tim Horton]

Created attachment 192411 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 192411 [details]
patch

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.

[Tim Horton]

(In reply to comment #5)
> (From update of attachment 192411 [details])
> 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?

Not ... really. We construct it piecemeal, so we need to have it while loading, but once we create our PDFDocument we could throw it away.

This brought up a good point, though, download and open-in-preview should use the mutated data too.

> > 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?

I think so, I'll check.

> > 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.

liveResourceData()? (it's not necessarily just for saving)

> 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.

I added getResourceData, so it shouldn't be a problem.

> > 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.

Copy-paste-o.

[Alexey Proskuryakov]

> > Is this #include actually needed?
>
> I think so, I'll check.

A forward declaration is sufficient for a type in RefPtr or OwnPtr, as long as the object itself is not accessed. Most of the time, this means that constructors and destructors should not be inline, which is a good idea for polymorphic classes anyway.

[Tim Horton]

Created attachment 192568 [details]
patch

[WebKit Review Bot]

Attachment 192568 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp', u'Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.h', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.mm', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginChoiceAnnotation.mm', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm', u'Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.h', u'Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm', u'Source/WebKit2/WebProcess/Plugins/Plugin.h', u'Source/WebKit2/WebProcess/Plugins/PluginProxy.cpp', u'Source/WebKit2/WebProcess/Plugins/PluginProxy.h', u'Source/WebKit2/WebProcess/Plugins/PluginView.cpp', u'Source/WebKit2/WebProcess/Plugins/PluginView.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp']" exit_code: 1
Source/WebKit2/ChangeLog:35:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 192572 [details]
appease stylebot

[Alexey Proskuryakov]

Comment on attachment 192572 [details]
appease stylebot

View in context: https://bugs.webkit.org/attachment.cgi?id=192572&action=review

> Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.h:75
> +    RetainPtr<NSData> liveData() const;
> +    RetainPtr<NSData> rawData() const { return (NSData *)m_data.get(); }

Looks like returning a pointer should work for both.

> Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm:689
> +    if (!m_pdfDocument)
> +        return 0;

Is this check necessary?

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:909
> -        return false;
> +        return PassRefPtr<WebCore::SharedBuffer>();

My advice was probably not that good - the unimportant win in performance is not worth it.

[Tim Horton]

http://trac.webkit.org/changeset/145477