Bug 103289 – PDFPlugin: Support conversion of PostScript documents

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

[Tim Horton]

PDFViewController supported this previously, so we should continue to.

<rdar://problem/10235708>

[Tim Horton]

Created attachment 176128 [details]
patch

Comment on attachment 176128 [details]
patch

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

> Source/WebCore/platform/LocalizedStrings.cpp:736
> +    return WEB_UI_STRING("PostScript", "Description of the PostScript type supported (and converted to PDF) by the PDF pseudo plug-in. Visible in Installed Plug-ins page in Safari.");

You should correct the comment for pdfDocumentTypeDescription, since it is no longer the only type supported by the plug-in. I don’t think “(and converted to PDF)” is relevant to the person who needs to localize this string, so I’d remove it. You should also add “the” before “Install Plug-ins” here and in pdfDocumentTypeDescription().

> Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm:185
> +    MimeClassInfo psMimeClassInfo;

I’d call this postScriptMimeClassInfo.

> Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm:353
> +static RetainPtr<CFMutableDataRef> convertPostScriptDataSourceToPDF(RetainPtr<CFDataRef> psData)

What does “Source” mean in this context? I’d name the parameter postScriptData.

> Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm:359
> +    RetainPtr<CGPSConverterRef> converter(AdoptCF, CGPSConverterCreate(0, &callbacks, 0));

The newly-preferred style for this is, I think, … converter = adoptCF(…)

> Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm:362
> +    RetainPtr<CGDataProviderRef> provider(AdoptCF, CGDataProviderCreateWithCFData((CFDataRef)psData.get()));

I’m surprised that we need to cast to CFDataRef here.

> Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm:365
> +    RetainPtr<CFMutableDataRef> result(AdoptCF, CFDataCreateMutable(kCFAllocatorDefault, 0));

I’d call this pdfData.

> Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm:370
> +

Not sure all of these assertions are useful. If any of these are NULL, won’t we crash in an obvious way?

> Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm:662
> +    if (equalIgnoringCase(mimeType, "application/postscript"))

Since “application/postscript” is repeated multiple times in this file, I think it makes sense to put it in a named constant. Or perhaps add a static bool isPostScriptMIMEType().

[Tim Horton]

(In reply to comment #2)
> (From update of attachment 176128 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176128&action=review
> 
> > Source/WebCore/platform/LocalizedStrings.cpp:736
> > +    return WEB_UI_STRING("PostScript", "Description of the PostScript type supported (and converted to PDF) by the PDF pseudo plug-in. Visible in Installed Plug-ins page in Safari.");
> 
> You should correct the comment for pdfDocumentTypeDescription, since it is no longer the only type supported by the plug-in. I don’t think “(and converted to PDF)” is relevant to the person who needs to localize this string, so I’d remove it. You should also add “the” before “Install Plug-ins” here and in pdfDocumentTypeDescription().

Surely! I wasn't sure I could change the descriptions without breaking localization stuff that I know nothing about.

> > Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm:359
> > +    RetainPtr<CGPSConverterRef> converter(AdoptCF, CGPSConverterCreate(0, &callbacks, 0));
> 
> The newly-preferred style for this is, I think, … converter = adoptCF(…)

Oh really? People had always been pointing me in the opposite direction (and I, other people!)

[Tim Horton]

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