[webkit-reviews] review granted: [Bug 102448] PDFPlugin: The "Open in Preview" HUD button should work : [Attachment 174574] 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 102448: PDFPlugin: The "Open in Preview" HUD button should work
https://bugs.webkit.org/show_bug.cgi?id=102448

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

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


Did you test that this works with PostScript files?

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:586
> +    CoreIPC::DataReference dataReference(CFDataGetBytePtr(m_pdfData.get()),
CFDataGetLength(m_pdfData.get()));

I find it confusing that a CoreIPC type is used in UIProcess with no intention
of IPC happening.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:591
> +    if (!m_temporaryPDFID)

Can this be an early return before creating a data reference?

> Source/WebKit2/UIProcess/WebPageProxy.h:1255
> +    WTF::Vector<String> m_temporaryPDFFiles;

s/WTF:://

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:522
> +void WebPageProxy::savePDFToFileInTemporaryFolder(const String&
suggestedFilename, const String& originatingURLString, const
CoreIPC::DataReference& data, uint32_t& pdfID)

originatingURLString looks unused. I think that we should put it into the right
place.

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:533
> +    if (!nsPath)
> +	   return;

I think that that it may be appropriate to write something to system console in
early return cases.

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:535
> +    String path(nsPath);

Looks like this variable is not needed - it's only used once, and implicit
conversion would work there.

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:539
> +    RetainPtr<NSNumber> permissions(AdoptNS, [[NSNumber alloc]
initWithInt:S_IRUSR]);
> +    RetainPtr<NSDictionary> fileAttributes(AdoptNS, [[NSDictionary alloc]
initWithObjectsAndKeys:permissions.get(), NSFilePosixPermissions, nil]);
> +    RetainPtr<NSData> nsData(AdoptNS, [[NSData alloc]
initWithBytesNoCopy:(void *)data.data() length:data.size() freeWhenDone:NO]);

= adoptNS() is our new style.

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:551
> +    if (pdfID > m_temporaryPDFFiles.size())
> +	   return;

What if it's 0?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:107
> +    CoreIPC::DataReference pdfDocumentDataReference();

Do we really need to use a CoreIPC type in plug-in code? This looks like a
wrong layer for it.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:727
> +	  
webFrame()->page()->sendSync(Messages::WebPageProxy::SavePDFToFileInTemporaryFo
lder(suggestedFilename(), webFrame()->url(), dataReference),
Messages::WebPageProxy::SavePDFToFileInTemporaryFolder::Reply(m_temporaryPDFID)
);

I'm not thrilled that you add a sync message here. Please don't do that.

It's not too likely to happen in practice, but there is no strong guarantee
that sync and async messages between UIProcess and WebProcess are handled in
order.