[webkit-reviews] review denied: [Bug 4151] USPS.com Label Printing Malfunction : [Attachment 39827] Executes document-level JavaScript actions in PDFs, providing a Doc object with a print method

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied mitz at webkit.org's request for
review:
Bug 4151: USPS.com Label Printing Malfunction
https://bugs.webkit.org/show_bug.cgi?id=4151

Attachment 39827: Executes document-level JavaScript actions in PDFs, providing
a Doc object with a print method
https://bugs.webkit.org/attachment.cgi?id=39827&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
> +#import <JavaScriptCore/JSBase.h>
> +
> + at class WebDataSource;
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +JSObjectRef makeJSPDFDoc(JSContextRef, WebDataSource *);
> +#ifdef __cplusplus
> +}
> +#endif

Why does this need to be extern "C"?

> +static void jsPDFDocInitialize(JSContextRef ctx, JSObjectRef object)
> +{
> +    WebDataSource *dataSource = (WebDataSource *)JSObjectGetPrivate(object);

> +    [dataSource retain];
> +}

This should use CFRetain or it'll break under GC

> +
> +static void jsPDFDocFinalize(JSObjectRef object)
> +{
> +    WebDataSource *dataSource = (WebDataSource *)JSObjectGetPrivate(object);

> +    [dataSource release];
> +}

Same here, use CFRelease.

> +
> +static JSValueRef jsPDFDocPrint(JSContextRef ctx, JSObjectRef function,
JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[],
JSValueRef* exception)
> +{
> +    WebDataSource *dataSource = (WebDataSource
*)JSObjectGetPrivate(thisObject);
> +
> +    WebView *webView = [[dataSource webFrame] webView];
> +    CallUIDelegate(webView, @selector(webView:printFrameView:), [[dataSource
webFrame] frameView]);

Is there no internal method for printing a frame view?

> +
> +    return JSValueMakeNull(ctx);

"void" javascript functions usually return undefined. Are you sure this should
return null?

> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +void addWebPDFDocumentExtras(Class);
> +#ifdef __cplusplus
> +}
> +#endif

Again, why does this need to be extern C?

> +
> +void addWebPDFDocumentExtras(Class pdfDocumentClass)
> +{
> +#ifndef BUILDING_ON_TIGER
> +    class_addMethod(pdfDocumentClass, @selector(_web_allScripts),
(IMP)web_PDFDocumentAllScripts, "@@:");
> +#else
> +    struct objc_method_list methodList = { 0, 1, {
@selector(_web_allScripts), (IMP)web_PDFDocumentAllScripts, "@@:" } };
> +    class_addMethods(pdfDocumentClass, &methodList);
> +#endif

Why do you need to add this to the class? Can't you just use a category method?


> ++ (void)initialize
> +{
> +    Class pdfDocumentClass = [self PDFDocumentClass];
> +    if (pdfDocumentClass)
> +	   addWebPDFDocumentExtras(pdfDocumentClass);

+initialize should check if self == [WebPDFRepresentation class] to guard
against adding the same extras more than once.