Bug 52968 – Use a separate NSView for printing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Alexey Proskuryakov]

Printing an actual WKView means that we need to check for destination all the time, making code slower and more complicated. Also, it will be more difficult to get right when we start printing from a separate thread.

[Alexey Proskuryakov]

Created attachment 79857 [details]
proposed patch

[WebKit Review Bot]

Attachment 79857 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/mac/WKPrintingView.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

IIRC the Japan office is working on printing for Chromium and might be curious to see this go by.

Comment on attachment 79857 [details]
proposed patch

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
> +@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.

[Alexey Proskuryakov]

> +    Vector<WebCore::IntRect> _webPrintingPageRects;
> +    double _webTotalScaleFactorForPrinting;
>
> Any reason for the “web” prefix in these two? 

Just consistency with the third one.

> Should this be a CGFloat instead of a double?

Not sure, why? I need a double for total scale factor in cross-platform code, and using a standard type as much as possible seems preferable.

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

It doesn't seem like WebKit2 follows this convention, and I'm not sure if there is any difference. But I've changed it to WebNSViewDetails.

> 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.

I now see that starting with 10.6, NSPrintInfo has a proper scalingFactor method.

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

I felt uncomfortable using CGFloats with -doubleValue and +numberWithDouble, so changed to double.

[Alexey Proskuryakov]

Created attachment 79863 [details]
patch for landing

(In reply to comment #5)
> > +    Vector<WebCore::IntRect> _webPrintingPageRects;
> > +    double _webTotalScaleFactorForPrinting;
> >
> > Any reason for the “web” prefix in these two? 
> 
> Just consistency with the third one.

The first one is “webFrame” because “frame” has a different meaning in the context of NSViews.

> > Should this be a CGFloat instead of a double?
> 
> Not sure, why? I need a double for total scale factor in cross-platform code, and using a standard type as much as possible seems preferable.

Agreed.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 79863 [details]:

animations/suspend-resume-animation.html bug 48161 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 79863 [details]
patch for landing

Clearing flags on attachment: 79863

Committed r76470: <http://trac.webkit.org/changeset/76470>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.