[Webkit-unassigned] [Bug 4151] USPS.com Label Printing Malfunction

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=4151





--- Comment #16 from mitz at webkit.org  2009-09-21 10:43:12 PDT ---
(In reply to comment #15)
> (From update of attachment 39827 [details])
> > +#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"?

Because it is called from Objective-C (in WebPDFRepresentation.m).

> > +static void jsPDFDocInitialize(JSContextRef ctx, JSObjectRef object)
> > +{
> > +    WebDataSource *dataSource = (WebDataSource *)JSObjectGetPrivate(object);
> > +    [dataSource retain];
> > +}
> 
> This should use CFRetain or it'll break under GC

I will change it. Why isn’t it enough that the JavaScript object has the data
source as its private object to protect it from Obj-C GC?

> > +
> > +static void jsPDFDocFinalize(JSObjectRef object)
> > +{
> > +    WebDataSource *dataSource = (WebDataSource *)JSObjectGetPrivate(object);
> > +    [dataSource release];
> > +}
> 
> Same here, use CFRelease.

Will change.

> 
> > +
> > +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?

I think there is. I am not sure why you are asking. This is analogous to how
window.print is handled, and to what clicking the Print button in the PDF HUD
does.

> > +    return JSValueMakeNull(ctx);
> 
> "void" javascript functions usually return undefined. Are you sure this should
> return null?

No. I will change it to return undefined.

> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +void addWebPDFDocumentExtras(Class);
> > +#ifdef __cplusplus
> > +}
> > +#endif
> 
> Again, why does this need to be extern C?

Because it is called from Objective-C (in WebPDFRepresentation.m).

> 
> > +
> > +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?

There is no way to add category methods dynamically at runtime. As far as I can
tell, at least in the Objective-C 2.0 runtime, categories are not a runtime
concept. For what it’s worth, I do declare the method as 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.

I will add the check.

Thanks for the review!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.