Bug 52968

Created attachment 79857 [details] proposed patch

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.

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.

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

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.

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 on attachment 79863 [details] patch for landing Clearing flags on attachment: 79863 Committed r76470: <http://trac.webkit.org/changeset/76470>

All reviewed patches have been landed. Closing bug.