<rdar://problem/13352282>
Created attachment 192407 [details] patch
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
Comment on attachment 192407 [details] patch Attachment 192407 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17134079
Created attachment 192411 [details] patch
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.
(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.
> > 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.
Created attachment 192568 [details] patch
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.
Created attachment 192572 [details] appease stylebot
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.
http://trac.webkit.org/changeset/145477