[webkit-reviews] review granted: [Bug 52968] Use a separate NSView for printing : [Attachment 79857] proposed patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted Alexey Proskuryakov <ap at webkit.org>'s request for
review:
Bug 52968: Use a separate NSView for printing
https://bugs.webkit.org/show_bug.cgi?id=52968

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

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=79857&action=review

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.h:26
> +#import <Cocoa/Cocoa.h>

I see this imported from WebKit2Prefix.h. Isn’t that enough?

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.h:40
> +    Vector<WebCore::IntRect> _webPrintingPageRects;
> +    double _webTotalScaleFactorForPrinting;

Any reason for the “web” prefix in these two? Should this be a CGFloat instead
of a double?

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:32
> + at interface NSView (Details)

In WebKit1, the convention for a category name like this is (WebNSViewDetails).


> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:42
> +static float currentPrintOperationScale()

Seems like callers to this function already have the current operation, so this
could have been a function that takes an NSPrintOperation * or simply an
instance method in an NSOperation category. But I see that you’re just moving
this code around.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:83
> +	   originalTopMargin = [info topMargin];
> +	   originalBottomMargin = [info bottomMargin];

-topMargin and -bottomMargin return CGFloats. Maybe this code should use more
CGFloats (or doubles) and fewer floats.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:99
> +// Return the number of pages available for printing
> +- (BOOL)knowsPageRange:(NSRangePointer)range

Actually, it returns YES. I’m not sure the comment helps.