Bug 102448 – PDFPlugin: The "Open in Preview" HUD button should work

• Status: RESOLVED
• Resolution: FIXED
• Component: PDF

[Tim Horton]

<rdar://problem/12695729>

[Tim Horton]

Created attachment 174574 [details]
patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/WebPageProxy.h:728:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 8 files


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

[Alexey Proskuryakov]

Comment on attachment 174574 [details]
patch

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::SavePDFToFileInTemporaryFolder(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.

[Tim Horton]

(In reply to comment #3)
> (From update of attachment 174574 [details])
> 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.

Would it not be weirder to use NSData/CFData in WebPageProxy? Or would you prefer the argument be a raw data pointer and a size? I'm not sure what to use here if not DataReference.

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

Not 100% sure how to do that, I'll look into it.

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

Seems reasonable.

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

I can get rid of the convenience function, but I'll still need it down below when I send the message, no? Maybe I'm misunderstanding.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:727
> > +        webFrame()->page()->sendSync(Messages::WebPageProxy::SavePDFToFileInTemporaryFolder(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.

I'm switching this to be savePDFToTemporaryFolderAndOpenWithNativeApplication so it does it all in one fell swoop, no need for the sync reply. Though I still need to work out how to get the ID back, but I think that's easy.

[Tim Horton]

(In reply to comment #3)
> (From update of attachment 174574 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174574&action=review
> 
> Did you test that this works with PostScript files?

It does.

[Tim Horton]

(In reply to comment #3)
> (From update of attachment 174574 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174574&action=review
> 
> > 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.

WTFLogAlways? NSLog?

[Tim Horton]

Created attachment 179534 [details]
patch

I believe this addresses Alexey's concerns except the one about saving the originating URL. I tried using WKSetMetadataURL, but it was unclear that it was actually having any effect. I'd like to file that as a separate bug and get Open in Preview working.

[Tim Horton]

Created attachment 179550 [details]
rebase

[Alexey Proskuryakov]

Comment on attachment 179550 [details]
rebase

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

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:581
> +        m_temporaryPDFUUID = WebCore::createCanonicalUUIDString();

Can "using namespace WebCore" be added to this file?

> Source/WebKit2/UIProcess/WebPageProxy.h:730
> +    void savePDFToTemporaryFolderAndOpenWithNativeApplicationRaw(const String& suggestedFilename, const String& originatingURLString, const uint8_t* data, unsigned long size, const String& pdfUUID);
> +    void savePDFToTemporaryFolderAndOpenWithNativeApplication(const String& suggestedFilename, const String& originatingURLString, const CoreIPC::DataReference&, const String& pdfUUID);

I can't tell the difference between "raw" and "cooked" looking at this file. Is there a semantic difference between these two?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:780
> +        // FIXME: We should probably notify the user that they can't save before the document is finished loading.
> +        // PDFViewController does an NSBeep(), but that seems insufficient.

Would be nice to grey out the icon in HUD. It's too late to explain now!

[Tim Horton]

(In reply to comment #9)
> (From update of attachment 179550 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179550&action=review
> 
> > Source/WebKit2/UIProcess/WebPageProxy.h:730
> > +    void savePDFToTemporaryFolderAndOpenWithNativeApplicationRaw(const String& suggestedFilename, const String& originatingURLString, const uint8_t* data, unsigned long size, const String& pdfUUID);
> > +    void savePDFToTemporaryFolderAndOpenWithNativeApplication(const String& suggestedFilename, const String& originatingURLString, const CoreIPC::DataReference&, const String& pdfUUID);
> 
> I can't tell the difference between "raw" and "cooked" looking at this file. Is there a semantic difference between these two?

No, the only difference is that raw takes the raw data and the cooked one takes a DataReference (and is the message target). It seemed like everything in the generated IPC-y code got angry at me when I tried to make the raw one an overload, but maybe there's a trick I'm missing?

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:780
> > +        // FIXME: We should probably notify the user that they can't save before the document is finished loading.
> > +        // PDFViewController does an NSBeep(), but that seems insufficient.
> 
> Would be nice to grey out the icon in HUD. It's too late to explain now!

Yes, it would! That's not WebKit's job, though.

[Tim Horton]

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

Will file a followup about WKSetMetadataURL, and if someone knows a trick allowing overloads of message targets, I'll fix that too.