Bug 52968 - Use a separate NSView for printing
Summary: Use a separate NSView for printing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-22 14:37 PST by Alexey Proskuryakov
Modified: 2011-01-23 14:28 PST (History)
7 users (show)

See Also:


Attachments
proposed patch (29.25 KB, patch)
2011-01-22 14:42 PST, Alexey Proskuryakov
mitz: review+
Details | Formatted Diff | Diff
patch for landing (28.81 KB, patch)
2011-01-22 21:11 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2011-01-22 14:37:53 PST
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.
Comment 1 Alexey Proskuryakov 2011-01-22 14:42:23 PST
Created attachment 79857 [details]
proposed patch
Comment 2 WebKit Review Bot 2011-01-22 14:43:57 PST
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.
Comment 3 Eric Seidel (no email) 2011-01-22 15:02:37 PST
IIRC the Japan office is working on printing for Chromium and might be curious to see this go by.
Comment 4 mitz 2011-01-22 20:10:35 PST
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.
Comment 5 Alexey Proskuryakov 2011-01-22 21:10:33 PST
> +    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.
Comment 6 Alexey Proskuryakov 2011-01-22 21:11:02 PST
Created attachment 79863 [details]
patch for landing
Comment 7 mitz 2011-01-22 21:15:40 PST
(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.
Comment 8 WebKit Commit Bot 2011-01-23 14:27:29 PST
The commit-queue encountered the following flaky tests while processing attachment 79863 [details]:

animations/suspend-resume-animation.html bug 48161 (author: cmarrin@apple.com)
The commit-queue is continuing to process your patch.
Comment 9 WebKit Commit Bot 2011-01-23 14:28:09 PST
Comment on attachment 79863 [details]
patch for landing

Clearing flags on attachment: 79863

Committed r76470: <http://trac.webkit.org/changeset/76470>
Comment 10 WebKit Commit Bot 2011-01-23 14:28:15 PST
All reviewed patches have been landed.  Closing bug.