[webkit-reviews] review granted: [Bug 103289] PDFPlugin: Support conversion of PostScript documents : [Attachment 176128] patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org <mitz at webkit.org> has granted Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 103289: PDFPlugin: Support conversion of PostScript documents
https://bugs.webkit.org/show_bug.cgi?id=103289

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

------- Additional Comments from mitz at webkit.org <mitz at webkit.org>
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().